evidence: buffer evidence from consensus #5890
Conversation
cmwaters
requested review from
ebuchman,
erikgrinaker,
melekes and
tessr
as code owners
Codecov Report
@@ Coverage Diff @@
## master #5890 +/- ##
=======================================
Coverage 59.90% 59.91%
=======================================
Files 269 269
Lines 24544 24550 +6
=======================================
+ Hits 14703 14709 +6
Misses 8244 8244
Partials 1597 1597
|
| func (evpool *Pool) flushConsensusBuffer() { | ||
| for _, ev := range evpool.consensusBuffer { | ||
| if err := evpool.addPendingEvidence(ev); err != nil { | ||
| evpool.logger.Error("failed to flush evidence from consensus buffer to pending list: %w", err) |
There was a problem hiding this comment.
| evpool.logger.Error("failed to flush evidence from consensus buffer to pending list: %w", err) | |
| evpool.logger.Error("failed to flush evidence from consensus buffer to pending list: %w", err) | |
| continue |
?
| evpool.evidenceList.PushBack(ev) | ||
| } | ||
| // reset consensus buffer | ||
| evpool.consensusBuffer = make([]types.Evidence, 0) |
There was a problem hiding this comment.
They are at different stages of consensus I believe but it might pay to put a mutex on the slice just in case
evidence/pool.go
Outdated
| @@ -46,6 +46,8 @@ type Pool struct { | |||
|
|
|||
| pruningHeight int64 | |||
| pruningTime time.Time | |||
|
|
|||
| consensusBuffer []types.Evidence | |||
There was a problem hiding this comment.
can we add a comment here too?
| evpool.state = state | ||
| // flushConsensusBuffer moves the evidence produced from consensus into the evidence pool | ||
| // and list so that it can be broadcasted and proposed | ||
| func (evpool *Pool) flushConsensusBuffer() { |
There was a problem hiding this comment.
nit: I would've added a write-lock here instead of using it in Pool#Update. That way, it becomes:
evpool.flushConsensusBuffer()
evpool.updateState(state)I say this because we have updateState already that does write-locking, but now Pool#Update does it :-/
There was a problem hiding this comment.
Yeah I guess this makes sense to avoid someone in the future from using evpool.flushConsensusBuffer() without taking out a lock
There was a problem hiding this comment.
I also wasn't too sure if the best practice was to create another lock or use the existing lock on state for consensusBuffer
There was a problem hiding this comment.
since we have auxiliary logic (method) that does all the business logic, I believe we should place the lock here. This isn't necessarily critical though 👍
Description
Evidence coming from consensus is stored in a temporary buffer before being flushed to the evidence pool and thus halted until the voting in the height of the evidence is finished before being broadcasted and proposed. This avoids the issue of some nodes verifying and proposing evidence at a height where the block hasn't been committed yet which could cause those peers who never saw the double votes to panic.