block index: split ResetChainStats from UpdateChainStats
Needs RevisionPublic
Actions

Authored by PiRK on Fri, Mar 28, 21:28.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Summary

In D6308 the tasks of resetting nChainTx to 0 (for new blocks that don't yet have transactions downloaded) or to it's correct value (for connected blocks) was factored into a single UpdateChainStats function.

In a following backport of https://github.com/bitcoin/bitcoin/pull/29370/commits/ef29c8b662309a438121a83f27fd7bdd1779700c, I will have to make the resetting in ChainstateManager::ReceivedBlockTransactions conditional on the block not being a pruned block which is being downloaded again and not being the assumeutxo snapshot's base block. In BlockManager::LoadBlockIndex the resetting will be done under slighty different conditions (having parent blocks with missing transactions and not being the snapshot's base block)

Prepare for this change of behavior by splitting the resetting code from the updating code, and introducing a MaybeResetChainStats wrapper that will check for the conditions before maybe calling ResetChainStats

Test Plan

ninja all check-all bitcoin-fuzzers

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32847
Build 65179: Build Diff	build-without-wallet · build-clang-tidy · build-debug · build-clang · lint-circular-dependencies · build-diff · build-fuzzer
Build 65178: arc lint + arc unit

Event Timeline

PiRK created this revision.Fri, Mar 28, 21:28

Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Mar 28, 21:28

PiRK requested review of this revision.Fri, Mar 28, 21:28

PiRK added inline comments.Fri, Mar 28, 21:34

src/blockindex.cpp
48	nChainSize seems to be completely unused. Not sure what it was used for in the past. Maybe we can remove it?

PiRK added a child revision: D17873: assumeutxo: Get rid of faked nTx and nChainTx values.Fri, Mar 28, 21:37

Harbormaster completed remote builds in B32847: Diff 53301.Fri, Mar 28, 21:40

Fabien requested changes to this revision.Mon, Mar 31, 13:38

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/blockindex.cpp
48	Or add it to some RPC if it's not available via another method already.
src/node/blockstorage.cpp
279	This is a change in behavior ? Is that intended to be in this diff ?
src/validation.cpp
4123	Same question

This revision now requires changes to proceed.Mon, Mar 31, 13:38

PiRK added inline comments.Mon, Mar 31, 15:35

src/node/blockstorage.cpp
279	I don't think it is a change in behavior. If `pindex->UpdateChainStats()` returns True, we don't arrive here. Otherwise, we know that `UpdateChainStats` didn't set `nchaintx` and that previously it would have set it to 0, so we set it to 0.
src/validation.cpp
4123	same answer, except the `UpdateChainStats()` tests are done below. Either `UpdateChainStats()` will overwrite the value with the correct one, or else it would have set it to 0 anyway prior to this diff.