-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
*: support extra dBFT stage #118
Conversation
a2d89af
to
01f9e6c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #118 +/- ##
==========================================
- Coverage 63.31% 59.35% -3.96%
==========================================
Files 27 32 +5
Lines 1510 1823 +313
==========================================
+ Hits 956 1082 +126
- Misses 489 661 +172
- Partials 65 80 +15 ☔ View full report in Codecov by Sentry. |
3460b23
to
942dd05
Compare
It will be reused by anti-MEV related dBFT interfaces implementations. Signed-off-by: Anna Shaleva <[email protected]>
890c324
to
7d7004b
Compare
pre_block.go
Outdated
Data() []byte // required | ||
// SetData generates and sets PreBlock's data CNs need to exchange during Commit | ||
// phase. | ||
SetData(key PrivateKey) error // required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getters/setters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you want? GetData
instead of Data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually getters/setters mean that something is wrong with the interface. Likely SetData
can be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it’s wrong with the old implementation because the logic is the same as for our old Commit.SetSignature() and Commit.Signature(). Need to check, maybe we can pass signature directly to the Commit constructor.
1966e0a
to
659de52
Compare
Ref. #112. Signed-off-by: Anna Shaleva <[email protected]>
438f9df
to
d5baa08
Compare
Add custom PreBlock and Block interfaces implementation, custom Commit and CommitAck, adjust testing logic. Signed-off-by: Anna Shaleva <[email protected]>
Technically, this is implied by d.ResponseSent(), but we still have BlockSent() check as well, so these don't hurt. Signed-off-by: Roman Khimov <[email protected]>
It's done at the start of OnReceive() and it's even more strict there. Signed-off-by: Roman Khimov <[email protected]>
It's a security event, this should never happen and we better know if it does. Signed-off-by: Roman Khimov <[email protected]>
It's useless, we log tx number down below anyway. Signed-off-by: Roman Khimov <[email protected]>
Commit payloads can't be checked with PrepareRequest data only in AMEV case, we don't yet have a complete header. So check them after we have enough PreCommit payloads. Signed-off-by: Roman Khimov <[email protected]>
Be a bit more optimistic about the message (similar to how onPrepareResponse() works). Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
Supposedly AMEV code will do its magic here and it doesn't need to do it twice. Signed-off-by: Roman Khimov <[email protected]>
For a single height/view there is a single proper hash and it's not a final block hash, likely logging it won't help much. Can be done in future if needed, but let's have view number for now. Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
start() was designed to be called at every view change, but looks like it _never_ worked this way. Which means two things: * on every view change Primary doesn't send PrepareRequest during initialization (which is mostly OK, OnTimeout() will be triggered immediately with 0 timeout) * our future message caching system has never really worked since start() is the only place where messages can be picked up from it Just drop start(), make caches work and add a test for them. Signed-off-by: Roman Khimov <[email protected]>
d5baa08
to
2fca051
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find anything suspicious here, LGTM.
WIP, just something I have on hands right now, need to test it and integrate with NeoGo/NeoX nodes.
TODO:
Ref. #112.