Add certificates to Istanbul BFT messages for liveness #200
Conversation
| var num int | ||
| if rc.HasPreparedCertificate() { | ||
| if err := c.handlePreparedCertificate(rc.PreparedCertificate); err != nil { | ||
| // TODO(asa): Should we still accept the round change message without the certificate if this fails? |
There was a problem hiding this comment.
I think it's probably find to ignore, as the round change message itself is technically invalid?
There was a problem hiding this comment.
My hunch is that it's fine either way
| } | ||
|
|
||
| func (s *roundState) GetLockedHash() common.Hash { | ||
| // TODO(asa): Should we use COMMITs as well in the prepared certificate if they're available? |
There was a problem hiding this comment.
I think so because they form the basis for why a validator themselves would have sent out a COMMIT? Otherwise faulty validators could send out COMMIT, causing f+1 validators to advance, without ever producing a valid prepareCertificate?
consensus/istanbul/core/prepare.go
Outdated
| } | ||
|
|
||
| seen := make(map[common.Address]bool) | ||
| for _, message := range preparedCertificate.PrepareMessages { |
There was a problem hiding this comment.
Could we skip the processing if the current prepareCertificate has the same proposal as this one?
There was a problem hiding this comment.
Maybe? It's possible you have a PREPARED certificate already because you heard 2f+1 PREPARE/COMMIT messages but no one else did, and the next proposer put out a PREPREPARE with an invalid PREPARED certificate but with the same proposal. With your proposal, you would still accept, while everyone else would reject, which I suppose could be okay but it feels safer to reject PREPREPAREs with invalid PREPARED certificates
There was a problem hiding this comment.
Ah great point. I guess I wonder how big the processing burden is anyways, my first intuition was that checking O(n) round changes messages that contain the same O(n) PREPARE certificates would be wasteful, but maybe it doesn't matter. If it does, i guess we could dedupe against the hash of the certificate?
|
Note the addition of:
|
+1 to this. The plan is to merge it with Tim's changes and throw a bunch of faulty nodes at it and make sure everything runs smoothly |
Codecov Report
@@ Coverage Diff @@
## master #200 +/- ##
==========================================
- Coverage 52.88% 52.87% -0.01%
==========================================
Files 580 580
Lines 69931 70098 +167
==========================================
+ Hits 36982 37064 +82
- Misses 29960 30025 +65
- Partials 2989 3009 +20
Continue to review full report at Codecov.
|
|
Closing as this has been replaced by #366 |
Description
This PR removes the locking mechanism in IBFT and adds:
This addresses a bug in which F nodes could lock onto block A, while 2F nodes could lock onto block B, causing a permanent loss of liveness.
Tested
Unit tests
Other changes
Related issues