block index: split ResetChainStats from UpdateChainStats
ClosedPublic
Actions

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

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC36faf8a589c2: block index: split ResetChainStats from UpdateChainStats

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

Depends on D17878

Test Plan

ninja all check-all bitcoin-fuzzers

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32863
Build 65211: Build Diff	lint-circular-dependencies · build-without-wallet · build-fuzzer · build-diff · build-debug · build-clang-tidy · build-clang
Build 65210: 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 ↗	(On Diff #53301)	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 ↗	(On Diff #53301)	Or add it to some RPC if it's not available via another method already.
src/node/blockstorage.cpp
279 ↗	(On Diff #53301)	This is a change in behavior ? Is that intended to be in this diff ?
src/validation.cpp
4123 ↗	(On Diff #53301)	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 ↗	(On Diff #53301)	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 ↗	(On Diff #53301)	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.

rebase on D17878

Harbormaster completed remote builds in B32863: Diff 53321.Wed, Apr 2, 08:13

Build Bitcoin ABC Diffs / Diff Testing (build-fuzzer) failed.

Tail of the build log:

[366/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_interpreter.cpp.o
[367/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_ops.cpp.o
[368/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sign.cpp.o
[369/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/scriptnum_ops.cpp.o
[370/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ecdsa_signature_parse_der_lax.cpp.o
[371/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/secp256k1_ec_seckey_import_export_der.cpp.o
[372/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/signature_checker.cpp.o
[373/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/connman.cpp.o
[374/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/span.cpp.o
[375/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/spanparsing.cpp.o
[376/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/string.cpp.o
[377/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/strprintf.cpp.o
[378/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/system.cpp.o
[379/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/timedata.cpp.o
[380/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/cuckoocache.cpp.o
[381/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_in.cpp.o
[382/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/tx_out.cpp.o
[383/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/txrequest.cpp.o
[384/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/util.cpp.o
[385/424] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[386/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[387/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[388/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[389/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[390/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[391/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[392/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/pow.cpp.o
[393/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[394/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/interfaces.cpp.o
[395/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[396/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/receive.cpp.o
[397/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[398/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/encrypt.cpp.o
[399/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/signmessage.cpp.o
[400/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/coins_view.cpp.o
[401/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/util.cpp.o
[402/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[403/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[404/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/spend.cpp.o
[405/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[406/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/transaction.cpp.o
[407/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[408/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[409/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/load_external_block_file.cpp.o
[410/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/net.cpp.o
[411/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_message.cpp.o
[412/424] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[413/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/socks5.cpp.o
[414/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/process_messages.cpp.o
[415/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/script_sigcache.cpp.o
[416/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/transaction.cpp.o
[417/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o
[418/424] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[419/424] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/deserialize.cpp.o
[420/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpc/backup.cpp.o
[421/424] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[422/424] Linking CXX static library src/wallet/libwallet.a
[423/424] Linking CXX static library src/libserver.a
ninja: build stopped: cannot make progress due to previous errors.
Build build-fuzzer failed with exit code 1

rebase

Harbormaster completed remote builds in B32866: Diff 53325.Wed, Apr 2, 09:54

Fabien accepted this revision.Wed, Apr 2, 14:42

This revision is now accepted and ready to land.Wed, Apr 2, 14:42

Closed by commit rABC36faf8a589c2: block index: split ResetChainStats from UpdateChainStats (authored by PiRK). · Explain WhyWed, Apr 2, 19:47

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC36faf8a589c2: block index: split ResetChainStats from UpdateChainStats.