validation: UpdateTip/CheckBlockIndex assumeutxo support

jamesob commented at 7:39 pm on March 24, 2021: member

This is part of the assumeutxo project (parent PR: #15606)

Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying g_best_block behavior (previously untested at the unit level) and various changes necessary for running and testing ProcessNewBlock()-like behavior on the background validation chainstate.

This changeset introduces a new block index nStatus flag called BLOCK_ASSUMED_VALID, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using BLOCK_VALID_* flags for snapshot chain block entries, and preserves the original meaning of those flags.

Note: this PR previously incorporated changes to LoadBlockIndex() and RewindBlockIndex() as noted in Russ’ comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.

fanquake added the label Validation on Mar 24, 2021

laanwj added this to the "Chasing Concept ACK" column in a project

laanwj moved this from the "Chasing Concept ACK" to the "Blockers" column in a project

DrahtBot commented at 9:36 pm on March 24, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22937 (refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method by ryanofsky)
#22932 (consensus: require CBlockIndex::GetBlockPos() to hold mutex cs_main by jonatack)
#22564 ([WIP] refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)
#21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
#19790 (Flag when blocks have had their scripts checked instead of skipped by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky commented at 1:41 am on March 29, 2021: member

re: #21526#issue-600020266

Don’t run UpdateTip for background validation chainstates.

Does this prevent bugs in the future when there can be a background validation chainstate? It would help to say what would happen without this change. It could also be helpful to say more about the motivation for this set of changes, and maybe summarize the current state of things for forgetful people like me who somewhat lost the thread on assumeutxo :tropical_fish:

Modify LoadBlockIndex() to populate the “faked” nChainTx value when using a yet-unvalidated snapshot chainstate. This allows assumeutxo sync-to-tip to offer normal progress estimates on the basis of having nChainTx without the associated block data.

IIUC, the faking isn’t new, and the value was faked before this change, but startup code is now fixed so things work if the node is restarted before the assumeutxo snapshot is validated? Again could help to say what bad things would happen without this.

In LoadBlockIndex(), ensure that assumed-valid CBlockIndex entries aren’t added to the background validation chainstate’s setBlockIndexCandidates (since we want that chainstate to actually validate those entries at some point).

Wondering also what bad things happen without this change, and why it’s in the same commit as the last change. Is it just that both changes affect the LoadBlockIndex() function?

Prevent RewindBlockIndex() from rewinding past the assumeutxo base on the snapshot chainstate.

I’m a little confused by this code (also the existing pruning code here). It says the purpose of the break is to prevent the DisconnectTip call immediately below from failing. But don’t we want to fail in these cases? It seems like now if rewind isn’t possible, this just flushes some things and returns true. I’m also wondering if there’s a reason this change is in the same commit as the other changes, which seem tangentially related.

Also, there’s no test coverage here. Maybe is the best we can do when adding dead code to older functions that weren’t really designed to be unit-tested. But are there at least future tests in #15606 that would fail without these different fixes?

Thanks for any clarifications, and great to see assumeutxo work moving along again!

jamesob commented at 3:22 pm on March 30, 2021: member

@ryanofsky thanks for the thorough look! I agree that I should take a look at writing unittests for these changes and do so if practical. Also appreciate your questions on the logic - I’ll get together some responses for those and either adjust the code (especially since RewindBlockIndex() is probably going away in #21009) or add some motivating documentation.

fanquake removed this from the "Blockers" column in a project

fanquake added this to the "In progress" column in a project

jamesob force-pushed on Apr 8, 2021

jamesob renamed this:
~~validation: misc. multi-chainstate preparation~~
validation: UpdateTip/CheckBlockIndex assumeutxo support
on Apr 8, 2021

jamesob force-pushed on Apr 8, 2021

jamesob commented at 2:58 pm on April 8, 2021: member

Okay, after taking @ryanofsky’s advice, I sat down to write some unittests for the chainstate-dependent UpdateTip() behavior. It turns out that in the process, trying to get those tests to work uncovered a number of changes that needed to be made to other test-related stuff, including slightly modifying CheckBlockIndex() to account for the specific nature of the background validation chainstate. Additionally I’ve added some unittesting utils that help with testing multiple chainstates.

Because that’s dragged in so much new code, I’ve narrowed the scope of this PR to the above and I’ll deal with the other BlockIndex-related changes in a follow-up PR.

So all the logic changes in this are now tested (and this actually adds unit-level coverage of the UpdateTip() behvaior that we didn’t have previously). So hopefully that entices people to review, because testing future assumeutxo-related changes will probably depend on some of the unittest utils added here.

Also worth mentioning is that I had to remove some of the active chainstate assertions that @dongcarl added for the sake of his work in #20158. This should all be okay because those assertions are actually made obsolete by assumeutxo functionality.

jamesob force-pushed on Apr 12, 2021

ryanofsky commented at 9:54 pm on April 14, 2021: member

Started review (will update list below with progress). Thanks for splitting this up!

b9f805b703c12a51192069144b1d7fe924988896 validation: change UpdateTip for multiple chainstates (1/6)
d3cec2a5b3febd091b8002e39f8b7431b7e99c4a validation: fix CheckBlockIndex for multiple chainstates (2/6)
0fd884d46f49a63eeee61d56d7ef7634c02c1617 move-only: unittest: add test/util/chainstate.h (3/6)
6e5871c0c7b7d7b27b56df4f2c0705921025a6d5 test: refactor: separate CreateBlock in TestChain100Setup (4/6)
d7fc5c342010028855634d7c87b473b5af4484a3 refactor: remove assertions preventing multiple chainstates (5/6)
3f1346a0178afef20db3599cc545dbc54200d635 test: validation: add unittest for UpdateTip behavior (6/6)

in src/validation.cpp:4686 in d3cec2a5b3 outdated

4836+        assert(forward.size() == m_blockman.m_block_index.size());
4837+    } else {
4838+        // For the background validation chainstate, don't consider the full block tree -
4839+        // only consider headers on the IBD chain. Otherwise, we will fail assertions
4840+        // related to `setBlockIndexCandidates` below, which corresponds to this subset of
4841+        // the block tree for this chainstate.

ryanofsky commented at 6:43 pm on April 16, 2021:

In commit “validation: fix CheckBlockIndex for multiple chainstates” (d3cec2a5b3febd091b8002e39f8b7431b7e99c4a)

This is ok, but it seems to avoid other checks below besides the setBlockIndexCandidates checks. And if more checks are added below in the future they could be bypassed unnecessarily. I wonder if it might be more straightforward and futureproof just to add && is_active_chain or || !is_active_chain to the specific assertions below which are failing instead of pruning branches off this tree.

jamesob commented at 6:36 pm on April 23, 2021:

Good call, fixed.

in src/test/util/setup_common.h:135 in 6e5871c0c7 outdated

129+     * scriptPubKey. If no chainstate is specified, default to the active.
130+     */
131+    CBlock CreateBlock(
132+        const std::vector<CMutableTransaction>& txns,
133+        const CScript& scriptPubKey,
134+        CChainState* chainstate = nullptr);

ryanofsky commented at 6:59 pm on April 16, 2021:

In commit “test: refactor: separate CreateBlock in TestChain100Setup” (6e5871c0c7b7d7b27b56df4f2c0705921025a6d5)

IMO, it is reasonable for CreateAndProcessBlock to have a default-active chainstate pointer for backwards compatibility, but it might be better for this new CreateBlock method to take an explicit CChainState& reference for more clarity in test code and to prevent mistakes forgetting to specify it.

jamesob commented at 6:36 pm on April 23, 2021:

Fixed.

in src/test/validation_chainstate_tests.cpp:133 in 3f1346a017 outdated

127+        BOOST_CHECK(checked);
128+        bool accepted = background_cs.AcceptBlock(
129+            pblock, state, chainparams, &pindex, true, nullptr, &newblock);
130+        BOOST_CHECK(accepted);
131+    }
132+    bool block_added = background_cs.ActivateBestChain(state, chainparams, pblock);

ryanofsky commented at 7:21 pm on April 16, 2021:

In commit “test: validation: add unittest for UpdateTip behavior” (3f1346a0178afef20db3599cc545dbc54200d635)

Since this test is designed to check UpdateTip background behavior, but the test doesn’t call UpdateTip directly, it may be good to add a comment saying this line is what calls UpdateTip.

Also could maybe a simple check could be added here to verify the new background tip hash?

Also curious if there’s an existing test (or a planned one) to cover the background chain catching up to the activated snapshot.

jamesob commented at 6:37 pm on April 23, 2021:

Done.

Also curious if there’s an existing test (or a planned one) to cover the background chain catching up to the activated snapshot.

Yes; I’ll look at writing a test for this sometime today in addition to some chainstate-concurrency torture tests.

ryanofsky approved

ryanofsky commented at 7:27 pm on April 16, 2021: member

Code review ACK 3f1346a0178afef20db3599cc545dbc54200d635. This is great! Changes are small and easy to understand and new test is surprisingly straightforward, not requiring too much setup.

DrahtBot added the label Needs rebase on Apr 17, 2021

jamesob force-pushed on Apr 23, 2021

jamesob commented at 6:38 pm on April 23, 2021: member

Thanks for the great feedback, @ryanofsky!

DrahtBot removed the label Needs rebase on Apr 23, 2021

DrahtBot commented at 9:32 am on May 3, 2021: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

in src/test/util/setup_common.cpp:243 in 6d5d09634a outdated

233@@ -234,21 +234,38 @@ void TestChain100Setup::mineBlocks(int num_blocks)
234     }
235 }
236 
237-CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns, const CScript& scriptPubKey)
238+CBlock TestChain100Setup::CreateBlock(
239+    const std::vector<CMutableTransaction>& txns,
240+    const CScript& scriptPubKey,
241+    CChainState* chainstate)

ryanofsky commented at 0:16 am on May 4, 2021:

In commit “test: refactor: separate CreateBlock in TestChain100Setup” (6d5d09634ac354c16c298c83d7b699b2147a98b3)

Would be nice to make the last argument a CChainState& reference instead of a pointer since it isn’t allowed to be null (and it’s a new method so there’s no backward compatility concern)

ryanofsky commented at 3:35 pm on June 21, 2021:

In commit “test: refactor: separate CreateBlock in TestChain100Setup” (e061837697ed8ebf8d5443478829b48f3fe0e8db)

Would be nice to make the last argument a CChainState& reference instead of a pointer since it isn’t allowed to be null (and it’s a new method so there’s no backward compatility concern)

(This is still applicable, in case the PR gets another update)

ryanofsky approved

ryanofsky commented at 0:30 am on May 4, 2021: member

Code review ACK eb9623d690e6f390a5f6a8b30763374b72aee282. Since last review just rebase and various suggestions applied, biggest one updating CheckBlockIndex in a different way where I think it does some more checks than the previous change and some fewer, but is now a simpler change. Overall this looks good and please ignore my new suggestion unless something needs to be updated for another reason.

Recommend this PR for any reviewer looking for a simple review of some critical code. The first commit’s the only code commit that’s even a little complicated and the other commits are either test updates or trivial code changes.

in src/validation.cpp:2288 in 23c2d26d42 outdated

2354@@ -2321,8 +2355,7 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
2355     }
2356 
2357     bilingual_str warning_messages;
2358-    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));

Sjors commented at 5:36 pm on May 5, 2021:

23c2d26d424e5a8b523f7210653958eff10db0f2: given the above return, you don’t have to drop this assert

in src/validation.cpp:2251 in 23c2d26d42 outdated

2319+static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const CChainParams& chainParams, CChainState& chainstate)
2320     EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
2321 {
2322+    auto& func_name = __func__;
2323+    auto& coins_view = chainstate.CoinsTip();
2324+    auto log_progress = [pindexNew, &func_name, &coins_view, &chainParams](

Sjors commented at 5:46 pm on May 5, 2021:

0validation.cpp:2321:38: warning: lambda capture 'func_name' is not required to be captured for this use [-Wunused-lambda-capture]
1    auto log_progress = [pindexNew, &func_name, &coins_view, &chainParams](
2                                  ~~~^~~~~~~~~
31 warning generated.

jamesob commented at 8:14 am on May 6, 2021:

Fails CI when addressing this warning.

in src/miner.cpp:127 in 23b5ebd02e outdated

115@@ -116,7 +116,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
116     pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
117 
118     LOCK2(cs_main, m_mempool.cs);
119-    assert(std::addressof(*::ChainActive().Tip()) == std::addressof(*m_chainstate.m_chain.Tip()));

Sjors commented at 6:11 pm on May 5, 2021:

23b5ebd02e584c7bb982ccbc28bbabd927b9c57e why would CreateNewBlock ever be called on the background chainstate?

jamesob commented at 8:14 am on May 6, 2021:

Gets called during unittests.

Sjors commented at 9:18 am on May 6, 2021:

But why?

jamesob commented at 5:01 pm on May 12, 2021:

Because (unlike in actual snapshot usage) we don’t have pre-prepared snapshots to use from within unittests, we have to use the same chainstate objects that we ultimately test on to generate snapshots for use during the test. As a result, the background validation chainstate is already at the snapshot base block, so in order to test appending to the bg chainstate, we have to call CreateNewBlock with it.

While I agree this usage isn’t realistic, we don’t have great alternatives within tests. I guess another option would involve using a temporary chainstate to generate the snapshot and then loading it into a new chainstatemanager object, but that sounds even hairier than what we’re doing here, and much more code.

in src/txmempool.cpp:641 in 23b5ebd02e outdated

634@@ -635,7 +635,6 @@ void CTxMemPool::check(CChainState& active_chainstate) const
635     uint64_t innerUsage = 0;
636 
637     CCoinsViewCache& active_coins_tip = active_chainstate.CoinsTip();
638-    assert(std::addressof(::ChainstateActive().CoinsTip()) == std::addressof(active_coins_tip)); // TODO: REVIEW-ONLY, REMOVE IN FUTURE COMMIT

Sjors commented at 6:14 pm on May 5, 2021:

23b5ebd02e584c7bb982ccbc28bbabd927b9c57e: I thought the background chain didn’t have a mempool? Or it had a dummy mempool. I guess it’s fine to remove this assert though.

jamesob commented at 8:14 am on May 6, 2021:

Same as above; called during unittests.

Sjors commented at 6:18 pm on May 5, 2021: member

Mostly happy with eb9623d690e6f390a5f6a8b30763374b72aee282, but I’m a bit confused about some code paths that I thought shouldn’t be possible on the background chainstate.

I’d like to do some testing with the parent PR, if you don’t mind rebasing that.

ryanofsky commented at 9:41 pm on May 24, 2021: member

Would be good to see this PR move ahead. Has 2 ACKs. There are a few simple code changes, and most of the PR is adding tests. People interested in assumeutxo should take a look.

benthecarman commented at 8:05 am on May 27, 2021: contributor

tACK eb9623d690e6f390a5f6a8b30763374b72aee282

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Jun 1, 2021

jamesob force-pushed on Jun 10, 2021

jamesob commented at 1:13 pm on June 10, 2021: member

Rebased to fix small conflict. au.multi-chainstates.pt1.8 -> au.multi-chainstates.pt1.9

 0$ git range-diff master au.multi-chainstates.pt1.8 au.multi-chainstates.pt1.9
 1
 21:  23c2d26d4 ! 1:  28f800ba6 validation: change UpdateTip for multiple chainstates
 3    @@ -91,5 +91,5 @@
 4      extern std::condition_variable g_best_block_cv;
 5     +/** Used to notify getblocktemplate RPC of new tips. */
 6      extern uint256 g_best_block;
 7    - extern std::atomic_bool fImporting;
 8    - extern std::atomic_bool fReindex;
 9    + /** Whether there are dedicated script-checking threads running.
10    +  * False indicates all script checking is done on the main threadMessageHandler thread.
112:  4600865b4 = 2:  ea5fe5a50 validation: fix CheckBlockIndex for multiple chainstates
123:  5fa0b0437 = 3:  f049daf72 move-only: unittest: add test/util/chainstate.h
134:  6d5d09634 ! 4:  87e872c5b test: refactor: separate CreateBlock in TestChain100Setup
14    @@ -26,11 +26,7 @@
15
16          Assert(block.vtx.size() == 1);
17          for (const CMutableTransaction& tx : txns) {
18    -         block.vtx.push_back(MakeTransactionRef(tx));
19    -     }
20    --    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return g_chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock));
21    -+    CBlockIndex* prev_block = WITH_LOCK(::cs_main, return chainstate->m_blockman.LookupBlockIndex(block.hashPrevBlock));
22    -     RegenerateCommitments(block, prev_block);
23    +@@
24
25          while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
26
275:  23b5ebd02 = 5:  fc73625bd refactor: remove assertions preventing multiple chainstates
286:  eb9623d69 = 6:  adb0e5a67 test: validation: add unittest for UpdateTip behavior

DrahtBot removed the label Needs rebase on Jun 10, 2021

DrahtBot added the label Needs rebase on Jun 12, 2021

jamesob force-pushed on Jun 15, 2021

jamesob commented at 1:28 pm on June 15, 2021: member

au.multi-chainstates.pt1.9 -> au.multi-chainstates.pt1.10

I’ve rebased on top of master (made necessary after the merge of #21866 :tada:). This branch now includes an additional commit (https://github.com/bitcoin/bitcoin/pull/21526/commits/acb5215743de11df5aef519260521e2b17c5b40d) necessitated by the fact that the chainstate manager is no longer a global; the commit attaches a chainman reference to each owned chainstate instance. The alternative would be to change a whole bunch of function signatures and thread a ChainstateManager argument deep through call stacks. I think this approach makes more sense.

 0$ git range-diff master au.multi-chainstates.pt1.9 au.multi-chainstates.pt1.10
 1
 2-:  ---------- > 1:  acb5215743 validation: add chainman ref to CChainState
 31:  28f800ba60 ! 2:  0589bf841f validation: change UpdateTip for multiple chainstates
 4    @@ -44,7 +44,7 @@
 5     +
 6     +    // The contents of this function are either not relevant if we're not
 7     +    // given the active chainstate, so return otherwise.
 8    -+    if (&chainstate != &::g_chainman.ActiveChainstate()) {
 9    ++    if (&chainstate != &chainstate.m_chainman.ActiveChainstate()) {
10     +        // Only log every so often so that we don't bury log messages at the tip.
11     +        constexpr int BACKGROUND_LOG_INTERVAL = 2000;
12     +        if (pindexNew->nHeight % BACKGROUND_LOG_INTERVAL == 0) {
13    @@ -60,7 +60,6 @@
14          }
15
16          bilingual_str warning_messages;
17    --    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
18     -    if (!active_chainstate.IsInitialBlockDownload()) {
19     +    if (!chainstate.IsInitialBlockDownload()) {
20              const CBlockIndex* pindex = pindexNew;
21    @@ -70,7 +69,6 @@
22                  }
23              }
24          }
25    --    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
26     -    LogPrintf("%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n", __func__,
27     -      pindexNew->GetBlockHash().ToString(), pindexNew->nHeight, pindexNew->nVersion,
28     -      log(pindexNew->nChainWork.getdouble())/log(2.0), (unsigned long)pindexNew->nChainTx,
292:  ea5fe5a504 ! 3:  929de6413c validation: fix CheckBlockIndex for multiple chainstates
30    @@ -15,7 +15,7 @@
31
32          LOCK(cs_main);
33
34    -+    bool is_active_chain = this == &::g_chainman.ActiveChainstate();
35    ++    bool is_active_chain = this == &m_chainman.ActiveChainstate();
36     +
37          // During a reindex, we read the genesis block and call CheckBlockIndex before ActivateBestChain,
38          // so we have the genesis block in m_blockman.m_block_index but no active chain. (A few of the
393:  f049daf72a = 4:  3a6899f468 move-only: unittest: add test/util/chainstate.h
404:  87e872c5b2 ! 5:  b6ddc34706 test: refactor: separate CreateBlock in TestChain100Setup
41    @@ -21,7 +21,7 @@
42      {
43          const CChainParams& chainparams = Params();
44          CTxMemPool empty_pool;
45    --    CBlock block = BlockAssembler(::ChainstateActive(), empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
46    +-    CBlock block = BlockAssembler(m_node.chainman->ActiveChainstate(), empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
47     +    CBlock block = BlockAssembler(*chainstate, empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
48
49          Assert(block.vtx.size() == 1);
505:  fc73625bd3 < -:  ---------- refactor: remove assertions preventing multiple chainstates
516:  adb0e5a67e = 6:  f14c68ac6d test: validation: add unittest for UpdateTip behavior

DrahtBot removed the label Needs rebase on Jun 15, 2021

jamesob force-pushed on Jun 15, 2021

ryanofsky approved

ryanofsky commented at 7:52 pm on June 16, 2021: member

Code review ACK ede45e2036891fda851fe8c4e8363e5602c7ef08. No changes since last review other than rebase and new commit e5e1d49ccf390bca49f66a0650c84c7c51f3af04 giving CChainState objects a reference back to ChainstateManager. This seems reasonable and preferable to the alternative of adding ChainstateManager& parameters to UpdateTip() and CChainState::CheckBlockIndex() and functions and all the functions calling these functions.

jamesob force-pushed on Jun 17, 2021

jamesob commented at 9:34 pm on June 17, 2021: member

au.multi-chainstates.pt1.11 -> au.multi-chainstates.pt1.12

The functional test I recently added in the parent branch (https://github.com/bitcoin/bitcoin/pull/15606/commits/aa65b21d79d9cc0d79925879bc62e1c240a1cf59) has uncovered some cases I hadn’t previously considered in CheckBlockIndex(), so I’ve pushed fixes for those, as well as some corequisite ChainstateManager methods for easily getting the snapshot height.

  0$ git range-diff master au.multi-chainstates.pt1.11 au.multi-chainstates.pt1.12
  1
  21:  e5e1d49cc = 1:  e5e1d49cc validation: add chainman ref to CChainState
  32:  4a836f708 = 2:  4cf9de9d7 validation: change UpdateTip for multiple chainstates
  43:  7377caacd ! 3:  7ff4c234e validation: fix CheckBlockIndex for multiple chainstates
  5    @@ -16,6 +16,7 @@
  6          LOCK(cs_main);
  7
  8     +    bool is_active_chain = this == &m_chainman.ActiveChainstate();
  9    ++    int snapshot_height = m_chainman.GetSnapshotHeight().value_or(-1);
 10     +
 11          // During a reindex, we read the genesis block and call CheckBlockIndex before ActivateBestChain,
 12          // so we have the genesis block in m_blockman.m_block_index but no active chain. (A few of the
 13    @@ -28,6 +29,29 @@
 14          for (const std::pair<const uint256, CBlockIndex*>& entry : m_blockman.m_block_index) {
 15              forward.insert(std::make_pair(entry.second->pprev, entry.second));
 16          }
 17    +@@
 18    +     while (pindex != nullptr) {
 19    +         nNodes++;
 20    +         if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
 21    +-        if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex;
 22    +-        if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex;
 23    ++        // Don't start being rigorous about missing block data if we're within a utxo snapshot
 24    ++        if (pindex->nHeight > snapshot_height) {
 25    ++            if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex;
 26    ++            if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex;
 27    ++        }
 28    +         if (pindex->pprev != nullptr && pindexFirstNotTreeValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TREE) pindexFirstNotTreeValid = pindex;
 29    +         if (pindex->pprev != nullptr && pindexFirstNotTransactionsValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS) pindexFirstNotTransactionsValid = pindex;
 30    +         if (pindex->pprev != nullptr && pindexFirstNotChainValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_CHAIN) pindexFirstNotChainValid = pindex;
 31    +@@
 32    +         if (!pindex->HaveTxsDownloaded()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock)
 33    +         // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred).
 34    +         // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred.
 35    +-        if (!fHavePruned) {
 36    ++        if (!fHavePruned && (pindex->nHeight > snapshot_height)) {
 37    +             // If we've never pruned, then HAVE_DATA should be equivalent to nTx > 0
 38    +             assert(!(pindex->nStatus & BLOCK_HAVE_DATA) == (pindex->nTx == 0));
 39    +             assert(pindexFirstMissing == pindexFirstNeverProcessed);
 40     @@
 41                      // is valid and we have all data for its parents, it must be in
 42                      // setBlockIndexCandidates.  m_chain.Tip() must also be there
 43    @@ -53,3 +77,52 @@
 44              }
 45              // Check whether this block is in m_blocks_unlinked.
 46              std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> rangeUnlinked = m_blockman.m_blocks_unlinked.equal_range(pindex->pprev);
 47    +@@
 48    +         // Fake nChainTx so that GuessVerificationProgress reports accurately
 49    +         index->nChainTx = index->pprev ? index->pprev->nChainTx + index->nTx : 1;
 50    +
 51    ++        // Since this block is assumed-valid, set nStatus accordingly.
 52    ++        index->nStatus |= BLOCK_VALID_MASK;
 53    ++
 54    +         // Fake BLOCK_OPT_WITNESS so that CChainState::NeedsRedownload()
 55    +         // won't ask to rewind the entire assumed-valid chain on startup.
 56    +         if (index->pprev && ::IsWitnessEnabled(index->pprev, ::Params().GetConsensus())) {
 57    +@@
 58    +         }
 59    +     }
 60    + }
 61    ++
 62    ++std::optional<CBlockIndex*> ChainstateManager::GetSnapshotBaseBlock()
 63    ++{
 64    ++    auto blockhash_op = SnapshotBlockhash();
 65    ++    if (!blockhash_op) {
 66    ++        return std::nullopt;
 67    ++    }
 68    ++    return m_blockman.LookupBlockIndex(*blockhash_op);
 69    ++}
 70    ++
 71    ++std::optional<int> ChainstateManager::GetSnapshotHeight()
 72    ++{
 73    ++    std::optional<CBlockIndex*> base = this->GetSnapshotBaseBlock();
 74    ++    if (!base) {
 75    ++        return std::nullopt;
 76    ++    }
 77    ++    return (*base)->nHeight;
 78    ++}
 79    +
 80    + diff --git a/src/validation.h b/src/validation.h
 81    + --- a/src/validation.h
 82    + +++ b/src/validation.h
 83    +@@
 84    +     //! ResizeCoinsCaches() as needed.
 85    +     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 86    +
 87    ++    //! [@returns](/bitcoin-bitcoin/contributor/returns/) height at which the active UTXO snapshot was taken, if applicable.
 88    ++    std::optional<CBlockIndex*> GetSnapshotBaseBlock() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 89    ++
 90    ++    //! [@returns](/bitcoin-bitcoin/contributor/returns/) height at which the active UTXO snapshot was taken, if a snapshot is being used.
 91    ++    std::optional<int> GetSnapshotHeight() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 92    ++
 93    +     ~ChainstateManager() {
 94    +         LOCK(::cs_main);
 95    +         UnloadBlockIndex(/* mempool */ nullptr, *this);
 964:  d3e949fb8 ! 4:  c9d332d03 move-only: unittest: add test/util/chainstate.h
 97    @@ -33,6 +33,7 @@
 98     +#include <fs.h>
 99     +#include <node/context.h>
100     +#include <node/utxo_snapshot.h>
101    ++#include <rpc/blockchain.h>
102     +#include <validation.h>
103     +
104     +#include <univalue.h>
1055:  b75e71c7c = 5:  d20161e9c test: refactor: separate CreateBlock in TestChain100Setup
1066:  ede45e203 = 6:  881150900 test: validation: add unittest for UpdateTip behavior

jamesob force-pushed on Jun 18, 2021

jamesob commented at 7:05 pm on June 18, 2021: member

au.multi-chainstates.pt1.12 -> au.multi-chainstates.pt1.13

Argh sorry for the churn; botched rebase pushed some old code that caused segfault in unittests. Fixed.

 0$ git range-diff master au.multi-chainstates.pt1.12 au.multi-chainstates.pt1.13
 1
 21:  e5e1d49cc = 1:  e5e1d49cc validation: add chainman ref to CChainState
 32:  4cf9de9d7 = 2:  4cf9de9d7 validation: change UpdateTip for multiple chainstates
 43:  7ff4c234e ! 3:  cf7afe34c validation: fix CheckBlockIndex for multiple chainstates
 5    @@ -96,9 +96,13 @@
 6     +{
 7     +    auto blockhash_op = SnapshotBlockhash();
 8     +    if (!blockhash_op) {
 9    -+        return std::nullopt;
10    ++  return std::nullopt;
11    ++    }
12    ++    CBlockIndex* pindex = m_blockman.LookupBlockIndex(*blockhash_op);
13    ++    if (pindex == nullptr) {
14    ++  return std::nullopt;
15     +    }
16    -+    return m_blockman.LookupBlockIndex(*blockhash_op);
17    ++    return pindex;
18     +}
19     +
20     +std::optional<int> ChainstateManager::GetSnapshotHeight()
214:  c9d332d03 = 4:  c1c1f95a0 move-only: unittest: add test/util/chainstate.h
225:  d20161e9c = 5:  1c4d35b06 test: refactor: separate CreateBlock in TestChain100Setup
236:  881150900 = 6:  5a3ed1ae0 test: validation: add unittest for UpdateTip behavior

benthecarman approved

benthecarman commented at 10:41 pm on June 18, 2021: contributor

utACK 5a3ed1ae09cefe030662bf2793bbacec056f0523

rebase looks good

MarcoFalke commented at 9:28 am on June 21, 2021: member

0This diff appears to have added new lines with tab characters instead of spaces.
1The following changes were suspected:
2
3diff --git a/src/validation.cpp b/src/validation.cpp
4@@ -5083,0 +5136,22 @@ void ChainstateManager::MaybeRebalanceCaches()
5+	return std::nullopt;
6+	return std::nullopt;
7^---- failure generated from test/lint/lint-whitespace.sh

jamesob force-pushed on Jun 21, 2021

jamesob commented at 1:43 pm on June 21, 2021: member

au.multi-chainstates.pt1.13 -> au.multi-chainstates.pt1.14

Pushed whitespace fix; sorry for the holdup.

 0$ git range-diff master au.multi-chainstates.pt1.13 au.multi-chainstates.pt1.14
 1
 21:  e5e1d49cc = 1:  e5e1d49cc validation: add chainman ref to CChainState
 32:  4cf9de9d7 = 2:  4cf9de9d7 validation: change UpdateTip for multiple chainstates
 43:  cf7afe34c ! 3:  0dd5757fd validation: fix CheckBlockIndex for multiple chainstates
 5    @@ -96,11 +96,11 @@
 6     +{
 7     +    auto blockhash_op = SnapshotBlockhash();
 8     +    if (!blockhash_op) {
 9    -+  return std::nullopt;
10    ++        return std::nullopt;
11     +    }
12     +    CBlockIndex* pindex = m_blockman.LookupBlockIndex(*blockhash_op);
13     +    if (pindex == nullptr) {
14    -+  return std::nullopt;
15    ++        return std::nullopt;
16     +    }
17     +    return pindex;
18     +}
194:  c1c1f95a0 = 4:  c7b47bf9c move-only: unittest: add test/util/chainstate.h
205:  1c4d35b06 = 5:  e06183769 test: refactor: separate CreateBlock in TestChain100Setup
216:  5a3ed1ae0 = 6:  b6dffada1 test: validation: add unittest for UpdateTip behavior

in src/validation.cpp:4352 in 0dd5757fd7 outdated

4378@@ -4375,8 +4379,11 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4379     while (pindex != nullptr) {
4380         nNodes++;
4381         if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
4382-        if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex;
4383-        if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex;
4384+        // Don't start being rigorous about missing block data if we're within a utxo snapshot

ryanofsky commented at 1:55 pm on June 21, 2021:

In commit “validation: fix CheckBlockIndex for multiple chainstates” (0dd5757fd7c5b1d151abe45843711780a4b3885b)

Commit message could be updated. It’s still only referring to setBlockIndexCandidates checks, but commit was later extended to skip more checks #21526 (comment)

in src/validation.h:1032 in 0dd5757fd7 outdated

1023@@ -1024,6 +1024,12 @@ class ChainstateManager
1024     //! ResizeCoinsCaches() as needed.
1025     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1026 
1027+    //! @returns height at which the active UTXO snapshot was taken, if applicable.
1028+    std::optional<CBlockIndex*> GetSnapshotBaseBlock() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

ryanofsky commented at 3:28 pm on June 21, 2021:

In commit “validation: fix CheckBlockIndex for multiple chainstates” (0dd5757fd7c5b1d151abe45843711780a4b3885b)

Not very important since this function is only called one place, but it seems like it would be nicer to just return CBlockIndex* instead of std::optional<CBlockIndex*>. This way callers don’t need to double-dereference, and can just check whether this is null or not null, instead having to check for three states (nullopt, nullptr, or block), or having to ignore the nullptr case (which I assume is not currently possible)

JeremyRubin commented at 7:44 pm on July 17, 2021:

I was going to suggest changing to std::optional<CBlockIndex&> since the inner type is not nullable. If you want rebindable, reference_wrapper.

ryanofsky commented at 9:44 pm on July 19, 2021:

In commit “validation: fix CheckBlockIndex for multiple chainstates” (d7804383748e85bda1be40cf0782a3ca2e457066)

re: #21526 (review)

I was going to suggest changing to std::optional<CBlockIndex&> since the inner type is not nullable. If you want rebindable, reference_wrapper.

I’m probably missing a subtlety but I don’t know what reason you’d prefer optional<T&> to T*. It seems like just reinventing a pointer without using a pointer.

ryanofsky approved

ryanofsky commented at 3:47 pm on June 21, 2021: member

Code review ACK b6dffada18bf874c619d17baf9df45346e52b871. Change since last review was updating CheckBlockIndex to deal with missing blocks before the snapshot.

benthecarman approved

benthecarman commented at 7:44 am on June 22, 2021: contributor

reACK b6dffada18bf874c619d17baf9df45346e52b871

in src/validation.cpp:2287 in b6dffada18 outdated

2283@@ -2245,7 +2284,7 @@ static void UpdateTip(CTxMemPool& mempool, const CBlockIndex* pindexNew, const C
2284     }
2285 
2286     bilingual_str warning_messages;
2287-    if (!active_chainstate.IsInitialBlockDownload()) {
2288+    if (!chainstate.IsInitialBlockDownload()) {

ariard commented at 5:53 pm on June 24, 2021:

IIUC this log warns node operator about new consensus rules activated through the BIP9 logic. In case of the assumed-utxo-set being dated after either the locked-in or activation, your foreground validation is going to be blinded and your background validation is going to skim over this warning ? Thus you might have a client not fully-validating the chain, or at least with a set of consensus rules you might be interested with.

I don’t think this is impossible in case of contentious softfork locked-in, for e.g a minor release with activation logic feature-gated by a node operator setting followed by a major release with a newer hardcoded assumed-utxo ?

ryanofsky commented at 6:45 pm on June 24, 2021:

re: #21526 (review)

IIUC this log warns node operator about new consensus rules activated through the BIP9 logic. In case of the assumed-utxo-set being dated after either the locked-in or activation, your foreground validation is going to be blinded and your background validation is going to skim over this warning ?

I don’t think this is a change because the warning is skipped anyway in IBD. So loading a snapshot doesn’t skip this warning because it would be skipped anyway. Loading a snapshot just makes the node more usable while blocks are still being downloaded.

ariard commented at 10:31 pm on July 1, 2021:

I agree it’s not a change, my point is more about how assume-utxo is breaking the notion of a sequential IBD and introducing out-of-order validation which might invite us to reconsider the design purpose of such warning.

IsInitialBlockDownload’s last check relies on our chain tip being older than DEFAULT_MAX_TIP_AGE (24h) and i think that’s a reasonable assumption than hardcoded assumed-utxo will be always older than 24h. So background chainstate is always going to be considered as IBD, and such warning skipped, without giving a chance to flag out during foreground chainstate processing if it did start after the locked-in period, and let user react in consequence. For e.g, stop a Bitcoin merchant stopping trade, or increasing number of required confs during softforks activation, if it’s a contentious one.

Though I concede, it’s tangential to this PR and belong more to a wider discussion on how assume-utxo is interacting with softfork deployments.

in src/validation.cpp:4361 in b6dffada18 outdated

4383-        if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex;
4384+        // Don't start being rigorous about missing block data if we're within a utxo snapshot
4385+        if (pindex->nHeight > snapshot_height) {
4386+            if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex;
4387+            if (pindexFirstNeverProcessed == nullptr && pindex->nTx == 0) pindexFirstNeverProcessed = pindex;
4388+        }

ariard commented at 6:06 pm on June 24, 2021:

Once the background validation has catched up with the snapshot_height, is GetSnapshotHeight going to return std::nullopt and those checks play out ?

jamesob commented at 7:49 pm on July 16, 2021:

I’ll make a note to ensure that CompleteSnapshotValidation() (in #15606) nulls out m_snapshot_blockhash so that what you’re saying here is actually what happens. Otherwise we would just run the full suite of checks after restart when (what was previously) the snapshot chainstate gets loaded in as a “regular” IBD’d chainstate.

ryanofsky commented at 10:12 pm on July 19, 2021:

In commit “validation: fix CheckBlockIndex for multiple chainstates” (d7804383748e85bda1be40cf0782a3ca2e457066)

re: #21526 (review)

I’ll make a note to ensure that CompleteSnapshotValidation() (in #15606) nulls out m_snapshot_blockhash so that what you’re saying here is actually what happens. Otherwise we would just run the full suite of checks after restart when (what was previously) the snapshot chainstate gets loaded in as a “regular” IBD’d chainstate.

This would be good to do but it does seem fragile that you have to clear a CChainState atttribute to make more checking happen. I think (again) more ideally in the future assumeutxo snapshot logic would be limited to ChainStateManager. Chain state objects could have a CChainState::m_validation_starting_height member that could be used here instead of snapshot_height and ChainStateManager could set, so this code doesn’t have to know about ChainStateManager and ask it about snapshots.

jamesob commented at 2:27 pm on July 21, 2021:

Totally agree @ryanofsky, and I’ve got some changes coming that will attempt to limit the degree to which the notion of snapshots leak out of ChainstateManager, but I want to point out two things:

having an m_validation_starting_height member on CChainState would still have to be toggled after snapshot validation completes, so the fragility you rightfully point out would still be there, and
whether we call it “snapshot_height” or “m_validation_starting_height” or “assumed_valid_before”, we’re going to have some notion of an assumed-valid region of the chain that parts of the system have to kind of know about to a certain extent. I think it’s a legitimate argument that “assmed-valid” is maybe a more appropriate, general label than talking about snapshots, but fundamentally they’re isomorphic at the moment AFAICT.

I think the only real way to avoid the fragility of the checks here is to, instead of relying on CChainState member variables, introduce a new block index status flag, BLOCK_ASSUMED_VALID, and have CheckBlockIndex et al gate on that. I’m going to look at potentially doing this in a future changeset because this involves to some extent revamping the assumeutxo changes more generally.

ryanofsky commented at 5:40 pm on July 21, 2021:

having an m_validation_starting_height member on CChainState would still have to be toggled after snapshot validation completes, so the fragility you rightfully point out would still be there

Right, but the point for me is to just locate the complexity in chainstatemanager instead of having it exposed to other parts of the code. I think in this case complexity is like a conserved quantity, you can’t get rid of it but you can move it from one place to another. If complexity is encapsulated one place and not exposed other places that’s the best outcome.

Also I guess I personally have a bias to wanting to name things after how they are used or what they mean, not where they came from. Data can come from lots of places, and snapshots are a new source of data. But it adds mental overhead for pruning, indexing, and other validation code to know about the snapshot lifecycle or care where blocks came from when they don’t need to, so something like “validated_starting_height” or “locally_validated_starting_height” seems easier to understand and reason about than “snapshot_height” in code that doesn’t otherwise have to deal with snapshots.

BLOCK_ASSUMED_VALID could be good too. (I do have some skepticism that it conveys similar information, but not the same information, so might leave code making unclear assumptions like every block before the last assume valid block has the flag, and every block after doesn’t have it. Also it would seem more natural to have a flag indicating what kind of validation was done, instead of what kind of validation was skipped, so maybe BLOCK_VALIDATED_FROM_GENESIS, BLOCK_VALIDATED_FROM_SNAPSHOT could be more descriptive.)

But everything after the first paragraph here is just differences of opinion about naming, and you should chose the names you like and ignore the feedback you don’t like. I’m not trying to play architect here, just say what things I’m thinking and maybe generate some useful ideas.

jamesob commented at 5:16 pm on July 23, 2021:

“Snapshot height” has now been completely removed as something this code is concerned about; I’ll post a longer comment summarizing the changes, but we now rely on a block index flag called BLOCK_ASSUMED_VALID to change CheckBlockIndex() behavior as necessary. We now no longer have to worry about ChainstateManager variables to bring this behavior “back to normal,” since BLOCK_ASSUMED_VALID is removed from the corresponding pindex as blocks are validated.

in src/validation.cpp:4371 in b6dffada18 outdated

4397@@ -4357,7 +4398,7 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
4398         if (!pindex->HaveTxsDownloaded()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock)
4399         // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred).
4400         // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred.
4401-        if (!fHavePruned) {
4402+        if (!fHavePruned && (pindex->nHeight > snapshot_height)) {

ariard commented at 6:09 pm on June 24, 2021:

nit: “// Don’t start being rigorous about missing block data if we’ve never pruned and we’re within a utxo snapshot” ?

in src/validation.h:1027 in b6dffada18 outdated

1023@@ -1014,6 +1024,12 @@ class ChainstateManager
1024     //! ResizeCoinsCaches() as needed.
1025     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1026 
1027+    //! @returns height at which the active UTXO snapshot was taken, if applicable.

ariard commented at 6:11 pm on June 24, 2021:

nit: s/height/block metadata/g ?

jamesob commented at 7:30 pm on July 16, 2021:

Fixed, thanks!

ariard commented at 6:17 pm on June 24, 2021: member

Just see the comment about unknown new consensus rules state change not being warned off in case of background validation, and it might matter a bit.

ariard commented at 10:56 pm on July 1, 2021: member

Code Review ACK b6dffad

Still feel free to take my minor comments if you retouch the branch :)

DrahtBot added the label Needs rebase on Jul 15, 2021

validation: add chainman ref to CChainState

Add an upwards reference to chainstate instances to the owning
ChainstateManager. This is necessary because there are a number
of `this_chainstate == chainman.ActiveChainstate()` checks that
will happen (as a result of assumeutxo) in functions that otherwise
don't have an easily-accessible reference to the chainstate's
ChainManager.

9f6bb53935

refactor: remove unused assumeutxo methods

After feedback from Russ, I realized that there are some extraneous assumeutxo methods
that are not necessary and probably just overly confusing. These include

- `Validated*()`
- `IsBackgroundIBD()`

and they can be removed.

ac4051d891

jamesob force-pushed on Jul 16, 2021

in src/validation.cpp:2229 in 1ffc89e119 outdated

2224+            this->CoinsTip().DynamicMemoryUsage() * (1.0 / (1<<20)),
2225+            this->CoinsTip().GetCacheSize(),
2226+            !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages) : "");
2227+    };
2228+
2229+    // The contents of this function are either not relevant if we're not

jonatack commented at 8:12 pm on July 16, 2021:

94e67d6 This comment seems unclear to me; remove “either” and s/otherwise/in that case/? (not sure)

0    // The contents of this function are not relevant if we're not

jamesob commented at 2:31 pm on July 17, 2021:

Done, thanks.

in src/validation.cpp:4313 in 1ffc89e119 outdated

4308@@ -4282,6 +4309,9 @@ void CChainState::CheckBlockIndex()
4309 
4310     LOCK(cs_main);
4311 
4312+    bool is_active_chain = this == &m_chainman.ActiveChainstate();
4313+    int snapshot_height = m_chainman.GetSnapshotHeight().value_or(-1);

jonatack commented at 8:17 pm on July 16, 2021:

86e996aaef89 can be const, which seems good/useful in a function as long as CChainState::CheckBlockIndex()

0-    bool is_active_chain = this == &m_chainman.ActiveChainstate();
1-    int snapshot_height = m_chainman.GetSnapshotHeight().value_or(-1);
2+    const bool is_active_chain{this == &m_chainman.ActiveChainstate()};
3+    const int snapshot_height{m_chainman.GetSnapshotHeight().value_or(-1)};

jamesob commented at 2:31 pm on July 17, 2021:

Done, thanks.

in src/test/util/setup_common.cpp:270 in 1ffc89e119 outdated

265+    if (!chainstate) {
266+        chainstate = &Assert(m_node.chainman)->ActiveChainstate();
267+    }
268+
269+    const CChainParams& chainparams = Params();
270+    CBlock block = this->CreateBlock(txns, scriptPubKey, chainstate);

jonatack commented at 8:30 pm on July 16, 2021:

ef47823cbec0a9600

0    const CBlock block = this->CreateBlock(txns, scriptPubKey, chainstate);

jamesob commented at 2:31 pm on July 17, 2021:

Done

jonatack commented at 8:36 pm on July 16, 2021: member

Debug-built and did an initial commit-by-commit familiarization + unittest runs at 1ffc89e1192089aca8cdb0de80305d24f943ebfe and looks good. Will try to do a second pass soon to be able to ACK. Feel free to ignore the comments below.

DrahtBot removed the label Needs rebase on Jul 16, 2021

jamesob force-pushed on Jul 17, 2021

jamesob commented at 2:37 pm on July 17, 2021: member

au.multi-chainstates.pt1.14 -> au.multi-chainstates.pt1.16

After looking into @ryanofsky’s feedback on #15606, I found that there were some parts of the ChainstateManager interface that were ultimately unused (and probably confusing), so I’ve removed them here in https://github.com/bitcoin/bitcoin/pull/21526/commits/ac4051d891e2d5c8ac130da16b85b9d880b44720. All other changes are either rebasing onto #22415 or responding to feedback.

To check the range-diff, run:

0git remote add jamesob https://github.com/jamesob/bitcoin.git
1git fetch jamesob
2git range-diff master au.multi-chainstates.pt1.14 au.multi-chainstates.pt1.16

in src/test/util/chainstate.h:19 in 1901a9819d outdated

14+
15+#include <univalue.h>
16+
17+#include <boost/test/unit_test.hpp>
18+
19+auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){};

JeremyRubin commented at 7:29 pm on July 17, 2021:

is this necessary? Can we declare NoMalleation the old fashioned way?

JeremyRubin commented at 7:45 pm on July 17, 2021:

(it’s just a test, and it’s moveonly, so NBD)

ryanofsky commented at 5:29 pm on September 15, 2021:

In commit “move-only: unittest: add test/util/chainstate.h” (529f5055675f6c7f734796a8339761b6c1ba13b9)

Can declare this const auto instead of auto to avoid link errors marco and achow are reporting

jamesob commented at 10:22 pm on September 15, 2021:

Done! And yet again @ryanofsky bails me out of C++ jail.

achow101 commented at 10:39 pm on September 15, 2021:

Indeed this fixed the linker error.

JeremyRubin commented at 7:47 pm on July 17, 2021: contributor

did some lite code review; generally looks OK but I’m not super familiar with this part of the code so an ACK from me wouldn’t count for much.

in src/validation.cpp:2268 in daa6902795 outdated

2268-      pindexNew->GetBlockHash().ToString(), pindexNew->nHeight, pindexNew->nVersion,
2269-      log(pindexNew->nChainWork.getdouble())/log(2.0), (unsigned long)pindexNew->nChainTx,
2270-      FormatISO8601DateTime(pindexNew->GetBlockTime()),
2271-      GuessVerificationProgress(m_params.TxData(), pindexNew), this->CoinsTip().DynamicMemoryUsage() * (1.0 / (1<<20)), this->CoinsTip().GetCacheSize(),
2272-      !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages.original) : "");
2273+    log_progress("", warning_messages.original);

ryanofsky commented at 8:14 pm on July 19, 2021:

In commit “validation: change UpdateTip for multiple chainstates” (daa6902795b082832734b084a6c118eb8c682c3d)

It seems like a more natural place for this log print would be line 2239 right next to the background validation log print so logging code here is more consolidated and straightforward. It would be a minor change in logging behavior though, logging at beginning instead of end of this function.

ryanofsky commented at 8:27 pm on July 19, 2021:

In commit “validation: change UpdateTip for multiple chainstates” (daa6902795b082832734b084a6c118eb8c682c3d)

Questions about previous code I guess, but why is BACKGROUND_LOG_INTERVAL rate limiting code is needed for this function in the this != active background case, but it is not needed in the this->IsInitialBlockDownload background case? I’d expect you’d want the same rate limiting both cases.

Also, it’s kind of weird that in the this != active case, this function does nothing other than optionally print a log statement and return. It could make sense to move m_chain.SetTip calls into this function to dedup call sites and make this do more real work.

jamesob commented at 7:23 pm on August 13, 2021:

I’ve looked at doing this, but I don’t think it’s worth the complication: consider that we aggregate warning_messages at the end of this function, which is another reason to split the log up.

in src/validation.cpp:2216 in daa6902795 outdated

2209@@ -2210,6 +2210,33 @@ static void AppendWarning(bilingual_str& res, const bilingual_str& warn)
2210 
2211 void CChainState::UpdateTip(const CBlockIndex* pindexNew)
2212 {
2213+    auto& func_name = __func__;
2214+    auto log_progress = [this, pindexNew, &func_name](
2215+            const std::string& prefix,
2216+            const std::string& warning_messages) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)

ryanofsky commented at 9:37 pm on July 19, 2021:

In commit “validation: change UpdateTip for multiple chainstates” (daa6902795b082832734b084a6c118eb8c682c3d)

If going to use EXCLUSIVE_LOCKS_REQUIRED on a lambda right now, should actually use AssertLockHeld inside the lambda as well, because just adding the REQUIRED to a lambda does not actually require anything. (It just makes the analysis assume that the lock is held with no enforcement. Offtopic: I think this is bad and previously argued against using REQUIRED annotations to make unenforced assumptions in favor of adding a better assert with enforced assumptions, but I couldn’t convince reviewers about it in #19918.)

jamesob commented at 5:13 pm on July 23, 2021:

Fixed, thanks.

ryanofsky approved

ryanofsky commented at 10:27 pm on July 19, 2021: member

Code review ACK 1901a9819d507425f9a2ea440d78892acbbb0e7f. Since last review there was a rebase on #22415, a little more streamlining of assumeutxo snapshot/background/validated chainstate code in chainstate manager, and a few other cleanups.

I added some review suggestions below but these aren’t important and I think this looks good to be merged as-is (maybe with some previous reviewers reacking)

in src/validation.h:1032 in 1901a9819d outdated

1027@@ -1040,6 +1028,12 @@ class ChainstateManager
1028     //! ResizeCoinsCaches() as needed.
1029     void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
1030 
1031+    //! @returns block metadata for the base block of the active UTXO snapshot, if applicable.
1032+    std::optional<CBlockIndex*> GetSnapshotBaseBlock() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

MarcoFalke commented at 8:14 pm on July 20, 2021:

Still not fixed?

jamesob force-pushed on Jul 23, 2021

jamesob commented at 1:37 pm on July 24, 2021: member

au.multi-chainstates.pt1.16 -> au.multi-chainstates.pt1.18

After spending some time slightly reworking #15606 to incorporate good design feedback from @ryanofsky, I’ve pushed some fairly substantive changes here. Sorry, of course, for these changes so late in the game on this PR - but better to make them now than post-merge.

In a nutshell, I’ve reworked assumeutxo so that the notion of a “UTXO snapshot” is basically an implementation detail of ChainstateManager and doesn’t leak out from behind it. Other parts of validation and the system at large deal with more general concepts like assumed-valid block index entries, and aren’t concerned with things like what height the base block of the snapshot is at.

This is described in an assumeutxo design document (docs/assumeutxo.md) I’ve added in the parent PR. The immediate changes here are

introduction of a new block index nStatus flag: BLOCK_ASSUMED_VALID, which marks block entries that are for the moment assumed to be valid and are pending asynchronous validation,
use of this new flag to modify CheckBlockIndex() logic as needed - which is more resilient than the previous approach because as assumd-valid index entries are marked as valid, the CheckBlockIndex() checks gradually return to “normal” operation (instead of having to wait on complete snapshot validation, and hoping that we set ChainstateManager state correctly).

I think these changes not only represent a better design for the assumeutxo implementation, but they address all pending feedback here in this PR.

Thanks again for bearing with the changes and continuing to help with review. Once we get this merged, there are a number of other (smaller) assumeutxo PRs I can open.

jamesob commented at 7:25 pm on August 5, 2021: member

Anyone have any advice on what’s needed to move this forward? I think it’s in pretty good shape.

ryanofsky commented at 9:25 pm on August 5, 2021: member

Anyone have any advice on what’s needed to move this forward? I think it’s in pretty good shape.

My bad! I reviewed most of the new changes here while I was going through the larger PR #15606, but I forgot to come back and re-ack.

Aside from me, some previous reviewers might be interested to continue review or re-ack:

@sjors #21526#pullrequestreview-652577266
@benthecarman #21526#pullrequestreview-689166399
@ariard #21526 (comment)
@jonatack #21526#pullrequestreview-708717465
JeremyRubin #21526 (comment)

in src/chain.h:134 in d502108c82 outdated

130+    BLOCK_OPT_WITNESS        =   128, //!< block data in blk*.data was received with a witness-enforcing client
131+
132+    /**
133+     * If set, this indicates that the block index entry is assumed-valid.
134+     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
135+     * It almost certainly means that the block's full validation is pending

ryanofsky commented at 9:40 pm on August 5, 2021:

In commit “chain: add BLOCK_ASSUMED_VALID for use with assumeutxo” (d502108c82e64b6acec4d2788fb224c118fccaff)

Very little difference, but in case you update, you might backport commit d502108c82e64b6acec4d2788fb224c118fccaff from #15606 to this pr (dropping almost certainly", #15606 (review))

in src/chain.h:323 in d502108c82 outdated

320-        if (nStatus & BLOCK_FAILED_MASK)
321-            return false;
322+        if (nStatus & BLOCK_FAILED_MASK) return false;
323+
324+        // If this block had been marked assumed-valid and we're raising
325+        // its validity to a certain point, there is no longer an assumption.

ryanofsky commented at 9:58 pm on August 5, 2021:

In commit “chain: add BLOCK_ASSUMED_VALID for use with assumeutxo” (d502108c82e64b6acec4d2788fb224c118fccaff)

It would be good if this comment said what the reason for unsetting the flag is. I.e. is unsetting the flag just bookkeeping or is there actually code that would be broken if block was assumed valid and later validated, and multiple flags were present?

It’s not a big deal, but my instinct would be to say that any code making extra assumptions about validity based on ASSUMED_VALID is already fragile and buggy, and it would be better to fix that code than to make it depend on this new RaiseValidity flag toggling behavior (and it would seem be nice not to have to add this behavior at all).

UPDATE: Looking ahead at the later commits, I can see some new code added in CheckBlockIndex and calling IsAssumedValid() is assuming the block has not been locally validated yet if true. I think this code would probably be clearer and more robust if it checked BLOCK_VALID first and then checked BLOCK_ASSUMED_VALID secondarily instead of putting the assumed valid case at the forefront. But :shrug: it’s not really a big deal and probably not worth more iterations updating and discussing.

in src/validation.cpp:5056 in 9a8ad49723 outdated

5000@@ -5001,11 +5001,24 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
5001         // Fake nChainTx so that GuessVerificationProgress reports accurately
5002         index->nChainTx = index->pprev ? index->pprev->nChainTx + index->nTx : 1;
5003 
5004+        if (!index->IsValid(BLOCK_VALID_SCRIPTS)) {
5005+            // This flag will be removed once the block is fully validated by a
5006+            // background chainstate.
5007+            index->nStatus |= BLOCK_ASSUMED_VALID;
5008+        }
5009+

ryanofsky commented at 10:02 pm on August 5, 2021:

In commit “validation: set BLOCK_ASSUMED_VALID during snapshot load” (9a8ad497238c8c8aba1ba8679bc7cdbe409c635e)

Similar to comment in last commit it would seem nice to just be able to drop this interaction/toggling of flags and just do unconditional index->nStatus |= BLOCK_ASSUMED_VALID

jamesob commented at 7:45 pm on August 13, 2021:

I don’t think it makes sense to apply the ASSUMED_VALID classification for blocks we’ve already validated, especially since (i) it’s easy to avoid, and (ii) there will almost certainly be blocks at the base of the chain that have been validated prior to loading a UTXO snapshot.

in src/validation.cpp:4359 in 4941a532eb outdated

4348@@ -4348,12 +4349,29 @@ void CChainState::CheckBlockIndex()
4349     while (pindex != nullptr) {
4350         nNodes++;
4351         if (pindexFirstInvalid == nullptr && pindex->nStatus & BLOCK_FAILED_VALID) pindexFirstInvalid = pindex;
4352-        if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA)) pindexFirstMissing = pindex;
4353+        if (pindexFirstMissing == nullptr && !(pindex->nStatus & BLOCK_HAVE_DATA) && !pindex->IsAssumedValid()) {

ryanofsky commented at 10:37 pm on August 5, 2021:

In commit “validation: set BLOCK_ASSUMED_VALID during snapshot load” (9a8ad497238c8c8aba1ba8679bc7cdbe409c635e)

Would be nice to have comment explaining the IsAssumedValid condition, since logic is a little convoluted. Maybe “Treat assumed valid blocks that are missing data as if they have data for the purpose of the [whatever] check below”

in src/validation.cpp:4366 in 4941a532eb outdated

4358-        if (pindex->pprev != nullptr && pindexFirstNotTransactionsValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS) pindexFirstNotTransactionsValid = pindex;
4359-        if (pindex->pprev != nullptr && pindexFirstNotChainValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_CHAIN) pindexFirstNotChainValid = pindex;
4360-        if (pindex->pprev != nullptr && pindexFirstNotScriptsValid == nullptr && (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_SCRIPTS) pindexFirstNotScriptsValid = pindex;
4361+
4362+        if (pindex->pprev != nullptr && !pindex->IsAssumedValid()) {
4363+            // Skip validity flag checks for BLOCK_ASSUMED_VALID index entries.

ryanofsky commented at 10:52 pm on August 5, 2021:

In commit “validation: set BLOCK_ASSUMED_VALID during snapshot load” (9a8ad497238c8c8aba1ba8679bc7cdbe409c635e)

Would be nice if comment could be less vague. Maybe “Treat assumed valid blocks as if they are valid for the purpose of the [whatever] checks below”

jamesob commented at 8:58 pm on August 13, 2021:

Fixed, thanks!

ryanofsky approved

ryanofsky commented at 10:56 pm on August 5, 2021: member

Code review ACK 3a9ce0e2d41cac3b072225aa1ed34caca4191392. Looks good! All comments just FYI and maybe for future followup.

Changes since last review: adding assertlockheld to UpdateTip lambda, adding BLOCK_ASSUMED_VALID logic, and doing a pointer->reference cleanup in tests.

benthecarman approved

benthecarman commented at 8:14 pm on August 6, 2021: contributor

utACK 3a9ce0e2d41cac3b072225aa1ed34caca4191392

in src/validation.cpp:2214 in 3a9ce0e2d4 outdated

2209@@ -2205,6 +2210,34 @@ static void AppendWarning(bilingual_str& res, const bilingual_str& warn)
2210 
2211 void CChainState::UpdateTip(const CBlockIndex* pindexNew)
2212 {
2213+    auto& func_name = __func__;
2214+    auto log_progress = [this, pindexNew, &func_name](

jonatack commented at 9:00 pm on August 12, 2021:

1142b4f Seeing this warning like @Sjors reported (rebased on master, Clang 13 debug build). What CI failure were you seeing when it was addressed?

0validation.cpp:2203:44: warning: lambda capture 'func_name' is not required to be captured for this use [-Wunused-lambda-capture]
1    auto log_progress = [this, pindexNew, &func_name](
2                                        ~~~^~~~~~~~~
31 warning generated.

jonatack commented at 8:25 am on August 13, 2021:

How did you address it when the CI didn’t pass? e.g. with [this, pindexNew], or [&], or using a struct instead of a lambda?

jamesob commented at 2:49 pm on August 13, 2021:

Can’t dig up the exact stacktrace, but on one of the more exotic CI platforms (mac, win, or mingw32), the variable isn’t captured unless explicitly named, and compilation fails. I think I tried your first two suggestions, but am not sure what you mean by “struct instead of a lambda.”

ryanofsky commented at 3:06 pm on August 13, 2021:

Can’t dig up the exact stacktrace, but on one of the more exotic CI platforms (mac, win, or mingw32), the variable isn’t captured unless explicitly named, and compilation fails. I think I tried your first two suggestions, but am not sure what you mean by “struct instead of a lambda.”

Related: I did have an earlier suggestion for eliminating the lambda here: #21526 (review). Instead of using the lambda to repeat the logging at the beginning and end of the function, just log without a lambda at the beginning of the function

I don’t think is too important, though

jamesob commented at 4:01 pm on August 13, 2021:

@ryanofsky good point - I’ll give that a shake, and take a look at addressing your other feedback while I’m at it.

jonatack commented at 5:14 pm on August 13, 2021:

This is new to me, but if I understand correctly, the issue is compiler/system-dependent lambda capture of __func__. Maybe convert the log_progress lambda to a function: https://github.com/jonatack/bitcoin/commit/e95e25e8aa6581d8658b05581f0b1a04e8712387.

jonatack commented at 5:29 pm on August 13, 2021:

(by “struct instead of a lambda”, I was thinking of a struct like 38a81a8 (#21261) that I originally wrote as a lambda, before turning it into a struct to be more similar to the other peer eviction comparators)

jamesob commented at 7:27 pm on August 13, 2021:

Took @jonatack’s suggestion and moved the lambda to a static function.

jamesob force-pushed on Aug 13, 2021

in src/validation.cpp:2242 in 011b52beb9 outdated

2238+    // the active chainstate, so return if need be.
2239+    if (this != &m_chainman.ActiveChainstate()) {
2240+        // Only log every so often so that we don't bury log messages at the tip.
2241+        constexpr int BACKGROUND_LOG_INTERVAL = 2000;
2242+        if (pindexNew->nHeight % BACKGROUND_LOG_INTERVAL == 0) {
2243+            UpdateTipLog(coins_tip, pindexNew, m_params, __func__, "[background validation] ", "");

jonatack commented at 7:51 pm on August 13, 2021:

here and below s/__func__/func_name/ (or remove line 2234, I don’t know if the conversion from char[10] to std:string is needed for any of the compilers, can try dropping it to see…but if the conversion is needed, maybe const std::string func_name{__func__})

0validation.cpp: In member function ‘void CChainState::UpdateTip(const CBlockIndex*)’:
1validation.cpp:2223:11: error: unused variable ‘func_name’ [-Werror=unused-variable]
2     auto& func_name = __func__;
3           ^~~~~~~~~

jamesob commented at 8:59 pm on August 13, 2021:

Whoops, fixed. Thanks!

jonatack commented at 3:07 pm on August 14, 2021:

Nice, it worked without line 2234. Cool.

jamesob force-pushed on Aug 13, 2021

ryanofsky approved

ryanofsky commented at 12:10 pm on August 14, 2021: member

Code review ACK a30a5d46b3574a49f9e3611caeb730bf12d748fd. Only changes since last review are changing lambda function to regular function and updating some comments

benthecarman commented at 7:24 pm on August 14, 2021: contributor

reACK a30a5d46b3574a49f9e3611caeb730bf12d748fd

in src/chain.h:129 in a30a5d46b3 outdated

125@@ -126,7 +126,15 @@ enum BlockStatus: uint32_t {
126     BLOCK_FAILED_CHILD       =   64, //!< descends from failed block
127     BLOCK_FAILED_MASK        =   BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD,
128 
129-    BLOCK_OPT_WITNESS       =   128, //!< block data in blk*.data was received with a witness-enforcing client
130+    BLOCK_OPT_WITNESS        =   128, //!< block data in blk*.data was received with a witness-enforcing client

jonatack commented at 1:36 pm on August 16, 2021:

212b80da these two alignments are off by one, as the enumerator values/commas/comments are right-justified

 0@@ -126,7 +126,7 @@ enum BlockStatus: uint32_t {
 1     BLOCK_FAILED_CHILD       =   64, //!< descends from failed block
 2     BLOCK_FAILED_MASK        =   BLOCK_FAILED_VALID | BLOCK_FAILED_CHILD,
 3 
 4-    BLOCK_OPT_WITNESS        =   128, //!< block data in blk*.data was received with a witness-enforcing client
 5+    BLOCK_OPT_WITNESS        =  128, //!< block data in blk*.data was received with a witness-enforcing client
 6 
 7     /**
 8      * If set, this indicates that the block index entry is assumed-valid.
 9@@ -134,7 +134,7 @@ enum BlockStatus: uint32_t {
10      * It almost certainly means that the block's full validation is pending
11      * on a background chainstate. See `docs/assumeutxo.md`.
12      */
13-    BLOCK_ASSUMED_VALID      =   256,
14+    BLOCK_ASSUMED_VALID      =  256,
15 };

fjahr commented at 10:15 pm on August 25, 2021:

in 212b80dad5c19a043c007d79109963e301cd1400

The block files actually also have a .dat extension, not .data, if you retouch…

in src/validation.cpp:2224 in a30a5d46b3 outdated

2219+
2220+    AssertLockHeld(::cs_main);
2221+    LogPrintf("%s%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n",
2222+        prefix, func_name,
2223+        tip->GetBlockHash().ToString(), tip->nHeight, tip->nVersion,
2224+        log(tip->nChainWork.getdouble())/log(2.0), (unsigned long)tip->nChainTx,

jonatack commented at 1:38 pm on August 16, 2021:

e747b53 nit, clang-formatting

0        log(tip->nChainWork.getdouble()) / log(2.0), (unsigned long)tip->nChainTx,

in src/validation.cpp:2227 in a30a5d46b3 outdated

2222+        prefix, func_name,
2223+        tip->GetBlockHash().ToString(), tip->nHeight, tip->nVersion,
2224+        log(tip->nChainWork.getdouble())/log(2.0), (unsigned long)tip->nChainTx,
2225+        FormatISO8601DateTime(tip->GetBlockTime()),
2226+        GuessVerificationProgress(params.TxData(), tip),
2227+        coins_tip.DynamicMemoryUsage() * (1.0 / (1<<20)),

jonatack commented at 1:39 pm on August 16, 2021:

e747b53 nit, clang-formatting

0        coins_tip.DynamicMemoryUsage() * (1.0 / (1 << 20)),

in src/chain.h:135 in a30a5d46b3 outdated

131+
132+    /**
133+     * If set, this indicates that the block index entry is assumed-valid.
134+     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
135+     * It almost certainly means that the block's full validation is pending
136+     * on a background chainstate. See `docs/assumeutxo.md`.

jonatack commented at 1:53 pm on August 16, 2021:

212b80dad5c19a043c007d79109963e301cd1400 bad url?

0     * on a background chainstate. See `doc/assumeutxo.md`.

ariard commented at 11:44 pm on August 25, 2021:

I had an overlook on assumeutxo.md, I think it’s a mature enough documentation to be landed in-tree in a follow-up PR. At least, it was helpful for me as a reminder on design trade-offs.

in src/validation.cpp:5051 in a30a5d46b3 outdated

5045@@ -4972,11 +5046,24 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
5046         // Fake nChainTx so that GuessVerificationProgress reports accurately
5047         index->nChainTx = index->pprev ? index->pprev->nChainTx + index->nTx : 1;
5048 
5049+        if (!index->IsValid(BLOCK_VALID_SCRIPTS)) {

jonatack commented at 2:26 pm on August 16, 2021:

0a0a525 a comment here may be nice (this one is from the commit message)

0+       // Mark the block index entries beneath the snapshot base block as assumed-valid.
1        if (!index->IsValid(BLOCK_VALID_SCRIPTS)) {

in src/validation.h:654 in a30a5d46b3 outdated

652-     * missing the data for the block.
653+     * The set of all CBlockIndex entries with either BLOCK_VALID_TRANSACTIONS (for
654+     * itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if using background
655+     * chainstates) and as good as our current tip or better. Entries may be failed,
656+     * though, and pruning nodes may be missing the data for the block.
657      */

jonatack commented at 2:59 pm on August 16, 2021:

00b44cda suggestion to be unambiguous that as good as our current tip or better applies to both cases, BLOCK_VALID_TRANSACTIONS and BLOCK_ASSUMED_VALID

0     /**
1-     * The set of all CBlockIndex entries with either BLOCK_VALID_TRANSACTIONS (for
2-     * itself and all ancestors) *or* BLOCK_ASSUMED_VALID (if using background
3-     * chainstates) and as good as our current tip or better. Entries may be failed,
4-     * though, and pruning nodes may be missing the data for the block.
5+     * The set of all CBlockIndex entries as good as our current tip or better
6+     * with either BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) *or*
7+     * BLOCK_ASSUMED_VALID (if using background chainstates). Entries may be
8+     * failed, though, and pruning nodes may be missing the data for the block.
9      */

MarcoFalke commented at 12:53 pm on September 24, 2021:

Does it apply to both? Would be good to clarify. (It is not possible to tell from the code in this pull)

in src/test/validation_chainstate_tests.cpp:11 in a30a5d46b3 outdated

 7 #include <uint256.h>
 8 #include <consensus/validation.h>
 9 #include <sync.h>
10+#include <rpc/blockchain.h>
11 #include <test/util/setup_common.h>
12+#include <test/util/chainstate.h>

jonatack commented at 3:52 pm on August 16, 2021:

a30a5d46 nit, sort

 0 #include <chainparams.h>
 1-#include <random.h>
 2-#include <uint256.h>
 3 #include <consensus/validation.h>
 4-#include <sync.h>
 5+#include <random.h>
 6 #include <rpc/blockchain.h>
 7-#include <test/util/setup_common.h>
 8+#include <sync.h>
 9 #include <test/util/chainstate.h>
10+#include <test/util/setup_common.h>
11+#include <uint256.h>
12 #include <validation.h>

in src/test/validation_chainstatemanager_tests.cpp:11 in a30a5d46b3 outdated

 8@@ -9,12 +9,12 @@
 9 #include <rpc/blockchain.h>
10 #include <sync.h>
11 #include <test/util/setup_common.h>
12+#include <test/util/chainstate.h>

jonatack commented at 3:55 pm on August 16, 2021:

6b502ae nit, sort

0-#include <test/util/setup_common.h>
1 #include <test/util/chainstate.h>
2+#include <test/util/setup_common.h>

in src/test/validation_chainstate_tests.cpp:104 in a30a5d46b3 outdated

 99+    curr_tip = ::g_best_block;
100+
101+    // Mine a new block on top of the activated snapshot chainstate.
102+    mineBlocks(1);  // Defined in TestChain100Setup.
103+
104+    // After adding some blocks to the snapshot tip, best block should have changed.

jonatack commented at 4:44 pm on August 16, 2021:

a30a5d46

0    // After adding a block to the snapshot tip, best block should have changed.

jonatack commented at 4:50 pm on August 16, 2021: member

Code review ACK a30a5d46b3574a49f9e3611caeb730bf12d748fd caught up on context, rebased to master, and for each commit code review/debug build/unit test run

Various non-blocking minor comments follow, feel free to pick/choose/ignore, happy to re-ACK if you update.

Edit: am running a mainnet node on this branch with -checkblockindex=1

in src/chain.h:324 in a30a5d46b3 outdated

321-            return false;
322+        if (nStatus & BLOCK_FAILED_MASK) return false;
323+
324+        // If this block had been marked assumed-valid and we're raising
325+        // its validity to a certain point, there is no longer an assumption.
326+        if (nStatus & BLOCK_ASSUMED_VALID && nUpTo >= BLOCK_VALID_SCRIPTS) {

jonatack commented at 5:32 pm on August 16, 2021:

212b80dad perhaps use the bool helper added in this commit?

0        if (IsAssumedValid() && nUpTo >= BLOCK_VALID_SCRIPTS) {

fjahr commented at 11:01 pm on August 25, 2021:

in 212b80dad5c19a043c007d79109963e301cd1400

If a block had nStatus = BLOCK_VALID_SCRIPTS | BLOCK_ASSUMED_VALID and RaiseValidity(BLOCK_VALID_SCRIPTS) was called on it then nStatus would change but the function would still return false if I see that correctly. But it seems like an extreme edge case that may be impossible to hit but I am not 100% sure. In general it would be nicer if the function could never return false if a change in status happens.

jamesob commented at 8:43 pm on September 8, 2021:

Given I’m unsetting the same flag just below I think this could be somewhat confusing.

jamesob commented at 1:24 pm on September 9, 2021:

I’ve changed the code to ensure this can’t happen, thanks.

in src/chain.h:311 in 212b80dad5 outdated

307@@ -300,13 +308,23 @@ class CBlockIndex
308         return ((nStatus & BLOCK_VALID_MASK) >= nUpTo);
309     }
310 
311+    //! @returns true if the block is assumed-valid; probably means it is queued to be

fjahr commented at 7:30 pm on August 25, 2021:

in 212b80dad5c19a043c007d79109963e301cd1400

Why only probably? Is it possible or planned that this will not be the case?

in src/validation.cpp:4320 in ed0d83a3d2 outdated

4316@@ -4317,6 +4317,7 @@ void CChainState::CheckBlockIndex()
4317     }
4318 
4319     LOCK(cs_main);
4320+    bool is_active = this == &m_chainman.ActiveChainstate();

fjahr commented at 7:49 pm on August 25, 2021:

in ed0d83a3d2c5752ef2424dee7c849933fdc39452

nit: could be const but also is_active is only used once below in a line that might not even run due to other conditionals so maybe it doesn’t need to be placed up here? Unless this is used more in follow-ups?

in src/validation.cpp:4396 in ed0d83a3d2 outdated

4391@@ -4370,7 +4392,9 @@ void CChainState::CheckBlockIndex()
4392         if (!pindex->HaveTxsDownloaded()) assert(pindex->nSequenceId <= 0); // nSequenceId can't be set positive for blocks that aren't linked (negative is used for preciousblock)
4393         // VALID_TRANSACTIONS is equivalent to nTx > 0 for all nodes (whether or not pruning has occurred).
4394         // HAVE_DATA is only equivalent to nTx > 0 (or VALID_TRANSACTIONS) if no pruning has occurred.
4395-        if (!fHavePruned) {
4396+        // Unless these indexes are assumed valid and pending block download on a
4397+        // background chainstate..

fjahr commented at 8:50 pm on August 25, 2021:

in ed0d83a3d2c5752ef2424dee7c849933fdc39452

nit: s/.././

in src/test/validation_chainstate_tests.cpp:78 in a30a5d46b3 outdated

74@@ -72,4 +75,77 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
75     WITH_LOCK(::cs_main, manager.Unload());
76 }
77 
78+//! Test UpdateTip behavior for both active and background chainstates.

fjahr commented at 11:30 pm on August 25, 2021:

in a30a5d46b3574a49f9e3611caeb730bf12d748fd

nit: I find it a bit strange to refer to a specific function being tested in a unit test when it’s not actually called in the test itself since such comments may rot without notice. I would have preferred something a bit more generic like “Test that active and background chainstates can update their tip independently of each other”.

in src/validation.cpp:2221 in e747b5379f outdated

2216+    const std::string& prefix,
2217+    const std::string& warning_messages) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
2218+{
2219+
2220+    AssertLockHeld(::cs_main);
2221+    LogPrintf("%s%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n",

ariard commented at 11:36 pm on August 25, 2021:

Note, if you want to provide one supplemental information to the node operator and find an usage for the IsBackgroundIBD deleted in the previous commit, I think you can add a chainstate=%s by passing an arg const bool is_snapshot and value=!m_chainman.IsBackgroundIBD() ?

jamesob commented at 8:45 pm on September 8, 2021:

I don’t understand… this already distinguishes for background chainstates with the prefix string.

ariard commented at 11:29 pm on September 19, 2021:

That’s right.

fjahr commented at 11:48 pm on August 25, 2021: member

Code review ACK a30a5d46b3574a49f9e3611caeb730bf12d748fd

Sorry for being late to the party, none of my comments are blocking but if you decide to address them (or have to retouch for another reason), I will be quick to re-review.

in src/chain.h:137 in 212b80dad5 outdated

133+     * If set, this indicates that the block index entry is assumed-valid.
134+     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
135+     * It almost certainly means that the block's full validation is pending
136+     * on a background chainstate. See `docs/assumeutxo.md`.
137+     */
138+    BLOCK_ASSUMED_VALID      =   256,

ariard commented at 11:59 pm on August 25, 2021:

Note, I’m dubious on the flag name choice as we already have a distinct lightweight IBD feature assumevalid. Even if their security model are pretty similar, I don’t think they are fully-overlapping. What consensus failure case which would be caught by an assumevalid=X client but not by an assume-utxo=X client (before background IBD is over) would be an inflation bug similar to CVE-2018-17144. For an assumevalid client, CheckTransaction then CheckBlock then ConnectBlock which is not skipped by !hashAssumeValid.IsNull().

I believe another flag name like BLOCK_UTXO_ASSUMED would be more adequate to avoid features confusion towards developers and node operators.

What do you think ?

naumenkogs commented at 12:11 pm on September 3, 2021:

I low-key agree.

ryanofsky commented at 6:21 pm on September 8, 2021:

re: #21526 (review)

James asked me about the flag again, and I don’t have much new to add so I’ll just restate what I wrote #21526 (review) and #21526 (review), which is that I don’t like complexity of flags being added and then removed instead of just being cumulative. I don’t like flags trying to tell me what I should be assuming about a block, instead of just telling me what kind of validation was actually done on the block. So I’d drop the “assume” flag and use something like BLOCK_VALIDATED_FROM_GENESIS, BLOCK_VALIDATED_FROM_SNAPSHOT flags instead. Or not have any new flag and just have validated begin/end pointers or height ranges.

But assuming flag implementation isn’t going to change at this point, ariard’s suggestion to add something about utxos or snapshots to the flag name does not seem like a bad idea. I don’t really see how confusion with the assumevalid feature would be possible, but also don’t see drawbacks to making the flag name more descriptive.

And all of this is definitely bikeshedding! Current flag seems perfectly reasonable and not confusing in any way I could realistically see.

jamesob commented at 9:00 pm on September 8, 2021:

Thanks again @ryanofsky for weighing in.

I don’t like complexity of flags being added and then removed instead of just being cumulative

I agree that ideally it would be better to set flags once and never touch them subsequently, but there is a need to remove the ASSUMED flag on block index entries as they’re validated so that CheckBlockIndex eventually returns to its normal behavior. I’m sure there’s some way we could do that with some combination of flags that are set once and never unset, but I think that would probably require a much bigger change and is more trouble than I think is worth.

@ariard @naumenkogs unless there is significant dissent, I’m keeping this name as-is. Part of what motivated this change in the first place was to avoid leaking “assumeutxo” specifics out into the rest of the system; if we change the name of this flag, we then have to change pindex->IsAssumedValid() to something like IsAssumedUtxo() which then leaks that detail again; and I don’t find that name change to be sufficiently descriptive to make that worthwhile.

BLOCK_ASSUMED_VALID is the correct name here because the block is assumed to be valid; it has not yet actually been encountered by the system. I don’t think that confusion with the -assumevalid= CLI flag (which should really be something like -skipsigsbeneath=) is really going to trip someone up here looking at block index flags. The other thing is that BLOCK_UTXO_ASSUMED doesn’t strike me as sufficiently descriptive - the block index shouldn’t care why we’re assuming the thing valid, and if we end up doing parallel validation with something like utreexo, the over-specified name will only be more confusing and out of date.

naumenkogs commented at 9:18 am on September 9, 2021:

Not gonna insist. Perhaps, the confusion with -assumevalid= matters only for me, someone who looks at this part of the code for the second time.

JeremyRubin commented at 5:07 pm on September 15, 2021:

One of the benefits of the ‘cumulative’ flags is that you can prove the block validity state transitions are finite through consumption of the flag.

However, it’s easy to prove this by inspection as well, so I think the current design is OK.

ariard commented at 11:49 pm on September 19, 2021:

So I’d drop the “assume” flag and use something like BLOCK_VALIDATED_FROM_GENESIS, BLOCK_VALIDATED_FROM_SNAPSHOT flags instead.

Yes, I think flags informing on the level of validation which has been performed so far would be better than tagging blocks with the validation process they belong to.

I agree, let’s not stuck on that, we can improve with ‘cumulative’ flags and better description of the different validation models (e.g skipsigbeneath) in the future.

in src/validation.cpp:3776 in 00b44cdaec outdated

3772@@ -3773,7 +3773,9 @@ bool BlockManager::LoadBlockIndex(
3773             pindex->nStatus |= BLOCK_FAILED_CHILD;
3774             setDirtyBlockIndex.insert(pindex);
3775         }
3776-        if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr)) {
3777+        if (pindex->IsAssumedValid() ||

ariard commented at 0:17 am on August 26, 2021:

I don’t get the rational of this change, IsAssumeValid blocks are ones lower than the best current snapshot tip and as such won’t be on a better-work chain and shouldn’t be candidate for tip inclusion in FindMostWorkChain ? Or do you have a commit in #15606 pointing how it is used ?

jamesob commented at 5:22 pm on September 8, 2021:

I don’t totally understand your comment here. This is a necessary change so that when loading a snapshot-based chainstate, the (assumed-valid) tip is able to be added as a tip candidate, since otherwise it would be excluded on the basis of !HaveTxsDownloaded().

ariard commented at 11:42 pm on September 19, 2021:

I had the wrong mental model reviewing. I thought that only blocks before the snapshot height were marked as IsAssumeValid, thanks for the clarification!

ariard commented at 0:18 am on August 26, 2021: member

Few questions to solve, thought overall I think it’s pretty mature for merging.

naumenkogs commented at 12:29 pm on September 3, 2021: member

Light code review ACK, I will do good review once pending suggestions are resolved.

jamesob commented at 1:30 pm on September 7, 2021: member

Hi, sorry for the slowness here and thanks to everyone for the review. I got pretty sick in the last two weeks and was unable to work. As I recover in the coming days I’ll address the new feedback.

jamesob force-pushed on Sep 8, 2021

naumenkogs commented at 10:04 am on September 9, 2021: member

utACK a07e850482ab22f31d070af6d080a0db66564c18

Among the remaining comments, I’d ask you to reconsider this one if you decide to retouch. Not a blocker at all.

jamesob commented at 5:18 pm on September 9, 2021: member

Thanks for the looks, everyone. I’ve pushed a number of small changes addressing the feedback given.

jonatack approved

jonatack commented at 1:06 pm on September 11, 2021: member

re-ACK a07e850482ab22f31d070af6d080a0db66564c18 per git diff a30a5d4 a07e850, reviewed diff, verified rebase to current master, debug build clean, unit tests green, briefly ran bitcoind with -checkblockindex

The main change is a move-only one in chain.h to address #21526 (review):

 0@@ -319,13 +319,13 @@ public:
 1         assert(!(nUpTo & ~BLOCK_VALID_MASK)); // Only validity flags allowed.
 2         if (nStatus & BLOCK_FAILED_MASK) return false;
 3 
 4-        // If this block had been marked assumed-valid and we're raising
 5-        // its validity to a certain point, there is no longer an assumption.
 6-        if (nStatus & BLOCK_ASSUMED_VALID && nUpTo >= BLOCK_VALID_SCRIPTS) {
 7-            nStatus &= ~BLOCK_ASSUMED_VALID;
 8-        }
 9-
10         if ((nStatus & BLOCK_VALID_MASK) < nUpTo) {
11+            // If this block had been marked assumed-valid and we're raising
12+            // its validity to a certain point, there is no longer an assumption.
13+            if (nStatus & BLOCK_ASSUMED_VALID && nUpTo >= BLOCK_VALID_SCRIPTS) {
14+                nStatus &= ~BLOCK_ASSUMED_VALID;
15+            }
16+
17             nStatus = (nStatus & ~BLOCK_VALID_MASK) | nUpTo;
18             return true;

benthecarman approved

benthecarman commented at 9:20 pm on September 11, 2021: contributor

reACK a07e850482ab22f31d070af6d080a0db66564c18

achow101 commented at 8:14 pm on September 13, 2021: member

ACK a07e850482ab22f31d070af6d080a0db66564c18

Tried to build and I got this error:

/usr/bin/ld: test/test_bitcoin-validation_chainstatemanager_tests.o:././test/util/chainstate.h:19: multiple definition of `NoMalleation'; test/test_bitcoin-validation_chainstate_tests.o:././test/util/chainstate.h:19: first defined here

jamesob commented at 9:19 pm on September 13, 2021: member

@achow101 thanks for reviewing and testing. What’s your platform information? I just tried to reproduce your error locally and was unable, and CI is able to build. Maybe try make distclean; ./configure ..., then remake?

ryanofsky approved

ryanofsky commented at 9:38 pm on September 13, 2021: member

Code review ACK a07e850482ab22f31d070af6d080a0db66564c18. Since last review just simple cleanups, and corner case RaiseValidity behavior change (which IIUC is just to intended to give a more consistent return value, not to change anything in practice)

achow101 commented at 10:28 pm on September 13, 2021: member

@jamesob

What’s your platform information? I just tried to reproduce your error locally and was unable, and CI is able to build. Maybe try make distclean; ./configure ..., then remake?

I’ve tried all sorts of clean and make to no avail. My system is Arch and I’m using GCC 11.1. Here’s the config.log if that’s useful.

jamesob commented at 11:21 am on September 15, 2021: member

@achow101 did you try clearing ccache? I’ve found lately that sometimes it’s overzealous in caching and can cause problems. If you haven’t, give ccache -C a try before building.

MarcoFalke commented at 11:44 am on September 15, 2021: member

Can’t link either on a fresh Ubuntu Impish:

 0make V=1
 1Making all in src
 2make[1]: Entering directory '/bitcoin-core/src'
 3make[2]: Entering directory '/bitcoin-core/src'
 4make[3]: Entering directory '/bitcoin-core'
 5make[3]: Leaving directory '/bitcoin-core'
 6/bin/bash ../libtool  --tag=CXX --preserve-dup-deps  --mode=link /usr/bin/ccache g++ -std=c++17 -fdebug-prefix-map=/bitcoin-core/src=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wswitch -Wredundant-decls -Wunused-variable -Wdate-time -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-deprecated-copy    -fPIE -g -O2 -fno-extended-identifiers   -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -pie     -pthread -lpthread -static  -o test/test_bitcoin test/test_bitcoin-main.o  wallet/test/test_test_bitcoin-wallet_test_fixture.o wallet/test/test_test_bitcoin-init_test_fixture.o test/test_bitcoin-arith_uint256_tests.o test/test_bitcoin-addrman_tests.o test/test_bitcoin-amount_tests.o test/test_bitcoin-allocator_tests.o test/test_bitcoin-base32_tests.o test/test_bitcoin-base58_tests.o test/test_bitcoin-base64_tests.o test/test_bitcoin-bech32_tests.o test/test_bitcoin-bip32_tests.o test/test_bitcoin-blockchain_tests.o test/test_bitcoin-blockencodings_tests.o test/test_bitcoin-blockfilter_tests.o test/test_bitcoin-blockfilter_index_tests.o test/test_bitcoin-bloom_tests.o test/test_bitcoin-bswap_tests.o test/test_bitcoin-checkqueue_tests.o test/test_bitcoin-coins_tests.o test/test_bitcoin-coinstatsindex_tests.o test/test_bitcoin-compilerbug_tests.o test/test_bitcoin-compress_tests.o test/test_bitcoin-crypto_tests.o test/test_bitcoin-cuckoocache_tests.o test/test_bitcoin-denialofservice_tests.o test/test_bitcoin-descriptor_tests.o test/test_bitcoin-flatfile_tests.o test/test_bitcoin-fs_tests.o test/test_bitcoin-getarg_tests.o test/test_bitcoin-hash_tests.o test/test_bitcoin-i2p_tests.o test/test_bitcoin-interfaces_tests.o test/test_bitcoin-key_io_tests.o test/test_bitcoin-key_tests.o test/test_bitcoin-logging_tests.o test/test_bitcoin-dbwrapper_tests.o test/test_bitcoin-validation_tests.o test/test_bitcoin-mempool_tests.o test/test_bitcoin-merkle_tests.o test/test_bitcoin-merkleblock_tests.o test/test_bitcoin-miner_tests.o test/test_bitcoin-multisig_tests.o test/test_bitcoin-net_peer_eviction_tests.o test/test_bitcoin-net_tests.o test/test_bitcoin-netbase_tests.o test/test_bitcoin-pmt_tests.o test/test_bitcoin-policy_fee_tests.o test/test_bitcoin-policyestimator_tests.o test/test_bitcoin-pow_tests.o test/test_bitcoin-prevector_tests.o test/test_bitcoin-raii_event_tests.o test/test_bitcoin-random_tests.o test/test_bitcoin-reverselock_tests.o test/test_bitcoin-rpc_tests.o test/test_bitcoin-sanity_tests.o test/test_bitcoin-scheduler_tests.o test/test_bitcoin-script_p2sh_tests.o test/test_bitcoin-script_tests.o test/test_bitcoin-script_standard_tests.o test/test_bitcoin-scriptnum_tests.o test/test_bitcoin-serfloat_tests.o test/test_bitcoin-serialize_tests.o test/test_bitcoin-settings_tests.o test/test_bitcoin-sighash_tests.o test/test_bitcoin-sigopcount_tests.o test/test_bitcoin-skiplist_tests.o test/test_bitcoin-sock_tests.o test/test_bitcoin-streams_tests.o test/test_bitcoin-sync_tests.o test/test_bitcoin-system_tests.o test/test_bitcoin-util_threadnames_tests.o test/test_bitcoin-timedata_tests.o test/test_bitcoin-torcontrol_tests.o test/test_bitcoin-transaction_tests.o test/test_bitcoin-txindex_tests.o test/test_bitcoin-txrequest_tests.o test/test_bitcoin-txvalidation_tests.o test/test_bitcoin-txvalidationcache_tests.o test/test_bitcoin-uint256_tests.o test/test_bitcoin-util_tests.o test/test_bitcoin-validation_block_tests.o test/test_bitcoin-validation_chainstate_tests.o test/test_bitcoin-validation_chainstatemanager_tests.o test/test_bitcoin-validation_flush_tests.o test/test_bitcoin-validationinterface_tests.o test/test_bitcoin-versionbits_tests.o wallet/test/test_test_bitcoin-psbt_wallet_tests.o wallet/test/test_test_bitcoin-wallet_tests.o wallet/test/test_test_bitcoin-walletdb_tests.o wallet/test/test_test_bitcoin-wallet_crypto_tests.o wallet/test/test_test_bitcoin-coinselector_tests.o wallet/test/test_test_bitcoin-init_tests.o wallet/test/test_test_bitcoin-ismine_tests.o wallet/test/test_test_bitcoin-scriptpubkeyman_tests.o wallet/test/test_test_bitcoin-db_tests.o     libtest_util.a libbitcoin_server.a libbitcoin_common.a libbitcoin_util.a crypto/libbitcoin_crypto_base.a libbitcoin_wallet.a libbitcoin_server.a libbitcoin_cli.a libbitcoin_common.a libbitcoin_util.a libbitcoin_consensus.a crypto/libbitcoin_crypto_base.a crypto/libbitcoin_crypto_sse41.a crypto/libbitcoin_crypto_avx2.a crypto/libbitcoin_crypto_shani.a univalue/libunivalue.la leveldb/libleveldb.a crc32c/libcrc32c.a crc32c/libcrc32c_sse42.a   leveldb/libmemenv.a -L/usr/lib/x86_64-linux-gnu -lboost_system -lboost_filesystem -lboost_unit_test_framework secp256k1/libsecp256k1.la -levent -levent_pthreads -levent -ldb_cxx   -lsqlite3  
 7libtool: link: /usr/bin/ccache g++ -std=c++17 -fdebug-prefix-map=/bitcoin-core/src=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wall -Wextra -Wformat -Wformat-security -Wvla -Wswitch -Wredundant-decls -Wunused-variable -Wdate-time -Wsign-compare -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-deprecated-copy -fPIE -g -O2 -fno-extended-identifiers -Wl,-z -Wl,relro -Wl,-z -Wl,now -Wl,-z -Wl,separate-code -pie -pthread -o test/test_bitcoin test/test_bitcoin-main.o wallet/test/test_test_bitcoin-wallet_test_fixture.o wallet/test/test_test_bitcoin-init_test_fixture.o test/test_bitcoin-arith_uint256_tests.o test/test_bitcoin-addrman_tests.o test/test_bitcoin-amount_tests.o test/test_bitcoin-allocator_tests.o test/test_bitcoin-base32_tests.o test/test_bitcoin-base58_tests.o test/test_bitcoin-base64_tests.o test/test_bitcoin-bech32_tests.o test/test_bitcoin-bip32_tests.o test/test_bitcoin-blockchain_tests.o test/test_bitcoin-blockencodings_tests.o test/test_bitcoin-blockfilter_tests.o test/test_bitcoin-blockfilter_index_tests.o test/test_bitcoin-bloom_tests.o test/test_bitcoin-bswap_tests.o test/test_bitcoin-checkqueue_tests.o test/test_bitcoin-coins_tests.o test/test_bitcoin-coinstatsindex_tests.o test/test_bitcoin-compilerbug_tests.o test/test_bitcoin-compress_tests.o test/test_bitcoin-crypto_tests.o test/test_bitcoin-cuckoocache_tests.o test/test_bitcoin-denialofservice_tests.o test/test_bitcoin-descriptor_tests.o test/test_bitcoin-flatfile_tests.o test/test_bitcoin-fs_tests.o test/test_bitcoin-getarg_tests.o test/test_bitcoin-hash_tests.o test/test_bitcoin-i2p_tests.o test/test_bitcoin-interfaces_tests.o test/test_bitcoin-key_io_tests.o test/test_bitcoin-key_tests.o test/test_bitcoin-logging_tests.o test/test_bitcoin-dbwrapper_tests.o test/test_bitcoin-validation_tests.o test/test_bitcoin-mempool_tests.o test/test_bitcoin-merkle_tests.o test/test_bitcoin-merkleblock_tests.o test/test_bitcoin-miner_tests.o test/test_bitcoin-multisig_tests.o test/test_bitcoin-net_peer_eviction_tests.o test/test_bitcoin-net_tests.o test/test_bitcoin-netbase_tests.o test/test_bitcoin-pmt_tests.o test/test_bitcoin-policy_fee_tests.o test/test_bitcoin-policyestimator_tests.o test/test_bitcoin-pow_tests.o test/test_bitcoin-prevector_tests.o test/test_bitcoin-raii_event_tests.o test/test_bitcoin-random_tests.o test/test_bitcoin-reverselock_tests.o test/test_bitcoin-rpc_tests.o test/test_bitcoin-sanity_tests.o test/test_bitcoin-scheduler_tests.o test/test_bitcoin-script_p2sh_tests.o test/test_bitcoin-script_tests.o test/test_bitcoin-script_standard_tests.o test/test_bitcoin-scriptnum_tests.o test/test_bitcoin-serfloat_tests.o test/test_bitcoin-serialize_tests.o test/test_bitcoin-settings_tests.o test/test_bitcoin-sighash_tests.o test/test_bitcoin-sigopcount_tests.o test/test_bitcoin-skiplist_tests.o test/test_bitcoin-sock_tests.o test/test_bitcoin-streams_tests.o test/test_bitcoin-sync_tests.o test/test_bitcoin-system_tests.o test/test_bitcoin-util_threadnames_tests.o test/test_bitcoin-timedata_tests.o test/test_bitcoin-torcontrol_tests.o test/test_bitcoin-transaction_tests.o test/test_bitcoin-txindex_tests.o test/test_bitcoin-txrequest_tests.o test/test_bitcoin-txvalidation_tests.o test/test_bitcoin-txvalidationcache_tests.o test/test_bitcoin-uint256_tests.o test/test_bitcoin-util_tests.o test/test_bitcoin-validation_block_tests.o test/test_bitcoin-validation_chainstate_tests.o test/test_bitcoin-validation_chainstatemanager_tests.o test/test_bitcoin-validation_flush_tests.o test/test_bitcoin-validationinterface_tests.o test/test_bitcoin-versionbits_tests.o wallet/test/test_test_bitcoin-psbt_wallet_tests.o wallet/test/test_test_bitcoin-wallet_tests.o wallet/test/test_test_bitcoin-walletdb_tests.o wallet/test/test_test_bitcoin-wallet_crypto_tests.o wallet/test/test_test_bitcoin-coinselector_tests.o wallet/test/test_test_bitcoin-init_tests.o wallet/test/test_test_bitcoin-ismine_tests.o wallet/test/test_test_bitcoin-scriptpubkeyman_tests.o wallet/test/test_test_bitcoin-db_tests.o  -lpthread libtest_util.a libbitcoin_server.a libbitcoin_common.a libbitcoin_util.a crypto/libbitcoin_crypto_base.a libbitcoin_wallet.a libbitcoin_server.a libbitcoin_cli.a libbitcoin_common.a libbitcoin_util.a libbitcoin_consensus.a crypto/libbitcoin_crypto_base.a crypto/libbitcoin_crypto_sse41.a crypto/libbitcoin_crypto_avx2.a crypto/libbitcoin_crypto_shani.a univalue/.libs/libunivalue.a leveldb/libleveldb.a crc32c/libcrc32c.a crc32c/libcrc32c_sse42.a leveldb/libmemenv.a -L/usr/lib/x86_64-linux-gnu -lboost_system -lboost_filesystem -lboost_unit_test_framework secp256k1/.libs/libsecp256k1.a -levent -levent_pthreads -levent -ldb_cxx -lsqlite3 -pthread
 8/usr/bin/ld: test/test_bitcoin-validation_chainstatemanager_tests.o:././test/util/chainstate.h:19: multiple definition of `NoMalleation'; test/test_bitcoin-validation_chainstate_tests.o:././test/util/chainstate.h:19: first defined here
 9collect2: error: ld returned 1 exit status
10make[2]: *** [Makefile:6405: test/test_bitcoin] Error 1
11make[2]: Leaving directory '/bitcoin-core/src'
12make[1]: *** [Makefile:16184: all-recursive] Error 1
13make[1]: Leaving directory '/bitcoin-core/src'
14make: *** [Makefile:824: all-recursive] Error 1

in src/validation.cpp:2240 in b3825c27bb outdated

2235+
2236+    // The remainder of the function isn't relevant if we are not acting on
2237+    // the active chainstate, so return if need be.
2238+    if (this != &m_chainman.ActiveChainstate()) {
2239+        // Only log every so often so that we don't bury log messages at the tip.
2240+        constexpr int BACKGROUND_LOG_INTERVAL = 2000;

JeremyRubin commented at 5:01 pm on September 15, 2021:

should this be configurable (e.g., in case needed for debugging?)

jamesob commented at 9:57 pm on September 15, 2021:

Yeah that might be nice, though it’d feel hamfisted to have a CLI option just for that I think. Probably something to think about for a followup.

in src/validation.h:112 in b3825c27bb outdated

108@@ -109,6 +109,7 @@ extern RecursiveMutex cs_main;
109 typedef std::unordered_map<uint256, CBlockIndex*, BlockHasher> BlockMap;
110 extern Mutex g_best_block_mutex;
111 extern std::condition_variable g_best_block_cv;
112+/** Used to notify getblocktemplate RPC of new tips. */

JeremyRubin commented at 5:03 pm on September 15, 2021:

nit: why this comment here?

jamesob commented at 7:43 pm on September 15, 2021:

I noticed it was undocumented and this is set by UpdateTip so it’s tangentially related… but agree it looks confusing in this commit. I’ll break it out into a separate commit.

in src/chain.h:325 in ae115458a1 outdated

322+        if (nStatus & BLOCK_FAILED_MASK) return false;
323+
324         if ((nStatus & BLOCK_VALID_MASK) < nUpTo) {
325+            // If this block had been marked assumed-valid and we're raising
326+            // its validity to a certain point, there is no longer an assumption.
327+            if (nStatus & BLOCK_ASSUMED_VALID && nUpTo >= BLOCK_VALID_SCRIPTS) {

JeremyRubin commented at 5:16 pm on September 15, 2021:

nit: an invalid nUpTo such as 6 or 7 could corrupt the result here. Consider 4|3 which is (BLOCK_VALID_CHAIN|BLOCK_VALID_TRANSACTIONS). I don’t think this can actually happen though.

It might be better to explicitly check nUpTo is either exactly BLOCK_VALID_SCRIPTS?

jamesob commented at 10:21 pm on September 15, 2021:

This is a really good point, but I think we’ve got bigger problems if there’s an invalid nUpTo being passed (that’s still within the BLOCK_VALID_MASK range as you note; for subsequent reviewers: see the assert at the beginning of the function).

I think if we were to address this, it’d be a more general fix of clamping down this function and others like IsValid() with a ceiling check (at the moment BLOCK_VALID_SCRIPTS = 5); then of course that ceiling would need to be updated in sync with any addtional BLOCK_VALID_* additions. So anyway I think that’s best treated as a follow-up.

in src/validation.cpp:5063 in 4907b8673a outdated

5018         // won't ask to rewind the entire assumed-valid chain on startup.
5019         if (index->pprev && DeploymentActiveAt(*index, ::Params().GetConsensus(), Consensus::DEPLOYMENT_SEGWIT)) {
5020             index->nStatus |= BLOCK_OPT_WITNESS;
5021         }
5022+
5023+        setDirtyBlockIndex.insert(index);

JeremyRubin commented at 5:20 pm on September 15, 2021:

shouldn’t we be tracking if we actually made any changes before marking dirty?

jamesob commented at 10:24 pm on September 15, 2021:

Syncing the entire block index is negligible in comparison to syncing the coins, and we have to do both at the end of snapshot load, so I figured better to err on the side of over-syncing.

achow101 commented at 6:19 pm on September 15, 2021: member

@achow101 did you try clearing ccache? I’ve found lately that sometimes it’s overzealous in caching and can cause problems. If you haven’t, give ccache -C a try before building.

Still fails with the same error.

doc: add comment for g_best_block 665072a36d

validation: change UpdateTip for multiple chainstates

Only perform certain behavior (namely that related to servicing
the getblocktemplate RPC call) for the active chainstate when
calling UpdateTip.

Co-authored-by: Jon Atack <jon@atack.com>

b217020df7

chain: add BLOCK_ASSUMED_VALID for use with assumeutxo

Instead of (ab)using the existing BLOCK_VALID_* flags to mark CBlockIndex entries which
we haven't yet fully validated (but assume validity for use with UTXO snapshot
loading), introduce a status flag that specifically marks an assumed-valid state.

This state is then removed in RaiseValidity() when the block has actually been
validated.

This distinction will allow us to make the necessary changes to various parts of the
system to facilitate assumeutxo/background chainstate validation but without leaking
details like snapshot height, as we had done previously.

Changes that actually make use of this flag follow in future commits.

42b2520db9

validation: set BLOCK_ASSUMED_VALID during snapshot load

Mark the block index entries that are beneath the snapshot base block as
assumed-valid.  Subsequent commits will make use of this flag in other
parts of the system.

01a9b8fe71

validation: insert assumed-valid block index entries into candidates 5a807736da

validation: fix CheckBlockIndex for multiple chainstates

Adjust CheckBlockIndex to account for
- assumed-valid block indexes lacking transaction data, and
- setBlockIndexCandidates for the background chainstate not containing certain entries
  which rely on assumed-valid ancestors.

8f5710fd0a

move-only: unittest: add test/util/chainstate.h

and move `CreateAndActivateUTXOSnapshot()` into it for reuse
in future test modules.

071200993f

test: refactor: declare NoMalleation const auto

To avoid linker error on some platforms:
    https://github.com/bitcoin/bitcoin/pull/21526#discussion_r709404714

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

298bf5d563

test: refactor: separate CreateBlock in TestChain100Setup

This is so we can create blocks within unittests and have them
be processed by specific chainstates (instead of the just the
active one).

2705570109

test: validation: add unittest for UpdateTip behavior 673a5bd337

jamesob force-pushed on Sep 15, 2021

achow101 commented at 10:44 pm on September 15, 2021: member

ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c

jamesob commented at 4:46 pm on September 16, 2021: member

Okay, thanks everyone for bearing with the build issues. I think I’ve addressed all feedback here; in the latest change I’ve

made the auto -> const auto change (thanks again @ryanofsky), and
moved the g_best_block documentation change into its own commit (thanks @JeremyRubin).

I think this is in good shape pending some reACKs.

jonatack commented at 5:02 pm on September 16, 2021: member

@jamesob in case it’s accidental, just noticed one commit (b217020df78b) has a different email for you than the others.

Code-review re-ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c reviewed diff, rebased to master/debug build/ran unit+functional tests

ryanofsky approved

ryanofsky commented at 6:08 pm on September 16, 2021: member

Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c. Just linker fix and split commit changes mentioned #21526 (comment) since last review

naumenkogs commented at 8:48 am on September 17, 2021: member

ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c

benthecarman approved

benthecarman commented at 11:23 pm on September 18, 2021: contributor

ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c

fjahr commented at 8:10 pm on September 19, 2021: member

Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c

ariard commented at 11:56 pm on September 19, 2021: member

utACK 673a5bd3

ryanofsky commented at 2:11 pm on September 22, 2021: member

Anything holding this up for merge at this point?

laanwj merged this on Sep 23, 2021

laanwj closed this on Sep 23, 2021

sidhujag referenced this in commit 41eedca916 on Sep 24, 2021

fanquake referenced this in commit f3f027fc88 on Sep 24, 2021

in src/chain.h:135 in 673a5bd337

131+
132+    /**
133+     * If set, this indicates that the block index entry is assumed-valid.
134+     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
135+     * It almost certainly means that the block's full validation is pending
136+     * on a background chainstate. See `doc/assumeutxo.md`.

MarcoFalke commented at 12:11 pm on September 24, 2021:

There is no such document. Also, it would be good to explain how the unlikely case where this is not pending on background validation might happen. Even more so when this comment contradicts the very next comment (IsAssumedValid).

laanwj removed this from the "Blockers" column in a project

fanquake moved this from the "In progress" to the "Done" column in a project

rebroad referenced this in commit 2f9c0f2acf on Oct 13, 2021

MarcoFalke referenced this in commit 23ae7931be on Nov 3, 2021

janus referenced this in commit 4c0f1d6b83 on Nov 11, 2021

Fabcien referenced this in commit bd99c7d928 on Oct 17, 2022

Fabcien referenced this in commit 08dd1b2dff on Oct 17, 2022

Fabcien referenced this in commit 954fc73925 on Oct 17, 2022

Fabcien referenced this in commit 7db9e191cd on Oct 19, 2022

Fabcien referenced this in commit a1093fa5f5 on Oct 19, 2022

DrahtBot locked this on Oct 30, 2022

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Conflicts