Skip to content

Commit da8e397

Browse files
achow101achow101
committed
Merge bitcoin#28685: coinstats, assumeutxo: fix hash_serialized2 calculation
4bfaad4 chainparams, assumeutxo: Fix signet txoutset hash (Fabian Jahr) a503cd0 chainparams, assumeutxo: Fix testnet txoutset hash (Fabian Jahr) f621392 assumeutxo: Check deserialized coins for out of range values (Fabian Jahr) 6686544 docs: Add release notes for bitcoin#28685 (Fabian Jahr) cb03368 scripted-diff: Rename hash_serialized_2 to hash_serialized_3 (Fabian Jahr) 351370a coinstats: Fix hash_serialized2 calculation (Fabian Jahr) Pull request description: Closes bitcoin#28675 The last commit demonstrates that theStack's analysis [here](bitcoin#28675 (comment)) seems to be correct. There will be more changes needed for the rest of the test suite but the `feature_assumeutxo.py` with my additional tests pass. ACKs for top commit: achow101: ACK 4bfaad4 theStack: Code-review ACK 4bfaad4 ryanofsky: Code review ACK 4bfaad4 Tree-SHA512: 2f6abc92b282f7c5da46391803cf0804d13978d191d541f2509b532c538abccd0a081e46cda23d80d47206a05fa2b5d41b7ab246e6a263db7a7461d6292116ef
2 parents 5c32c59 + 4bfaad4 commit da8e397

15 files changed

+78
-74
lines changed

‎contrib/devtools/utxo_snapshot.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ ${BITCOIN_CLI_CALL} invalidateblock "${PIVOT_BLOCKHASH}"
3434

3535
if [[ "${OUTPUT_PATH}" = "-" ]]; then
3636
(>&2 echo "Generating txoutset info...")
37-
${BITCOIN_CLI_CALL} gettxoutsetinfo | grep hash_serialized_2 | sed 's/^.*: "\(.\+\)\+",/\1/g'
37+
${BITCOIN_CLI_CALL} gettxoutsetinfo | grep hash_serialized_3 | sed 's/^.*: "\(.\+\)\+",/\1/g'
3838
else
3939
(>&2 echo "Generating UTXO snapshot...")
4040
${BITCOIN_CLI_CALL} dumptxoutset "${OUTPUT_PATH}"

‎doc/release-notes-28685.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
RPC
2+
---
3+
4+
The `hash_serialized_2` value has been removed from `gettxoutsetinfo` since the value it calculated contained a bug and did not take all data into account. It is superseded by `hash_serialized_3` which provides the same functionality but serves the correctly calculated hash.

‎src/index/coinstatsindex.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@
1515
#include <undo.h>
1616
#include <validation.h>
1717

18+
using kernel::ApplyCoinHash;
1819
using kernel::CCoinsStats;
1920
using kernel::GetBogoSize;
20-
using kernel::TxOutSer;
21+
using kernel::RemoveCoinHash;
2122

2223
static constexpr uint8_t DB_BLOCK_HASH{'s'};
2324
static constexpr uint8_t DB_BLOCK_HEIGHT{'t'};
@@ -166,7 +167,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
166167
continue;
167168
}
168169

169-
m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
170+
ApplyCoinHash(m_muhash, outpoint, coin);
170171

171172
if (tx->IsCoinBase()) {
172173
m_total_coinbase_amount += coin.out.nValue;
@@ -187,7 +188,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
187188
Coin coin{tx_undo.vprevout[j]};
188189
COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};
189190

190-
m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));
191+
RemoveCoinHash(m_muhash, outpoint, coin);
191192

192193
m_total_prevout_spent_amount += coin.out.nValue;
193194

@@ -443,7 +444,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
443444
continue;
444445
}
445446

446-
m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));
447+
RemoveCoinHash(m_muhash, outpoint, coin);
447448

448449
if (tx->IsCoinBase()) {
449450
m_total_coinbase_amount -= coin.out.nValue;
@@ -464,7 +465,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
464465
Coin coin{tx_undo.vprevout[j]};
465466
COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n};
466467

467-
m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
468+
ApplyCoinHash(m_muhash, outpoint, coin);
468469

469470
m_total_prevout_spent_amount -= coin.out.nValue;
470471

‎src/kernel/chainparams.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ class CTestNetParams : public CChainParams {
269269
m_assumeutxo_data = {
270270
{
271271
.height = 2'500'000,
272-
.hash_serialized = AssumeutxoHash{uint256S("0x2a8fdefef3bf75fa00540ccaaaba4b5281bea94229327bdb0f7416ef1e7a645c")},
272+
.hash_serialized = AssumeutxoHash{uint256S("0xf841584909f68e47897952345234e37fcd9128cd818f41ee6c3ca68db8071be7")},
273273
.nChainTx = 66484552,
274274
.blockhash = uint256S("0x0000000000000093bcb68c03a9a168ae252572d348a2eaeba2cdf9231d73206f")
275275
}
@@ -378,7 +378,7 @@ class SigNetParams : public CChainParams {
378378
m_assumeutxo_data = {
379379
{
380380
.height = 160'000,
381-
.hash_serialized = AssumeutxoHash{uint256S("0x5225141cb62dee63ab3be95f9b03d60801f264010b1816d4bd00618b2736e7be")},
381+
.hash_serialized = AssumeutxoHash{uint256S("0xfe0a44309b74d6b5883d246cb419c6221bcccf0b308c9b59b7d70783dbdf928a")},
382382
.nChainTx = 2289496,
383383
.blockhash = uint256S("0x0000003ca3c99aff040f2563c2ad8f8ec88bd0fd6b8f0895cfaf1ef90353a62c")
384384
}
@@ -494,14 +494,14 @@ class CRegTestParams : public CChainParams
494494
m_assumeutxo_data = {
495495
{
496496
.height = 110,
497-
.hash_serialized = AssumeutxoHash{uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618")},
497+
.hash_serialized = AssumeutxoHash{uint256S("0x6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1")},
498498
.nChainTx = 111,
499499
.blockhash = uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c")
500500
},
501501
{
502502
// For use by test/functional/feature_assumeutxo.py
503503
.height = 299,
504-
.hash_serialized = AssumeutxoHash{uint256S("0xef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0")},
504+
.hash_serialized = AssumeutxoHash{uint256S("0x61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53")},
505505
.nChainTx = 300,
506506
.blockhash = uint256S("0x7e0517ef3ea6ecbed9117858e42eedc8eb39e8698a38dcbd1b3962a283233f4c")
507507
},

‎src/kernel/coinstats.cpp

Lines changed: 27 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,35 @@ uint64_t GetBogoSize(const CScript& script_pub_key)
4848
script_pub_key.size() /* scriptPubKey */;
4949
}
5050

51-
DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin)
51+
template <typename T>
52+
static void TxOutSer(T& ss, const COutPoint& outpoint, const Coin& coin)
5253
{
53-
DataStream ss{};
5454
ss << outpoint;
55-
ss << static_cast<uint32_t>(coin.nHeight * 2 + coin.fCoinBase);
55+
ss << static_cast<uint32_t>((coin.nHeight << 1) + coin.fCoinBase);
5656
ss << coin.out;
57-
return ss;
5857
}
5958

59+
static void ApplyCoinHash(HashWriter& ss, const COutPoint& outpoint, const Coin& coin)
60+
{
61+
TxOutSer(ss, outpoint, coin);
62+
}
63+
64+
void ApplyCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin)
65+
{
66+
DataStream ss{};
67+
TxOutSer(ss, outpoint, coin);
68+
muhash.Insert(MakeUCharSpan(ss));
69+
}
70+
71+
void RemoveCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin)
72+
{
73+
DataStream ss{};
74+
TxOutSer(ss, outpoint, coin);
75+
muhash.Remove(MakeUCharSpan(ss));
76+
}
77+
78+
static void ApplyCoinHash(std::nullptr_t, const COutPoint& outpoint, const Coin& coin) {}
79+
6080
//! Warning: be very careful when changing this! assumeutxo and UTXO snapshot
6181
//! validation commitments are reliant on the hash constructed by this
6282
//! function.
@@ -69,32 +89,13 @@ DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin)
6989
//! It is also possible, though very unlikely, that a change in this
7090
//! construction could cause a previously invalid (and potentially malicious)
7191
//! UTXO snapshot to be considered valid.
72-
static void ApplyHash(HashWriter& ss, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
73-
{
74-
for (auto it = outputs.begin(); it != outputs.end(); ++it) {
75-
if (it == outputs.begin()) {
76-
ss << hash;
77-
ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u);
78-
}
79-
80-
ss << VARINT(it->first + 1);
81-
ss << it->second.out.scriptPubKey;
82-
ss << VARINT_MODE(it->second.out.nValue, VarIntMode::NONNEGATIVE_SIGNED);
83-
84-
if (it == std::prev(outputs.end())) {
85-
ss << VARINT(0u);
86-
}
87-
}
88-
}
89-
90-
static void ApplyHash(std::nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs) {}
91-
92-
static void ApplyHash(MuHash3072& muhash, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
92+
template <typename T>
93+
static void ApplyHash(T& hash_obj, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
9394
{
9495
for (auto it = outputs.begin(); it != outputs.end(); ++it) {
9596
COutPoint outpoint = COutPoint(hash, it->first);
9697
Coin coin = it->second;
97-
muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
98+
ApplyCoinHash(hash_obj, outpoint, coin);
9899
}
99100
}
100101

@@ -118,8 +119,6 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c
118119
std::unique_ptr<CCoinsViewCursor> pcursor(view->Cursor());
119120
assert(pcursor);
120121

121-
PrepareHash(hash_obj, stats);
122-
123122
uint256 prevkey;
124123
std::map<uint32_t, Coin> outputs;
125124
while (pcursor->Valid()) {
@@ -180,15 +179,6 @@ std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV
180179
return stats;
181180
}
182181

183-
// The legacy hash serializes the hashBlock
184-
static void PrepareHash(HashWriter& ss, const CCoinsStats& stats)
185-
{
186-
ss << stats.hashBlock;
187-
}
188-
// MuHash does not need the prepare step
189-
static void PrepareHash(MuHash3072& muhash, CCoinsStats& stats) {}
190-
static void PrepareHash(std::nullptr_t, CCoinsStats& stats) {}
191-
192182
static void FinalizeHash(HashWriter& ss, CCoinsStats& stats)
193183
{
194184
stats.hashSerialized = ss.GetHash();

‎src/kernel/coinstats.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#define BITCOIN_KERNEL_COINSTATS_H
77

88
#include <consensus/amount.h>
9+
#include <crypto/muhash.h>
910
#include <streams.h>
1011
#include <uint256.h>
1112

@@ -72,7 +73,8 @@ struct CCoinsStats {
7273

7374
uint64_t GetBogoSize(const CScript& script_pub_key);
7475

75-
DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin);
76+
void ApplyCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin);
77+
void RemoveCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin);
7678

7779
std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point = {});
7880
} // namespace kernel

‎src/rpc/blockchain.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -820,7 +820,7 @@ static RPCHelpMan pruneblockchain()
820820

821821
CoinStatsHashType ParseHashType(const std::string& hash_type_input)
822822
{
823-
if (hash_type_input == "hash_serialized_2") {
823+
if (hash_type_input == "hash_serialized_3") {
824824
return CoinStatsHashType::HASH_SERIALIZED;
825825
} else if (hash_type_input == "muhash") {
826826
return CoinStatsHashType::MUHASH;
@@ -867,7 +867,7 @@ static RPCHelpMan gettxoutsetinfo()
867867
"\nReturns statistics about the unspent transaction output set.\n"
868868
"Note this call may take some time if you are not using coinstatsindex.\n",
869869
{
870-
{"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
870+
{"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_3"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_3' (the legacy algorithm), 'muhash', 'none'."},
871871
{"hash_or_height", RPCArg::Type::NUM, RPCArg::DefaultHint{"the current best block"}, "The block hash or height of the target height (only available with coinstatsindex).",
872872
RPCArgOptions{
873873
.skip_type_check = true,
@@ -882,7 +882,7 @@ static RPCHelpMan gettxoutsetinfo()
882882
{RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at which these statistics are calculated"},
883883
{RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs"},
884884
{RPCResult::Type::NUM, "bogosize", "Database-independent, meaningless metric indicating the UTXO set size"},
885-
{RPCResult::Type::STR_HEX, "hash_serialized_2", /*optional=*/true, "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"},
885+
{RPCResult::Type::STR_HEX, "hash_serialized_3", /*optional=*/true, "The serialized hash (only present if 'hash_serialized_3' hash_type is chosen)"},
886886
{RPCResult::Type::STR_HEX, "muhash", /*optional=*/true, "The serialized hash (only present if 'muhash' hash_type is chosen)"},
887887
{RPCResult::Type::NUM, "transactions", /*optional=*/true, "The number of transactions with unspent outputs (not available when coinstatsindex is used)"},
888888
{RPCResult::Type::NUM, "disk_size", /*optional=*/true, "The estimated size of the chainstate on disk (not available when coinstatsindex is used)"},
@@ -942,7 +942,7 @@ static RPCHelpMan gettxoutsetinfo()
942942
}
943943

944944
if (hash_type == CoinStatsHashType::HASH_SERIALIZED) {
945-
throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_2 hash type cannot be queried for a specific block");
945+
throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_3 hash type cannot be queried for a specific block");
946946
}
947947

948948
if (!index_requested) {
@@ -971,7 +971,7 @@ static RPCHelpMan gettxoutsetinfo()
971971
ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs);
972972
ret.pushKV("bogosize", (int64_t)stats.nBogoSize);
973973
if (hash_type == CoinStatsHashType::HASH_SERIALIZED) {
974-
ret.pushKV("hash_serialized_2", stats.hashSerialized.GetHex());
974+
ret.pushKV("hash_serialized_3", stats.hashSerialized.GetHex());
975975
}
976976
if (hash_type == CoinStatsHashType::MUHASH) {
977977
ret.pushKV("muhash", stats.hashSerialized.GetHex());

‎src/test/validation_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,11 +137,11 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo)
137137
}
138138

139139
const auto out110 = *params->AssumeutxoForHeight(110);
140-
BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618");
140+
BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1");
141141
BOOST_CHECK_EQUAL(out110.nChainTx, 111U);
142142

143143
const auto out110_2 = *params->AssumeutxoForBlockhash(uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c"));
144-
BOOST_CHECK_EQUAL(out110_2.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618");
144+
BOOST_CHECK_EQUAL(out110_2.hash_serialized.ToString(), "6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1");
145145
BOOST_CHECK_EQUAL(out110_2.nChainTx, 111U);
146146
}
147147

‎src/validation.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5399,6 +5399,11 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
53995399
coins_count - coins_left);
54005400
return false;
54015401
}
5402+
if (!MoneyRange(coin.out.nValue)) {
5403+
LogPrintf("[snapshot] bad snapshot data after deserializing %d coins - bad tx out value\n",
5404+
coins_count - coins_left);
5405+
return false;
5406+
}
54025407

54035408
coins_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin));
54045409

‎test/functional/feature_assumeutxo.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,16 +94,18 @@ def expected_error(log_msg="", rpc_details=""):
9494

9595
self.log.info(" - snapshot file with alternated UTXO data")
9696
cases = [
97-
[b"\xff" * 32, 0, "29926acf3ac81f908cf4f22515713ca541c08bb0f0ef1b2c3443a007134d69b8"], # wrong outpoint hash
98-
[(1).to_bytes(4, "little"), 32, "798266c2e1f9a98fe5ce61f5951cbf47130743f3764cf3cbc254be129142cf9d"], # wrong outpoint index
97+
[b"\xff" * 32, 0, "05030e506678f2eca8d624ffed97090ab3beadad1b51ee6e5985ba91c5720e37"], # wrong outpoint hash
98+
[(1).to_bytes(4, "little"), 32, "7d29cfe2c1e242bc6f103878bb70cfffa8b4dac20dbd001ff6ce24b7de2d2399"], # wrong outpoint index
99+
[b"\x81", 36, "f03939a195531f96d5dff983e294a1af62af86049fa7a19a7627246f237c03f1"], # wrong coin code VARINT((coinbase ? 1 : 0) | (height << 1))
100+
[b"\x83", 36, "e4577da84590fb288c0f7967e89575e1b0aa46624669640f6f5dfef028d39930"], # another wrong coin code
99101
]
100102

101103
for content, offset, wrong_hash in cases:
102104
with open(bad_snapshot_path, "wb") as f:
103105
f.write(valid_snapshot_contents[:(32 + 8 + offset)])
104106
f.write(content)
105107
f.write(valid_snapshot_contents[(32 + 8 + offset + len(content)):])
106-
expected_error(log_msg=f"[snapshot] bad snapshot content hash: expected ef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0, got {wrong_hash}")
108+
expected_error(log_msg=f"[snapshot] bad snapshot content hash: expected 61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53, got {wrong_hash}")
107109

108110
def run_test(self):
109111
"""
@@ -150,7 +152,7 @@ def run_test(self):
150152

151153
assert_equal(
152154
dump_output['txoutset_hash'],
153-
'ef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0')
155+
'61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53')
154156
assert_equal(dump_output['nchaintx'], 300)
155157
assert_equal(n0.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT)
156158

0 commit comments

Comments
 (0)