Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC03af9518eaa9: simplify ChainstateManager::SnapshotBlockhash() return semantics

Summary

https://github.com/bitcoin/bitcoin/pull/19806/commits/f6e2da5fb7c6406c37612c838c998078ea8d2252

Don't return null snapshotblockhash values to avoid caller complexity/confusion.

https://github.com/bitcoin/bitcoin/pull/21681/commits/931684b24a89aba884cb18c13fa67ccca339ee8c

validation: fix ActivateSnapshot to use hardcoded nChainTx

This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.

Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.

https://github.com/bitcoin/bitcoin/pull/21681/commits/91d93aac4e3fe6fff5ef492ed152c4d8fa6f2672

validation: remove nchaintx from assumeutxo metadata

This value is no longer used and is instead specified statically
in chainparams. This change means that previously generated
snapshots will no longer be usable.

https://github.com/bitcoin/bitcoin/pull/21582/commits/fa9b74f5ea89624e052934c48391b5076a87ffef

Fix assumeutxo crash due to missing base_blockhash

https://github.com/bitcoin/bitcoin/pull/21584/commits/fae33f98e6a8d5934edbdce2eb8688112eac41a8

Fix assumeutxo crash due to invalid base_blockhash

Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

https://github.com/bitcoin/bitcoin/pull/21585/commits/fa73ce6e653d00824eb68f772fd29b7f8fb93d84

Fix assumeutxo crash due to truncated file

This is a backport of core#19806 [5/8], core#21681 (partial), core#21582 (partial), core#21584 (partial) and core#21585

Notes:

The test related changes from core#21681, core#21582 and core#21584 are in D11241. This is because they depend on a missing (for now) test fixture for malleating the utxo snapshots.

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.Mar 22 2022, 15:09

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 22 2022, 15:09

PiRK requested review of this revision.Mar 22 2022, 15:09

teamcity edited the summary of this revision. (Show Details)Mar 22 2022, 15:09

PiRK added a child revision: D11240: tests: add snapshot activation test.Mar 22 2022, 15:14

rebase after moving the expected circular dependency in the previous commit

Harbormaster completed remote builds in B18593: Diff 32885.Mar 22 2022, 15:26

Harbormaster completed remote builds in B18596: Diff 32888.Mar 22 2022, 15:41

rebase

Harbormaster completed remote builds in B18607: Diff 32907.Mar 23 2022, 09:27

Fabien requested changes to this revision.Mar 23 2022, 09:56

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/blockindex.h
86 ↗	(On Diff #32907)	Probably not
src/test/validation_chainstatemanager_tests.cpp
17 ↗	(On Diff #32907)	Why are all these new inclusions which are not in the source material ?

This revision now requires changes to proceed.Mar 23 2022, 09:56

I think you have a rebase issue in your stack

fix rebase accident, remove irrelevant comment for CBlockIndex::nStatus (the corresponding code was correctly removed while backporting the commit)

Harbormaster completed remote builds in B18608: Diff 32908.Mar 23 2022, 10:26

Fabien requested changes to this revision.Mar 23 2022, 14:29

Fabien added inline comments.

src/blockindex.h
77 ↗	(On Diff #32908)	Is there a missing dependency here ?
src/validation.cpp
6339 ↗	(On Diff #32908)	can be `auto max_secs_to_wait_for_headers = 600s` ?
6348 ↗	(On Diff #32908)	can be `std::this_thread::sleep_for(1s);`?
6408 ↗	(On Diff #32908)	https://github.com/bitcoin/bitcoin/pull/21681

This revision now requires changes to proceed.Mar 23 2022, 14:29

squash with core#21681:

This fixes an oversight from the move of nChainTx from the user-supplied

snapshot metadata into the hardcoded assumeutxo chainparams.

PiRK edited the summary of this revision. (Show Details)Mar 23 2022, 17:30

PiRK mentioned this in D11241: test: Add tests with maleated snapshot data.Mar 23 2022, 17:33

Harbormaster completed remote builds in B18614: Diff 32916.Mar 23 2022, 18:04

You missed the unit test changes to validation_chainstatemanager_tests.cpp from PR21681

This revision now requires changes to proceed.Mar 24 2022, 20:42

PiRK planned changes to this revision.Mar 31 2022, 15:20

squash with bufgixes (see summary for a list of commits)

PiRK edited the summary of this revision. (Show Details)Apr 11 2022, 15:24

Harbormaster completed remote builds in B18781: Diff 33152.Apr 11 2022, 15:37

PiRK edited the summary of this revision. (Show Details)Apr 11 2022, 15:50

Fabien requested changes to this revision.Apr 11 2022, 20:13

Fabien added inline comments.

src/validation.cpp
6084 ↗	(On Diff #33152)	This will cause the build to fail after D11331. You're missing a dependency link between these diffs
6258 ↗	(On Diff #33152)	Fix the layout, and you can remove the pointless comment. Our linter is smarter.