core/qbft: only trigger each upon rule once per round #536
Conversation
|
|
leolara
force-pushed
the
leolara/I523/qbft-only-trigger-upon-rule-once
branch
2 times, most recently
from
5646511
to
9de3b65
Compare
Codecov Report
@@ Coverage Diff @@
## main #536 +/- ##
==========================================
- Coverage 54.90% 54.52% -0.38%
==========================================
Files 91 91
Lines 8468 8482 +14
==========================================
- Hits 4649 4625 -24
- Misses 3210 3247 +37
- Partials 609 610 +1
Continue to review full report at Codecov.
|
| @@ -202,6 +203,29 @@ func Run[I any, V comparable](ctx context.Context, d Definition[I, V], t Transpo | |||
| dedupIn = dedup | |||
| } | |||
|
|
|||
| isDuplicatedRule := func(rule uponRule) bool { | |||
There was a problem hiding this comment.
please add comments to the functions
core/qbft/qbft.go
Outdated
| } | ||
|
|
||
| changeRound := func(newRound int64) { | ||
| resetDedupRules() |
There was a problem hiding this comment.
can probably inline resetDedupRules since it is just a single line function, and I would also move trimBuffer here:
// changeRound updates the round and clears state of the previous round
changeRound := func(newRound int64) {
round = newRound
dedupRules = make(map[uponRule]bool)
trimBuffer()
}
core/qbft/qbft.go
Outdated
| return true | ||
| } | ||
|
|
||
| // first time for this rule |
There was a problem hiding this comment.
nit (also please capitalise comment sentences): // First time for this rule in current round
core/qbft/qbft.go
Outdated
| @@ -202,6 +203,29 @@ func Run[I any, V comparable](ctx context.Context, d Definition[I, V], t Transpo | |||
| dedupIn = dedup | |||
| } | |||
|
|
|||
| isDuplicatedRule := func(rule uponRule) bool { | |||
| if rule == uponJustifiedDecided { | |||
There was a problem hiding this comment.
On second thought, I think it is sending of qCommit that may be triggered multiple times per round, not the receiving of qCommit. Receiving it once is sufficient, since then we have decided and we are done. So I think we must remove this line.
core/qbft/qbft.go
Outdated
| @@ -155,6 +155,7 @@ func Run[I any, V comparable](ctx context.Context, d Definition[I, V], t Transpo | |||
| dedupIn = make(map[dedupKey]bool) | |||
| timerChan <-chan time.Time | |||
| stopTimer func() | |||
| dedupRules = make(map[uponRule]bool) | |||
There was a problem hiding this comment.
nit: I would put the two dedup maps next to each other. We could also rename dedupIn to dedupMsgs
leolara
changed the title
leolara
force-pushed
the
leolara/I523/qbft-only-trigger-upon-rule-once
branch
from
9de3b65
to
e62962b
Compare
leolara
added
the
merge when ready
obol-bulldozer
bot
deleted the
leolara/I523/qbft-only-trigger-upon-rule-once
branch
The QBFT paper states that each upon rules should only be triggered once per round (with the exception of uponJustifiedDecided.
The current implementation allows for rules to be triggered multiple times.
Note this isn't such a big problem, since the algorithm is idempotent, the only affect is that more messages are sent over the wire then necessary.
category: refactor
ticket: #523