validation: UTXO snapshot activation

jamesob commented at 6:49 pm on August 25, 2020: member

Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

This change proposes logic for activating UTXO snapshots, which is unused at the moment aside from an included unittest. There are a few moveonyish/refactoring commits to allow for halfway decent unittests.

Basic structure is included for specifying and checking the assumeutxo hash values used to validate activated snapshots. Initially I had specified a few height/hash pairs for mainnet in this change, but because of the security-critical nature of those parameters, I figured it was better to leave their inclusion to a future PR that includes only that change - my intent being that reviewers will be more likely to verify those parameters firsthand in a dedicated PR.

Aside from that and the snapshot activation logic, there are a few related changes:

allow caching the nChainTx value in the CCoinsViewDB; this is set during snapshot activation. Because we don’t necessarily have access to the full chain at the time of snapshot load, this value is communicated through the snapshot metadata and must be cached within the chainstate to survive restarts.
break out CreateUTXOSnapshot() from dumptxoutset. This is essentially a move-only change to allow the reuse of snapshot creation logic from within unittests.
…and a few other misc. changes that are solely related to unittests.

The move-onlyish commit is most easily reviewed with --color-moved=zebra.

jamesob force-pushed on Aug 25, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 25, 2020

DrahtBot added the label UTXO Db and Indexes on Aug 25, 2020

DrahtBot added the label Validation on Aug 25, 2020

DrahtBot commented at 1:08 am on August 26, 2020: 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:

#21121 ([test] Small unit test improvements, including helper to make mempool transaction by amitiuttarwar)
#21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
#21003 (test: Move MakeNoLogFileContext to libtest_util, and use it in bench by MarcoFalke)
#20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
#19888 (rpc: Fix getblockstats issues by fjahr)
#19652 (Avoid locking CTxMemPool::cs recursively in Mempool{Info}ToJSON() by hebasto)
#13875 ([doc] nChainTx needs to become a 64-bit earlier due to SegWit by Sjors)
#9384 (CCoinsViewCache code cleanup & deduplication by ryanofsky)

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.

jamesob force-pushed on Aug 26, 2020

DrahtBot added the label Needs rebase on Aug 26, 2020

jamesob force-pushed on Aug 26, 2020

DrahtBot removed the label Needs rebase on Aug 26, 2020

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

in src/validation.cpp:5274 in d6396c4be4 outdated

5269@@ -5269,6 +5270,34 @@ CChainState& ChainstateManager::InitializeChainstate(const uint256& snapshot_blo
5270     return *to_modify;
5271 }
5272 
5273+/**
5274+ * Return the expected assumeutxo value for a given height, if one exists.

fjahr commented at 8:35 am on August 29, 2020:

What is your reasoning behind calling the utxo set hash assumeutxo here? It makes it harder for me to reason about but i may be in the minority and this is easier for the casual user. Was that your intention? If we have different hashes in the future and they might be used for assumeutxo as well it would be probably best to use the explicit name for this type of hash hash_serialized_2.

jamesob commented at 3:53 am on August 30, 2020:

It was for consistency with assumevalid but I’m happy to rename it in lieu of something better.

in src/validation.cpp:5285 in d6396c4be4 outdated

5280+ */
5281+static bool ExpectedAssumeutxo(int height, uint256& expected_out)
5282+{
5283+    const CChainParams& params = ::Params();
5284+
5285+    if (params.NetworkIDString() == "regtest") {

fjahr commented at 8:36 am on August 29, 2020:

This makes me a bit uncomfortable because we can’t test the actual functionality of this function with this. Do you plan to keep this or change it in a follow-up with changes to the testing framework that allow for a better test?

fjahr commented at 1:28 pm on August 29, 2020:

One alternative way would be hardcode a pre-mined chain into the test and use it, similar to what signet does here: https://github.com/bitcoin/bitcoin/pull/18267/commits/6206c2e8e87fcc70848e4a0fab871d5fd9ea6b16

jamesob commented at 3:50 am on August 30, 2020:

Thanks for the look. This is good feedback; I’ll see if I can get a seed-based deterministic generation of a reg test chain and then add some values/tests based on that. When I originally wrote this I think I’d forgotten that we can mock time.

MarcoFalke commented at 5:03 pm on August 31, 2020:

Agree that regtest should have an assumeutxo hash baked in. Maybe we could even get the 200-blocks premine to be deterministic and use that?

MarcoFalke commented at 5:40 pm on August 31, 2020:

If nothing else, you can take the hash from ./test/functional/rpc_dumptxoutset.py

jamesob commented at 10:25 pm on August 31, 2020:

I’ve updated the regtest assumeutxo values for two separate heights and added some utilities to the unittest framework for generating a deterministic chain, so we now have some decent tests in place for this.

in src/validation.cpp:5346 in bdf182e81e outdated

5341+        snapshot_chainstate->InitCoinsDB(
5342+            current_coinsdb_cache_size * snapshot_cache_perc, in_memory, false, "chainstate");
5343+        snapshot_chainstate->InitCoinsCache(current_coinstip_cache_size * snapshot_cache_perc);
5344+    }
5345+
5346+    bool snapshot_ok = this->PopulateAndValidateSnapshot(

fjahr commented at 10:16 am on August 29, 2020:

0    const bool snapshot_ok = this->PopulateAndValidateSnapshot(

in src/txdb.cpp:430 in 3cd76cd1b2 outdated

421+{
422+    // We choose 1 and not 0 because, in the unlikely event that we can't read
423+    // a value from this key, we don't want LoadBlockIndex() to malfunction for
424+    // snapshot chainstates being loaded. Returning 1 here will break the
425+    // progress= measure, but returning 0 would cause us to not be able to add
426+    // chain tips for the snapshot chainstate. This shouldn't happen and is

fjahr commented at 10:42 am on August 29, 2020:

I assume it wouldn’t work because of a divide by zero error in the progress function? Shouldn’t that be rather dealt with at that layer?

ryanofsky commented at 10:21 pm on September 2, 2020:

I assume it wouldn’t work because of a divide by zero error in the progress function? Shouldn’t that be rather dealt with at that layer?

Would agree that handling this in LoadBlockIndex would be preferable to having to hardcode 1’s here and in ChainstateManager::GetSnapshotNChainTx along with comments describing other layers of code. Both CCoinsViewDB::GetNChainTx and ChainstateManager::GetSnapshotNChainTx could return Optional<int> to avoid the need to hardcode something

fjahr commented at 10:48 am on August 29, 2020: member

Concept ACK

Just a few questions and comments from a first pass, code looks already in good shape. Will test soon.

in src/validation.cpp:5283 in d6396c4be4 outdated

5278+ *
5279+ * @returns bool - false if no assumeutxo value exists for the given height.
5280+ */
5281+static bool ExpectedAssumeutxo(int height, uint256& expected_out)
5282+{
5283+    const CChainParams& params = ::Params();

MarcoFalke commented at 5:01 pm on August 31, 2020:

in commit d6396c4be4

For pure-utility functions it would be good to pass in params instead of relying on the global

jamesob commented at 10:25 pm on August 31, 2020:

Done

in src/txdb.cpp:33 in 3cd76cd1b2 outdated

27@@ -28,6 +28,10 @@ static const char DB_FLAG = 'F';
28 static const char DB_REINDEX_FLAG = 'R';
29 static const char DB_LAST_BLOCK = 'l';
30 
31+//! Used to cache the nChainTx value for the base block of a UTXO snapshot.
32+//! This only contains a value for chainstates created based on a snapshot.
33+static constexpr char DB_NCHAINTX = 'n';

MarcoFalke commented at 5:12 pm on August 31, 2020:

Can’t this be hardcoded beside the blockheight and assumeutxo hash in the chain params?

(If not, I’ll need to think more about how the txdb value for nchaintx interacts with major version upgrades, which bump the assumeutxo)

ryanofsky commented at 9:34 pm on September 2, 2020:

Can’t this be hardcoded beside the blockheight and assumeutxo hash in the chain params?

(If not, I’ll need to think more about how the txdb value for nchaintx interacts with major version upgrades, which bump the assumeutxo)

Hardcoding this seems does seem like it might be a good simplification. It also seems like hardcoding might be useful for validating the snapshot? Otherwise, would a bad SnapshotMetadata::m_nchaintx value in the snapshot go undetected?

Another place this value could be stored might is in chainstate_ directory name alongside the block hash, but maybe that is too verbose.

Anyway, txdb does seem like a reasonable place to keep this number, especially if it can be checked and removed when the background sync completes.

jamesob commented at 11:24 pm on September 2, 2020:

Yep, I think @MarcoFalke’s approach is the right one. Will implement tomorrow.

in src/coins.h:236 in bdf182e81e outdated

232@@ -232,6 +233,9 @@ class CCoinsViewBacked : public CCoinsView
233 };
234 
235 
236+class ChainstateManager;

MarcoFalke commented at 5:13 pm on August 31, 2020:

in commit: bdf182e81e

forward decls right after the includes, pls. kthx

in src/validation.h:859 in bdf182e81e outdated

854+    //!   faking nTx* block index data along the way.
855+    //!
856+    //! - Move the new chainstate to `m_snapshot_chainstate` and make it our
857+    //!   ChainstateActive().
858+    NODISCARD bool ActivateSnapshot(
859+        CAutoFile* coins_file, SnapshotMetadata metadata, bool in_memory);

MarcoFalke commented at 5:18 pm on August 31, 2020:

in commit bdf182e81e

this can’t be nullptr, so & seems more appropriate. Also the metadata should be read-only here?

0        CAutoFile& coins_file, const SnapshotMetadata& metadata, bool in_memory);

jamesob commented at 10:26 pm on August 31, 2020:

Yup, good call. Done.

in src/validation.cpp:5315 in bdf182e81e outdated

5310+    int64_t current_coinsdb_cache_size{0};
5311+    int64_t current_coinstip_cache_size{0};
5312+
5313+    // Cache percentages to allocate to each chainstate.
5314+    constexpr float ibd_cache_perc = 0.01;
5315+    constexpr float snapshot_cache_perc = 0.99;

MarcoFalke commented at 5:21 pm on August 31, 2020:

nit: compile time constants can be UPPER_CASE

in src/validation.cpp:5328 in bdf182e81e outdated

5323+        // `MaybeRebalanceCaches()` once we're done with this function to ensure
5324+        // the right allocation (including the possibility that no snapshot was activated
5325+        // and that we should restore the active chainstate caches to their original size).
5326+        //
5327+        current_coinsdb_cache_size = ::ChainstateActive().m_coinsdb_cache_size_bytes;
5328+        current_coinstip_cache_size = ::ChainstateActive().m_coinstip_cache_size_bytes;

MarcoFalke commented at 5:22 pm on August 31, 2020:

nit: Please prefer the identical ChainstateManager::ActiveChainstate

jamesob commented at 10:26 pm on August 31, 2020:

Oof, dumb move on my part. Thanks, fixed.

in src/validation.cpp:5337 in bdf182e81e outdated

5332+        ::ChainstateActive().ResizeCoinsCaches(
5333+            current_coinstip_cache_size * ibd_cache_perc,
5334+            current_coinsdb_cache_size * ibd_cache_perc);
5335+    }
5336+
5337+    auto snapshot_chainstate = MakeUnique<CChainState>(m_blockman, base_blockhash);

MarcoFalke commented at 5:24 pm on August 31, 2020:

0    auto snapshot_chainstate = MakeUnique<CChainState>(mempool, m_blockman, base_blockhash);

needs rebase

jamesob commented at 10:26 pm on August 31, 2020:

Fixed, thanks.

in src/validation.cpp:5463 in bdf182e81e outdated

5458+    coins_cache.Flush(); // TODO: if #17487 is merged, add erase=false here for better performance.
5459+
5460+    assert(coins_cache.GetBestBlock() == base_blockhash);
5461+
5462+    CCoinsStats stats;
5463+    if (!GetUTXOStats(&snapshot_chainstate.CoinsDB(), stats, CoinStatsHashType::HASH_SERIALIZED, [] {})) {

MarcoFalke commented at 5:32 pm on August 31, 2020:

0    if (!GetUTXOStats(&snapshot_chainstate.CoinsDB(), stats, CoinStatsHashType::HASH_SERIALIZED, [] {/*TODO*/})) {

could make sense to make this interruptible if it takes a long time

in src/validation.cpp:5417 in bdf182e81e outdated

5475+    while (max_secs_to_wait_for_headers > 0) {
5476+        snapshot_start_block = LookupBlockIndex(base_blockhash);
5477+        max_secs_to_wait_for_headers -= 1;
5478+
5479+        if (!snapshot_start_block) {
5480+            std::this_thread::sleep_for(std::chrono::seconds(1));

MarcoFalke commented at 5:35 pm on August 31, 2020:

So in other words this will deadlock with -nonetworkactive or -noconnect?

If yes, what about including the preceding headers in the snapshot? I know we had a discussion about that, but I forget the result. Please remind me :sweat_smile:

jamesob commented at 10:29 pm on August 31, 2020:

Well, if by “deadlock” you mean “sleep-loop for ten minutes and then fail the activation,” I guess so. But I wouldn’t call this a deadlock personally.

Maybe others can chime in here but IMO it still makes sense to obtain headers from the network, since ultimately what use is a snapshot without network connectivity and peers to get you to the tip of the chain? There may have been a more concrete reason but I can’t remember it.

MarcoFalke commented at 5:34 am on September 1, 2020:

Oh I missed the max_secs_to_wait_for_headers :sleeping:

MarcoFalke commented at 5:38 pm on August 31, 2020: member

Concept ACK 2cb1f842dc, but I had some approach questions. Didn’t closely review, nor did I review the tests.

jamesob force-pushed on Aug 31, 2020

in src/test/validation_tests.cpp:81 in 1aa7b91370 outdated

73@@ -74,4 +74,43 @@ BOOST_AUTO_TEST_CASE(test_combiner_all)
74     Test.disconnect(&ReturnTrue);
75     BOOST_CHECK(Test());
76 }
77+
78+BOOST_AUTO_TEST_SUITE_END()
79+
80+
81+BOOST_FIXTURE_TEST_SUITE(validation_tests_regtest, RegTestingSetup)

fjahr commented at 1:15 pm on September 1, 2020:

0The test suite in file src/test/foo_tests.cpp should be named
1"foo_tests". Please make sure the following test suites follow
2that convention:
3src/test/validation_tests.cpp:BOOST_FIXTURE_TEST_SUITE(validation_tests_regtest, RegTestingSetup)
4^---- failure generated from test/lint/lint-tests.sh

jamesob force-pushed on Sep 1, 2020

in src/node/coinstats.cpp:27 in 60bd91334a outdated

23@@ -24,6 +24,9 @@ static uint64_t GetBogoSize(const CScript& scriptPubKey)
24            scriptPubKey.size() /* scriptPubKey */;
25 }
26 
27+//! XXX: be very careful when changing this! assumeutuxo and UTXO snapshot

ryanofsky commented at 7:22 pm on September 2, 2020:

In commit “add allowed assumeutxo values” (60bd91334a3654fe59dac0bd28fb8824b897edf7)

Minor: Suggest s/XXX/Warning/. Some editors treat XXX like TODO

jamesob commented at 6:55 pm on October 16, 2020:

Done.

in src/validation.h:966 in 60bd91334a outdated

961+ * @param height[in]         Get the assumeutxo value for this height.
962+ * @param expected_out[out]  Set to the expected assumeutxo hash value if one exists.
963+ *
964+ * @returns bool - false if no assumeutxo value exists for the given height.
965+ */
966+bool ExpectedAssumeutxo(const int height, uint256& expected_out, const CChainParams& params);

ryanofsky commented at 8:34 pm on September 2, 2020:

In commit “add allowed assumeutxo values” (60bd91334a3654fe59dac0bd28fb8824b897edf7)

Note: unit test coverage for this function is added in later commit “tests: add unittest for ExpectedAssumeutxo”

Just a suggestion, but I think it would be nice to combine these two commits. Adding new tests along with code makes it easier to:

Understand the code change (seeing how a function is called can be even more helpful than seeing how it’s defined)
Check that test coverage is adequate
Check that intermediate commits aren’t broken

jamesob commented at 6:55 pm on October 16, 2020:

Done.

in src/txdb.cpp:32 in bc622c33ce outdated

27@@ -28,6 +28,10 @@ static const char DB_FLAG = 'F';
28 static const char DB_REINDEX_FLAG = 'R';
29 static const char DB_LAST_BLOCK = 'l';
30 
31+//! Used to cache the nChainTx value for the base block of a UTXO snapshot.
32+//! This only contains a value for chainstates created based on a snapshot.

ryanofsky commented at 10:02 pm on September 2, 2020:

In commit “txdb: add nChainTx cache for snapshot chainstates” (bc622c33ce925159906e3b00be77b11d598397ed)

“Only contains a value” suggests the row may be present without a value. Would suggest saying something like “This is not present in the normal coins database. It is only saved and used in in temporary UTXO snapshots.”

in src/txdb.cpp:31 in bc622c33ce outdated

27@@ -28,6 +28,10 @@ static const char DB_FLAG = 'F';
28 static const char DB_REINDEX_FLAG = 'R';
29 static const char DB_LAST_BLOCK = 'l';
30 
31+//! Used to cache the nChainTx value for the base block of a UTXO snapshot.

ryanofsky commented at 10:10 pm on September 2, 2020:

In commit “txdb: add nChainTx cache for snapshot chainstates” (bc622c33ce925159906e3b00be77b11d598397ed)

Can you remind what this is used for? Just estimating sync progress, or other things too? Would also be good to make comment more specific, maybe “Cached CBlockIndex::nChainTx value (number of transactions in chain) at the base block of a UTXO snapshot that is being imported. This is used to […]”

ryanofsky commented at 10:29 pm on September 2, 2020: member

Started review (will update list below with progress)

60bd91334a3654fe59dac0bd28fb8824b897edf7 add allowed assumeutxo values (1/9)
bc622c33ce925159906e3b00be77b11d598397ed txdb: add nChainTx cache for snapshot chainstates (2/9)
9c1db71dbd82342b0a1d33401b329e34a7a6cee8 validation: add ChainstateManager::ActivateSnapshot (3/9)
86032053d14bf454f94f4318ebfcbaf54bc0285e txdb: don’t reset during in-memory cache resize (4/9)
cf0c07340885c770a383e340ee09f922bb90a45b move-onlyish: break out CreateUTXOSnapshot from dumptxoutset (5/9)
75d1e7754274d3f015a49e71871c3a069cbe020b simplify ChainstateManager::SnapshotBlockhash() return semantics (6/9)
17f833d4c826eadf7992cdb22198cae70cba61d9 tests: add unittest for ExpectedAssumeutxo (7/9)
35861eb9ced11da85a9f3659437d60c1507b4075 tests: add deterministic chain generation unittest fixture (8/9)
89c9182ec0c81d38248fa92d0284fb4da2a71927 tests: add snapshot activation test (9/9)

in src/validation.cpp:5152 in 75d1e77542 outdated

5234@@ -5235,7 +5235,8 @@ class CMainCleanup
5235 static CMainCleanup instance_of_cmaincleanup;
5236 
5237 Optional<uint256> ChainstateManager::SnapshotBlockhash() const {
5238-    if (m_active_chainstate != nullptr) {
5239+    if (m_active_chainstate != nullptr &&
5240+            !m_active_chainstate->m_from_snapshot_blockhash.IsNull()) {

ryanofsky commented at 7:51 pm on September 4, 2020:

In commit “simplify ChainstateManager::SnapshotBlockhash() return semantics” (75d1e7754274d3f015a49e71871c3a069cbe020b)

There doesn’t seem to be any test coverage for this, or at least test seem to pass with change reverted. Would be good to have test coverage when changing a corner case.

ryanofsky commented at 10:15 pm on September 4, 2020:

In commit “simplify ChainstateManager::SnapshotBlockhash() return semantics” (75d1e7754274d3f015a49e71871c3a069cbe020b)

I think if all the calling code is going to continue to do .value_or(uint256()) it makes little sense for this to return an optional. Would be simpler to just use Optional<uint256> everywhere (make m_from_snapshot_blockhash an Optional<uint256> and stop treating zero hash specially) or use uint256 everywhere and keep treating zero hash specially. But having back and forth nullopt <-> null hash conversions doesn’t make sense over just picking one format and sticking with it.

jamesob commented at 3:09 pm on October 13, 2020:

Yeah I don’t disagree; initially had it that way but changed it at the request of a reviewer in a previous PR.

jamesob commented at 6:56 pm on October 16, 2020:

Thanks, added test coverage in the same commit.

jamesob commented at 6:57 pm on October 16, 2020:

Cleaned up the usages to actually make use of the option. Thanks.

in src/validation.cpp:5435 in 9c1db71dbd outdated

5430+    // Important that we set this. This and the cacheCoins accesses above are
5431+    // sort of a layer violation, but either we reach into the innards of
5432+    // CCoinsViewCache here or we have to invert some of the CChainState to
5433+    // embed them in a snapshot-activation-specific CCoinsViewCache bulk load
5434+    // method.
5435+    coins_cache.hashBlock = base_blockhash;

ryanofsky commented at 9:13 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

Would be good to use SetBestBlock so this is consistent and it’s clear this is overwriting random value above. Also it would be good to move this next to the Flush call below for consistency with the sequence above.

On layer violation comment, IMO it would be reasonable to write a CAutoFile -> CCoinsViewCache function and do the loading in coins.cpp rather than validation.cpp, so validation changes are smaller and not as many assumptions about the coins cache implementation have to be made outside of coins.cpp.

jamesob commented at 11:28 pm on September 7, 2020:

Would be good to use SetBestBlock

Fixed, thanks.

it would be reasonable to write a CAutoFile -> CCoinsViewCache function

Hm yeah, this is definitely worth some consideration.

in src/validation.cpp:5393 in 9c1db71dbd outdated

5457+    coins_cache.Flush(); // TODO: if #17487 is merged, add erase=false here for better performance.
5458+
5459+    assert(coins_cache.GetBestBlock() == base_blockhash);
5460+
5461+    CCoinsStats stats;
5462+    auto breakpoint_fnc = [] { /* TODO insert breakpoint here? */ };

ryanofsky commented at 9:20 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

Probably it makes sense for this function to take an interruption_point argument instead of defining its own internally.

jamesob commented at 2:47 pm on October 13, 2020:

Not sure what you mean here. I was a little confused when rebasing because afaict we don’t have any remaining usages of interruption_point in the codebase (aside from the RpcInterruptionPoint).

ryanofsky commented at 10:50 pm on October 19, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5)

re: #19806 (review)

Not sure what you mean here. I was a little confused when rebasing because afaict we don’t have any remaining usages of interruption_point in the codebase (aside from the RpcInterruptionPoint).

It’s not important, but the suggestion is for PopulateAndValidateSnapshot and ActivateSnapshot to take a const std::function<void()>& interruption_point so when this code is called from loadtxoutset in #15606 it is interruptable.

laanwj commented at 10:16 pm on December 15, 2020:

The naming “breakpoint” in the comment confused me here at first, and made me think at first this was a remnant of debug code.

ct we don’t have any remaining usages of interruption_point

Well, the boost interruption point should no longer be used, If you want to check for interruption it should be done by passing in a polling function as @ryanofsky says.

in src/validation.cpp:5375 in 9c1db71dbd outdated

5370+bool ChainstateManager::PopulateAndValidateSnapshot(
5371+    CChainState& snapshot_chainstate,
5372+    CAutoFile& coins_file,
5373+    const SnapshotMetadata& metadata)
5374+{
5375+    LOCK(::cs_main);

ryanofsky commented at 9:30 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

Can you add a comment explaining locking in this function. It’s not clear to me why cs_main needs to be held at all while reading the snapshot file and populating the chainstate when the chainstate is still being constructed shouldn’t be referenced externally. It’s also not clear why it’s safe to keep cs_main locked for up to 10 minutes below while downloading headers.

jamesob commented at 11:26 pm on September 7, 2020:

Yep, you’re right about this - I’ll remove the lock acquisition. I had initially put this in to simplify testing, and had some vague rationale about snapshot load being the most important task and so meriting “stopping” everything else by holding cs_main, but that doesn’t make much sense. Also probably good to keep cs_main free while deserializing the snapshot so that we have a better chance of having finished headers retrieval by the time we get to the related check below.

in src/validation.cpp:5503 in 9c1db71dbd outdated

5498+        LogPrintf("[snapshot] assumeutxo value in snapshot metadata not valid for " /* Continued */
5499+            "height %s - refusing to load snapshot\n", base_height);
5500+        return false;
5501+    }
5502+
5503+    if (stats.hashSerialized != expected_contents_hash) {

ryanofsky commented at 9:34 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

Is there a reason hash is being computed above before sleeping but only checked now after sleeping? It seems like it’d be good to return an error as early as possible and not wait for headers if this is going to fail.

jamesob commented at 11:31 pm on September 7, 2020:

Yeah, that’s a good point. Unfortunately we need the headers chain to resolve the base_height before retrieving the expected assumeutxo data.

in src/validation.cpp:5453 in 9c1db71dbd outdated

5506+        return false;
5507+    }
5508+
5509+    snapshot_chainstate.m_chain.SetTip(snapshot_start_block);
5510+
5511+    // Fake various pieces of CBlockIndex state:

ryanofsky commented at 9:49 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

Ideally, we would not have to fake these things and external code would code better with missing data, right? Or do you disagree? It would be good if comment would say one way or the other what ultimate fate of faking should be.

It would also be good if comments in chain.h documenting CBlockIndex fields mentioned the fake values and said how to determine whether values are real or fake, to reduce likelihood of new bugs being introduced because of fake values.

jamesob commented at 3:04 pm on October 13, 2020:

Ideally you’re right here, but there’s a lot of complexity in cleaning up the “consumer” side of this data. Take for example nChainTx; in order to avoid faking that value, there are various points of consumption in net_processing (by way of HaveTxsDownloaded()) that need to be addressed. What makes these sites complicated is that they lack the chainstate context, having access only to the CBlockIndex objects.

I’m not saying it isn’t possible to do what you’re suggesting here, but I think it will probably involve some deep thinking about how much regions like net_processing should know about chainstate semantics. I’m going to leave this for someone else to think about unless there is a fundamental objection to fudging this data for snapshot chainstates.

ryanofsky commented at 3:16 pm on October 20, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5)

re: #19806 (review)

I’m not saying it isn’t possible to do what you’re suggesting here

Sorry, wasn’t objecting to fudging data now, just suggesting documenting in chain.h which fields may contain fake data and when the fake data is set. Documenting seems less important if fake data can be cleaned up easily, more important if fake data will be a more permanent thing

jamesob commented at 6:14 pm on October 28, 2020:

Oh sorry, somehow misinterpreted what you’d said. Added some comments in chain.h that at least provide references to where the faked values are created and used; if you can think of anything else that’d be useful I’m happy to apply a patch.

in src/validation.cpp:5213 in 9c1db71dbd outdated

5290@@ -5291,6 +5291,262 @@ bool ExpectedAssumeutxo(const int height, uint256& expected_out, const CChainPar
5291     return false;
5292 }
5293 
5294+bool ChainstateManager::ActivateSnapshot(
5295+        CAutoFile& coins_file,
5296+        const SnapshotMetadata& metadata,
5297+        bool in_memory)
5298+{

ryanofsky commented at 9:51 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

This function and PopulateAndValidateSnapshot below are doing a lot of things, and I think might benefit from being split up into smaller functions:

Rebalance caches
Load snapshot file -> ccoinsview and verify expected hash
Wait for headers
Set chain tip and fake blockindex data

Some of these could be marked EXCLUSIVE_LOCKS_REQUIRED(cs_main) to make locking requirements more explicit.

jamesob commented at 3:07 pm on October 13, 2020:

I’m not opposed, but I’m not sure I see a lot of benefit either. E.g. I don’t think splitting this stuff up allows us to do a more comprehensive job of testing. I’m happy to apply a diff though.

Sjors commented at 10:32 am on October 20, 2020:

I tend to agree that (in the current incarnation 5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5 PopulateAndValidateSnapshot does quite a lot. Although I can follow along, it might be more clear if ActivateSnapshot called more specific helper functions in the right order, e.g. WaitForHeaders.

in src/validation.cpp:5329 in 9c1db71dbd outdated

5354+        LOCK(::cs_main);
5355+        m_snapshot_chainstate.swap(snapshot_chainstate);
5356+        assert(m_snapshot_chainstate->LoadChainTip(::Params()));
5357+
5358+        m_active_chainstate = m_snapshot_chainstate.get();
5359+        // Don't rebalance disk or FlushStateToDisk

ryanofsky commented at 9:57 pm on September 4, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (9c1db71dbd82342b0a1d33401b329e34a7a6cee8)

I don’t understand implication of this comment. Maybe it should say why not to rebalance or flush. I’m not clear on what rebalancing would do here, and it seems like the PopulateAndValidateSnapshot call already flushes the snapshot chainstate, at least.

jamesob commented at 2:41 pm on October 13, 2020:

After revisiting this comment, I don’t understand it either! Seems like we should always MaybeRebalanceCaches() at the end of this function. It probably had something to do with not wanting to clear the coinscache of the new chainstate throughout the course of the flush, but since we’re doing that anyway (#17487 doesn’t look like it’s going to get merged anytime soon) I’ll just add that call to the end here.

ryanofsky commented at 10:23 pm on September 4, 2020: member

Mostly finished review (updated progress in #19806#pullrequestreview-481228137). Feel free to ignore any of my comments that don’t make sense. They are all not too important and a little half baked as I was figuring things out. All the changes here are basically straightforward and seem good.

jamesob force-pushed on Sep 7, 2020

jamesob commented at 11:42 pm on September 7, 2020: member

Thanks for the review so far, @ryanofsky. I’ve pushed an update (d690546) that removes the nChainTx caching changes and hardcodes that value per @MarcoFalke’s feedback. I also reduced locking during snapshot activation per your recommendations. I’ll get around to addressing the rest of your feedback in the next day or so.

Sjors commented at 12:22 pm on September 18, 2020: member

Would it make sense to add the 110 and 210 block regtest snapshots to the repo, and then add a functional test to load them? That should demonstrate all the behavior we need without committing to a testnet or mainnet block.

I can’t compile on macOS.

 0validation.cpp:5334:67: error: passing variable 'm_blockman' by reference requires holding mutex 'cs_main' [-Werror,-Wthread-safety-reference]
 1    auto snapshot_chainstate = MakeUnique<CChainState>(::mempool, m_blockman, base_blockhash);
 2                                                                  ^
 3validation.cpp:5375:56: error: calling function 'CoinsTip' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
 4    CCoinsViewCache& coins_cache = snapshot_chainstate.CoinsTip();
 5                                                       ^
 6validation.cpp:5413:37: error: calling function 'GetCoinsCacheSizeState' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
 7            if (snapshot_chainstate.GetCoinsCacheSizeState(&::mempool) >= CoinsCacheSizeState::CRITICAL) {
 8                                    ^
 9validation.cpp:5462:44: error: calling function 'CoinsDB' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
10    if (!GetUTXOStats(&snapshot_chainstate.CoinsDB(), stats, CoinStatsHashType::HASH_SERIALIZED, breakpoint_fnc)) {

DrahtBot added the label Needs rebase on Sep 22, 2020

jamesob force-pushed on Sep 30, 2020

DrahtBot removed the label Needs rebase on Sep 30, 2020

DrahtBot added the label Needs rebase on Oct 8, 2020

jamesob force-pushed on Oct 13, 2020

DrahtBot removed the label Needs rebase on Oct 13, 2020

jamesob force-pushed on Oct 13, 2020

jamesob force-pushed on Oct 15, 2020

jamesob force-pushed on Oct 16, 2020

jamesob commented at 7:02 pm on October 16, 2020: member

Okay, rebased. After a few days of fighting on CI with an apparent msan compiler bug (doesn’t like const map values - thanks to @ryanofsky for the help) and a few macOS-specific sign comparison warnings (I’m beginning to understand why no one writes tests in C++), this thing is building on CI. The lock annotation warnings that @Sjors helpfully pointed out have been fixed (and were introduced when I removed the cs_main hold throughout snapshot activation).

I spoke to Sjors offline per his recommendation about adding testnet snapshots and I think that’s a good idea, but we can do it in a followup - especially given that the big benefit of doing that would be the ability to use the snapshot files in functional tests, but since the behavior here isn’t accessible via RPC (and really can’t be until the remaining assumeutxo changes are in), I think we can wait to do that until later.

I think I’ve addressed all the feedback here, thanks to everyone so far for that.

in src/chainparams.h:51 in a1aa47484f outdated

46+    const unsigned int nChainTx;
47+};
48+
49+std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud);
50+
51+using MapAssumeutxo = std::map<int, const AssumeutxoData>;

ryanofsky commented at 9:45 pm on October 19, 2020:

In commit “chainparams: add allowed assumeutxo values” (a1aa47484fb7c272a1b957a37dc8c4595a0774bb)

It might be good to say in comment that int is a height. Also, it looks like strictly speaking there is no need for this data structure to reference heights. E.g. it could just be a simple map from hash -> nChainTx. Not important, though.

jamesob commented at 4:39 pm on October 28, 2020:

Hm, you’re right about that. I kind of like the constraint that keying by height creates (enforces single entry per height), but that’s neither here nor there. I’ll leave as-is unless we can think of a good reason to change.

in src/validation.cpp:5221 in 5e08b73c02 outdated

5287+    int64_t current_coinsdb_cache_size{0};
5288+    int64_t current_coinstip_cache_size{0};
5289+
5290+    // Cache percentages to allocate to each chainstate.
5291+    constexpr double IBD_CACHE_PERC = 0.01;
5292+    constexpr double SNAPSHOT_CACHE_PERC = 0.99;

ryanofsky commented at 9:59 pm on October 19, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5)

What’s the reason this uses 99:1 ratio while MaybeRebalanceCaches uses 95:5 ratio? Maybe there should be an explanatory comment, or the two pieces of code could use a common helper function or constants.

jamesob commented at 6:02 pm on October 28, 2020:

Added a comment here.

in src/validation.cpp:5276 in 5e08b73c02 outdated

5331+        return false;
5332+    }
5333+
5334+    {
5335+        LOCK(::cs_main);
5336+        m_snapshot_chainstate.swap(snapshot_chainstate);

ryanofsky commented at 10:12 pm on October 19, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5)

Would it make sense to assert m_snap_chainstate is null before this?

jamesob commented at 6:10 pm on October 28, 2020:

Makes sense to me. Fixed.

in src/validation.cpp:5282 in 5e08b73c02 outdated

5277+        bool in_memory)
5278+{
5279+    uint256 base_blockhash = metadata.m_base_blockhash;
5280+    uint256 existing_blockhash = this->SnapshotBlockhash().value_or(uint256());
5281+
5282+    if (!existing_blockhash.IsNull()) {

Sjors commented at 9:56 am on October 20, 2020:

5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5: You may want to move the refactor from 83a67ec24c053f6d085101c83e448e76806e8355 up a bit, so you can use if (this->SnapshotBlockhash()) from the get go.

jamesob commented at 6:03 pm on October 28, 2020:

Fixed, thanks.

in src/validation.cpp:5254 in 5e08b73c02 outdated

5310+            (size_t)(current_coinstip_cache_size * IBD_CACHE_PERC),
5311+            (size_t)(current_coinsdb_cache_size * IBD_CACHE_PERC));
5312+    }
5313+
5314+    auto snapshot_chainstate = WITH_LOCK(::cs_main, return MakeUnique<CChainState>(
5315+            this->ActiveChainstate().m_mempool, m_blockman, base_blockhash));

Sjors commented at 10:12 am on October 20, 2020:

5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5 : why is the snapshot mempool shared with the ibd mempool?

jamesob commented at 1:09 pm on October 28, 2020:

Each CChainState instance needs a mempool. In practice, the non-active mempool will never be used, but since each chainstate can be the active throughout runtime, they each need a reference to the same mempool.

in src/test/validation_chainstatemanager_tests.cpp:236 in 59013d8006 outdated

225+        chainman.ActiveChainstate().m_from_snapshot_blockhash,
226+        chainman.SnapshotBlockhash().value_or(uint256()));
227+
228+    // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can
229+    // be found.
230+    mineBlocks(10);

Sjors commented at 10:44 am on October 20, 2020:

It’s a bit weird to activate a snapshot that is identical to the actual fully validated chain. It would be nice if we could mine headers-only. Calling ProcessNewBlockHeaders instead of ProcessNewBlock in the test helper might do the trick.

Together with mock time you could test the functionality of waiting for headers too.

jamesob commented at 1:11 pm on October 28, 2020:

I don’t really understand this. There’s no way to generate a snapshot to activate without mining blocks on some chain. The chainstate we’re loading the snapshot into has no knowledge of the mined blocks. I don’t know how we can do better?

Sjors commented at 4:16 pm on December 14, 2020:

mineBlocks() calls CreateAndProcessBlock which calls m_node.chainman->ProcessNewBlock. Doesn’t that mean the blocks are fully processed by the test node, including the right values for nTx and BLOCK_OPT_WITNESS for all blocks before the snapshot? Which would mean we’re not testing the code path that sets dummy values.

Perhaps one approach could be to split CreateAndActivateUTXOSnapshot so you can create a snapshot, wipe the node / chainstate manager, and then load it. But you’d have to save and reload the headers.

If this makes any sense, it can also wait.

(in reply to #19806 (review))

Sjors commented at 10:48 am on October 20, 2020: member

Functional tests might indeed be too much to add here. However there are lots of potential failure modes, and it’s not obvious that 59013d80067948e5073da10a06802a1e9b84e7d7 tests for all of those. The use of smaller helper function might make it easier to cover those cases. Alternatively a new exception class or error/result enum (like TransactionError) could help tease them out better.

in src/validation.cpp:5466 in 5e08b73c02 outdated

5513+    CBlockIndex* index = nullptr;
5514+    for (int i = 0; i <= snapshot_chainstate.m_chain.Height(); ++i) {
5515+        index = snapshot_chainstate.m_chain[i];
5516+
5517+        if (!index->nTx) {
5518+            index->nTx = 1;

ryanofsky commented at 3:31 pm on October 20, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5)

If these blockindexes are shared across different chains and can be accessed without snapshot_chainstate is some locking needed to update them? Might be good to have a comment saying why cs_main isn’t acquired here. (Feel free to skip if I’m just missing a basic assumption)

jamesob commented at 1:41 pm on October 28, 2020:

Oh this is definitely a problem, thanks for finding it. Yet another vestige of originally having ::cs_main held during the entirety of PopulateAndValidateSnapshot.

jamesob commented at 6:02 pm on October 28, 2020:

Fixed, thanks.

in src/validation.cpp:5364 in 5e08b73c02 outdated

5415+    }
5416+
5417+    // Important that we set this. This and the coins_cache accesses above are
5418+    // sort of a layer violation, but either we reach into the innards of
5419+    // CCoinsViewCache here or we have to invert some of the CChainState to
5420+    // embed them in a snapshot-activation-specific CCoinsViewCache bulk load

ryanofsky commented at 3:39 pm on October 20, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (5e08b73c023bd7f9e79d1b67c25a2ed2b4248fe5)

FWIW, I don’t think a CoinsView bulk load method would be a crazy thing to have. But I can see how making it not snapshot-activation specific could be a pain, so the friend ChainstateManager approach does seem ok too.

in src/test/util/setup_common.cpp:262 in e08cc16f53 outdated

258@@ -230,6 +259,7 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
259 TestChain100Setup::~TestChain100Setup()
260 {
261     gArgs.ForceSetArg("-segwitheight", "0");
262+    SetMockTime(0);

ryanofsky commented at 3:48 pm on October 20, 2020:

In commit “tests: add deterministic chain generation unittest fixture” (e08cc16f53b7b465f4f1e3b6f7bf83b37356b043)

Perhaps should use if (m_deterministic) condition for consistency with the other SetMockTime calls in this class

jamesob commented at 6:02 pm on October 28, 2020:

Fixed, thanks.

in src/test/validation_chainstatemanager_tests.cpp:222 in 59013d8006 outdated

217+        BOOST_CHECK_EQUAL(total_coins, initial_total_coins);
218+        BOOST_CHECK_EQUAL(initial_size, initial_total_coins);
219+    }
220+
221+    // Snapshot should refuse to load at this height.
222+    assert(!CreateAndActivateUTXOSnapshot(m_node, m_path_root));

ryanofsky commented at 3:58 pm on October 20, 2020:

In commit “tests: add snapshot activation test” (59013d80067948e5073da10a06802a1e9b84e7d7)

Better to replace assert with BOOST_REQUIRE to integrate with test framework. Also because it’s good not to do things with side effects in c asserts in case there are custom CPPFLAGS

jamesob commented at 6:02 pm on October 28, 2020:

Fixed, thanks.

ryanofsky approved

ryanofsky commented at 4:15 pm on October 20, 2020: member

Code review ACK 59013d80067948e5073da10a06802a1e9b84e7d7. This looks good and there are no important suggestions here from me. This unit test is very clean, and I’d suggest looking at the unit test as a starting point for other reviewers to see what functionality this PR implements. I think this PR would also be a good candidate for https://bitcoincore.reviews/.

re: #19806#pullrequestreview-512534741

Functional tests might indeed be too much to add here.

I think it’s mostly not possible to add function tests yet before the ActivateSnapshot method is called, but #15606 should eventually have these

Platinumwrist approved

jamesob force-pushed on Oct 28, 2020

ryanofsky approved

ryanofsky commented at 8:25 pm on November 10, 2020: member

Code review ACK 700e66dc6bf6ed1b6bc2a642686a734cb69f4932. Just many suggested updates since last review. Adding cs_main for cblockindex updates, adding lots of comments, rearranging commits, tweaking test and optional block hash code

in src/test/validation_chainstatemanager_tests.cpp:79 in f92a76b62f outdated

75@@ -65,6 +76,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
76     BlockValidationState _;
77     BOOST_CHECK(c2.ActivateBestChain(_, chainparams, nullptr));
78 
79+

fjahr commented at 11:57 pm on November 12, 2020:

in f92a76b62fad04213884ec602822601ee84f3825: nit: This new line doesn’t seem necessary

in src/node/coinstats.cpp:27 in d2d1eb2333 outdated

23@@ -24,6 +24,9 @@ static uint64_t GetBogoSize(const CScript& scriptPubKey)
24            scriptPubKey.size() /* scriptPubKey */;
25 }
26 
27+//! Warning: be very careful when changing this! assumeutuxo and UTXO snapshot

fjahr commented at 3:34 pm on November 15, 2020:

in d2d1eb23331fdb1c36a8a879aff222ff0d731210: Typo: assumeutuxo

in src/validation.cpp:5444 in 07936ca7c0 outdated

5439+            coins_count - coins_left);
5440+        return false;
5441+    }
5442+
5443+    LogPrintf("[snapshot] loaded %d (%.2f MB) coins from snapshot %s\n",
5444+        coins_count - coins_left,

fjahr commented at 4:13 pm on November 15, 2020:

in 07936ca7c067f007f7fe9dc04e8feb2c934122a2: coins_left should always be 0 here so it can be removed I think

in src/validation.cpp:5377 in 07936ca7c0 outdated

5372+    LogPrintf("[snapshot] loading coins from snapshot %s\n", base_blockhash.ToString());
5373+    int64_t flush_now{0};
5374+    int64_t coins_processed{0};
5375+
5376+    while (coins_left > 0) {
5377+        coins_file >> outpoint;

fjahr commented at 6:52 pm on November 15, 2020:

in 07936ca7c067f007f7fe9dc04e8feb2c934122a2: This fails if m_coins_count has a higher count than the number of actual coins in coins_file. See also my test in https://github.com/fjahr/bitcoin/commit/2835c8118fe35f5551886ac5251e535bb337571a.

jamesob commented at 2:14 pm on November 19, 2020:

I think failure is the desired behavior; if the coins count attached to the snapshot deviates from the actual contents of the snapshot, that’s an indication that the snapshot has been modified somehow.

jamesob commented at 2:23 pm on November 19, 2020:

Oh I think I see what you’re saying here - it halts execution instead of return false? Yeah, maybe we should wrap this in a try for testability’s sake.

in src/chainparams.cpp:164 in d2d1eb2333 outdated

162@@ -164,6 +163,10 @@ class CMainParams : public CChainParams {
163             }
164         };
165 
166+        m_assumeutxo_data = MapAssumeutxo{
167+         // TODO to be specified in a future patch.

fjahr commented at 7:05 pm on November 15, 2020:

in d2d1eb23331fdb1c36a8a879aff222ff0d731210: Just a thought: Should a node refuse to start if it has a chain that does not match with the assumeutxo data here? Maybe in the next steps it could make sense to add such a check to init but I haven’t thought about it much and maybe you already plan to do this.

sipa commented at 9:38 pm on December 29, 2020:

Response to https://github.com/bitcoin/bitcoin/pull/19806/commits/d684ecd5f11b2cf63235cd483858f3fd27f8c712#r523799037: @fjahr I’d say no. assume* values are optimizations where we know some computation can be avoided because it’s known to be valid. But refusing to start with a mismatching one should just mean you don’t get the optimization; doing anything else is very close to making it a checkpoint, with all repercussions (actually affecting which chain the network can accept etc.).

fjahr commented at 10:51 pm on January 7, 2021:

@sipa Right, I didn’t look at it from that perspective. Thanks!

in src/validation.cpp:5445 in 07936ca7c0 outdated

5434+        // We expect an exception since we should be out of coins.
5435+        out_of_coins = true;
5436+    }
5437+    if (!out_of_coins) {
5438+        LogPrintf("[snapshot] bad snapshot - coins left over after deserializing %d coins\n",
5439+            coins_count - coins_left);

fjahr commented at 7:30 pm on November 15, 2020:

in 07936ca7c067f007f7fe9dc04e8feb2c934122a2: This statement does not have to be true afaict. The count in the metadata can be different from the actual count of the coins in the file. Also, the loop above doesn’t seem to break unless coins_left is 0 anyway.

ryanofsky commented at 6:31 pm on December 1, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot” (a2e7b295a33d90c895da649ee479fa090f20276b)

Could drop - coins_left to be consistent with [snapshot] loaded log print immediately below. Assuming no change in behavior since it will always be zero. Or maybe just move the “snapshot loaded” print above the “bad snapshot” print so the loading information only needs to be printed one place

in src/test/validation_chainstatemanager_tests.cpp:17 in 700e66dc6b outdated

12 #include <optional.h>
13 #include <uint256.h>
14 #include <validation.h>
15 #include <validationinterface.h>
16 
17+#include <univalue.h>

fjahr commented at 8:50 pm on November 15, 2020:

in 700e66dc6bf6ed1b6bc2a642686a734cb69f4932: nit: not sure about where univalue really belongs but mostly it seems to be in the very last block of includes while tinyformat is in the block above. But again, not sure which style guide we actually follow there. Either way, if you keep it like this, sorting between univalue and tinyformat is wrong.

fjahr commented at 9:15 pm on November 15, 2020: member

Close to finished. Great to have the deterministic chain as an option in the tests now :)

I was playing with the tests a bit to check the behavior with maleated snapshot data and maybe that is interesting to add here or in a follow-up, so I pushed it up here: https://github.com/fjahr/bitcoin/commit/2835c8118fe35f5551886ac5251e535bb337571a. Feel free to use any way you like.

jamesob force-pushed on Nov 19, 2020

jamesob commented at 2:35 pm on November 19, 2020: member

Thanks @ryanofsky @fjahr for the review, and thanks especially for the test Fabian - that’s great. I’ve addressed your feedback and slightly modified the test patch for formatting. I fixed up the snapshot activation so that we can properly test both sides of a false coin count without exception, and I’ve added your commit as the head of this branch.

fjahr commented at 11:36 pm on November 19, 2020: member

Code review ACK 2aba960c3360da1e840ffef3b0f95c4fde4f86a6

Also played with the tests extensively looking for edge cases.

fanquake deleted a comment on Nov 20, 2020

jamesob commented at 5:03 pm on November 28, 2020: member

The Cirrus fuzzer/valgrind test is reliably timing out here. Not sure if that’s unique to this PR or not.

Otherwise, I think this is close to ready for merge?

MarcoFalke commented at 5:10 pm on November 28, 2020: member

Jup, can be ignored. (Or fixed with a rebase)

ryanofsky approved

ryanofsky commented at 6:41 pm on December 1, 2020: member

Code review ACK 2aba960c3360da1e840ffef3b0f95c4fde4f86a6. Changes since last review: better error handling for missing coins in snapshot and a new commit to test it. Also minor style cleanups

DrahtBot added the label Needs rebase on Dec 7, 2020

jamesob force-pushed on Dec 10, 2020

DrahtBot removed the label Needs rebase on Dec 10, 2020

ryanofsky approved

ryanofsky commented at 3:55 pm on December 11, 2020: member

Code review ACK 80121fbb2d0c7c14f9c99cb873859e5ed9c83a0e. Only changes since last review: rebasing after minor conflicts, tweaking log print as suggested, switching to [[nodiscard]] instead of NODISCARD

fjahr commented at 11:55 pm on December 13, 2020: member

Code review ACK 80121fbb2d0c7c14f9c99cb873859e5ed9c83a0e. Changes since last review: rebased, changed a logprint in validation.cpp (see GH comments), changed NODISCARD to [[nodiscard]].

Sjors commented at 1:22 pm on December 14, 2020: member

In ba2779f98490f1492644a4ee4320fa689560272c you mock nTx, nChainTx (memory only) and BLOCK_OPT_WITNESS in m_active_chainstate for all blocks below the snapshot height. Meanwhile the background sync from genesis takes place on m_ibd_chainstate which sets the correct values. I spent a bunch of time being confused, but eventually realised pblocktree is a global. It might be nice to pull that into ChainstateManager as well in another PR.

in src/validation.cpp:5267 in ba2779f984 outdated

5262+
5263+    {
5264+        LOCK(::cs_main);
5265+        assert(!m_snapshot_chainstate);
5266+        m_snapshot_chainstate.swap(snapshot_chainstate);
5267+        assert(m_snapshot_chainstate->LoadChainTip(::Params()));

Sjors commented at 2:11 pm on December 14, 2020:

ba2779f98490f1492644a4ee4320fa689560272c : Better to avoid asserts with side-effects.

jamesob commented at 6:14 pm on December 15, 2020:

Fixed, thanks.

in src/txdb.cpp:52 in ea0e1d005a outdated

52-    m_db.reset();
53-    m_db = MakeUnique<CDBWrapper>(
54-        m_ldb_path, new_cache_size, m_is_memory, /*fWipe*/ false, /*obfuscate*/ true);
55+    // We can't do this operation with an in-memory DB since we'll lose all the coins upon
56+    // reset.
57+    if (!m_is_memory) {

Sjors commented at 4:00 pm on December 14, 2020:

ea0e1d005a455d8ddef63d428dbedcd9f54a2f9e: maybe put an assert here and have the caller do if (!m_is_memory)? It’s also not very clear to me why it’s fine to skip the resize (within tests).

jamesob commented at 6:10 pm on December 15, 2020:

We have no choice but to skip the resize in tests because, as the comment says, the operations necessary for a resize when using an in-memory db result in an emptying of the coinsdb.

I like the idea of some kind of assert here to prevent this from happening outside of a test setting, but what could we assert on? Sounds like a good small follow-up.

in src/rpc/blockchain.h:66 in 6bb63e9efe outdated

58@@ -57,4 +59,10 @@ CTxMemPool& EnsureMemPool(const util::Ref& context);
59 ChainstateManager& EnsureChainman(const util::Ref& context);
60 CBlockPolicyEstimator& EnsureFeeEstimator(const util::Ref& context);
61 
62+/**
63+ * Helper to create UTXO snapshots given a chainstate and a file handle.
64+ * @return a UniValue map containing metadata about the snapshot.
65+ */
66+UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile);

Sjors commented at 4:05 pm on December 14, 2020:

6bb63e9efeccfad3e5f422453902cb22a22c33b4 : alternatively, it could return path rather than a UniValue, so it could even live outside of RPC land.

jonatack commented at 11:28 am on December 22, 2020:

9a2c888d agree with returning path

It looks like NodeContext should be passed by reference to const (“in” param), not by reference for an “out” param

0UniValue CreateUTXOSnapshot(const NodeContext& node, CChainState& chainstate, CAutoFile& afile);

in src/test/util/setup_common.h:81 in e82498b056 outdated

78@@ -79,7 +79,6 @@ struct BasicTestingSetup {
79     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
80     ~BasicTestingSetup();
81 
82-private:

Sjors commented at 4:08 pm on December 14, 2020:

e82498b05634608e59a1d1acc1a8c1b8ece62ffc: why are you dropping private here?

jamesob commented at 4:13 pm on December 15, 2020:

I have to make use of it here: https://github.com/bitcoin/bitcoin/pull/19806/files#diff-dbada1fe3a3d0af884304dd28be8c9df74b592401dec2c6400f6b491aefe6c9bR230

MarcoFalke commented at 11:58 am on April 3, 2021:

Could use GetDataDir() instead

Sjors approved

Sjors commented at 5:23 pm on December 14, 2020: member

ACK 80121fb

jamesob force-pushed on Dec 15, 2020

jamesob commented at 6:14 pm on December 15, 2020: member

au.activate.27 -> au.activate.28

 0$ git range-diff master au.activate.27 au.activate.28
 1
 21:  35188e646c = 1:  a9fb5a3174 chainparams: add allowed assumeutxo values
 32:  96d70a3897 = 2:  7356672cc1 simplify ChainstateManager::SnapshotBlockhash() return semantics
 43:  ba2779f984 ! 3:  164ccb2cf6 validation: add ChainstateManager::ActivateSnapshot
 5    @@ -140,7 +140,8 @@
 6     +        LOCK(::cs_main);
 7     +        assert(!m_snapshot_chainstate);
 8     +        m_snapshot_chainstate.swap(snapshot_chainstate);
 9    -+        assert(m_snapshot_chainstate->LoadChainTip(::Params()));
10    ++        bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(::Params());
11    ++        assert(chaintip_loaded);
12     +
13     +        m_active_chainstate = m_snapshot_chainstate.get();
14     +
154:  ea0e1d005a = 4:  3f69b370fb txdb: don't reset during in-memory cache resize
165:  6bb63e9efe = 5:  290b567736 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset
176:  e82498b056 = 6:  78cdc6271a tests: add deterministic chain generation unittest fixture
187:  77efd7e0a4 = 7:  b9b819ae12 tests: add snapshot activation test
198:  80121fbb2d = 8:  68dadbdcb9 test: Add tests with maleated snapshot data

jonatack commented at 6:47 pm on December 15, 2020: member

Concept ACK, hope to review this tomorrow. Building now.

in test/lint/lint-circular-dependencies.sh:23 in 68dadbdcb9 outdated

19@@ -20,6 +20,7 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
20     "txmempool -> validation -> txmempool"
21     "wallet/fees -> wallet/wallet -> wallet/fees"
22     "wallet/wallet -> wallet/walletdb -> wallet/wallet"
23+    "node/coinstats -> validation -> node/coinstats"

laanwj commented at 10:05 pm on December 15, 2020:

Having to add a circular dependency here is kind of a shame.

jamesob commented at 11:28 pm on December 15, 2020:

I’m with ya. This is cached (and may be stale) but I think the only workable alternative is to fold coinstats into validation. If you can think of something else I’m happy to make the change.

laanwj commented at 8:37 am on December 16, 2020:

~~It isn’t possible to factor out the part of node/coinstats that’s used by validation to a third module, to prevent the validation -> node/coinstats side of the cycle?~~ Apparently that’s almost all of it. Ugh.

laanwj commented at 9:32 am on December 16, 2020:

I don’t think we want to roll more into validation, but less.

My only idea here is to have the snapshot loading functionality not in validation itself, but in a separate implementation unit (which can depend on both,. but neither will depend on it).

in test/lint/lint-includes.sh:64 in 68dadbdcb9 outdated

60@@ -61,6 +61,7 @@ EXPECTED_BOOST_INCLUDES=(
61     boost/multi_index/sequenced_index.hpp
62     boost/multi_index_container.hpp
63     boost/optional.hpp
64+    boost/optional/optional_io.hpp

laanwj commented at 10:07 pm on December 15, 2020:

Is there a C++17 equivalent for optional_io?

jamesob commented at 11:24 pm on December 15, 2020:

Unfortunately it doesn’t look like it to me. The contents of the boost header don’t seem to have any equivalent implementations in c++17, but that was a good idea to check. I tried removing the inclusion of this and it broke the build, so I guess it’s required.

laanwj commented at 8:33 am on December 16, 2020:

Okay so you’re not intentionally using anything from it, needing to include it is only a by-effect of how boost is structured internally? Interesting but yes I guess no way around it then.

jnewbery commented at 10:04 am on December 16, 2020:

You can avoid this by using std::optionals everywhere. Minimal diff to get this working here (but other uses of boost::optional also need to be removed):

 0diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
 1index 37450ec18f..be9b6ccad2 100644
 2--- a/src/test/validation_tests.cpp
 3+++ b/src/test/validation_tests.cpp
 4@@ -5,14 +5,14 @@
 5 #include <chainparams.h>
 6 #include <net.h>
 7 #include <signet.h>
 8-#include <optional.h>
 9 #include <uint256.h>
10 #include <validation.h>
11 
12 #include <test/util/setup_common.h>
13 
14 #include <boost/test/unit_test.hpp>
15-#include <boost/optional/optional_io.hpp>
16+
17+#include <optional>
18 
19 BOOST_FIXTURE_TEST_SUITE(validation_tests, TestingSetup)
20 
21@@ -133,14 +133,14 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
22 
23     for (auto empty : bad_heights) {
24         const auto out = ExpectedAssumeutxo(empty, *params);
25-        BOOST_CHECK_EQUAL(out, nullopt);
26+        BOOST_CHECK(!out.has_value());
27     }
28 
29-    const auto out110 = ExpectedAssumeutxo(110, *params).get();
30+    const auto out110 = ExpectedAssumeutxo(110, *params).value();
31     BOOST_CHECK_EQUAL(out110.hash_serialized, uint256S("76fd7334ac7c1baf57ddc0c626f073a655a35d98a4258cd1382c8cc2b8392e10"));
32     BOOST_CHECK_EQUAL(out110.nChainTx, (unsigned int)110);
33 
34-    const auto out210 = ExpectedAssumeutxo(210, *params).get();
35+    const auto out210 = ExpectedAssumeutxo(210, *params).value();
36     BOOST_CHECK_EQUAL(out210.hash_serialized, uint256S("9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"));
37     BOOST_CHECK_EQUAL(out210.nChainTx, (unsigned int)210);
38 }
39diff --git a/src/validation.cpp b/src/validation.cpp
40index e25d38d89f..09621420b5 100644
41--- a/src/validation.cpp
42+++ b/src/validation.cpp
43@@ -5185,7 +5185,7 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const
44     return *to_modify;
45 }
46 
47-Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& chainparams)
48+std::optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& chainparams)
49 {
50     const MapAssumeutxo& valid_assumeutxos_map = chainparams.Assumeutxo();
51     const auto assumeutxo_found = valid_assumeutxos_map.find(height);
52diff --git a/src/validation.h b/src/validation.h
53index 0b8a72b8b0..db2cc1b643 100644
54--- a/src/validation.h
55+++ b/src/validation.h
56@@ -29,6 +29,7 @@
57 #include <atomic>
58 #include <map>
59 #include <memory>
60+#include <optional>
61 #include <set>
62 #include <stdint.h>
63 #include <string>
64@@ -1011,6 +1012,6 @@ inline bool IsBlockPruned(const CBlockIndex* pblockindex)
65  *
66  * [@returns](/bitcoin-bitcoin/contributor/returns/) empty if no assumeutxo configuration exists for the given height.
67  */
68-Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& params);
69+std::optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& params);
70 
71 #endif // BITCOIN_VALIDATION_H

We really shouldn’t be adding new boost dependencies into validation.h, so I think changing all the boost::optionals to std::optionals needs to happen anyway.

jnewbery commented at 3:41 pm on December 21, 2020:

boost::optional -> std::optional has now been merged: #20671.

This boost include can be removed (and some of the optional methods need to be changed).

fjahr commented at 0:14 am on December 16, 2020: member

Code review ACK 68dadbdcb9afe60b77e027d6b066c87b86dbf7a9

Only change since last review was introduction of intermediary variable chaintip_loaded to fix #19806 (review).

DrahtBot added the label Needs rebase on Dec 21, 2020

laanwj commented at 3:35 pm on December 21, 2020: member

This has many ACKs and seems (besides needing rebase again, sorry) ready for merge. So I think what we need to decide here is whether we’ll accept a circular dependency added momentarily, and resolve that in a later PR. I don’t insist on holding it up on that.

jamesob commented at 3:53 pm on December 21, 2020: member

Thanks for the clear feedback - I’ll rebase tonight and at the very least remove the unnecessary optional stuff.

jamesob force-pushed on Dec 22, 2020

jamesob commented at 3:57 am on December 22, 2020: member

au.activate.28 -> au.activate.30

  0$ git range-diff master au.activate.28 au.activate.30
  1
  21:  a9fb5a3174 ! 1:  d684ecd5f1 chainparams: add allowed assumeutxo values
  3    @@ -140,17 +140,10 @@
  4      #include <chainparams.h>
  5      #include <net.h>
  6      #include <signet.h>
  7    -+#include <optional.h>
  8     +#include <uint256.h>
  9      #include <validation.h>
 10
 11      #include <test/util/setup_common.h>
 12    -
 13    - #include <boost/test/unit_test.hpp>
 14    -+#include <boost/optional/optional_io.hpp>
 15    -
 16    - BOOST_FIXTURE_TEST_SUITE(validation_tests, TestingSetup)
 17    -
 18     @@
 19          BOOST_CHECK(!CheckSignetBlockSolution(block, signet_params->GetConsensus()));
 20      }
 21    @@ -166,14 +159,14 @@
 22     +
 23     +    for (auto empty : bad_heights) {
 24     +        const auto out = ExpectedAssumeutxo(empty, *params);
 25    -+        BOOST_CHECK_EQUAL(out, nullopt);
 26    ++        BOOST_CHECK(!out.has_value());
 27     +    }
 28     +
 29    -+    const auto out110 = ExpectedAssumeutxo(110, *params).get();
 30    ++    const auto out110 = ExpectedAssumeutxo(110, *params).value();
 31     +    BOOST_CHECK_EQUAL(out110.hash_serialized, uint256S("76fd7334ac7c1baf57ddc0c626f073a655a35d98a4258cd1382c8cc2b8392e10"));
 32     +    BOOST_CHECK_EQUAL(out110.nChainTx, (unsigned int)110);
 33     +
 34    -+    const auto out210 = ExpectedAssumeutxo(210, *params).get();
 35    ++    const auto out210 = ExpectedAssumeutxo(210, *params).value();
 36     +    BOOST_CHECK_EQUAL(out210.hash_serialized, uint256S("9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"));
 37     +    BOOST_CHECK_EQUAL(out210.nChainTx, (unsigned int)210);
 38     +}
 39    @@ -235,15 +228,3 @@
 40     +Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& params);
 41     +
 42      #endif // BITCOIN_VALIDATION_H
 43    -
 44    - diff --git a/test/lint/lint-includes.sh b/test/lint/lint-includes.sh
 45    - --- a/test/lint/lint-includes.sh
 46    - +++ b/test/lint/lint-includes.sh
 47    -@@
 48    -     boost/multi_index/sequenced_index.hpp
 49    -     boost/multi_index_container.hpp
 50    -     boost/optional.hpp
 51    -+    boost/optional/optional_io.hpp
 52    -     boost/preprocessor/cat.hpp
 53    -     boost/preprocessor/stringize.hpp
 54    -     boost/process.hpp
 552:  7356672cc1 ! 2:  e3240046d2 simplify ChainstateManager::SnapshotBlockhash() return semantics
 56    @@ -15,19 +15,11 @@
 57      #include <uint256.h>
 58      #include <validation.h>
 59      #include <validationinterface.h>
 60    -@@
 61    - #include <vector>
 62    -
 63    - #include <boost/test/unit_test.hpp>
 64    -+#include <boost/optional/optional_io.hpp>
 65    -
 66    - BOOST_FIXTURE_TEST_SUITE(validation_chainstatemanager_tests, ChainTestingSetup)
 67    -
 68     @@
 69          std::vector<CChainState*> chainstates;
 70          const CChainParams& chainparams = Params();
 71
 72    -+    BOOST_CHECK_EQUAL(manager.SnapshotBlockhash(), nullopt);
 73    ++    BOOST_CHECK(!manager.SnapshotBlockhash().has_value());
 74     +
 75          // Create a legacy (IBD) chainstate.
 76          //
 77    @@ -36,7 +28,7 @@
 78          auto& validated_cs = manager.ValidatedChainstate();
 79          BOOST_CHECK_EQUAL(&validated_cs, &c1);
 80
 81    -+    BOOST_CHECK_EQUAL(manager.SnapshotBlockhash(), nullopt);
 82    ++    BOOST_CHECK(!manager.SnapshotBlockhash().has_value());
 83     +
 84          // Create a snapshot-based chainstate.
 85          //
 86    @@ -46,7 +38,7 @@
 87     +        mempool, snapshot_blockhash));
 88          chainstates.push_back(&c2);
 89     +
 90    -+    BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().get(), snapshot_blockhash);
 91    ++    BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().value(), snapshot_blockhash);
 92     +
 93          c2.InitCoinsDB(
 94              /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
 953:  164ccb2cf6 ! 3:  80f5bacf55 validation: add ChainstateManager::ActivateSnapshot
 96    @@ -305,7 +305,7 @@
 97     +        return false;
 98     +    }
 99     +
100    -+    AssumeutxoData au_data = maybe_au_data.get();
101    ++    AssumeutxoData au_data = maybe_au_data.value();
102     +
103     +    if (stats.hashSerialized != au_data.hash_serialized) {
104     +        LogPrintf("[snapshot] bad snapshot content hash: expected %s, got %s\n",
1054:  3f69b370fb = 4:  822fdb59cf txdb: don't reset during in-memory cache resize
1065:  290b567736 = 5:  9a2c888df3 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset
1076:  78cdc6271a = 6:  414ba87784 tests: add deterministic chain generation unittest fixture
1087:  b9b819ae12 = 7:  51f3f97490 tests: add snapshot activation test
1098:  68dadbdcb9 = 8:  e62e2a96f6 test: Add tests with maleated snapshot data

DrahtBot removed the label Needs rebase on Dec 22, 2020

in src/validation.cpp:5188 in e62e2a96f6 outdated

5184@@ -5183,6 +5185,296 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const
5185     return *to_modify;
5186 }
5187 
5188+Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& chainparams)

jonatack commented at 9:46 am on December 22, 2020:

d684ecd5 pass cheaply copied types by non-const value

0Optional<AssumeutxoData> ExpectedAssumeutxo(int height, const CChainParams& chainparams)

Sjors commented at 1:32 pm on December 22, 2020:

This is not performance critical I think. ~~const has the additional benefit over being easier to reason about.~~ (nvm, it’s not a reference)

jonatack commented at 2:05 pm on December 22, 2020:

Yes, there have been a few conversations about this recently, like #19845 (review) and others. The developer notes mention referring to the C++ Core Guidelines but maybe a line about this in the notes would save everyone time.

sipa commented at 9:58 pm on December 29, 2020:

Response to https://github.com/bitcoin/bitcoin/pull/19806/commits/d684ecd5f11b2cf63235cd483858f3fd27f8c712#r547175326: @jonatack C++ Core Guidelines only say “don’t enforce const arguments for function arguments”, not don’t use them. I’d consider whether someone uses them to be personal style.

jonatack commented at 7:15 pm on January 7, 2021:

@sipa these C++ Core Guidelines appear to recommend not passing cheaply copied values as const:

sipa commented at 7:23 pm on January 7, 2021:

The only relevant discussion about by-value function arguments declared const I can see is:

Exception

Function arguments are rarely mutated, but also rarely declared const. To avoid confusion and lots of false positives, don’t enforce this rule for function arguments.

So I read this as: the general advice (make immutable things const) applies, but you’re not supposed to warn about it for function arguments because that would confuse people.

I consider this personal style, and up to the author of the code.

jonatack commented at 7:32 pm on January 7, 2021:

@sipa per the replies by you and @vasild at #19845 (review), My take is the same as the isocpp guidelines - no const for function arguments that are passed by value, I had the impression that this is considered a guideline, but I won’t comment on it further in reviews.

in src/chainparams.cpp:450 in e62e2a96f6 outdated

445+            },
446+            {
447+                210,
448+                {uint256S("0x9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"), 210},
449+            },
450+        };

jonatack commented at 10:16 am on December 22, 2020:

Sorry if this was previously discussed in the earlier, can MapAssumeutxo be simplified to not store the height twice?

sipa commented at 9:41 pm on December 29, 2020:

Follow-up to https://github.com/bitcoin/bitcoin/pull/19806/commits/d684ecd5f11b2cf63235cd483858f3fd27f8c712#r547190362: @jonatack It’s not the height, but the nChainTx value. It just happens to be equal to the height in chains that have never had anything but coinbase transactions.

jonatack commented at 7:53 pm on January 8, 2021:

Thanks, I must have been confused here. Will re-review this.

in src/validation.h:1014 in e62e2a96f6 outdated

1009+ *
1010+ * @param height[in] Get the assumeutxo value for this height.
1011+ *
1012+ * @returns empty if no assumeutxo configuration exists for the given height.
1013+ */
1014+Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& params);

jonatack commented at 10:22 am on December 22, 2020:

d684ecd5f11b2cf63235cd483858f3fd27f8c712

0Optional<AssumeutxoData> ExpectedAssumeutxo(int height, const CChainParams& params);

in src/validation.cpp:25 in e62e2a96f6 outdated

21@@ -22,6 +22,7 @@
22 #include <logging/timer.h>
23 #include <node/ui_interface.h>
24 #include <optional.h>
25+#include <node/coinstats.h>

jonatack commented at 10:34 am on December 22, 2020:

nit: sort

in src/validation.h:19 in e62e2a96f6 outdated

10@@ -11,10 +11,12 @@
11 #endif
12 
13 #include <amount.h>
14+#include <attributes.h>
15 #include <coins.h>
16 #include <crypto/common.h> // for ReadLE64
17 #include <fs.h>
18 #include <optional.h>
19+#include <node/utxo_snapshot.h>

jonatack commented at 10:34 am on December 22, 2020:

nit: sort

in src/validation.cpp:5240 in e62e2a96f6 outdated

5235+
5236+        // Temporarily resize the active coins cache to make room for the newly-created
5237+        // snapshot chain.
5238+        this->ActiveChainstate().ResizeCoinsCaches(
5239+            (size_t)(current_coinstip_cache_size * IBD_CACHE_PERC),
5240+            (size_t)(current_coinsdb_cache_size * IBD_CACHE_PERC));

jonatack commented at 10:49 am on December 22, 2020:

80f5bacf here and lines 5249 and 5252 below, use named casts for error avoidance. Named casts are more specific than a C-style or functional cast, allowing the compiler to catch some errors.

0-            (size_t)(current_coinstip_cache_size * IBD_CACHE_PERC),
1-            (size_t)(current_coinsdb_cache_size * IBD_CACHE_PERC));
2+            static_cast<size_t>(current_coinstip_cache_size * IBD_CACHE_PERC),
3+            static_cast<size_t>(current_coinsdb_cache_size * IBD_CACHE_PERC))

jamesob commented at 3:10 am on January 8, 2021:

Fixed (as well as a few others here), thanks.

in src/validation.cpp:5267 in e62e2a96f6 outdated

5262+
5263+    {
5264+        LOCK(::cs_main);
5265+        assert(!m_snapshot_chainstate);
5266+        m_snapshot_chainstate.swap(snapshot_chainstate);
5267+        bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(::Params());

jonatack commented at 10:58 am on December 22, 2020:

80f5bacf5 nit

0        const bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(::Params());

in src/validation.cpp:5316 in e62e2a96f6 outdated

5311+        coins_cache.cacheCoins.emplace(
5312+            std::piecewise_construct,
5313+            std::forward_as_tuple(std::move(outpoint)),
5314+            std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
5315+        coins_left -= 1;
5316+        coins_processed += 1;

jonatack commented at 11:04 am on December 22, 2020:

80f5bacf55

0-        coins_left -= 1;
1-        coins_processed += 1;
2+        --coins_left;
3+        ++coins_processed;

in src/validation.cpp:5321 in e62e2a96f6 outdated

5316+        coins_processed += 1;
5317+
5318+        if (coins_processed % 1000000 == 0) {
5319+            LogPrintf("[snapshot] %d coins loaded (%.2f%%, %.2f MB)\n",
5320+                coins_processed,
5321+                (float) coins_processed * 100 / (float) coins_count,

jonatack commented at 11:05 am on December 22, 2020:

80f5bacf use named casts

in src/validation.cpp:5406 in e62e2a96f6 outdated

5401+    int max_secs_to_wait_for_headers = 60 * 10;
5402+    CBlockIndex* snapshot_start_block = nullptr;
5403+
5404+    while (max_secs_to_wait_for_headers > 0) {
5405+        snapshot_start_block = WITH_LOCK(::cs_main, return LookupBlockIndex(base_blockhash));
5406+        max_secs_to_wait_for_headers -= 1;

jonatack commented at 11:13 am on December 22, 2020:

80f5bacf55

0        --max_secs_to_wait_for_headers;

in src/rpc/blockchain.cpp:2424 in e62e2a96f6 outdated

2415+    return result;
2416+},
2417+    };
2418+}
2419+
2420+UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile)

jonatack commented at 11:29 am on December 22, 2020:

9a2c888d NodeContext should be passed by reference to const

0UniValue CreateUTXOSnapshot(const NodeContext& node, CChainState& chainstate, CAutoFile& afile);

in src/test/util/setup_common.cpp:232 in e62e2a96f6 outdated

233+
234+void TestChain100Setup::mineBlocks(int num_blocks)
235+{
236     CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
237-    for (int i = 0; i < COINBASE_MATURITY; i++) {
238+    for (int i = 0; i < num_blocks; i++)

jonatack commented at 11:32 am on December 22, 2020:

414ba8778 here and 51f3f974 in a few places in src/test/validation_chainstatemanager_tests.cpp, per developer-notes.md prefer the prefix operator

0    for (int i = 0; i < num_blocks; ++i)

in src/test/validation_chainstatemanager_tests.cpp:217 in e62e2a96f6 outdated

213+        LOCK(::cs_main);
214+        CCoinsViewCache& ibd_coinscache = chainman.ActiveChainstate().CoinsTip();
215+        initial_size = ibd_coinscache.GetCacheSize();
216+        size_t total_coins{0};
217+
218+        for (CTransactionRef& txn : m_coinbase_txns) {

jonatack commented at 11:40 am on December 22, 2020:

51f3f974 here and lines 291 and 320

0        for (const CTransactionRef& txn : m_coinbase_txns) {

in src/test/validation_chainstatemanager_tests.cpp:260 in e62e2a96f6 outdated

256+            metadata.m_coins_count += 1;
257+    }));
258+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
259+        m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
260+            // Coins count is smaller than coins in file
261+            metadata.m_coins_count -= 1;

jonatack commented at 11:43 am on December 22, 2020:

51f3f974 here and line 245 above

0            --metadata.m_coins_count;

in src/test/validation_chainstatemanager_tests.cpp:255 in e62e2a96f6 outdated

251+            auto_infile >> coin;
252+    }));
253+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(
254+        m_node, m_path_root, [](CAutoFile& auto_infile, SnapshotMetadata& metadata) {
255+            // Coins count is larger than coins in file
256+            metadata.m_coins_count += 1;

jonatack commented at 11:43 am on December 22, 2020:

51f3f974

0            ++metadata.m_coins_count;

Sjors commented at 11:43 am on December 22, 2020: member

re-ACK e62e2a96f623f5d38845d472329d1a8253e146d0

jonatack commented at 11:51 am on December 22, 2020: member

ACK e62e2a96f623f5d38845d472329d1a8253e146d0

Various suggestions below, feel free to pick/choose/ignore/defer.

jamesob commented at 6:19 pm on December 22, 2020: member

@jonatack prefer to defer those cleanups for a follow-up PR unless others feel it necessary to do those there, thanks for the look.

fjahr commented at 7:40 pm on December 23, 2020: member

re-ACK e62e2a96f623f5d38845d472329d1a8253e146d0

Only changes since last review are updated usage of optional since std::optional was merged.

in src/validation.cpp:5187 in d684ecd5f1 outdated

5183@@ -5183,6 +5184,17 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const
5184     return *to_modify;
5185 }
5186 
5187+Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& chainparams)

sipa commented at 10:00 pm on December 29, 2020:

In commit “chainparams: add allowed assumeutxo values”

Returning a copy of the AssumeUtxoData here is unnecessary, as the data is immutable. You could return a Optional<std::reference_wrapper<AssumeutxoData>> instead.

in src/coins.h:253 in 80f5bacf55 outdated

248@@ -246,6 +249,9 @@ class CCoinsViewCache : public CCoinsViewBacked
249     /* Cached dynamic memory usage for the inner Coin objects. */
250     mutable size_t cachedCoinsUsage;
251 
252+    // Necessary so that we can write directly into cacheCoins during snapshot load.
253+    friend ChainstateManager;

sipa commented at 10:10 pm on December 29, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot”

This seems pretty ugly. I’d rather expose an actually public “danger” interface to CCoinsViewCache than needing to make CCoinsViewCache aware of ChainstateManager.

jamesob commented at 3:08 am on January 8, 2021:

Fixed, thanks. I called this EmplaceCoinInternalDANGER(), let me know if you’d prefer something else.

fjahr commented at 11:16 pm on January 9, 2021:

in 3a547327446a3608ef2af25a8d25a42954fc5116

With the introduction of EmplaceCoinInternalDANGER() you can remove this line now, right?

jamesob commented at 11:51 pm on January 9, 2021:

Ugh, embarrassing. Thanks.

in src/validation.cpp:5220 in 80f5bacf55 outdated

5215+    //
5216+    // These particular percentages don't matter so much since they will only be
5217+    // relevant during snapshot activation; caches are rebalanced at the conclusion of
5218+    // this function. We want to give (essentially) all available cache capacity to the
5219+    // snapshot to aid the bulk load later in this function.
5220+    constexpr double IBD_CACHE_PERC = 0.01;

sipa commented at 10:18 pm on December 29, 2020:

In commit “validation: add ChainstateManager::ActivateSnapshot”:

could be made static.

jamesob commented at 2:33 pm on January 5, 2021: member

Would appreciate some guidance here if maintainers are waiting on anything to be done.

in src/chainparams.cpp:11 in d684ecd5f1 outdated

 7@@ -8,7 +8,6 @@
 8 #include <chainparamsseeds.h>
 9 #include <consensus/merkle.h>
10 #include <hash.h> // for signet block challenge hash
11-#include <tinyformat.h>

MarcoFalke commented at 10:38 am on January 6, 2021:

7a6c46b37edb8bfa0085d202aa7e9427d5e4fceb: why is this removed. This is needed for strprintf

laanwj commented at 6:17 pm on January 7, 2021: member

Code review ACK e62e2a96f623f5d38845d472329d1a8253e146d0

laanwj commented at 6:19 pm on January 7, 2021: member

The cirrus CI error looks relevant, though strange (a race in ostream?):

 0Wrote UTXO snapshot to /tmp/test_common_Bitcoin Core/35ec263ac30d22e47a7de455d58dbf0fd5788ec03feee003dfe753772e175d86/test_snapshot.110.dat: {"coins_written":110,"base_hash":"5149f8fe2875e71f5ef664b0e78909cfbd651356fdf5505558323f8321405269","base_height":110}
 1make[3]: *** [Makefile:16615: test/validation_chainstatemanager_tests.cpp.test] Error 1
 2make[3]: *** Waiting for unfinished jobs....
 3make[3]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
 4make[2]: *** [Makefile:15156: check-am] Error 2
 5make[2]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
 6make[1]: *** [Makefile:14841: check-recursive] Error 1
 7make[1]: Leaving directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
 8make: *** [Makefile:806: check-recursive] Error 1
 9==================
10WARNING: ThreadSanitizer: data race (pid=26233)
11  Read of size 8 at 0x7f77ce57d290 by main thread:
12    [#0](/bitcoin-bitcoin/0/) std::__1::ios_base::width() const /usr/lib/llvm-10/bin/../include/c++/v1/ios:522:12 (test_bitcoin+0x163a0c)
13    [#1](/bitcoin-bitcoin/1/) std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> > std::__1::__pad_and_output<char, std::__1::char_traits<char> >(std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> >, char const*, char const*, char const*, std::__1::ios_base&, char) /usr/lib/llvm-10/bin/../include/c++/v1/locale:1385:29 (test_bitcoin+0x163a0c)
14    [#2](/bitcoin-bitcoin/2/) std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::__put_character_sequence<char, std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*, unsigned long) /usr/lib/llvm-10/bin/../include/c++/v1/ostream:730:17 (test_bitcoin+0x1638b3)
15    [#3](/bitcoin-bitcoin/3/) std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::operator<<<std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*) /usr/lib/llvm-10/bin/../include/c++/v1/ostream:869:12 (test_bitcoin+0x789ec1)
16    [#4](/bitcoin-bitcoin/4/) boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream, char [24], char const (&) [24]>::operator()(std::__1::basic_ostream<char, std::__1::char_traits<char> >&) const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/utils/lazy_ostream.hpp:67:29 (test_bitcoin+0x789ec1)
17    [#5](/bitcoin-bitcoin/5/) boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream, char [24], char const (&) [24]>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>::operator()(std::__1::basic_ostream<char, std::__1::char_traits<char> >&) const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/utils/lazy_ostream.hpp:67:16 (test_bitcoin+0x789d6b)
18    [#6](/bitcoin-bitcoin/6/) boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream, char [24], char const (&) [24]>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>, char [3], char const (&) [3]>::operator()(std::__1::basic_ostream<char, std::__1::char_traits<char> >&) const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/utils/lazy_ostream.hpp:67:16 (test_bitcoin+0x789c6b)
19    [#7](/bitcoin-bitcoin/7/) boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream_impl<boost::unit_test::lazy_ostream, char [24], char const (&) [24]>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>, char [3], char const (&) [3]>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>::operator()(std::__1::basic_ostream<char, std::__1::char_traits<char> >&) const /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/test/utils/lazy_ostream.hpp:67:16 (test_bitcoin+0x789b3b)
20    [#8](/bitcoin-bitcoin/8/) boost::unit_test::unit_test_log_t::operator<<(boost::unit_test::lazy_ostream const&) <null> (test_bitcoin+0xf0181b)
21    [#9](/bitcoin-bitcoin/9/) validation_chainstatemanager_tests::chainstatemanager_activate_snapshot_invoker() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/validation_chainstatemanager_tests.cpp:204:1 (test_bitcoin+0x7834d8)
22    [#10](/bitcoin-bitcoin/10/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1b109d)
23    [#11](/bitcoin-bitcoin/11/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xf3063a)
24  Previous write of size 8 at 0x7f77ce57d290 by thread T4 (mutexes: write M643):
25    [#0](/bitcoin-bitcoin/0/) std::__1::ios_base::width(long) /usr/lib/llvm-10/bin/../include/c++/v1/ios:530:14 (test_bitcoin+0x163b1c)
26    [#1](/bitcoin-bitcoin/1/) std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> > std::__1::__pad_and_output<char, std::__1::char_traits<char> >(std::__1::ostreambuf_iterator<char, std::__1::char_traits<char> >, char const*, char const*, char const*, std::__1::ios_base&, char) /usr/lib/llvm-10/bin/../include/c++/v1/locale:1417:11 (test_bitcoin+0x163b1c)
27    [#2](/bitcoin-bitcoin/2/) std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::__put_character_sequence<char, std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*, unsigned long) /usr/lib/llvm-10/bin/../include/c++/v1/ostream:730:17 (test_bitcoin+0x1638b3)
28    [#3](/bitcoin-bitcoin/3/) std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::operator<<<char, std::__1::char_traits<char>, std::__1::allocator<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/lib/llvm-10/bin/../include/c++/v1/ostream:1052:12 (test_bitcoin+0x163427)
29    [#4](/bitcoin-bitcoin/4/) $_0::operator()(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/main.cpp:25:15 (test_bitcoin+0x163427)
30    [#5](/bitcoin-bitcoin/5/) decltype(std::__1::forward<$_0&>(fp)(std::__1::forward<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>(fp0))) std::__1::__invoke<$_0&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>($_0&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3539:1 (test_bitcoin+0x163427)
31    [#6](/bitcoin-bitcoin/6/) void std::__1::__invoke_void_return_wrapper<void>::__call<$_0&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>($_0&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/lib/llvm-10/bin/../include/c++/v1/__functional_base:348:9 (test_bitcoin+0x163427)
32    [#7](/bitcoin-bitcoin/7/) std::__1::__function::__alloc_func<$_0, std::__1::allocator<$_0>, void (std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)>::operator()(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/lib/llvm-10/bin/../include/c++/v1/functional:1540:16 (test_bitcoin+0x163427)
33    [#8](/bitcoin-bitcoin/8/) std::__1::__function::__func<$_0, std::__1::allocator<$_0>, void (std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)>::operator()(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/lib/llvm-10/bin/../include/c++/v1/functional:1714:12 (test_bitcoin+0x163427)
34    [#9](/bitcoin-bitcoin/9/) std::__1::__function::__value_func<void (std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)>::operator()(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const /usr/lib/llvm-10/bin/../include/c++/v1/functional:1867:16 (test_bitcoin+0xc2a0c9)
35    [#10](/bitcoin-bitcoin/10/) std::__1::function<void (std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)>::operator()(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) const /usr/lib/llvm-10/bin/../include/c++/v1/functional:2473:12 (test_bitcoin+0xc2a0c9)
36    [#11](/bitcoin-bitcoin/11/) BCLog::Logger::LogPrintStr(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/logging.cpp:264:9 (test_bitcoin+0xc2a0c9)
37    [#12](/bitcoin-bitcoin/12/) void LogPrintf<char const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >(char const*, char const* const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./logging.h:176:23 (test_bitcoin+0xb63ad5)
38    [#13](/bitcoin-bitcoin/13/) CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13::operator()() const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/validationinterface.cpp:243:5 (test_bitcoin+0xb686a0)
39    [#14](/bitcoin-bitcoin/14/) decltype(std::__1::forward<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13&>(fp)()) std::__1::__invoke<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13&>(CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3539:1 (test_bitcoin+0xb686a0)
40    [#15](/bitcoin-bitcoin/15/) void std::__1::__invoke_void_return_wrapper<void>::__call<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13&>(CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13&) /usr/lib/llvm-10/bin/../include/c++/v1/__functional_base:348:9 (test_bitcoin+0xb686a0)
41    [#16](/bitcoin-bitcoin/16/) std::__1::__function::__alloc_func<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13, std::__1::allocator<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13>, void ()>::operator()() /usr/lib/llvm-10/bin/../include/c++/v1/functional:1540:16 (test_bitcoin+0xb686a0)
42    [#17](/bitcoin-bitcoin/17/) std::__1::__function::__func<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13, std::__1::allocator<CMainSignals::ChainStateFlushed(CBlockLocator const&)::$_13>, void ()>::operator()() /usr/lib/llvm-10/bin/../include/c++/v1/functional:1714:12 (test_bitcoin+0xb686a0)
43    [#18](/bitcoin-bitcoin/18/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-10/bin/../include/c++/v1/functional:1867:16 (test_bitcoin+0xbeaa1a)
44    [#19](/bitcoin-bitcoin/19/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-10/bin/../include/c++/v1/functional:2473:12 (test_bitcoin+0xbeaa1a)
45    [#20](/bitcoin-bitcoin/20/) SingleThreadedSchedulerClient::ProcessQueue() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/scheduler.cpp:173:5 (test_bitcoin+0xbeaa1a)
46    [#21](/bitcoin-bitcoin/21/) decltype(*(std::__1::forward<SingleThreadedSchedulerClient*&>(fp0)).*fp()) std::__1::__invoke<void (SingleThreadedSchedulerClient::*&)(), SingleThreadedSchedulerClient*&, void>(void (SingleThreadedSchedulerClient::*&)(), SingleThreadedSchedulerClient*&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3480:1 (test_bitcoin+0xbec9ca)
47    [#22](/bitcoin-bitcoin/22/) std::__1::__bind_return<void (SingleThreadedSchedulerClient::*)(), std::__1::tuple<SingleThreadedSchedulerClient*>, std::__1::tuple<>, __is_valid_bind_return<void (SingleThreadedSchedulerClient::*)(), std::__1::tuple<SingleThreadedSchedulerClient*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (SingleThreadedSchedulerClient::*)(), std::__1::tuple<SingleThreadedSchedulerClient*>, 0ul, std::__1::tuple<> >(void (SingleThreadedSchedulerClient::*&)(), std::__1::tuple<SingleThreadedSchedulerClient*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&) /usr/lib/llvm-10/bin/../include/c++/v1/functional:2770:12 (test_bitcoin+0xbec9ca)
48    [#23](/bitcoin-bitcoin/23/) std::__1::__bind_return<void (SingleThreadedSchedulerClient::*)(), std::__1::tuple<SingleThreadedSchedulerClient*>, std::__1::tuple<>, __is_valid_bind_return<void (SingleThreadedSchedulerClient::*)(), std::__1::tuple<SingleThreadedSchedulerClient*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>::operator()<>() /usr/lib/llvm-10/bin/../include/c++/v1/functional:2803:20 (test_bitcoin+0xbec9ca)
49    [#24](/bitcoin-bitcoin/24/) decltype(std::__1::forward<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>&>(fp)()) std::__1::__invoke<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>&>(std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>&) /usr/lib/llvm-10/bin/../include/c++/v1/type_traits:3539:1 (test_bitcoin+0xbec9ca)
50    [#25](/bitcoin-bitcoin/25/) void std::__1::__invoke_void_return_wrapper<void>::__call<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>&>(std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>&) /usr/lib/llvm-10/bin/../include/c++/v1/__functional_base:348:9 (test_bitcoin+0xbec9ca)
51    [#26](/bitcoin-bitcoin/26/) std::__1::__function::__alloc_func<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>, std::__1::allocator<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*> >, void ()>::operator()() /usr/lib/llvm-10/bin/../include/c++/v1/functional:1540:16 (test_bitcoin+0xbec9ca)
52    [#27](/bitcoin-bitcoin/27/) std::__1::__function::__func<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*>, std::__1::allocator<std::__1::__bind<void (SingleThreadedSchedulerClient::*)(), SingleThreadedSchedulerClient*> >, void ()>::operator()() /usr/lib/llvm-10/bin/../include/c++/v1/functional:1714:12 (test_bitcoin+0xbec9ca)
53    [#28](/bitcoin-bitcoin/28/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-10/bin/../include/c++/v1/functional:1867:16 (test_bitcoin+0xbe99d5)
54    [#29](/bitcoin-bitcoin/29/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-10/bin/../include/c++/v1/functional:2473:12 (test_bitcoin+0xbe99d5)
55    [#30](/bitcoin-bitcoin/30/) CScheduler::serviceQueue() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/scheduler.cpp:60:17 (test_bitcoin+0xbe99d5)
56    [#31](/bitcoin-bitcoin/31/) ChainTestingSetup::ChainTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&)::$_0::operator()() const::'lambda'()::operator()() const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:134:86 (test_bitcoin+0x8307f1)
57    [#32](/bitcoin-bitcoin/32/) void TraceThread<ChainTestingSetup::ChainTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&)::$_0::operator()() const::'lambda'()>(char const*, ChainTestingSetup::ChainTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&)::$_0::operator()() const::'lambda'()) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./util/system.h:438:9 (test_bitcoin+0x8307f1)
58    [#33](/bitcoin-bitcoin/33/) ChainTestingSetup::ChainTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&)::$_0::operator()() const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:134:37 (test_bitcoin+0x8307f1)
59    [#34](/bitcoin-bitcoin/34/) boost::detail::thread_data<ChainTestingSetup::ChainTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&)::$_0>::run() /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/thread/detail/thread.hpp:120:17 (test_bitcoin+0x8307f1)
60    [#35](/bitcoin-bitcoin/35/) boost::(anonymous namespace)::thread_proxy(void*) <null> (test_bitcoin+0xedacce)
61  Location is global 'std::__1::cout' of size 160 at 0x7f77ce57d270 (libc++.so.1+0x0000000c0290)
62  Mutex M643 (0x7b2400000240) created at:
63    [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock <null> (test_bitcoin+0xf10d6)
64    [#1](/bitcoin-bitcoin/1/) std::__1::mutex::lock() <null> (libc++.so.1+0x83505)
65    [#2](/bitcoin-bitcoin/2/) SeedInsecureRand(SeedRand) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./test/util/setup_common.h:60:9 (test_bitcoin+0x82ccd7)
66    [#3](/bitcoin-bitcoin/3/) BasicTestingSetup::BasicTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:99:5 (test_bitcoin+0x82ccd7)
67    [#4](/bitcoin-bitcoin/4/) ChainTestingSetup::ChainTestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:129:7 (test_bitcoin+0x82d273)
68    [#5](/bitcoin-bitcoin/5/) validation_chainstatemanager_tests::chainstatemanager::chainstatemanager() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/validation_chainstatemanager_tests.cpp:29:1 (test_bitcoin+0x77bd00)
69    [#6](/bitcoin-bitcoin/6/) validation_chainstatemanager_tests::chainstatemanager_invoker() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/validation_chainstatemanager_tests.cpp:29:1 (test_bitcoin+0x77bd00)
70    [#7](/bitcoin-bitcoin/7/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1b109d)
71    [#8](/bitcoin-bitcoin/8/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xf3063a)
72  Thread T4 'b-scheduler' (tid=26248, running) created by main thread at:
73    [#0](/bitcoin-bitcoin/0/) pthread_create <null> (test_bitcoin+0xd3d0b)
74    [#1](/bitcoin-bitcoin/1/) boost::thread::start_thread_noexcept() <null> (test_bitcoin+0xedabcd)
75    [#2](/bitcoin-bitcoin/2/) TestingSetup::TestingSetup(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::vector<char const*, std::__1::allocator<char const*> > const&) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:171:7 (test_bitcoin+0x82df0d)
76    [#3](/bitcoin-bitcoin/3/) RegTestingSetup::RegTestingSetup() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./test/util/setup_common.h:105:11 (test_bitcoin+0x82e768)
77    [#4](/bitcoin-bitcoin/4/) TestChain100Setup::TestChain100Setup(bool) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:205:20 (test_bitcoin+0x82e768)
78    [#5](/bitcoin-bitcoin/5/) TestChain100DeterministicSetup::TestChain100DeterministicSetup() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./test/util/setup_common.h:137:40 (test_bitcoin+0x783014)
79    [#6](/bitcoin-bitcoin/6/) validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::chainstatemanager_activate_snapshot() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/validation_chainstatemanager_tests.cpp:204:1 (test_bitcoin+0x783014)
80    [#7](/bitcoin-bitcoin/7/) validation_chainstatemanager_tests::chainstatemanager_activate_snapshot_invoker() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/validation_chainstatemanager_tests.cpp:204:1 (test_bitcoin+0x783014)
81    [#8](/bitcoin-bitcoin/8/) boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 (test_bitcoin+0x1b109d)
82    [#9](/bitcoin-bitcoin/9/) boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) <null> (test_bitcoin+0xf3063a)
83SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-10/bin/../include/c++/v1/ios:522:12 in std::__1::ios_base::width() const
84==================
85Exit status: 2

sipa commented at 7:04 pm on January 7, 2021: member

A few comments I wrote earlier but forgot to submit.

MarcoFalke commented at 8:43 pm on January 7, 2021: member

I could only reproduce the tsan failure once, so was unable to debug. Maybe just add race:validation_chainstatemanager_tests to the suppressions file?

jamesob force-pushed on Jan 8, 2021

jamesob commented at 3:10 pm on January 8, 2021: member

au.activate.30 -> au.activate.31

  0$ git range-diff master au.activate.30 au.activate.31
  1
  21:  d684ecd5f1 ! 1:  5b7fede907 chainparams: add allowed assumeutxo values
  3    @@ -162,11 +162,11 @@
  4     +        BOOST_CHECK(!out.has_value());
  5     +    }
  6     +
  7    -+    const auto out110 = ExpectedAssumeutxo(110, *params).value();
  8    ++    const auto out110 = ExpectedAssumeutxo(110, *params).value().get();
  9     +    BOOST_CHECK_EQUAL(out110.hash_serialized, uint256S("76fd7334ac7c1baf57ddc0c626f073a655a35d98a4258cd1382c8cc2b8392e10"));
 10     +    BOOST_CHECK_EQUAL(out110.nChainTx, (unsigned int)110);
 11     +
 12    -+    const auto out210 = ExpectedAssumeutxo(210, *params).value();
 13    ++    const auto out210 = ExpectedAssumeutxo(210, *params).value().get();
 14     +    BOOST_CHECK_EQUAL(out210.hash_serialized, uint256S("9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"));
 15     +    BOOST_CHECK_EQUAL(out210.nChainTx, (unsigned int)210);
 16     +}
 17    @@ -188,15 +188,16 @@
 18          return *to_modify;
 19      }
 20
 21    -+Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& chainparams)
 22    ++Optional<std::reference_wrapper<const AssumeutxoData>> ExpectedAssumeutxo(
 23    ++    const int height, const CChainParams& chainparams)
 24     +{
 25     +    const MapAssumeutxo& valid_assumeutxos_map = chainparams.Assumeutxo();
 26     +    const auto assumeutxo_found = valid_assumeutxos_map.find(height);
 27     +
 28     +    if (assumeutxo_found != valid_assumeutxos_map.end()) {
 29    -+        return assumeutxo_found->second;
 30    ++        return std::ref(assumeutxo_found->second);
 31     +    }
 32    -+    return {};
 33    ++    return std::nullopt;
 34     +}
 35     +
 36      CChainState& ChainstateManager::ActiveChainstate() const
 37    @@ -225,6 +226,7 @@
 38     + *
 39     + * [@returns](/bitcoin-bitcoin/contributor/returns/) empty if no assumeutxo configuration exists for the given height.
 40     + */
 41    -+Optional<AssumeutxoData> ExpectedAssumeutxo(const int height, const CChainParams& params);
 42    ++Optional<std::reference_wrapper<const AssumeutxoData>> ExpectedAssumeutxo(
 43    ++    const int height, const CChainParams& params);
 44     +
 45      #endif // BITCOIN_VALIDATION_H
 462:  e3240046d2 = 2:  cc86ebab20 simplify ChainstateManager::SnapshotBlockhash() return semantics
 473:  80f5bacf55 ! 3:  3a54732744 validation: add ChainstateManager::ActivateSnapshot
 48    @@ -34,6 +34,25 @@
 49
 50          //! block header
 51
 52    + diff --git a/src/coins.cpp b/src/coins.cpp
 53    + --- a/src/coins.cpp
 54    + +++ b/src/coins.cpp
 55    +@@
 56    +     cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
 57    + }
 58    +
 59    ++void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
 60    ++    cachedCoinsUsage += coin.DynamicMemoryUsage();
 61    ++    cacheCoins.emplace(
 62    ++        std::piecewise_construct,
 63    ++        std::forward_as_tuple(std::move(outpoint)),
 64    ++        std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
 65    ++}
 66    ++
 67    + void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
 68    +     bool fCoinbase = tx.IsCoinBase();
 69    +     const uint256& txid = tx.GetHash();
 70    +
 71      diff --git a/src/coins.h b/src/coins.h
 72      --- a/src/coins.h
 73      +++ b/src/coins.h
 74    @@ -64,12 +83,39 @@
 75      public:
 76          CCoinsViewCache(CCoinsView *baseIn);
 77
 78    +@@
 79    +      */
 80    +     void AddCoin(const COutPoint& outpoint, Coin&& coin, bool possible_overwrite);
 81    +
 82    ++    /**
 83    ++     * Emplace a coin into cacheCoins without performing any checks, marking
 84    ++     * the emplaced coin as dirty.
 85    ++     *
 86    ++     * NOT FOR GENERAL USE. Used only when loading coins from a UTXO snapshot.
 87    ++     * [@sa](/bitcoin-bitcoin/contributor/sa/) ChainstateManager::PopulateAndValidateSnapshot()
 88    ++     */
 89    ++    void EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin);
 90    ++
 91    +     /**
 92    +      * Spend a coin. Pass moveto in order to get the deleted data.
 93    +      * If no unspent output exists for the passed outpoint, this call
 94
 95      diff --git a/src/validation.cpp b/src/validation.cpp
 96      --- a/src/validation.cpp
 97      +++ b/src/validation.cpp
 98     @@
 99    -     return {};
100    + #include <index/txindex.h>
101    + #include <logging.h>
102    + #include <logging/timer.h>
103    ++#include <node/coinstats.h>
104    + #include <node/ui_interface.h>
105    + #include <optional.h>
106    +-#include <node/coinstats.h>
107    + #include <policy/policy.h>
108    + #include <policy/settings.h>
109    + #include <pow.h>
110    +@@
111    +     return std::nullopt;
112      }
113
114     +bool ChainstateManager::ActivateSnapshot(
115    @@ -93,8 +139,8 @@
116     +    // relevant during snapshot activation; caches are rebalanced at the conclusion of
117     +    // this function. We want to give (essentially) all available cache capacity to the
118     +    // snapshot to aid the bulk load later in this function.
119    -+    constexpr double IBD_CACHE_PERC = 0.01;
120    -+    constexpr double SNAPSHOT_CACHE_PERC = 0.99;
121    ++    static constexpr double IBD_CACHE_PERC = 0.01;
122    ++    static constexpr double SNAPSHOT_CACHE_PERC = 0.99;
123     +
124     +    {
125     +        LOCK(::cs_main);
126    @@ -112,8 +158,8 @@
127     +        // Temporarily resize the active coins cache to make room for the newly-created
128     +        // snapshot chain.
129     +        this->ActiveChainstate().ResizeCoinsCaches(
130    -+            (size_t)(current_coinstip_cache_size * IBD_CACHE_PERC),
131    -+            (size_t)(current_coinsdb_cache_size * IBD_CACHE_PERC));
132    ++            static_cast<size_t>(current_coinstip_cache_size * IBD_CACHE_PERC),
133    ++            static_cast<size_t>(current_coinsdb_cache_size * IBD_CACHE_PERC));
134     +    }
135     +
136     +    auto snapshot_chainstate = WITH_LOCK(::cs_main, return MakeUnique<CChainState>(
137    @@ -122,10 +168,10 @@
138     +    {
139     +        LOCK(::cs_main);
140     +        snapshot_chainstate->InitCoinsDB(
141    -+            (size_t)(current_coinsdb_cache_size * SNAPSHOT_CACHE_PERC),
142    ++            static_cast<size_t>(current_coinsdb_cache_size * SNAPSHOT_CACHE_PERC),
143     +            in_memory, false, "chainstate");
144     +        snapshot_chainstate->InitCoinsCache(
145    -+            (size_t)(current_coinstip_cache_size * SNAPSHOT_CACHE_PERC));
146    ++            static_cast<size_t>(current_coinstip_cache_size * SNAPSHOT_CACHE_PERC));
147     +    }
148     +
149     +    const bool snapshot_ok = this->PopulateAndValidateSnapshot(
150    @@ -140,7 +186,7 @@
151     +        LOCK(::cs_main);
152     +        assert(!m_snapshot_chainstate);
153     +        m_snapshot_chainstate.swap(snapshot_chainstate);
154    -+        bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(::Params());
155    ++        const bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(::Params());
156     +        assert(chaintip_loaded);
157     +
158     +        m_active_chainstate = m_snapshot_chainstate.get();
159    @@ -183,18 +229,15 @@
160     +            return false;
161     +        }
162     +        coins_file >> coin;
163    -+        coins_cache.cachedCoinsUsage += coin.DynamicMemoryUsage();
164    -+        coins_cache.cacheCoins.emplace(
165    -+            std::piecewise_construct,
166    -+            std::forward_as_tuple(std::move(outpoint)),
167    -+            std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
168    -+        coins_left -= 1;
169    -+        coins_processed += 1;
170    ++        coins_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));
171    ++
172    ++        --coins_left;
173    ++        ++coins_processed;
174     +
175     +        if (coins_processed % 1000000 == 0) {
176     +            LogPrintf("[snapshot] %d coins loaded (%.2f%%, %.2f MB)\n",
177     +                coins_processed,
178    -+                (float) coins_processed * 100 / (float) coins_count,
179    ++                static_cast<float>(coins_processed) * 100 / static_cast<float>(coins_count),
180     +                coins_cache.DynamicMemoryUsage() / (1000 * 1000));
181     +        }
182     +
183    @@ -279,7 +322,7 @@
184     +
185     +    while (max_secs_to_wait_for_headers > 0) {
186     +        snapshot_start_block = WITH_LOCK(::cs_main, return LookupBlockIndex(base_blockhash));
187    -+        max_secs_to_wait_for_headers -= 1;
188    ++        --max_secs_to_wait_for_headers;
189     +
190     +        if (!snapshot_start_block) {
191     +            std::this_thread::sleep_for(std::chrono::seconds(1));
192    @@ -305,7 +348,7 @@
193     +        return false;
194     +    }
195     +
196    -+    AssumeutxoData au_data = maybe_au_data.value();
197    ++    const AssumeutxoData& au_data = maybe_au_data.value();
198     +
199     +    if (stats.hashSerialized != au_data.hash_serialized) {
200     +        LogPrintf("[snapshot] bad snapshot content hash: expected %s, got %s\n",
201    @@ -366,11 +409,10 @@
202      #include <coins.h>
203      #include <crypto/common.h> // for ReadLE64
204      #include <fs.h>
205    - #include <optional.h>
206     +#include <node/utxo_snapshot.h>
207    + #include <optional.h>
208      #include <policy/feerate.h>
209      #include <protocol.h> // For CMessageHeader::MessageStartChars
210    - #include <script/script_error.h>
211     @@
212          //! by the background validation chainstate.
213          bool m_snapshot_validated{false};
2144:  822fdb59cf = 4:  94d8f69070 txdb: don't reset during in-memory cache resize
2155:  9a2c888df3 = 5:  5add970a98 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset
2166:  414ba87784 = 6:  6c6ec048f1 tests: add deterministic chain generation unittest fixture
2177:  51f3f97490 ! 7:  d14f6cf68d tests: add snapshot activation test
218    @@ -173,3 +173,15 @@
219     +}
220     +
221      BOOST_AUTO_TEST_SUITE_END()
222    +
223    + diff --git a/test/sanitizer_suppressions/tsan b/test/sanitizer_suppressions/tsan
224    + --- a/test/sanitizer_suppressions/tsan
225    + +++ b/test/sanitizer_suppressions/tsan
226    +@@
227    + race:BerkeleyDatabase
228    + race:DatabaseBatch
229    + race:leveldb::DBImpl::DeleteObsoleteFiles
230    ++race:validation_chainstatemanager_tests
231    + race:zmq::*
232    + race:bitcoin-qt
233    + # deadlock (TODO fix)
2348:  e62e2a96f6 = 8:  38b07a0a73 test: Add tests with maleated snapshot data

I’ve addressed all of @sipa’s feedback and a good chunk of @jonatack’s. Tests are passing. Thanks for the continued help everyone.

in src/validation.cpp:5188 in 38b07a0a73 outdated

5184@@ -5183,6 +5185,294 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const
5185     return *to_modify;
5186 }
5187 
5188+Optional<std::reference_wrapper<const AssumeutxoData>> ExpectedAssumeutxo(

jnewbery commented at 7:58 pm on January 8, 2021:

This seems like a pretty strange return type, especially since the only call site immediately assigns an Optional<AssumeutxoData> from the return value so you’re going to make a copy anyway. Why not just return a std::optional<AssumeutxoData> or AssumeutxoData*?

Also, prefer std::optional<> over Optional<> (Optional<> is just a wrapper for std::optional<> and is deprecated for new code).

jamesob commented at 8:18 pm on January 8, 2021:

See @sipa’s feedback here: #19806 (review)

fjahr commented at 11:33 pm on January 9, 2021:

Should probably still use std::optional though.

jamesob commented at 0:12 am on January 10, 2021:

Fixed.

jnewbery commented at 11:29 am on January 10, 2021:

I believe sipa’s point was that you could avoid a copy by returning a std::optional<std::reference_wrapper<T>>. However, you’re using that return to copy into a std::optional<T> in the only place that this is called:

https://github.com/bitcoin/bitcoin/pull/19806/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R5422

so you’re not actually saving a copy at all.

In any case, a copy of AssumeutxoData isn’t expensive (32 byte hash + 4 byte int) and this is an infrequent operation, so there’s really not problem just returning a std::optional<AssumeutxoData> (or indeed a const AssumeutxoData*> - returning a pointer with nullptr as a notfound sentinel is a common pattern).

Alternatively, if you want to avoid the copy, just don’t assign to std::optional<AssumeUtxoData> at the call site:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index b4206025be..8c95a837f8 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -5419,7 +5419,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
 5     // Assert that the deserialized chainstate contents match the expected assumeutxo value.
 6 
 7     int base_height = snapshot_start_block->nHeight;
 8-    Optional<AssumeutxoData> maybe_au_data = ExpectedAssumeutxo(base_height, ::Params());
 9+    auto maybe_au_data = ExpectedAssumeutxo(base_height, ::Params());
10 
11     if (!maybe_au_data) {
12         LogPrintf("[snapshot] assumeutxo height in snapshot metadata not recognized " /* Continued */
13@@ -5427,7 +5427,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
14         return false;
15     }
16 
17-    const AssumeutxoData& au_data = maybe_au_data.value();
18+    const AssumeutxoData& au_data = maybe_au_data.value().get();

I personally find the value().get() for fishing the reference out a bit ugly.

jamesob commented at 4:34 pm on January 10, 2021:

Gotcha, thanks @jnewbery.

in src/chainparams.h:114 in 38b07a0a73 outdated

109@@ -90,6 +110,11 @@ class CChainParams
110     const std::string& Bech32HRP() const { return bech32_hrp; }
111     const std::vector<SeedSpec6>& FixedSeeds() const { return vFixedSeeds; }
112     const CCheckpointData& Checkpoints() const { return checkpointData; }
113+
114+    //! Get allowed assumeutxo height-hash pairs.

jnewbery commented at 8:05 pm on January 8, 2021:

These aren’t height-hash pairs. It’s a map from height to <hash, nChainTx>.

felipsoarez commented at 1:09 pm on January 9, 2021: none

utACK

fjahr commented at 11:36 pm on January 9, 2021: member

Changes look good to me aside from the friend thingy.

jamesob force-pushed on Jan 10, 2021

fjahr commented at 0:41 am on January 10, 2021: member

Code review ACK a56b71054f9ba4f13cea9938826032f9359a39d4

Changes since last review only addressed review comments, most notably introduced EmplaceCoinInternalDANGER().

in src/validation.cpp:5422 in a56b71054f outdated

5417+    }
5418+
5419+    // Assert that the deserialized chainstate contents match the expected assumeutxo value.
5420+
5421+    int base_height = snapshot_start_block->nHeight;
5422+    Optional<AssumeutxoData> maybe_au_data = ExpectedAssumeutxo(base_height, ::Params());

jnewbery commented at 10:10 am on January 10, 2021:

Remove use of Optional wrapper.

in src/test/validation_chainstatemanager_tests.cpp:12 in a56b71054f outdated

 3@@ -4,13 +4,19 @@
 4 //
 5 #include <chainparams.h>
 6 #include <consensus/validation.h>
 7+#include <node/utxo_snapshot.h>
 8 #include <random.h>
 9+#include <rpc/blockchain.h>
10 #include <sync.h>
11 #include <test/util/setup_common.h>
12+#include <optional.h>

jnewbery commented at 10:11 am on January 10, 2021:

unused import

jamesob force-pushed on Jan 10, 2021

jamesob commented at 4:34 pm on January 10, 2021: member

au.activate.33 -> au.activate.34

 0$ git range-diff master au.activate.33 au.activate.34
 1
 21:  27f5ea82b5 = 1:  27f5ea82b5 chainparams: add allowed assumeutxo values
 32:  a1410726a1 ! 2:  b02730f8bc simplify ChainstateManager::SnapshotBlockhash() return semantics
 4    @@ -7,14 +7,6 @@
 5      diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
 6      --- a/src/test/validation_chainstatemanager_tests.cpp
 7      +++ b/src/test/validation_chainstatemanager_tests.cpp
 8    -@@
 9    - #include <random.h>
10    - #include <sync.h>
11    - #include <test/util/setup_common.h>
12    -+#include <optional.h>
13    - #include <uint256.h>
14    - #include <validation.h>
15    - #include <validationinterface.h>
16     @@
17          std::vector<CChainState*> chainstates;
18          const CChainParams& chainparams = Params();
193:  48c8af8f90 ! 3:  72470144a2 validation: add ChainstateManager::ActivateSnapshot
20    @@ -90,6 +90,24 @@
21           * Spend a coin. Pass moveto in order to get the deleted data.
22           * If no unspent output exists for the passed outpoint, this call
23
24    + diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp
25    + --- a/src/test/validation_tests.cpp
26    + +++ b/src/test/validation_tests.cpp
27    +@@
28    +         BOOST_CHECK(!out.has_value());
29    +     }
30    +
31    +-    const auto out110 = ExpectedAssumeutxo(110, *params).value().get();
32    ++    const AssumeutxoData& out110 = ExpectedAssumeutxo(110, *params).value();
33    +     BOOST_CHECK_EQUAL(out110.hash_serialized, uint256S("76fd7334ac7c1baf57ddc0c626f073a655a35d98a4258cd1382c8cc2b8392e10"));
34    +     BOOST_CHECK_EQUAL(out110.nChainTx, (unsigned int)110);
35    +
36    +-    const auto out210 = ExpectedAssumeutxo(210, *params).value().get();
37    ++    const AssumeutxoData& out210 = ExpectedAssumeutxo(210, *params).value();
38    +     BOOST_CHECK_EQUAL(out210.hash_serialized, uint256S("9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"));
39    +     BOOST_CHECK_EQUAL(out210.nChainTx, (unsigned int)210);
40    + }
41    +
42      diff --git a/src/validation.cpp b/src/validation.cpp
43      --- a/src/validation.cpp
44      +++ b/src/validation.cpp
45    @@ -330,7 +348,7 @@
46     +    // Assert that the deserialized chainstate contents match the expected assumeutxo value.
47     +
48     +    int base_height = snapshot_start_block->nHeight;
49    -+    Optional<AssumeutxoData> maybe_au_data = ExpectedAssumeutxo(base_height, ::Params());
50    ++    auto maybe_au_data = ExpectedAssumeutxo(base_height, ::Params());
51     +
52     +    if (!maybe_au_data) {
53     +        LogPrintf("[snapshot] assumeutxo height in snapshot metadata not recognized " /* Continued */
544:  3243fe11f0 = 4:  f53b871a4c txdb: don't reset during in-memory cache resize
555:  56bc86443f = 5:  8409053cc0 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset
566:  d28f6156c0 = 6:  86a8e742f8 tests: add deterministic chain generation unittest fixture
577:  3c9b16f8e4 ! 7:  065dc80745 tests: add snapshot activation test
58    @@ -14,8 +14,7 @@
59     +#include <rpc/blockchain.h>
60      #include <sync.h>
61      #include <test/util/setup_common.h>
62    - #include <optional.h>
63    -@@
64    + #include <uint256.h>
65      #include <validation.h>
66      #include <validationinterface.h>
67
688:  a56b71054f = 8:  f5fe848d67 test: Add tests with maleated snapshot data

fjahr commented at 11:29 pm on January 11, 2021: member

Code review ACK f5fe848d676b884692e25b6826737913afb020f5

Changes since last review only addressed @jnewbery ’s feedback on optional usage.

DrahtBot added the label Needs rebase on Jan 12, 2021

jamesob force-pushed on Jan 12, 2021

DrahtBot removed the label Needs rebase on Jan 12, 2021

fjahr commented at 9:23 pm on January 12, 2021: member

Code review ACK e77eea5150bd46d2972ec5086b86948df6e73a83

Confirmed only rebased since last review.

jonatack commented at 10:51 pm on January 12, 2021: member

ACK e77eea5150bd46d2972ec5086b86948df6e73a83 reviewed diff since my last review per git range-diff 7b97563 e62e2a9 e77eea5, then re-reviewed, debug building and running validation/validation_chainstatemanager unit tests at each commit, ran node at head as a sanity check.

in src/validation.cpp:5219 in 647c91957f outdated

5177@@ -5177,6 +5178,18 @@ CChainState& ChainstateManager::InitializeChainstate(CTxMemPool& mempool, const
5178     return *to_modify;
5179 }
5180 
5181+std::optional<std::reference_wrapper<const AssumeutxoData>> ExpectedAssumeutxo(

ryanofsky commented at 4:05 pm on January 15, 2021:

In commit “chainparams: add allowed assumeutxo values” (647c91957f99f5b9aed790e49941e3370fc3c0b2)

Should replace optional<reference_wrapper<T>> with T*. The advantage of using references instead of pointers is that references are not optional, so you don’t have to check for null. If you take reference and make it optional, you are just verbosely emulating a pointer.

ryanofsky approved

ryanofsky commented at 4:38 pm on January 15, 2021: member

Code review ACK e77eea5150bd46d2972ec5086b86948df6e73a83. Just rebase, std::optional updates, with_lock updates, EmplaceCoinInternalDANGER addition, const/cast updates since last review. Left one suggestion but feel free to ignore to avoid churn.

I don’t think there should be much hand-wringing about circular dependencies. As far as I know the only thing the circular dependencies linter provides is a vague indication that code organization sucks, and we already know that code organization sucks. It’s easy to satisfy the linter by just cramming everything into one file, but that wouldn’t make things better. Just listing the validation/coinstats dependency seems good in itself if it generates ideas for pulling a larger chunk of functionality out of validation.

in src/validation.cpp:5280 in c5154cad6c outdated

5260+        assert(!m_snapshot_chainstate);
5261+        m_snapshot_chainstate.swap(snapshot_chainstate);
5262+        const bool chaintip_loaded = m_snapshot_chainstate->LoadChainTip(::Params());
5263+        assert(chaintip_loaded);
5264+
5265+        m_active_chainstate = m_snapshot_chainstate.get();

ryanofsky commented at 2:56 pm on January 20, 2021:

In commit “validation: add ChainstateManager::ActivateSnapshot” (c5154cad6c195759409d6eb5134fad635b9aaade)

Credit to @jnewbery for pointing this out in #20749 (review), but the ChainStateManager::ActiveChainstate accessor isn’t currently locking cs_main or requiring cs_main to be locked. I think it needs to do one of these things now that the m_active_chainstate pointer can change here. It might also be appropriate to add GUARDED_BY annotation to m_active_chainstate.

dongcarl commented at 10:43 pm on January 20, 2021:

I’m most likely missing something, but I think it’d be sufficient just to lock cs_main in the ActiveChainstate accessor and not burden callers with locking cs_main. That way, callers of ActiveChainstate have the choice of either consistently using the chainstate reference they get, or locking cs_main themselves if they want to make sure that the chainstate doesn’t change from under them.

Incidentally, it seems to me that locking cs_main in ActiveChainstate may prevent a race condition between the assertion and the return in ActiveChainstate

jnewbery commented at 8:41 am on January 21, 2021:

I think eventually we’d like to move away from recursive locking, but that’s a long way off for cs_main, so there’s probably no harm in adding it here. Ultimately both are fine - as long as cs_main is held whenever m_active_chainstate is read or written to, then we’re safe. One thing that you’d need to check for is that none of the callers of the various Active*() functions are holding a lower lock (e.g. mempool.cs or g_cs_orphans) and not cs_main. If so, then adding a LOCK(cs_main) to ActiveChainstate() would be a lock inversion.

jnewbery commented at 10:04 pm on January 25, 2021:

There appear to be quite a few other locations where m_active_chainstate is read/written without holding cs_main. One example: https://github.com/bitcoin/bitcoin/blob/52d84a45e2fa3def71853cd31d5582ae31ea85d3/src/validation.cpp#L5145-L5151

This violates the conditions in the comment: “This means it is safe to acquire the contents of this pointer with ::cs_main held, release the lock, and then use the reference without concern of it being deconstructed.”

The earlier part of that comment also seems wrong after this PR: “Once this pointer is set to a corresponding chainstate, it will not be reset until init.cpp:Shutdown().” After this PR, the pointer is reset to a different value in ActivateSnapshot().

I haven’t been following the AssumeUTXO project, so perhaps this really is safe once snapshot activation is exposed, but from a quick code read it looks like there’s the potential for a data race on m_active_chainstate.

jamesob commented at 2:31 pm on January 26, 2021:

This violates the conditions in the comment: “This means it is safe to acquire the contents of this pointer with ::cs_main held, release the lock, and then use the reference without concern of it being deconstructed.”

How so? You can still acquire the pointer and use it without fear of segfault, since the underlying data will always correspond to either m_idb_chainstate or m_snapshot_chainstate. It may not be semantically what you want (though it may be too), but it will not segfault.

The earlier part of that comment also seems wrong after this PR

Yes, this does seem out of date.

jnewbery commented at 2:48 pm on January 26, 2021:

the underlying data will always correspond to either m_idb_chainstate or m_snapshot_chainstate

Not necessarily - it’s not safe to read the pointer value when it’s being written to by another thread since pointers aren’t guaranteed to be atomic https://stackoverflow.com/questions/8919818/is-pointer-assignment-atomic-in-c

dongcarl commented at 0:40 am on January 28, 2021: member

Hi all, I’ve had a chat offline with @jamesob about potential conflicts between this PR (#19806) and all of chainman-deglobalizing (#20158).

Thankfully, there is only one substantial conflict of note: the commit 385cb331bbf48cfba7b1e77e180d856100e1fdf1 pushed to #19806 yesterday which added lock annotations to the ActiveChain{,state}() suite of methods makes both rebase orders significantly harder, as it imposes additional locking requirements on its callers which are not easily scripted-diffable.

What I propose is that for now in #19806, we just lock ::cs_main inside ActiveChain{,state}() (mirroring how ::Chain{,state}Active() works) so that the two PRs don’t block each other. I have a modified version of #19806 here which demonstrates this change: https://github.com/dongcarl/bitcoin/tree/2021-01-au.activate-rebased-with-alt-locking. Of course, the lock annotation is preferable in the longer term, and I’d be happy to publish a separate PR for that.

With my proposed modification, there won’t be many conflicts, and I’ve tested that by playing out both merge orders:

Modified #19806 gets merged first, then #20158 (branch: https://github.com/dongcarl/bitcoin/tree/2021-01-autxo-rebase)
#20158 gets merged first, then modified #19806 (branch: https://github.com/dongcarl/bitcoin/tree/2021-01-autxo-rebase-me-first)

Both seem to be fairly straightforward, and arrive at the exact same tree (checked with git diff).

TL;DR #19806 and #20158 can be merged in any order without much pain if we redo #19806’s 385cb331bbf48cfba7b1e77e180d856100e1fdf1 like https://github.com/dongcarl/bitcoin/commit/08c6ece20ed398d7ed07b095353cb2a4bec7af2b and push the annotations in 385cb331bbf48cfba7b1e77e180d856100e1fdf1 to a followup PR.

jnewbery commented at 10:11 am on January 28, 2021: member

The most recent commit looks ok in that it addresses the issue pointed out here: #19806 (review). However, I think we should do more to ensure that there aren’t other potential data races in this series of PRs. For example:

https://github.com/bitcoin/bitcoin/blob/02b01651c56239db227bd58e85bc3174cda64519/src/validation.cpp#L5153-L5166

accesses both m_ibd_chainstate and m_snapshot_chainstate without locking cs_main first. The comment for both of those members says “it is safe to acquire the contents of this pointer with ::cs_main held, release the lock, and then use the reference without concern of it being deconstructed”. Currently, all callers of GetAll() are holding cs_main first, but there’s no enforcement of cs_main locking either by the compiler or at runtime.

I think that a good rule to follow is: if some mutable state can be read/written by multiple threads, it must be guarded by a mutex (or made atomic). Even if the code as written doesn’t have a data race because of the order in which functions are called, future changes could introduce very subtle bugs if those assumptions aren’t stated clearly.

For m_active_chainstate, I think it’s actually fine to just make it std::atomic<CChainstate*> since you don’t care which chainstate pointer it returns, as long as it’s valid. I think that’d make @dongcarl’s conflict here: #19806 (comment) much easier to resolve, since there would only be very minor changes at the call sites.

For m_ibd_chainstate and m_snapshot_chainstate, either they should be GUARDED_BY(::cs_main), or alternatively they could be made const std::unique_ptrs and initialized in the ChainstateManager constructor.

laanwj commented at 10:27 am on January 28, 2021: member

Hi all, I’ve had a chat offline with @jamesob about potential conflicts between this PR (#19806) and all of chainman-deglobalizing (#20158).

Glad that you two managed to resolve it, so both can move forward!

I think that a good rule to follow is: if some mutable state can be read/written by multiple threads, it must be guarded by a mutex (or made atomic).

I think this is a good rule and the motivation has to be really well argued to deviate from that. It’s important to keep in mind that while Intel is lenient here, different CPU architectures have different rules with regard to non-guarded memory access and multiple threads, with the regard to in which order and granularity the changes become visible (if at all) there no guarantees of sanity at all.

Would be something good to include in the developer notes.

dongcarl referenced this in commit c1d4af79b3 on Jan 28, 2021

dongcarl referenced this in commit 5b8118e0fd on Jan 28, 2021

dongcarl referenced this in commit f92dc6557a on Jan 28, 2021

dongcarl referenced this in commit e6eb8d2bbb on Jan 28, 2021

DrahtBot added the label Needs rebase on Feb 1, 2021

laanwj commented at 1:30 pm on February 1, 2021: member

Needs the (planned) rebase after #20749 merge.

MarcoFalke referenced this in commit f1239b70d1 on Feb 4, 2021

in src/validation.cpp:5326 in c5154cad6c outdated

5284+
5285+    uint256 base_blockhash = metadata.m_base_blockhash;
5286+
5287+    COutPoint outpoint;
5288+    Coin coin;
5289+    uint64_t coins_count = metadata.m_coins_count;

ariard commented at 2:40 pm on February 4, 2021:

03e0de1

I think you can constify coins_count?

in src/validation.cpp:5458 in c5154cad6c outdated

5437+    // Fake various pieces of CBlockIndex state:
5438+    //
5439+    //   - nChainTx: so that we accurately report IBD-to-tip progress
5440+    //   - nTx: so that LoadBlockIndex() loads assumed-valid CBlockIndex entries
5441+    //       (among other things)
5442+    //   - nStatus & BLOCK_OPT_WITNESS: so that RewindBlockIndex() doesn't zealously

ariard commented at 3:20 pm on February 4, 2021:

03e0de1

#21009 may lands first, will it be still valuable to mark assumed-valid blocks BLOCK_OPT_WITNESS ?

jamesob force-pushed on Feb 4, 2021

in src/validation.cpp:5456 in 03e0de1409 outdated

5463+    LOCK(::cs_main);
5464+
5465+    // Fake various pieces of CBlockIndex state:
5466+    //
5467+    //   - nChainTx: so that we accurately report IBD-to-tip progress
5468+    //   - nTx: so that LoadBlockIndex() loads assumed-valid CBlockIndex entries

ariard commented at 3:50 pm on February 4, 2021:

03e0de1

BlockManager::LoadBlockIndex only cares about nTx to update nChainTx. I think you want nTx > 0 to pass CheckBlockIndex ? See checks L4772 - L4780 ?

jamesob commented at 3:10 pm on February 11, 2021:

You may be right, but it requires more investigation - and certainly changing the logic in LoadBlockIndex, which I am hesitant to do. We can make this change later if we find it to be preferable for whatever reason.

sidhujag referenced this in commit 203242fcfc on Feb 4, 2021

DrahtBot removed the label Needs rebase on Feb 4, 2021

in src/node/coinstats.cpp:60 in f3bfa25a24 outdated

23@@ -24,6 +24,9 @@ static uint64_t GetBogoSize(const CScript& scriptPubKey)
24            scriptPubKey.size() /* scriptPubKey */;
25 }
26 
27+//! Warning: be very careful when changing this! assumeutxo and UTXO snapshot
28+//! validation commitments are reliant on the hash constructed by this
29+//! function.

ariard commented at 4:25 pm on February 4, 2021:

f3bfa25

Should you describe more the concerns with any bug slip in in ApplyStats ? A bug breaking validation of already committed assume-valid chains and thus leading to their reject is okay. What is really concerning would be to validate a mischievous utxo set under a committed assume-valid chain, it might lead to double-spend against the assumeutxo user.

in src/validation.h:1038 in f3bfa25a24 outdated

978@@ -978,4 +979,14 @@ inline bool IsBlockPruned(const CBlockIndex* pblockindex)
979     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
980 }
981 
982+/**
983+ * Return the expected assumeutxo value for a given height, if one exists.
984+ *
985+ * @param height[in] Get the assumeutxo value for this height.

ariard commented at 4:46 pm on February 4, 2021:

f3bfa25

Is it possible to obtain the same UTXO set hash at the same height but for two different chains ? Your UTXO set at block 100 might be committed with header X or header X’. Assuming a reorg deep enough switching from X-chain to X’-chain, your UTXO snapshot for X would become invalid due to its base_blockhash.

I think we’ll loose this property if we reference by block hash here.

in src/chain.h:176 in 03e0de1409 outdated

171     //! (memory only) Number of transactions in the chain up to and including this block.
172     //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
173     //! Change to 64-bit type when necessary; won't happen before 2030
174+    //!
175+    //! Note: this value is faked during use of a UTXO snapshot because we don't
176+    //! have the underlying block data available during snapshot load.

ariard commented at 4:49 pm on February 4, 2021:

03e0de1

If those values are going to be sanitized during background-IBD maybe you should document it.

in src/validation.cpp:5351 in 03e0de1409 outdated

5359+                LogPrintf("[snapshot] flushing coins cache (%.2f MB)... ", /* Continued */
5360+                    coins_cache.DynamicMemoryUsage() / (1000 * 1000));
5361+                flush_now = GetTimeMillis();
5362+
5363+                // This is a hack - we don't know what the actual best block is, but that
5364+                // doesn't matter for the purposes of flushing the cache here. We'll set this

ariard commented at 4:52 pm on February 4, 2021:

03e0de1

Maybe you can document the problem that’s the hack is circumventing w.r.t to coin cache requirements.

in src/validation.cpp:5479 in 03e0de1409 outdated

5486+        }
5487+    }
5488+
5489+    assert(index);
5490+    index->nChainTx = metadata.m_nchaintx;
5491+    snapshot_chainstate.setBlockIndexCandidates.insert(snapshot_start_block);

ariard commented at 5:00 pm on February 4, 2021:

03e0de1

“Mark snapshot starting block as eligible for the most-work chain”.

in src/validation.h:913 in 03e0de1409 outdated

879+    //!
880+    //! - Initialize an unused CChainState.
881+    //! - Load its `CoinsViews` contents from `coins_file`.
882+    //! - Verify that the hash of the resulting coinsdb matches the expected hash
883+    //!   per assumeutxo chain parameters.
884+    //! - Wait for our headers chain to include the base block of the snapshot.

ariard commented at 5:01 pm on February 4, 2021:

03e0de1

You should swap those two steps to be in-order ? Also mentions the GetUTXOStats one.

ariard commented at 5:05 pm on February 4, 2021: member

Just few lightweight comments, not really blockers. I think the new changes are correct but have not done yet an in-depth review.

in src/validation.cpp:2775 in 2369045c69 outdated

2780@@ -2782,14 +2781,12 @@ static bool NotifyHeaderTip(CChainState& chainstate) LOCKS_EXCLUDED(cs_main) {
2781 
2782         if (pindexHeader != pindexHeaderOld) {
2783             fNotify = true;
2784-            assert(std::addressof(::ChainstateActive()) == std::addressof(chainstate));
2785-            fInitialBlockDownload = chainstate.IsInitialBlockDownload();

ryanofsky commented at 12:08 pm on February 5, 2021:

In commit “validation: make NotifyHeaderTip not rely on CChainState object” (2369045c6909bc242b996976d635cd7e6f9c7f62)

I know this commit came from another reviewer’s suggestion, so feel free to ignore this comment, but this commit doesn’t seem great to me. It’s not simplifying code or semantics, and it seems like it might send a less accurate notification if it is unlocking and relocking cs_main between getting pindexHeader and is_in_ibd values instead of getting both values under the same lock.

jnewbery commented at 2:31 pm on February 5, 2021:

I tend to agree. What’s the benefit of this commit?

I haven’t looked in great detail, but I imagine we probably want pindexBestHeader to move from being a global to being a member of CChainState, guarded by cs_main.

jamesob commented at 10:49 pm on February 5, 2021:

As far as I can tell, this commit is necessary if we both (i) want chainstate usage to be covered by cs_main and (ii) want to preserve the lock exclusion annotation on NotifyHeaderTip; there is no way to satisfy both while passing in a chainstate as a parameter.

That said, I have no problem peeling off the last two commits and deferring locking considerations for another PR.

ryanofsky commented at 11:12 pm on February 5, 2021:

re: #19806 (review)

As far as I can tell, this commit is necessary if we both (i) want chainstate usage to be covered by cs_main and (ii) want to preserve the lock exclusion annotation on NotifyHeaderTip; there is no way to satisfy both while passing in a chainstate as a parameter.

That said, I have no problem peeling off the last two commits and deferring locking considerations for another PR.

Unless there’s a specific reason the commits are needed here, I’d peel them off. The original reason to involve locking in this PR according to #19806 (review) was to guard m_active_chainstate, because at the time it was unguarded, and this PR introduced code changing the m_active_chainstate value. But now that m_active_chainstate is guarded on master (https://github.com/bitcoin/bitcoin/pull/19806#pullrequestreview-584279133), I think these commits only complicate things here and would make more sense as a separate PR.

On https://github.com/bitcoin/bitcoin/commit/2369045c6909bc242b996976d635cd7e6f9c7f62, I don’t think there’s a problem continuing to use chainstate inside NotifyHeaderTip regardless of the LOCKS_EXCLUDED annotation, because NotifyHeaderTip is still locking cs_main internally a few lines up above this on line 2779

ryanofsky approved

ryanofsky commented at 12:37 pm on February 5, 2021: member

Code review ACK 74c8167b58db74597b624073fca0aaaab0066aea modulo new lock annotation error in the fuzz build https://cirrus-ci.com/task/4623722814373888?command=ci#L2378. Since last review there was a rebase with some trivial conflicts, and two new commits were added.

I think you could consider dropping the last two commits and leaving them for a followup, because after f92dc6557a153b390a1ae1d0808ff7ed5d02c66e from #20749 and 20677ffa22e93e7408daadbd15d433f1e42faa86 from #21025, these are locking cleanups basically orthogonal to this PR.

On the contents of the two new commits, I don’t really think the new is_ibd commit 2369045c6909bc242b996976d635cd7e6f9c7f62 is great and I left a comment about it. The lock annotation commit 74c8167b58db74597b624073fca0aaaab0066aea replacing cs_main locks with lock annotations is very welcome, though. It also could be extended to go further and remove other new cs_main locks added in f92dc6557a153b390a1ae1d0808ff7ed5d02c66e from #20749 and 20677ffa22e93e7408daadbd15d433f1e42faa86 from #21025, replacing them with lock annotations.

jamesob force-pushed on Feb 12, 2021

jamesob commented at 4:16 am on February 12, 2021: member

I’ve pushed a rebase that

peels off the latest two locking annotation commits. I’ll likely revive those in some form in a follow-up PR,
changes ExpectedAssumeutxo()’s return value per @ryanofsky’s advice,
removes confusing ChainstateManager comments about locking per @jnewbery’s advice,
adds commentary around the severity of changing the assumeutxo hash per @ariard’s advice.

DrahtBot added the label Needs rebase on Feb 12, 2021

chainparams: add allowed assumeutxo values

Values for mainnet and testnet will be specified in a follow-up PR that can be
scrutinized accordingly. This structure is required for use in snapshot activation
logic.

7a6c46b37e

simplify ChainstateManager::SnapshotBlockhash() return semantics

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

f6e2da5fb7

txdb: don't reset during in-memory cache resize

We can't support a reset of the dbwrapper object when in-memory configuration is used
because it results in the permanent loss of coins. This only affects unittest
configurations (since that's the only place we use in-memory CCoinsViewDB instances).

ad949ba449

move-onlyish: break out CreateUTXOSnapshot from dumptxoutset

This move/refactor is needed to set up a decent unittest for UTXO snapshot activation.

6606a4f8c6

tests: add deterministic chain generation unittest fixture 31d225274f

tests: add snapshot activation test 4d8de04f32

test: Add tests with maleated snapshot data 769a1ef9fd

doc: remove potentially confusing ChainstateManager comment 1afc0e4aa1

jamesob force-pushed on Feb 12, 2021

DrahtBot removed the label Needs rebase on Feb 12, 2021

fjahr commented at 4:47 pm on February 14, 2021: member

Code review ACK 1afc0e4aa1b910991d4f8a77d74e2197f370987c

Reviewed changes since last review and confirmed they were due to rebasing or addressing review comments as discussed above.

laanwj commented at 6:22 pm on February 16, 2021: member

Code review ACK 1afc0e4aa1b910991d4f8a77d74e2197f370987c

laanwj merged this on Feb 16, 2021

laanwj closed this on Feb 16, 2021

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

sidhujag referenced this in commit 4fa2f63d06 on Feb 16, 2021

in src/node/coinstats.cpp:69 in 1afc0e4aa1

64+//! failure, but it will force clients that were expecting to make use of
65+//! assumeutxo to do traditional IBD instead.
66+//!
67+//! It is also possible, though very unlikely, that a change in this
68+//! construction could cause a previously invalid (and potentially malicious)
69+//! UTXO snapshot to be considered valid.

MarcoFalke commented at 8:50 am on April 3, 2021:

7a6c46b37edb8bfa0085d202aa7e9427d5e4fceb: I don’t understand this section. Does this assume that the way outputs are applied to the hash_obj is broken? In that case it doesn’t require a “previously” invalid snapshot. Likely, any invalid snapshot can be generated/modified, so that it is considered valid.

If it assumes that the underlying hash function is broken, there is nothing we can do anyway and any invalid snapshot can be generated/modified to be valid, regardless of changing this construction.

in src/coins.h:23 in f6e2da5fb7 outdated

19@@ -20,6 +20,8 @@
20 #include <functional>
21 #include <unordered_map>
22 
23+class ChainstateManager;

MarcoFalke commented at 8:55 am on April 3, 2021:

f6e2da5fb7c6406c37612c838c998078ea8d2252: why is this needed?

MarcoFalke commented at 9:44 am on April 4, 2021:

Removed in #21592

in src/test/validation_chainstatemanager_tests.cpp:227 in 4d8de04f32 outdated

222+    // Snapshot should refuse to load at this height.
223+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(m_node, m_path_root));
224+    BOOST_CHECK(chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());
225+    BOOST_CHECK_EQUAL(
226+        chainman.ActiveChainstate().m_from_snapshot_blockhash,
227+        chainman.SnapshotBlockhash().value_or(uint256()));

MarcoFalke commented at 10:26 am on April 3, 2021:

4d8de04f32736199e4b41a14a2d29b1a4d0a15d4: Wouldn’t it be better to check !chainman.SnapshotBlockhash()? Otherwise it can incorrectly return a default constructed uint256 without this test noticing.

MarcoFalke commented at 9:43 am on April 4, 2021:

Fixed in #21584

in src/test/validation_chainstatemanager_tests.cpp:224 in 4d8de04f32 outdated

219+        BOOST_CHECK_EQUAL(initial_size, initial_total_coins);
220+    }
221+
222+    // Snapshot should refuse to load at this height.
223+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(m_node, m_path_root));
224+    BOOST_CHECK(chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());

MarcoFalke commented at 10:27 am on April 3, 2021:

Wouldn’t it be better to have the same interface as for the SnapshotBlockhash member function? I.e. return nullopt when there is no hash instead of 0.

MarcoFalke commented at 9:42 am on April 4, 2021:

Fixed in #21584

in src/test/util/setup_common.h:136 in 31d225274f outdated

131 };
132 
133+
134+struct TestChain100DeterministicSetup : public TestChain100Setup {
135+    TestChain100DeterministicSetup() : TestChain100Setup(true) { }
136+};

MarcoFalke commented at 11:52 am on April 3, 2021:

Any need for this? Seems odd to have an option to make a test non-deterministic

MarcoFalke commented at 9:42 am on April 4, 2021:

#21592

MarcoFalke commented at 1:29 pm on April 3, 2021: member

Concept ACK. I left some style comment, questions, and found a bunch of crashes/UB.

MarcoFalke commented at 3:47 pm on April 4, 2021: member

(commit title and description of f6e2da5 doesn’t match what it does)

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

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

in src/validation.cpp:5478 in 1afc0e4aa1

5473+            index->nStatus |= BLOCK_OPT_WITNESS;
5474+        }
5475+    }
5476+
5477+    assert(index);
5478+    index->nChainTx = metadata.m_nchaintx;

MarcoFalke commented at 6:33 am on April 13, 2021:

metadata.m_nchaintx is untrusted input, so this lets an attacker pick the value

jamesob commented at 4:47 pm on April 14, 2021:

Oh, yeah, good catch. This is outdated, and should be ... = au_data.nChainTx instead. This is an artifact from before we moved nChainTx into the hardcoded assumeutxo parameters.

jamesob commented at 5:32 pm on April 14, 2021:

Fixed here: #21681

Thanks for finding this.

MarcoFalke referenced this in commit 128b98fce3 on May 5, 2021

Fabcien referenced this in commit c50dcf5b67 on Jan 4, 2022

deadalnix referenced this in commit cc45d5a0d9 on Mar 22, 2022

Fabcien referenced this in commit ddb302e35b on Apr 12, 2022

Fabcien referenced this in commit 329ebe9632 on Apr 12, 2022

Fabcien referenced this in commit a5dc7a8acf on Apr 12, 2022

Fabcien referenced this in commit 03af9518ea on Apr 13, 2022

Fabcien referenced this in commit 0e3eebdb36 on Apr 13, 2022

Fabcien referenced this in commit 83ee112814 on Apr 13, 2022

Fabcien referenced this in commit 64fc457623 on Apr 13, 2022

DrahtBot locked this on Aug 18, 2022

validation: UTXO snapshot activation #19806

Conflicts