-
Notifications
You must be signed in to change notification settings - Fork 83
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
core/qbft: add drop tests and fix bugs #504
Conversation
Codecov Report
@@ Coverage Diff @@
## main #504 +/- ##
==========================================
+ Coverage 56.47% 56.49% +0.02%
==========================================
Files 87 87
Lines 7983 7989 +6
==========================================
+ Hits 4508 4513 +5
- Misses 2874 2878 +4
+ Partials 601 598 -3
Continue to review full report at Codecov.
|
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 like the spacing changes to align everything 😅
// TODO(corver): Fix trimming. | ||
// var selected []Msg[I, V] | ||
// for _, msg := range buffer { | ||
// if msg.Round() >= round-1 { | ||
// selected = append(selected, msg) | ||
// } | ||
// } | ||
// buffer = selected | ||
// | ||
// dedup := make(map[dedupKey]bool) | ||
// for k := range dedupIn { | ||
// if k.Round >= round { | ||
// dedup[k] = true | ||
// } | ||
// } | ||
// dedupIn = dedup |
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 is wrong with this approach? I assume there would be cases where we would need message of rounds before round-1
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.
We sometimes loose some PREPAREs from previous rounds, then we cannot trigger UnponQuorumRoundChange singe Qrc isn't justified. I have a plan to fix this in PR.
43e8e82
to
485fcb3
Compare
Add the 10% dropped message test, which found some bugs:
preparedValue
, only a single RC with that value (confirmed this with GoQuorumquorum/consensus/istanbul/qbft/core/justification.go:79
).category: bug
ticket: #445
feature_set: alpha