validation: UTXO snapshot activation #19806

jamesob · 2020-08-25T18:49:41Z

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.

DrahtBot · 2020-08-26T01:08:50Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

fjahr

Concept ACK

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

src/validation.cpp

fjahr · 2020-08-29T10:42:35Z

src/txdb.cpp

+    // progress= measure, but returning 0 would cause us to not be able to add
+    // chain tips for the snapshot chainstate. This shouldn't happen and is


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?

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

maflcko

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

src/validation.cpp

src/txdb.cpp

src/coins.h

src/validation.h

src/validation.cpp

fjahr · 2021-02-14T16:47:32Z

Code review ACK 1afc0e4

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

laanwj · 2021-02-16T18:22:00Z

Code review ACK 1afc0e4

maflcko

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

maflcko · 2021-01-06T10:38:24Z

src/chainparams.cpp

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


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

maflcko · 2021-04-03T08:50:41Z

src/node/coinstats.cpp

+//!
+//! It is also possible, though very unlikely, that a change in this
+//! construction could cause a previously invalid (and potentially malicious)
+//! UTXO snapshot to be considered valid.


7a6c46b: 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.

maflcko · 2021-04-03T08:55:23Z

src/coins.h

@@ -20,6 +20,8 @@
 #include <functional>
 #include <unordered_map>

+class ChainstateManager;


f6e2da5: why is this needed?

Removed in #21592

maflcko · 2021-04-03T10:26:48Z

src/test/validation_chainstatemanager_tests.cpp

+    BOOST_CHECK(chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());
+    BOOST_CHECK_EQUAL(
+        chainman.ActiveChainstate().m_from_snapshot_blockhash,
+        chainman.SnapshotBlockhash().value_or(uint256()));


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

Fixed in #21584

maflcko · 2021-04-03T10:27:51Z

src/test/validation_chainstatemanager_tests.cpp

+
+    // Snapshot should refuse to load at this height.
+    BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot(m_node, m_path_root));
+    BOOST_CHECK(chainman.ActiveChainstate().m_from_snapshot_blockhash.IsNull());


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.

Fixed in #21584

maflcko · 2021-04-03T11:52:22Z

src/test/util/setup_common.h

+
+struct TestChain100DeterministicSetup : public TestChain100Setup {
+    TestChain100DeterministicSetup() : TestChain100Setup(true) { }
+};


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

maflcko · 2021-04-03T11:58:42Z

src/test/util/setup_common.h

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

-private:


Could use GetDataDir() instead

maflcko · 2021-04-04T15:47:59Z

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

maflcko · 2021-04-13T06:33:32Z

src/validation.cpp

+    }
+
+    assert(index);
+    index->nChainTx = metadata.m_nchaintx;


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

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.

Fixed here: #21681

Thanks for finding this.

…hardcoded nChainTx 91d93aa validation: remove nchaintx from assumeutxo metadata (James O'Beirne) 931684b validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne) Pull request description: This fixes an oversight from the move of nChainTx from the user-supplied snapshot metadata into the hardcoded assumeutxo chainparams. Since the nChainTx is now unused in the metadata, it should be removed in a future commit. See: bitcoin/bitcoin#19806 (comment) ACKs for top commit: Sjors: utACK 91d93aa ryanofsky: Code review ACK 91d93aa. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests. Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c

Summary: This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [2/8] bitcoin/bitcoin@1add318 Note: due to out of order backport, I also had to remove `private:` in `setup_common.h` which is otherwise removed in [[bitcoin/bitcoin#19806 | core#19806]]. Depends on D10749 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10750

Summary: This move/refactor is needed to set up a decent unittest for UTXO snapshot activation. This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [1/8] bitcoin/bitcoin@6606a4f Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11224

Summary: 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). This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [2/8] bitcoin/bitcoin@ad949ba Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11226

Summary: This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [3/8] bitcoin/bitcoin@31d2252 Note that the hash of the block in `TestChain100Setup` is different because Bitcoin ABC blocks have differences with Bitcoin Core blocks. I added a test that does nothing but initialize a `TestChain100DeterministicSetup` to demonstrate that the hash in the assertion is correct (changing something in that string makes the test fail). Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11225

Summary: 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. This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [4/8] and [[bitcoin/bitcoin#21584 | core#21584]] [2 & 3/5] bitcoin/bitcoin@7a6c46b bitcoin/bitcoin@0000007 bitcoin/bitcoin@fa5668b (partial) Notes: - The new circular dependency was added by core in a later commit. We already need it here. - We also check the allowed hashes in a functional test reproducing the deterministic regtest chain and allowed hashes. Depends on D11328 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11238

Summary: bitcoin/bitcoin@f6e2da5 > Don't return null snapshotblockhash values to avoid caller complexity/confusion. ---- bitcoin/bitcoin@931684b > validation: fix ActivateSnapshot to use hardcoded nChainTx > > This fixes an oversight from the move of nChainTx from the user-supplied > snapshot metadata into the hardcoded assumeutxo chainparams. > > Since the nChainTx is now unused in the metadata, it should be removed > in a future commit. ---- bitcoin/bitcoin@91d93aa > validation: remove nchaintx from assumeutxo metadata > > This value is no longer used and is instead specified statically > in chainparams. This change means that previously generated > snapshots will no longer be usable. ---- bitcoin/bitcoin@fa9b74f > Fix assumeutxo crash due to missing base_blockhash ---- bitcoin/bitcoin@fae33f9 > Fix assumeutxo crash due to invalid base_blockhash > > Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space ---- bitcoin/bitcoin@fa73ce6 > Fix assumeutxo crash due to truncated file ---- This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [5/8], core#21681 (partial), core#21582 (partial), core#21584 (partial) and core#21585 Notes: - The test related changes from core#21681, core#21582 and core#21584 are in D11241. This is because they depend on a missing (for now) test fixture for malleating the utxo snapshots. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11239

Summary: This also tests the hashes added in D11238. This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [6/8] bitcoin/bitcoin@4d8de04 Depends on D11239 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11240

Summary: bitcoin/bitcoin@769a1ef > test: Add tests with maleated snapshot data ---- https://github.com/bitcoin/bitcoin/pull/21681/files > validation: fix ActivateSnapshot to use hardcoded nChainTx > validation: remove nchaintx from assumeutxo metadata Only test related changes for these commits are in this revision ---- bitcoin/bitcoin@fa8fffe > refactor: Prefer clean assert over UB in coinstats ---- bitcoin/bitcoin@fa9b74f > Fix assumeutxo crash due to missing base_blockhash Only test related changes from this commit are in this revision. ---- bitcoin/bitcoin@fae33f9 > Fix assumeutxo crash due to invalid base_blockhash Only test related changes from this commit are in this revision. ---- This is a backport of [[bitcoin/bitcoin#19806 | core#19806]] [7/8] and test related changes from [[bitcoin/bitcoin#21681 | core#21681]], [[bitcoin/bitcoin#21582 | core#21582]] and [[bitcoin/bitcoin#21584 | core#21584]] Depends on D11240 Test Plan: `ninja check` With UBSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11241

Summary: This concludes backport of [[bitcoin/bitcoin#19806 | core#19806]] [6/6] bitcoin/bitcoin@1afc0e4 Test Plan: NA Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11243

jamesob force-pushed the 2020-08-au.activate branch 2 times, most recently from 19b7517 to 08a0861 Compare August 25, 2020 18:59

DrahtBot added RPC/REST/ZMQ UTXO Db and Indexes Validation labels Aug 25, 2020

This was referenced Aug 26, 2020

rpc: Add dumpcoinstats #19792

Closed

test: Activate segwit in TestChain100Setup #19775

Merged

jamesob force-pushed the 2020-08-au.activate branch from 08a0861 to 82ad09b Compare August 26, 2020 02:31

DrahtBot added the Needs rebase label Aug 26, 2020

This was referenced Aug 26, 2020

Add address-based index (attempt 4?) #14053

Closed

CCoinsViewCache code cleanup & deduplication #9384

Closed

jamesob force-pushed the 2020-08-au.activate branch 2 times, most recently from 86355a1 to 2cb1f84 Compare August 26, 2020 15:17

DrahtBot removed the Needs rebase label Aug 26, 2020

This was referenced Aug 27, 2020

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

Add fee_est tool for debugging fee estimation code #10443

Closed

fjahr reviewed Aug 29, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 31, 2020

Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) #19849

Merged

maflcko reviewed Aug 31, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 31, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

jamesob force-pushed the 2020-08-au.activate branch 2 times, most recently from 461a521 to e7b1615 Compare August 31, 2020 22:38

DrahtBot removed the Needs rebase label Feb 12, 2021

laanwj merged commit 92fee79 into bitcoin:master Feb 16, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 16, 2021

Merge bitcoin#19806: validation: UTXO snapshot activation

4fa2f63

Sjors mentioned this pull request Feb 17, 2021

assumeutxo #15606

Closed

18 tasks

maflcko reviewed Apr 3, 2021

View reviewed changes

This was referenced Apr 3, 2021

Fix assumeutxo crash due to missing base_blockhash #21582

Merged

Fix assumeutxo crash due to truncated file #21585

Merged

jnewbery mentioned this pull request Apr 6, 2021

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

2 tasks

jamesob mentioned this pull request Apr 6, 2021

ChainstateManager locking improvements #21620

Closed

maflcko mentioned this pull request Apr 7, 2021

Fix assumeutxo crash due to invalid base_blockhash #21584

Merged

maflcko reviewed Apr 13, 2021

View reviewed changes

jamesob mentioned this pull request Apr 14, 2021

validation: fix ActivateSnapshot to use hardcoded nChainTx #21681

Merged

glozow mentioned this pull request Apr 19, 2021

Remove RewindBlockIndex logic #21009

Merged

ryanofsky mentioned this pull request Sep 24, 2021

refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method #22937

Merged

kwvg mentioned this pull request Oct 9, 2021

merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) dashpay/dash#4498

Merged

kwvg mentioned this pull request Feb 25, 2022

merge bitcoin#16240...21526: assumeutxo project backports (part 2) dashpay/dash#4703

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validation: UTXO snapshot activation #19806

validation: UTXO snapshot activation #19806

jamesob commented Aug 25, 2020 •

edited

Loading

DrahtBot commented Aug 26, 2020 •

edited

Loading

fjahr left a comment

fjahr Aug 29, 2020

ryanofsky Sep 2, 2020

maflcko left a comment

fjahr commented Feb 14, 2021

laanwj commented Feb 16, 2021

maflcko left a comment

maflcko Jan 6, 2021

maflcko Apr 3, 2021

maflcko Apr 3, 2021

maflcko Apr 4, 2021

maflcko Apr 3, 2021

maflcko Apr 4, 2021

maflcko Apr 3, 2021

maflcko Apr 4, 2021

maflcko Apr 3, 2021

maflcko Apr 4, 2021

maflcko Apr 3, 2021

maflcko commented Apr 4, 2021

maflcko Apr 13, 2021

jamesob Apr 14, 2021

jamesob Apr 14, 2021

		// progress= measure, but returning 0 would cause us to not be able to add
		// chain tips for the snapshot chainstate. This shouldn't happen and is

validation: UTXO snapshot activation #19806

validation: UTXO snapshot activation #19806

Conversation

jamesob commented Aug 25, 2020 • edited Loading

DrahtBot commented Aug 26, 2020 • edited Loading

Conflicts

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

fjahr commented Feb 14, 2021

laanwj commented Feb 16, 2021

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesob commented Aug 25, 2020 •

edited

Loading

DrahtBot commented Aug 26, 2020 •

edited

Loading