Repository navigation
Ensure Non-Ancestors are Pruned When a Future Block is Finalized - #623
Conversation
|
|
||
| // we do not care to process votes for rounds where we have finalized | ||
| func (e *Epoch) isVoteForFinalizedRound(round uint64) bool { | ||
| func (e *Epoch) isBelowHighestFinalizedRound(round uint64) bool { |
There was a problem hiding this comment.
renamed because isVoteForFinalizedRound was misleading, especially with the new callsites
| } | ||
|
|
||
| // A higher round was finalized while we were verifying, so this block can no longer be finalized. | ||
| if e.isBelowHighestFinalizedRound(md.Round) { |
There was a problem hiding this comment.
this runs async, so we need to ensure we don't have a race between
pruneToFinalizedChain call and this task finishing.
| defer e.lock.Unlock() | ||
|
|
||
| // A higher round was finalized while we were verifying, so this block can no longer be finalized. | ||
| if e.isBelowHighestFinalizedRound(md.Round) { |
|
|
||
| _, built := bb.BuildBlock(ctx, e.Metadata(), emptyBlacklist) | ||
| require.True(t, built) | ||
| block1 := bb.GetBuiltBlock() |
There was a problem hiding this comment.
Can we add an assertion in the test that shows whether block1 builds on top of block0 or genesis? It's not clear to me what's going on here.
There was a problem hiding this comment.
block 1 builds of block 0. added an assertion
| }) | ||
| } | ||
|
|
||
| func testEpochFinalizeVoteRebroadcastForTimedOutRound(t *testing.T, buildOnTimedOutRound bool) { |
There was a problem hiding this comment.
can we add an ASCII art tree that shows how are the blocks built one on the other?
There was a problem hiding this comment.
i'm no artist, but i asked my friend claude to graciously add some art
| require.Equal(t, NotarizationRecordType, wal.AssertNotarization(1)) | ||
| testutil.WaitToEnterRound(t, e, 2) | ||
|
|
||
| // block2 either extends block1, or builds off block0 |
There was a problem hiding this comment.
which one is what? can be more specific in the comment?
There was a problem hiding this comment.
not sure what yo mean byt this comment. the if here is determined by the type of test defined above
| }, nodes[2])) | ||
|
|
||
| // block2 skips round 1, so we need to send an empty notarization before block2 can be verified | ||
| if !buildOnTimedOutRound { |
There was a problem hiding this comment.
I think we have enough branching to justify just two tests. It's all right if they are similar. I think they're easier to read each test when it's separate.
There was a problem hiding this comment.
separated the tests
| require.Equal(t, uint64(3), e.Metadata().Round) | ||
| } | ||
|
|
||
| // TestEpochFinalizeVoteRebroadcastForTimedOutRound ensures that after finalizing a future sequence, |
There was a problem hiding this comment.
that after finalizing a future sequence
What does finalizing a future sequence mean?
There was a problem hiding this comment.
finalizing a future sequence as in -> creating a finalization for a block that is in the future. Which is possible because maybe other nodes have finalized a future sequence and we just missed the messages
|
|
||
| close(block1.VerificationDelay) | ||
|
|
||
| // To ensure block1 has finished verifying, we send our node block3. Because the verification scheduler only verifies |
There was a problem hiding this comment.
why do we need to do this though? It just makes the point we're trying to prove on shakier grounds because now it depends on a child block.
Isn't there another way of checking we have finished verifying block1?
There was a problem hiding this comment.
we can add a log intercept and remove block3, but I'm not sure of another way. Not sure what else is assertable
| if err != nil { | ||
| return err | ||
| } | ||
| e.pruneCompetingRounds(finalization) |
There was a problem hiding this comment.
this happens for each finalization in the WAL. maybe worth to open an issue to locate the finalization with the highest sequence and call it only once?
There was a problem hiding this comment.
referenced in an issue
It's important for honest nodes to never send out finalize votes for conflicting blocks of the same sequence. It's also important honest nodes don't send both an empty vote & a finalize vote for the same round.
These points have one small caveat -> when we receive a finalization of a future sequence, we can actually send out a finalize vote for any blocks we've notarized before it(as long as its part of the finalized chain). This is because a finalization cannot be forged, so when we receive one for a future round our node is allowed to help create finalize votes even if we have already sent an empty vote. Maybe a few nodes that created the future finalization have lost connection, so it's up to our node to help advance the chain.
We have a mechanism
rebroadcastPastFinalizeVotesthat does this. However the issue is that this method would simply create and send finalize votes for all rounds below the future finalization WITHOUT checking if the block in memory is an ancestor of the finalization. This is a problem because if we had a fork, then we would create and broadcast a finalize vote for a block in a conflicting chain.The solution is to prune our memory(rounds map) of non-ancestor blocks when a finalization occurs. This way our rounds map doesn't store blocks that will never be finalized.
rebroadcastPastFinalizeVotescan then work unchanged, because every previous round must be an ancestor because pruning is done before calling.We must also prune the memory when we restore finalization from the wal.
This issue was brought to my attention due to #290.
Closes #290.