Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) #29640
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process. ConflictsNo conflicts as of last run. |
sr-gi
changed the title
sr-gi
changed the title
sr-gi
force-pushed
the
202403-block-tiebreak
branch
from
43873be
to
18820ac
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
sr-gi
force-pushed
the
202403-block-tiebreak
branch
3 times, most recently
from
a705e00
to
77e1c45
Compare
sr-gi
force-pushed
the
202403-block-tiebreak
branch
2 times, most recently
from
8225bd6
to
f95e896
Compare
Before this, if we had two (or more) same work tip candidates and restarted our node, it could be the case that the block set as tip after bootstrap didn't match the one before stopping. That's because the work and `nSequenceId` of both block will be the same (the latter is only kept in memory), so the active chain after restart would have depended on what tip candidate was loaded first. This makes sure that we are consistent over reboots.
Adds tests to make sure we are consistent on activating the same chain over a node restart if two or more candidates have the same work when the node is shutdown
sr-gi
force-pushed
the
202403-block-tiebreak
branch
from
f95e896
to
e4cf8d0
Compare
|
Rebased to drop the custom log fix in favor of a more generic solution (#29640 (comment)) |
This PR grabs some interesting bits from #29284 and fixes some edge cases in how block tiebreaks are dealt with.
Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated #29284 (comment) (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check #29284 (comment) for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within
CBlockIndexisnSequenceId, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the samenSequenceId: 0.Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the
CBlockIndexWorkComparatorhas to default to its last rule: whatever block is loaded first (has a smaller memory address).This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way
nSequenceIdis initialized is changed to: