This is part of the assumeutxo project:
Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
This changeset defines the serialization format for UTXO snapshots and adds an RPC command for creating them, dumptxoutset. It also adds a convenience script for generating and verifying snapshots at a certain height, since that requires doing a hacky rewind of the chain via invalidateblock.
All of this is unused at the moment.
23@@ -24,7 +24,10 @@ struct CCoinsStats
24 uint64_t nDiskSize;
25 CAmount nTotalAmount;
26
27- CCoinsStats() : nHeight(0), nTransactions(0), nTransactionOutputs(0), nBogoSize(0), nDiskSize(0), nTotalAmount(0) {}
28+ // The number of coins contained.
29+ uint64_t coins_count;
30+
31+ CCoinsStats() : nHeight(0), nTransactions(0), nTransactionOutputs(0), nBogoSize(0), nDiskSize(0), nTotalAmount(0), coins_count(0) {}
I’d prefer to use C++11 member initialization
0uint64_t coins_count{0};
Feel free to keep the others (or switch them over as well)
2272+ " \"coins_written\": n, (numeric) the number of coins written in the snapshot\n"
2273+ " \"base_hash\": \"...\", (string) the hash of the base of the snapshot\n"
2274+ " \"base_height\": n, (string) the hash of the base of the snapshot\n"
2275+ " \"path\": \"...\" (string) the absolute path that the snapshot was written to\n"
2276+ "]\n"},
2277+ }.ToString()
.Check() syntax on RPCHelpMan?
2270+ RPCResults{},
2271+ RPCExamples{"{\n"
2272+ " \"coins_written\": n, (numeric) the number of coins written in the snapshot\n"
2273+ " \"base_hash\": \"...\", (string) the hash of the base of the snapshot\n"
2274+ " \"base_height\": n, (string) the hash of the base of the snapshot\n"
2275+ " \"path\": \"...\" (string) the absolute path that the snapshot was written to\n"
744@@ -744,6 +745,62 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
745 /** Remove invalidity status from a block and its descendants. */
746 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
747
748+/**
749+ * Serialized version of a UTXO set from which an assumed-valid CChainState
750+ * can be constructed.
751+ */
752+class SnapshotMetadata
src/node/utxo_snapshot.{h,cpp} and add this there if that seems preferable.
752+class SnapshotMetadata
753+{
754+public:
755+ //! The block header of the block that reflects the tip of the chain for the
756+ //! UTXO set contained in this snapshot.
757+ CDiskBlockIndex m_base_blockheader;
m_base_blockheader.nHeight to index into the expected assumeutxo hash values, but beyond that I don’t think we use other parts of the header. I can split out the height if you like, but I don’t see much harm in taking on a few extra bytes for the complete header in case it’s somehow advantageous to have in the future.
m_base_blockheader.nHeight different from LookupBlockIndex(m_base_blockhash)->nHeight?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
What was the conclusion on the question whether the dump should include all headers?
We need to connect to peers to make the node usable anyway, and headers take very little time to sync, so I don’t see much benefit to bundling the headers with the snapshot. Though if anyone thinks differently I’d be curious to hear why.
744@@ -744,6 +745,62 @@ bool InvalidateBlock(CValidationState& state, const CChainParams& chainparams, C
745 /** Remove invalidity status from a block and its descendants. */
746 void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
747
748+/**
749+ * Serialized version of a UTXO set from which an assumed-valid CChainState
760+ uint256 m_utxo_contents_hash = {};
761+
762+ //! The number of coins in the UTXO set contained in this snapshot.
763+ uint64_t m_coins_count = 0;
764+
765+ //! Necessary to "fake" the base nChainTx so that we can estimate progress during
770+ //! to the base block of the snapshot (`m_base_blockhash`).
771+ //!
772+ //! This is serialized (vs. being memory only) because once we complete
773+ //! validation of a snapshot, we update the metadata on disk so that the next
774+ //! time bitcoind starts up, we know we've validated the snapshot. This would
775+ //! be unnecessary if we overwrote the `chainstate` leveldb directory with the
utxo.dat file, which is going to be pass to other nodes, what’s the meaning of validation completed for them ?
re: #16899 (review)
In commit “validation: add unused SnapshotMetadata class” (3511d6b2ea65219a18dbb6d6ca45a4de62569dc1)
Don’t get this field, it is serialized down to a
utxo.datfile, which is going to be pass to other nodes, what’s the meaning of validation completed for them ?
I guess SnapshotMetadata is being used for two different things:
I think it would probably be clearer to use separate structs, maybe having the first one included in the second one to avoid duplication.
/datadir/chainstate_* directory to indicate an unvalidated assumeutxo chainstate, then overwrite /datadir/chainstate on validation.
2267+ },
2268+ RPCResult{
2269+ "{\n"
2270+ " \"coins_written\": n, (numeric) the number of coins written in the snapshot\n"
2271+ " \"base_hash\": \"...\", (string) the hash of the base of the snapshot\n"
2272+ " \"base_height\": n, (string) the hash of the base of the snapshot\n"
31+ assert_equal(out['path'], str(expected_path))
32+ assert out['base_hash']
33+
34+ # TODO I'd like to make assertions about the contents of the file
35+ # (ideally its hash), but because we can't get a deterministic block
36+ # header (we'd need to mock time) and we have no way of deserializing
Why can’t you use mock time here?
Also, the test isn’t running (see travis)
GetTime<...>() here: https://github.com/bitcoin/bitcoin/blob/master/src/miner.cpp#L93
nTimeStart is only used for logging benchmarks
757+ uint256 m_base_blockhash;
758+
759+ //! A hash of the snapshot contents as generated by `GetUTXOStats`.
760+ uint256 m_utxo_contents_hash = {};
761+
762+ //! The number of coins in the UTXO set contained in this snapshot.
In commit “validation: add unused SnapshotMetadata class” (3511d6b2ea65219a18dbb6d6ca45a4de62569dc1)
Comments for m_utxo_contents_hash m_coins_count fields could say what they are used for.
2254+ */
2255+UniValue dumptxoutset(const JSONRPCRequest& request)
2256+{
2257+ RPCHelpMan{
2258+ "dumptxoutset",
2259+ "\nWrite the serialized UTXO set to disk.\n"
In commit “rpc: add dumptxoutset” (b6af8f5443c9891ade4fa5ca92a244e3ff8cac79)
Up to you, but I think documentation could say more about
2277+ HelpExampleCli("dumptxoutset", "utxo.dat")
2278+ }
2279+ }.Check(request);
2280+
2281+ fs::path path = request.params[0].get_str();
2282+ if (path.is_relative()) {
In commit “rpc: add dumptxoutset” (b6af8f5443c9891ade4fa5ca92a244e3ff8cac79)
Think you could drop this conditional and just say path = fs::absolute(path, GetDataDir());
2337+ }
2338+
2339+ pcursor->Next();
2340+ }
2341+
2342+ afile.fclose();
In commit “rpc: add dumptxoutset” (b6af8f5443c9891ade4fa5ca92a244e3ff8cac79)
Implementation seems fine for now. Unclear if in the future we might want to:
2376@@ -2275,6 +2377,7 @@ static const CRPCCommand commands[] =
2377 { "blockchain", "preciousblock", &preciousblock, {"blockhash"} },
2378 { "blockchain", "scantxoutset", &scantxoutset, {"action", "scanobjects"} },
2379 { "blockchain", "getblockfilter", &getblockfilter, {"blockhash", "filtertype"} },
2380+ { "blockchain", "dumptxoutset", &dumptxoutset, {"path"} },
In commit “rpc: add dumptxoutset” (b6af8f5443c9891ade4fa5ca92a244e3ff8cac79)
Might want to move this to the hidden section, unless it’s useful for something other than debugging. Also, if the format or usage might change, it might be better to not expose so compatibility is less a concern.
A progress indicator would be nice. If it wasn’t for the system activity monitor showing CPU and disk write activity I would have interrupted the script somewhere during the ~ half an hour dumptxoutset takes.
Maybe the script should call setnetworkactive false. E.g. I’m seeing Warning: not punishing manually-connected peer, as well as a P2P timeout error.
Checksum on macOS 10.14.6 for block 597000: 3fd987a705a9b70f3197ebbe782ff457b1ea6ca373ac37b5c1a3764f7c414fe4 (produced twice, but on the same datadir)
0{
1 "coins_written": 62228899,
2 "base_hash": "0000000000000000000eadb01d27f7b6ca1c17e71f0ff33853de9d3b25c57e55",
3}
50@@ -51,6 +51,7 @@ bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats)
51 }
52 ss << stats.hashBlock;
53 uint256 prevkey;
54+ unsigned int coins_count{0};
I don’t really like that coins_count is initialized to zero at the beginning of the function, but the other members of stats aren’t. I’d prefer if all stats are initialized in one go:
0stats = CCoinsStats{}; // reset stats
this currently fails the new rpc_dumptxoutset.py test, both in CI and locally
02019-10-24T07:46:53.097000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20191024_094030/rpc_dumptxoutset_22
12019-10-24T07:46:53.489000Z TestFramework (ERROR): Assertion failed
2Traceback (most recent call last):
3 File "/store/orion/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 194, in main
4 self.run_test()
5 File "/home/orion/projects/bitcoin/bitcoin/test/functional/rpc_dumptxoutset.py", line 39, in run_test
6 '10c25df662424c94d06458aa5ecc7bd73680aa098f5093fdd5fa27fb2af08568')
7 File "/store/orion/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 46, in assert_equal
8 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
9AssertionError: not(4fac28ba32694cf370432ca408620e053a370f11de8b19488568ed1308d9b511 == 10c25df662424c94d06458aa5ecc7bd73680aa098f5093fdd5fa27fb2af08568)
102019-10-24T07:46:53.565000Z TestFramework (INFO): Stopping nodes
112019-10-24T07:46:53.817000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20191024_094030/rpc_dumptxoutset_22
122019-10-24T07:46:53.818000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20191024_094030/rpc_dumptxoutset_22/test_framework.log
132019-10-24T07:46:53.819000Z TestFramework (ERROR): Hint: Call /store/orion/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20191024_094030/rpc_dumptxoutset_22' to cons
14olidate all logs
19+ self.num_nodes = 1
20+
21+ def run_test(self):
22+ """Test a trivial usage of the dumptxoutset RPC command."""
23+ node = self.nodes[0]
24+ node.setmocktime(int(time.mktime(datetime.datetime(2019, 9, 9, 0, 0).timetuple())))
time.mktime uses local timezone). As this is a constant, and the rest of the test depends on this exact value, might was well hardcode a value here.
You can maybe use
0getblockheader(getblockhash(0))[time] + 1
6+"""
7+from test_framework.test_framework import BitcoinTestFramework
8+from test_framework.util import assert_equal, assert_raises_rpc_error
9+
10+import hashlib
11+import time
35+ # Blockhash should be deterministic based on mocked time.
36+ assert_equal(
37+ out['base_hash'],
38+ '6fd417acba2a8738b06fee43330c50d58e6a725046c3d843c8dd7e51d46d1ed6')
39+
40+ with open(expected_path, 'rb') as f:
https://travis-ci.org/bitcoin/bitcoin/jobs/602444006#L6364
0 with open(expected_path, 'rb') as f:
1 TypeError: invalid file: PosixPath('/home/travis/build/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_\u20bf_🏃_20191024_181754/rpc_dumptxoutset_22/node0/regtest/txoutset.dat')
2327+
2328+ COutPoint key;
2329+ Coin coin;
2330+
2331+ while (pcursor->Valid()) {
2332+ boost::this_thread::interruption_point();
IsRPCRunning() instead.
FindScriptPubKey at some point too. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L1970-L1978
au.dumptxoutset.13 -> au.dumptxoutset.14
Removed used of boost::interruption_point on @laanwj’s advice.
Also changes existing CCoinsStats attributes to be C++11 initialized.
Allows the creation of a UTXO snapshot to disk.
to allow easy (if not time-consuming) generation and verification of
snapshots.
We should remove all uses of boost::interruption_point in RPC code, but not in this PR.
ACK 92b2f5306ba0b3f031293cb8f415b67cb002c2f1
14+ echo " if <snapshot-out-path> is '-', don't produce a snapshot file but instead print the "
15+ echo " expected assumeutxo hash"
16+ echo
17+ echo 'Examples:'
18+ echo
19+ echo " ./contrib/devtools/utxo_snapshot.sh 570000 utxo.dat ./src/bitcoin-cli -datadir=\$(pwd)/testdata"
28+
29+# Block we'll invalidate/reconsider to rewind/fast-forward the chain.
30+PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) )
31+
32+(>&2 echo "Rewinding chain back to height ${GENERATE_AT_HEIGHT} (by invalidating ${PIVOT_BLOCKHASH}); this may take a while")
33+${BITCOIN_CLI_CALL} invalidateblock "${PIVOT_BLOCKHASH}"
setnetworkactive false first
Labels
Feature
RPC/REST/ZMQ
Validation
Scripts and tools
Milestone
0.20.0