Modify the algorithm of updateFinalizedBlockRoots
#13486
Conversation
rkapka
force-pushed
the
finalized-blocks-audit
branch
2 times, most recently
from
6c69629 to
12bd6f5
Compare
| @@ -374,63 +374,63 @@ func TestLoadBlocks_BadStart(t *testing.T) { | |||
| // \- B7 | |||
| func tree1(t *testing.T, beaconDB db.Database, genesisRoot []byte) ([][32]byte, []*ethpb.SignedBeaconBlock, error) { | |||
| b0 := util.NewBeaconBlock() | |||
| b0.Block.Slot = 0 | |||
| b0.Block.Slot = 1 | |||
There was a problem hiding this comment.
Incrementing slot everywhere because genesis block has slot 0
| require.NoError(t, err) | ||
| require.Equal(t, 10, len(filteredBlocks)) | ||
| require.Equal(t, 7, len(filteredBlocks)) |
There was a problem hiding this comment.
Interesting example to demonstrate the purpose of the new algorithm. The tree constructed in tree1 has orphaned blocks, but they were expected to be finalized.
rkapka
force-pushed
the
finalized-blocks-audit
branch
from
0cc22fe to
38579c4
Compare
# Conflicts: # beacon-chain/db/kv/finalized_block_roots.go
| if bytes.Equal(r, checkpointRoot) { | ||
| return true, [][]byte{r} | ||
| } | ||
| children := lookupValuesForIndices(ctx, map[string][]byte{string(blockParentRootIndicesBucket): r}, tx)[0] |
There was a problem hiding this comment.
This will panic if lookupValuesForIndices returns an empty slice, which is what happens if you send a genesis root here
There was a problem hiding this comment.
I added a condition for it although it is not strictly necessary. Since the passed in map contains one key, the resulting slice will have one item.
| @@ -297,3 +232,50 @@ func (s *Store) FinalizedChildBlock(ctx context.Context, blockRoot [32]byte) (in | |||
| tracing.AnnotateError(span, err) | |||
| return blk, err | |||
| } | |||
|
|
|||
| func pathToFinalizedCheckpoint(ctx context.Context, roots [][]byte, checkpointRoot []byte, tx *bolt.Tx) (bool, [][]byte) { | |||
| if len(roots) == 1 && roots[0] == nil { | |||
There was a problem hiding this comment.
if an empty slice is passed for roots we should also exit early
| if i == len(finalized)-1 { | ||
| // We don't know the finalized child of the new finalized checkpoint. | ||
| // It will be filled out in the next function call. | ||
| container = ðpb.FinalizedBlockRootContainer{} |
There was a problem hiding this comment.
This completely misses adding in ParentRoot for the container. In fact it appears this PR has completely removed it.
| container = ðpb.FinalizedBlockRootContainer{ | ||
| ParentRoot: finalized[i-1], | ||
| } | ||
| } else if i == 0 { |
There was a problem hiding this comment.
might be better to use a switch statement ? If not handling i in a progressive order is reads better
if i == 0 {
// do something
else if i == len(finalized) -1 {
// do something
} else {
// ...
}
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
* Revert "Modify the algorithm of `updateFinalizedBlockRoots` (#13486)" This reverts commit 32fb183. * migration to fix index corruption from pr 13486 * bail as soon as we see 10 epochs without the bug --------- Co-authored-by: Kasey Kirkham <[email protected]>
What type of PR is this?
Other
What does this PR do? Why is it needed?
The motivation for this PR is the following note from
IsFinalizedBlock:This means that an orphaned block can be viewed as finalized, as was reported in #13408. To fix this, I redesigned the algorithm so that it traverses parent->child block relationships in the
blockParentRootIndicesBucketbucket to find the path between the last finalized checkpoint and the current finalized checkpoint.TestStore_OrphanedBlockIsNotFinalizedis the test covering the main improvement. I completely removedTestStore_IsFinalized_ForkEdgeCasebecause it tested that a finalized checkpoint is reverted.The new algorithm shaves off a staggering 10ms from epoch processing time 😆
Which issues(s) does this PR fix?
Fixes #13408