validation: have LoadBlockIndex account for snapshot use
ClosedPublic
Actions

Authored by PiRK on Oct 17 2022, 15:14.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC95e826419ec1: validation: have LoadBlockIndex account for snapshot use

Summary

validation: insert assumed-valid block index entries into candidates

https://github.com/bitcoin/bitcoin/pull/21526/commits/5a807736dacfc3e6fa57231219336acf08be38fb

validation: have LoadBlockIndex account for snapshot use

Ensure that blocks past the snapshot base block (i.e. the end of the
assumed-valid region of the chain) are not included in
setBlockIndexCandidates for the background validation chainstate. These
blocks, while fully validated and lacking the BLOCK_ASSUMED_VALID flag,
*rely* on blocks which are assumed-valid, and so shouldn't be added to
the IBD chainstate.

Co-authored-by: Russ Yanofsky <russ@yanofsky.org>

https://github.com/bitcoin/bitcoin/pull/23174/commits/0fd599a51a700c028d6f7ed8067d2d9f6e6a04a5

test: add tests for LoadBlockIndex when using multiple chainstates

Incorporates feedback from Russ Yanofsky.

https://github.com/bitcoin/bitcoin/pull/23174/commits/2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24

This is a partial backport of core#21526 and core#23174

Notes:

See D6308 for CBlockIndex::UpdateChainStats().
See D8319 for why I have to upgrade the block dabase in setup_common.cpp to make the test work.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Oct 17 2022, 15:14

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 17 2022, 15:14

PiRK requested review of this revision.Oct 17 2022, 15:14

teamcity edited the summary of this revision. (Show Details)Oct 17 2022, 15:14

PiRK added a child revision: D12281: validation: fix CheckBlockIndex for multiple chainstates.Oct 17 2022, 15:16

Harbormaster completed remote builds in B20554: Diff 35761.Oct 17 2022, 16:14

No test

This revision now requires changes to proceed.Oct 18 2022, 09:51

PiRK removed a child revision: D12281: validation: fix CheckBlockIndex for multiple chainstates.Oct 19 2022, 08:14

PiRK edited parent revisions, added: D12281: validation: fix CheckBlockIndex for multiple chainstates; removed: D12279: chain: add and set ASSUMED_VALID_FLAG for assumeutxo.

squash with core#23174 to have the complete feature and the test coverage

I had to adjust setup_common.cpp to make the test work with Bitcoin ABC. This is because of our database upgrade feature (D8319). Calling Upgrade is how the block database is initialized in init.cpp as well.

Harbormaster completed remote builds in B20631: Diff 35877.Oct 20 2022, 08:24

Fabien requested changes to this revision.Oct 21 2022, 06:54

Fabien added inline comments.

src/test/util/setup_common.cpp
175 ↗	(On Diff #35877)	You can do this in the single test that use it, and save the cost for all the other unit tests
src/test/validation_chainstatemanager_tests.cpp
353 ↗	(On Diff #35877)	Please move the vars where they are used, this is not C89 and is barely readable as is.

This revision now requires changes to proceed.Oct 21 2022, 06:54

move variable declaration closer to where they are needed.
Take lock when accessing chain tip (would be fixed in backport of core#25077, unlikely to be backported soon).

Harbormaster completed remote builds in B20664: Diff 35929.Oct 21 2022, 12:13

Fabien accepted this revision.Oct 21 2022, 15:57

Fabien added inline comments.