From 351370a1d211615e3d5b158ccb0400cd79c5c085 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Wed, 18 Oct 2023 17:06:36 +0200 Subject: [PATCH] coinstats: Fix hash_serialized2 calculation The legacy serialization was vulnerable to maleation and is fixed by adopting the same serialization procedure as was already in use for MuHash. This also includes necessary test fixes where the hash_serialized2 was hardcoded as well as correction of the regtest chainparams. Co-authored-by: Sebastian Falbesoner --- src/index/coinstatsindex.cpp | 11 ++-- src/kernel/chainparams.cpp | 4 +- src/kernel/coinstats.cpp | 66 ++++++++++-------------- src/kernel/coinstats.h | 4 +- src/test/validation_tests.cpp | 4 +- test/functional/feature_assumeutxo.py | 10 ++-- test/functional/feature_utxo_set_hash.py | 2 +- test/functional/rpc_dumptxoutset.py | 2 +- 8 files changed, 49 insertions(+), 54 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 9dab8ca901b..ecd3fd21b5c 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -15,9 +15,10 @@ #include #include +using kernel::ApplyCoinHash; using kernel::CCoinsStats; using kernel::GetBogoSize; -using kernel::TxOutSer; +using kernel::RemoveCoinHash; static constexpr uint8_t DB_BLOCK_HASH{'s'}; static constexpr uint8_t DB_BLOCK_HEIGHT{'t'}; @@ -166,7 +167,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) continue; } - m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin))); + ApplyCoinHash(m_muhash, outpoint, coin); if (tx->IsCoinBase()) { m_total_coinbase_amount += coin.out.nValue; @@ -187,7 +188,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) Coin coin{tx_undo.vprevout[j]}; COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; - m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin))); + RemoveCoinHash(m_muhash, outpoint, coin); m_total_prevout_spent_amount += coin.out.nValue; @@ -443,7 +444,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex continue; } - m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin))); + RemoveCoinHash(m_muhash, outpoint, coin); if (tx->IsCoinBase()) { m_total_coinbase_amount -= coin.out.nValue; @@ -464,7 +465,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex Coin coin{tx_undo.vprevout[j]}; COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; - m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin))); + ApplyCoinHash(m_muhash, outpoint, coin); m_total_prevout_spent_amount -= coin.out.nValue; diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 3ac8756e412..f928520d3ea 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -494,14 +494,14 @@ public: m_assumeutxo_data = { { .height = 110, - .hash_serialized = AssumeutxoHash{uint256S("0x1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618")}, + .hash_serialized = AssumeutxoHash{uint256S("0x6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1")}, .nChainTx = 111, .blockhash = uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c") }, { // For use by test/functional/feature_assumeutxo.py .height = 299, - .hash_serialized = AssumeutxoHash{uint256S("0xef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0")}, + .hash_serialized = AssumeutxoHash{uint256S("0x61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53")}, .nChainTx = 300, .blockhash = uint256S("0x7e0517ef3ea6ecbed9117858e42eedc8eb39e8698a38dcbd1b3962a283233f4c") }, diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp index 527433f45ef..9bd755ed271 100644 --- a/src/kernel/coinstats.cpp +++ b/src/kernel/coinstats.cpp @@ -48,15 +48,35 @@ uint64_t GetBogoSize(const CScript& script_pub_key) script_pub_key.size() /* scriptPubKey */; } -DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin) +template +static void TxOutSer(T& ss, const COutPoint& outpoint, const Coin& coin) +{ + ss << outpoint; + ss << static_cast((coin.nHeight << 1) + coin.fCoinBase); + ss << coin.out; +} + +static void ApplyCoinHash(HashWriter& ss, const COutPoint& outpoint, const Coin& coin) +{ + TxOutSer(ss, outpoint, coin); +} + +void ApplyCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin) { DataStream ss{}; - ss << outpoint; - ss << static_cast(coin.nHeight * 2 + coin.fCoinBase); - ss << coin.out; - return ss; + TxOutSer(ss, outpoint, coin); + muhash.Insert(MakeUCharSpan(ss)); } +void RemoveCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin) +{ + DataStream ss{}; + TxOutSer(ss, outpoint, coin); + muhash.Remove(MakeUCharSpan(ss)); +} + +static void ApplyCoinHash(std::nullptr_t, const COutPoint& outpoint, const Coin& coin) {} + //! Warning: be very careful when changing this! assumeutxo and UTXO snapshot //! validation commitments are reliant on the hash constructed by this //! function. @@ -69,32 +89,13 @@ DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin) //! 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. -static void ApplyHash(HashWriter& ss, const uint256& hash, const std::map& outputs) -{ - for (auto it = outputs.begin(); it != outputs.end(); ++it) { - if (it == outputs.begin()) { - ss << hash; - ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); - } - - ss << VARINT(it->first + 1); - ss << it->second.out.scriptPubKey; - ss << VARINT_MODE(it->second.out.nValue, VarIntMode::NONNEGATIVE_SIGNED); - - if (it == std::prev(outputs.end())) { - ss << VARINT(0u); - } - } -} - -static void ApplyHash(std::nullptr_t, const uint256& hash, const std::map& outputs) {} - -static void ApplyHash(MuHash3072& muhash, const uint256& hash, const std::map& outputs) +template +static void ApplyHash(T& hash_obj, const uint256& hash, const std::map& outputs) { for (auto it = outputs.begin(); it != outputs.end(); ++it) { COutPoint outpoint = COutPoint(hash, it->first); Coin coin = it->second; - muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin))); + ApplyCoinHash(hash_obj, outpoint, coin); } } @@ -118,8 +119,6 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c std::unique_ptr pcursor(view->Cursor()); assert(pcursor); - PrepareHash(hash_obj, stats); - uint256 prevkey; std::map outputs; while (pcursor->Valid()) { @@ -180,15 +179,6 @@ std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsV return stats; } -// The legacy hash serializes the hashBlock -static void PrepareHash(HashWriter& ss, const CCoinsStats& stats) -{ - ss << stats.hashBlock; -} -// MuHash does not need the prepare step -static void PrepareHash(MuHash3072& muhash, CCoinsStats& stats) {} -static void PrepareHash(std::nullptr_t, CCoinsStats& stats) {} - static void FinalizeHash(HashWriter& ss, CCoinsStats& stats) { stats.hashSerialized = ss.GetHash(); diff --git a/src/kernel/coinstats.h b/src/kernel/coinstats.h index 54d0e4f6640..c0c363a8428 100644 --- a/src/kernel/coinstats.h +++ b/src/kernel/coinstats.h @@ -6,6 +6,7 @@ #define BITCOIN_KERNEL_COINSTATS_H #include +#include #include #include @@ -72,7 +73,8 @@ struct CCoinsStats { uint64_t GetBogoSize(const CScript& script_pub_key); -DataStream TxOutSer(const COutPoint& outpoint, const Coin& coin); +void ApplyCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin); +void RemoveCoinHash(MuHash3072& muhash, const COutPoint& outpoint, const Coin& coin); std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function& interruption_point = {}); } // namespace kernel diff --git a/src/test/validation_tests.cpp b/src/test/validation_tests.cpp index 26920372732..14440571eb6 100644 --- a/src/test/validation_tests.cpp +++ b/src/test/validation_tests.cpp @@ -137,11 +137,11 @@ BOOST_AUTO_TEST_CASE(test_assumeutxo) } const auto out110 = *params->AssumeutxoForHeight(110); - BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618"); + BOOST_CHECK_EQUAL(out110.hash_serialized.ToString(), "6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1"); BOOST_CHECK_EQUAL(out110.nChainTx, 111U); const auto out110_2 = *params->AssumeutxoForBlockhash(uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c")); - BOOST_CHECK_EQUAL(out110_2.hash_serialized.ToString(), "1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618"); + BOOST_CHECK_EQUAL(out110_2.hash_serialized.ToString(), "6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1"); BOOST_CHECK_EQUAL(out110_2.nChainTx, 111U); } diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index 2765509f846..9c265649d5b 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -94,8 +94,10 @@ class AssumeutxoTest(BitcoinTestFramework): self.log.info(" - snapshot file with alternated UTXO data") cases = [ - [b"\xff" * 32, 0, "29926acf3ac81f908cf4f22515713ca541c08bb0f0ef1b2c3443a007134d69b8"], # wrong outpoint hash - [(1).to_bytes(4, "little"), 32, "798266c2e1f9a98fe5ce61f5951cbf47130743f3764cf3cbc254be129142cf9d"], # wrong outpoint index + [b"\xff" * 32, 0, "05030e506678f2eca8d624ffed97090ab3beadad1b51ee6e5985ba91c5720e37"], # wrong outpoint hash + [(1).to_bytes(4, "little"), 32, "7d29cfe2c1e242bc6f103878bb70cfffa8b4dac20dbd001ff6ce24b7de2d2399"], # wrong outpoint index + [b"\x81", 36, "f03939a195531f96d5dff983e294a1af62af86049fa7a19a7627246f237c03f1"], # wrong coin code VARINT((coinbase ? 1 : 0) | (height << 1)) + [b"\x83", 36, "e4577da84590fb288c0f7967e89575e1b0aa46624669640f6f5dfef028d39930"], # another wrong coin code ] for content, offset, wrong_hash in cases: @@ -103,7 +105,7 @@ class AssumeutxoTest(BitcoinTestFramework): f.write(valid_snapshot_contents[:(32 + 8 + offset)]) f.write(content) f.write(valid_snapshot_contents[(32 + 8 + offset + len(content)):]) - expected_error(log_msg=f"[snapshot] bad snapshot content hash: expected ef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0, got {wrong_hash}") + expected_error(log_msg=f"[snapshot] bad snapshot content hash: expected 61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53, got {wrong_hash}") def run_test(self): """ @@ -150,7 +152,7 @@ class AssumeutxoTest(BitcoinTestFramework): assert_equal( dump_output['txoutset_hash'], - 'ef45ccdca5898b6c2145e4581d2b88c56564dd389e4bd75a1aaf6961d3edd3c0') + '61d9c2b29a2571a5fe285fe2d8554f91f93309666fc9b8223ee96338de25ff53') assert_equal(dump_output['nchaintx'], 300) assert_equal(n0.getblockchaininfo()["blocks"], SNAPSHOT_BASE_HEIGHT) diff --git a/test/functional/feature_utxo_set_hash.py b/test/functional/feature_utxo_set_hash.py index 0f510ced89c..49c01eee1d7 100755 --- a/test/functional/feature_utxo_set_hash.py +++ b/test/functional/feature_utxo_set_hash.py @@ -69,7 +69,7 @@ class UTXOSetHashTest(BitcoinTestFramework): assert_equal(finalized[::-1].hex(), node_muhash) self.log.info("Test deterministic UTXO set hash results") - assert_equal(node.gettxoutsetinfo()['hash_serialized_2'], "f9aa4fb5ffd10489b9a6994e70ccf1de8a8bfa2d5f201d9857332e9954b0855d") + assert_equal(node.gettxoutsetinfo()['hash_serialized_2'], "d1c7fec1c0623f6793839878cbe2a531eb968b50b27edd6e2a57077a5aed6094") assert_equal(node.gettxoutsetinfo("muhash")['muhash'], "d1725b2fe3ef43e55aa4907480aea98d406fc9e0bf8f60169e2305f1fbf5961b") def run_test(self): diff --git a/test/functional/rpc_dumptxoutset.py b/test/functional/rpc_dumptxoutset.py index f378878181c..1ea6cf52d1d 100755 --- a/test/functional/rpc_dumptxoutset.py +++ b/test/functional/rpc_dumptxoutset.py @@ -46,7 +46,7 @@ class DumptxoutsetTest(BitcoinTestFramework): 'b1bacb602eacf5fbc9a7c2ef6eeb0d229c04e98bdf0c2ea5929012cd0eae3830') assert_equal( - out['txoutset_hash'], '1f7e3befd45dc13ae198dfbb22869a9c5c4196f8e9ef9735831af1288033f890') + out['txoutset_hash'], 'a0b7baa3bf5ccbd3279728f230d7ca0c44a76e9923fca8f32dbfd08d65ea496a') assert_equal(out['nchaintx'], 101) # Specifying a path to an existing or invalid file will fail.