The fix isn’t enough. The file may be completely untrusted and corrupt, so just because the block height is a signed 32-bit, but positive integer, doesn’t mean that the height is correct.
You’ll have to check that the block height corresponds to the block hash, otherwise I fail to see how it can be used without adding bugs or risks or confusion. However, if you do the check, you may as well discard the deserialized value and just use the value from the check.
So again, my recommendation would be to remove it, because either:
- The value is untrusted, thus can not be used
- The value is checked, in which case it is redundant with the value from the check
An alternative would be to write a full file-content hash in the metadata as the second field (after the file magic), covering the whole file after the second field. This way, the file-content hash would be retrieved from the trusted chainparams, then calculated and compared. Afterwards the code can assume that all fields in the checked file are trusted to some extent, which would allow using the block height value.
However, that is a more involved fix, so my recommendation for now would be to drop the value.
Suggested diff:
0diff --git a/src/node/utxo_snapshot.h b/src/node/utxo_snapshot.h
1index a7c4135787..bd5b68ba30 100644
2--- a/src/node/utxo_snapshot.h
3+++ b/src/node/utxo_snapshot.h
4@@ -28,16 +28,17 @@ class Chainstate;
5 namespace node {
6 //! Metadata describing a serialized version of a UTXO set from which an
7 //! assumeutxo Chainstate can be constructed.
8+//! All metadata fields come from an untrusted file, so must be validated
9+//! before being used. Thus, new fields should be added only if needed.
10 class SnapshotMetadata
11 {
12- const uint16_t m_version{1};
13- const std::set<uint16_t> m_supported_versions{1};
14+ static const uint16_t VERSION{2};
15+ const std::set<uint16_t> m_supported_versions{VERSION};
16 const MessageStartChars m_network_magic;
17 public:
18 //! The hash of the block that reflects the tip of the chain for the
19 //! UTXO set contained in this snapshot.
20 uint256 m_base_blockhash;
21- uint32_t m_base_blockheight;
22
23
24 //! The number of coins in the UTXO set contained in this snapshot. Used
25@@ -54,15 +55,13 @@ public:
26 uint64_t coins_count) :
27 m_network_magic(network_magic),
28 m_base_blockhash(base_blockhash),
29- m_base_blockheight(base_blockheight),
30 m_coins_count(coins_count) { }
31
32 template <typename Stream>
33 inline void Serialize(Stream& s) const {
34 s << SNAPSHOT_MAGIC_BYTES;
35- s << m_version;
36+ s << VERSION;
37 s << m_network_magic;
38- s << m_base_blockheight;
39 s << m_base_blockhash;
40 s << m_coins_count;
41 }
42@@ -98,7 +97,6 @@ public:
43 }
44 }
45
46- s >> m_base_blockheight;
47 s >> m_base_blockhash;
48 s >> m_coins_count;
49 }
50diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
51index 9899a13a1e..ee72fb2c20 100644
52--- a/src/rpc/blockchain.cpp
53+++ b/src/rpc/blockchain.cpp
54@@ -2862,10 +2862,12 @@ static RPCHelpMan loadtxoutset()
55 throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot: %s. (%s)", util::ErrorString(activation_result).original, path.utf8string()));
56 }
57
58+ CBlockIndex& snap{*CHECK_NONFATAL(*activation_result)};
59+
60 UniValue result(UniValue::VOBJ);
61 result.pushKV("coins_loaded", metadata.m_coins_count);
62- result.pushKV("tip_hash", metadata.m_base_blockhash.ToString());
63- result.pushKV("base_height", metadata.m_base_blockheight);
64+ result.pushKV("tip_hash", snap.GetBlockHash().ToString());
65+ result.pushKV("base_height", snap.nHeight);
66 result.pushKV("path", fs::PathToString(path));
67 return result;
68 },
69diff --git a/src/validation.cpp b/src/validation.cpp
70index 2b8f64e81a..0b101d1551 100644
71--- a/src/validation.cpp
72+++ b/src/validation.cpp
73@@ -5633,31 +5633,31 @@ Chainstate& ChainstateManager::InitializeChainstate(CTxMemPool* mempool)
74 return destroyed && !fs::exists(db_path);
75 }
76
77-util::Result<void> ChainstateManager::ActivateSnapshot(
78+util::Result<CBlockIndex*> ChainstateManager::ActivateSnapshot(
79 AutoFile& coins_file,
80 const SnapshotMetadata& metadata,
81 bool in_memory)
82 {
83 uint256 base_blockhash = metadata.m_base_blockhash;
84- int base_blockheight = metadata.m_base_blockheight;
85
86 if (this->SnapshotBlockhash()) {
87 return util::Error{Untranslated("Can't activate a snapshot-based chainstate more than once")};
88 }
89
90+ CBlockIndex* snapshot_start_block{};
91+
92 {
93 LOCK(::cs_main);
94
95 if (!GetParams().AssumeutxoForBlockhash(base_blockhash).has_value()) {
96 auto available_heights = GetParams().GetAvailableSnapshotHeights();
97 std::string heights_formatted = util::Join(available_heights, ", ", [&](const auto& i) { return util::ToString(i); });
98- return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s, height: %s). The following snapshot heights are available: %s"),
99+ return util::Error{strprintf(Untranslated("assumeutxo block hash in snapshot metadata not recognized (hash: %s). The following snapshot heights are available: %s"),
100 base_blockhash.ToString(),
101- base_blockheight,
102 heights_formatted)};
103 }
104
105- CBlockIndex* snapshot_start_block = m_blockman.LookupBlockIndex(base_blockhash);
106+ snapshot_start_block = m_blockman.LookupBlockIndex(base_blockhash);
107 if (!snapshot_start_block) {
108 return util::Error{strprintf(Untranslated("The base block header (%s) must appear in the headers chain. Make sure all headers are syncing, and call loadtxoutset again"),
109 base_blockhash.ToString())};
110@@ -5668,7 +5668,7 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
111 return util::Error{strprintf(Untranslated("The base block header (%s) is part of an invalid chain"), base_blockhash.ToString())};
112 }
113
114- if (!m_best_header || m_best_header->GetAncestor(base_blockheight) != snapshot_start_block) {
115+ if (!m_best_header || m_best_header->GetAncestor(snapshot_start_block->nHeight) != snapshot_start_block) {
116 return util::Error{_("A forked headers-chain with more work than the chain with the snapshot base block header exists. Please proceed to sync without AssumeUtxo.")};
117 }
118
119@@ -5782,7 +5782,7 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
120 m_snapshot_chainstate->CoinsTip().DynamicMemoryUsage() / (1000 * 1000));
121
122 this->MaybeRebalanceCaches();
123- return {};
124+ return snapshot_start_block;
125 }
126
127 static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_loaded)
128diff --git a/src/validation.h b/src/validation.h
129index 08e672c620..0638bbe983 100644
130--- a/src/validation.h
131+++ b/src/validation.h
132@@ -1098,7 +1098,7 @@ public:
133 //! faking nTx* block index data along the way.
134 //! - Move the new chainstate to `m_snapshot_chainstate` and make it our
135 //! ChainstateActive().
136- [[nodiscard]] util::Result<void> ActivateSnapshot(
137+ [[nodiscard]] util::Result<CBlockIndex*> ActivateSnapshot(
138 AutoFile& coins_file, const node::SnapshotMetadata& metadata, bool in_memory);
139
140 //! Once the background validation chainstate has reached the height which
