From 03f82087f6ce1c29327f34d12945200494e6956d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 11 Oct 2023 09:20:48 +0200 Subject: [PATCH 01/18] doc: assumeutxo prune and index notes --- doc/design/assumeutxo.md | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/doc/design/assumeutxo.md b/doc/design/assumeutxo.md index 75a7b6c866..66962a629d 100644 --- a/doc/design/assumeutxo.md +++ b/doc/design/assumeutxo.md @@ -3,8 +3,44 @@ Assumeutxo is a feature that allows fast bootstrapping of a validating bitcoind instance. -The RPC commands `dumptxoutset` and `loadtxoutset` are used to -respectively generate and load UTXO snapshots. The utility script +## Loading a snapshot + +There is currently no canonical source for snapshots, but any downloaded snapshot +will be checked against a hash that's been hardcoded in source code. + +Once you've obtained the snapshot, you can use the RPC command `loadtxoutset` to +load it. + +### Pruning + +A pruned node can load a snapshot. To save space, it's possible to delete the +snapshot file as soon as `loadtxoutset` finishes. + +The minimum `-dbcache` setting is 550 MiB, but this functionality ignores that +minimum and uses at least 1100 MiB. + +As the background sync continues there will be temporarily two chainstate +directories, each multiple gigabytes in size (likely growing larger than the +the downloaded snapshot). + +### Indexes + +Indexes work but don't take advantage of this feature. They always start building +from the genesis block. Once the background validation reaches the snapshot block, +indexes will continue to build all the way to the tip. + +For indexes that support pruning, note that no pruning will take place between +the snapshot and the tip, until the background sync has completed - after which +everything is pruned. Depending on how old the snapshot is, this may temporarily +use a significant amount of disk space. + +## Generating a snapshot + +The RPC command `dumptxoutset` can be used to generate a snapshot. This can be used +to create a snapshot on one node that you wish to load on another node. +It can also be used to verify the hardcoded snapshot hash in the source code. + +The utility script `./contrib/devtools/utxo_snapshot.sh` may be of use. ## General background From 9af87cf3485ce3fac553a284cde37a35d1085c25 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 16 Oct 2023 14:56:10 -0400 Subject: [PATCH 02/18] test: Check that a failed wallet migration is cleaned up --- test/functional/wallet_migration.py | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 286dcb5fda..33ab8bf3f8 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -6,11 +6,14 @@ import random import shutil +import struct + from test_framework.address import ( script_to_p2sh, key_to_p2pkh, key_to_p2wpkh, ) +from test_framework.bdb import BTREE_MAGIC from test_framework.descriptors import descsum_create from test_framework.key import ECPubKey from test_framework.test_framework import BitcoinTestFramework @@ -20,6 +23,7 @@ assert_equal, assert_raises_rpc_error, find_vout_for_address, + sha256sum_file, ) from test_framework.wallet_util import ( get_generate_key, @@ -827,6 +831,43 @@ def test_hybrid_pubkey(self): wallet.unloadwallet() + def test_failed_migration_cleanup(self): + self.log.info("Test that a failed migration is cleaned up") + wallet = self.create_legacy_wallet("failed") + + # Make a copy of the wallet with the solvables wallet name so that we are unable + # to create the solvables wallet when migrating, thus failing to migrate + wallet.unloadwallet() + solvables_path = self.nodes[0].wallets_path / "failed_solvables" + shutil.copytree(self.nodes[0].wallets_path / "failed", solvables_path) + original_shasum = sha256sum_file(solvables_path / "wallet.dat") + + self.nodes[0].loadwallet("failed") + + # Add a multisig so that a solvables wallet is created + wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) + wallet.importaddress(get_generate_key().p2pkh_addr) + + assert_raises_rpc_error(-4, "Failed to create new watchonly wallet", wallet.migratewallet) + + assert "failed" in self.nodes[0].listwallets() + assert "failed_watchonly" not in self.nodes[0].listwallets() + assert "failed_solvables" not in self.nodes[0].listwallets() + + assert not (self.nodes[0].wallets_path / "failed_watchonly").exists() + # Since the file in failed_solvables is one that we put there, migration shouldn't touch it + assert solvables_path.exists() + new_shasum = sha256sum_file(solvables_path / "wallet.dat") + assert_equal(original_shasum, new_shasum) + + wallet.unloadwallet() + # Check the wallet we tried to migrate is still BDB + with open(self.nodes[0].wallets_path / "failed" / "wallet.dat", "rb") as f: + data = f.read(16) + _, _, magic = struct.unpack("QII", data) + assert_equal(magic, BTREE_MAGIC) + + def run_test(self): self.generate(self.nodes[0], 101) @@ -845,6 +886,7 @@ def run_test(self): self.test_migrate_raw_p2sh() self.test_conflict_txs() self.test_hybrid_pubkey() + self.test_failed_migration_cleanup() if __name__ == '__main__': WalletMigrationTest().main() From 118f2d7d70b584eee7b89e58b5cd2d61c59a9bbf Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 6 Oct 2023 16:57:42 -0400 Subject: [PATCH 03/18] wallet: Copy all tx metadata to watchonly wallet When moving a tx to the watchonly wallet during migration, make sure that all of the CWalletTx data follows it. --- src/wallet/transaction.cpp | 5 +++++ src/wallet/transaction.h | 8 ++++++-- src/wallet/wallet.cpp | 23 ++++++++++++++++++++--- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/wallet/transaction.cpp b/src/wallet/transaction.cpp index a46846c1d4..4f78fe7520 100644 --- a/src/wallet/transaction.cpp +++ b/src/wallet/transaction.cpp @@ -24,4 +24,9 @@ int64_t CWalletTx::GetTxTime() const int64_t n = nTimeSmart; return n ? n : nTimeReceived; } + +void CWalletTx::CopyFrom(const CWalletTx& _tx) +{ + *this = _tx; +} } // namespace wallet diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index 1a79d7db4e..fa6c8d6f3b 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -323,11 +323,15 @@ class CWalletTx const uint256& GetWitnessHash() const { return tx->GetWitnessHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } +private: // Disable copying of CWalletTx objects to prevent bugs where instances get // copied in and out of the mapWallet map, and fields are updated in the // wrong copy. - CWalletTx(CWalletTx const &) = delete; - void operator=(CWalletTx const &x) = delete; + CWalletTx(const CWalletTx&) = default; + CWalletTx& operator=(const CWalletTx&) = default; +public: + // Instead have an explicit copy function + void CopyFrom(const CWalletTx&); }; struct WalletTxOrderComparator { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5d2f9e2fb6..5caa43edb4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3908,6 +3908,14 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. // We need to go through these in the tx insertion order so that lookups to spends works. std::vector txids_to_delete; + std::unique_ptr watchonly_batch; + if (data.watchonly_wallet) { + watchonly_batch = std::make_unique(data.watchonly_wallet->GetDatabase()); + // Copy the next tx order pos to the watchonly wallet + LOCK(data.watchonly_wallet->cs_wallet); + data.watchonly_wallet->nOrderPosNext = nOrderPosNext; + watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext); + } for (const auto& [_pos, wtx] : wtxOrdered) { if (!IsMine(*wtx->tx) && !IsFromMe(*wtx->tx)) { // Check it is the watchonly wallet's @@ -3916,12 +3924,20 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) LOCK(data.watchonly_wallet->cs_wallet); if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { // Add to watchonly wallet - if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) { - error = _("Error: Could not add watchonly tx to watchonly wallet"); + const uint256& hash = wtx->GetHash(); + const CWalletTx& to_copy_wtx = *wtx; + if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { + if (!new_tx) return false; + ins_wtx.SetTx(to_copy_wtx.tx); + ins_wtx.CopyFrom(to_copy_wtx); + return true; + })) { + error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); return false; } + watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); // Mark as to remove from this wallet - txids_to_delete.push_back(wtx->GetHash()); + txids_to_delete.push_back(hash); continue; } } @@ -3930,6 +3946,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } } + watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { std::vector deleted_txids; From d616d30ea5fdfb897f8375ffd8b9f4536ae7835b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 6 Oct 2023 15:58:13 -0400 Subject: [PATCH 04/18] wallet: Reload watchonly and solvables wallets after migration When migrating, create the watchonly and solvables wallets without a context. Then unload and reload them after migration completes, as we do for the actual wallet. There is also additional handling for a failed reload. --- src/wallet/wallet.cpp | 84 +++++++++++++++++++++-------- test/functional/wallet_migration.py | 2 +- 2 files changed, 62 insertions(+), 24 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5caa43edb4..8f98624403 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4083,6 +4083,10 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseOptions options; options.require_existing = false; options.require_create = true; + options.require_format = DatabaseFormat::SQLITE; + + WalletContext empty_context; + empty_context.args = context.args; // Make the wallets options.create_flags = WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET | WALLET_FLAG_DESCRIPTORS; @@ -4098,8 +4102,14 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; std::string wallet_name = wallet.GetName() + "_watchonly"; - data->watchonly_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); - if (status != DatabaseStatus::SUCCESS) { + std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); + if (!database) { + error = strprintf(_("Wallet file creation failed: %s"), error); + return false; + } + + data->watchonly_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + if (!data->watchonly_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; } @@ -4129,8 +4139,14 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, DatabaseStatus status; std::vector warnings; std::string wallet_name = wallet.GetName() + "_solvables"; - data->solvable_wallet = CreateWallet(context, wallet_name, std::nullopt, options, status, error, warnings); - if (status != DatabaseStatus::SUCCESS) { + std::unique_ptr database = MakeWalletDatabase(wallet_name, options, status, error); + if (!database) { + error = strprintf(_("Wallet file creation failed: %s"), error); + return false; + } + + data->solvable_wallet = CWallet::Create(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + if (!data->solvable_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; } @@ -4233,47 +4249,69 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle success = DoMigration(*local_wallet, context, error, res); } + // In case of reloading failure, we need to remember the wallet dirs to remove + // Set is used as it may be populated with the same wallet directory paths multiple times, + // both before and after reloading. This ensures the set is complete even if one of the wallets + // fails to reload. + std::set wallet_dirs; if (success) { - // Migration successful, unload the wallet locally, then reload it. - assert(local_wallet.use_count() == 1); - local_wallet.reset(); - res.wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + // Migration successful, unload all wallets locally, then reload them. + const auto& reload_wallet = [&](std::shared_ptr& to_reload) { + assert(to_reload.use_count() == 1); + std::string name = to_reload->GetName(); + wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path()); + to_reload.reset(); + to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + return to_reload != nullptr; + }; + // Reload the main wallet + success = reload_wallet(local_wallet); + res.wallet = local_wallet; res.wallet_name = wallet_name; - } else { + if (success && res.watchonly_wallet) { + // Reload watchonly + success = reload_wallet(res.watchonly_wallet); + } + if (success && res.solvables_wallet) { + // Reload solvables + success = reload_wallet(res.solvables_wallet); + } + } + if (!success) { // Migration failed, cleanup // Copy the backup to the actual wallet dir fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); - // Remember this wallet's walletdir to remove after unloading - std::vector wallet_dirs; - wallet_dirs.emplace_back(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); - - // Unload the wallet locally - assert(local_wallet.use_count() == 1); - local_wallet.reset(); - // Make list of wallets to cleanup std::vector> created_wallets; + if (local_wallet) created_wallets.push_back(std::move(local_wallet)); if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet)); if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet)); // Get the directories to remove after unloading for (std::shared_ptr& w : created_wallets) { - wallet_dirs.emplace_back(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); + wallet_dirs.emplace(fs::PathFromString(w->GetDatabase().Filename()).parent_path()); } // Unload the wallets for (std::shared_ptr& w : created_wallets) { - if (!RemoveWallet(context, w, /*load_on_start=*/false)) { - error += _("\nUnable to cleanup failed migration"); - return util::Error{error}; + if (w->HaveChain()) { + // Unloading for wallets that were loaded for normal use + if (!RemoveWallet(context, w, /*load_on_start=*/false)) { + error += _("\nUnable to cleanup failed migration"); + return util::Error{error}; + } + UnloadWallet(std::move(w)); + } else { + // Unloading for wallets in local context + assert(w.use_count() == 1); + w.reset(); } - UnloadWallet(std::move(w)); } // Delete the wallet directories - for (fs::path& dir : wallet_dirs) { + for (const fs::path& dir : wallet_dirs) { fs::remove_all(dir); } diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 33ab8bf3f8..ad8dbe78ed 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -848,7 +848,7 @@ def test_failed_migration_cleanup(self): wallet.addmultisigaddress(2, [wallet.getnewaddress(), get_generate_key().pubkey]) wallet.importaddress(get_generate_key().p2pkh_addr) - assert_raises_rpc_error(-4, "Failed to create new watchonly wallet", wallet.migratewallet) + assert_raises_rpc_error(-4, "Failed to create database", wallet.migratewallet) assert "failed" in self.nodes[0].listwallets() assert "failed_watchonly" not in self.nodes[0].listwallets() From 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Fri, 6 Oct 2023 17:16:39 -0400 Subject: [PATCH 05/18] test: Check tx metadata is migrated to watchonly --- test/functional/wallet_migration.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index ad8dbe78ed..aede9281d5 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -7,6 +7,7 @@ import random import shutil import struct +import time from test_framework.address import ( script_to_p2sh, @@ -315,12 +316,17 @@ def test_other_watchonly(self): sent_watchonly_txid = send["txid"] self.generate(self.nodes[0], 1) + received_watchonly_tx_info = imports0.gettransaction(received_watchonly_txid, True) + received_sent_watchonly_tx_info = imports0.gettransaction(received_sent_watchonly_txid, True) balances = imports0.getbalances() spendable_bal = balances["mine"]["trusted"] watchonly_bal = balances["watchonly"]["trusted"] assert_equal(len(imports0.listtransactions(include_watchonly=True)), 4) + # Mock time forward a bit so we can check that tx metadata is preserved + self.nodes[0].setmocktime(int(time.time()) + 100) + # Migrate imports0.migratewallet() assert_equal(imports0.getwalletinfo()["descriptors"], True) @@ -338,8 +344,12 @@ def test_other_watchonly(self): assert_equal(watchonly_info["descriptors"], True) self.assert_is_sqlite("imports0_watchonly") assert_equal(watchonly_info["private_keys_enabled"], False) - watchonly.gettransaction(received_watchonly_txid) - watchonly.gettransaction(received_sent_watchonly_txid) + received_migrated_watchonly_tx_info = watchonly.gettransaction(received_watchonly_txid) + assert_equal(received_watchonly_tx_info["time"], received_migrated_watchonly_tx_info["time"]) + assert_equal(received_watchonly_tx_info["timereceived"], received_migrated_watchonly_tx_info["timereceived"]) + received_sent_migrated_watchonly_tx_info = watchonly.gettransaction(received_sent_watchonly_txid) + assert_equal(received_sent_watchonly_tx_info["time"], received_sent_migrated_watchonly_tx_info["time"]) + assert_equal(received_sent_watchonly_tx_info["timereceived"], received_sent_migrated_watchonly_tx_info["timereceived"]) watchonly.gettransaction(sent_watchonly_txid) assert_equal(watchonly.getbalance(), watchonly_bal) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", watchonly.gettransaction, received_txid) From 6bdff429ec17eae4138c3af1e21de3ec46f4ab13 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 20 Oct 2023 14:15:52 +0100 Subject: [PATCH 06/18] build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` The `ENABLE_TRACING` macro is expected to be defined in the `config/bitcoin-config.h` header. Therefore, the current code is error-prone as it depends on whether the `config/bitcoin-config.h` header was included before or not. --- src/util/trace.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/util/trace.h b/src/util/trace.h index 051921a0d2..1fe743f043 100644 --- a/src/util/trace.h +++ b/src/util/trace.h @@ -5,6 +5,10 @@ #ifndef BITCOIN_UTIL_TRACE_H #define BITCOIN_UTIL_TRACE_H +#if defined(HAVE_CONFIG_H) +#include +#endif + #ifdef ENABLE_TRACING #include From fac36b94ef32567c0f10b605a3a441d11559e56e Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 20 Oct 2023 13:28:11 +0200 Subject: [PATCH 07/18] refactor: Remove CBlockFileInfo::SetNull --- src/chain.h | 30 ++++++++---------------------- src/node/blockstorage.cpp | 2 +- src/node/blockstorage.h | 1 - 3 files changed, 9 insertions(+), 24 deletions(-) diff --git a/src/chain.h b/src/chain.h index 4bf2001f74..f9121fb861 100644 --- a/src/chain.h +++ b/src/chain.h @@ -42,13 +42,13 @@ static constexpr int64_t MAX_BLOCK_TIME_GAP = 90 * 60; class CBlockFileInfo { public: - unsigned int nBlocks; //!< number of blocks stored in file - unsigned int nSize; //!< number of used bytes of block file - unsigned int nUndoSize; //!< number of used bytes in the undo file - unsigned int nHeightFirst; //!< lowest height of block in file - unsigned int nHeightLast; //!< highest height of block in file - uint64_t nTimeFirst; //!< earliest time of block in file - uint64_t nTimeLast; //!< latest time of block in file + unsigned int nBlocks{}; //!< number of blocks stored in file + unsigned int nSize{}; //!< number of used bytes of block file + unsigned int nUndoSize{}; //!< number of used bytes in the undo file + unsigned int nHeightFirst{}; //!< lowest height of block in file + unsigned int nHeightLast{}; //!< highest height of block in file + uint64_t nTimeFirst{}; //!< earliest time of block in file + uint64_t nTimeLast{}; //!< latest time of block in file SERIALIZE_METHODS(CBlockFileInfo, obj) { @@ -61,21 +61,7 @@ class CBlockFileInfo READWRITE(VARINT(obj.nTimeLast)); } - void SetNull() - { - nBlocks = 0; - nSize = 0; - nUndoSize = 0; - nHeightFirst = 0; - nHeightLast = 0; - nTimeFirst = 0; - nTimeLast = 0; - } - - CBlockFileInfo() - { - SetNull(); - } + CBlockFileInfo() {} std::string ToString() const; diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f8f1aab551..53f616de23 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -254,7 +254,7 @@ void BlockManager::PruneOneBlockFile(const int fileNumber) } } - m_blockfile_info[fileNumber].SetNull(); + m_blockfile_info.at(fileNumber) = CBlockFileInfo{}; m_dirty_fileinfo.insert(fileNumber); } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index ac97728c05..ba44d31581 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -32,7 +32,6 @@ class BlockValidationState; class CAutoFile; class CBlock; -class CBlockFileInfo; class CBlockUndo; class CChainParams; class Chainstate; From fa21535551e300eaa988d209ad64cdc17fd7f66b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 20 Oct 2023 18:18:31 +0200 Subject: [PATCH 08/18] fuzz: Increase merge -rss_limit_mb --- test/fuzz/test_runner.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index 9d64591111..ec74f7705c 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -277,6 +277,7 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, src_dir, build_dir, merge_dirs for t in test_list: args = [ os.path.join(build_dir, 'src', 'test', 'fuzz', 'fuzz'), + '-rss_limit_mb=8000', '-set_cover_merge=1', # set_cover_merge is used instead of -merge=1 to reduce the overall # size of the qa-assets git repository a bit, but more importantly, From 351370a1d211615e3d5b158ccb0400cd79c5c085 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Wed, 18 Oct 2023 17:06:36 +0200 Subject: [PATCH 09/18] 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 | 64 ++++++++++-------------- 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, 48 insertions(+), 53 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 9dab8ca901..ecd3fd21b5 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 3ac8756e41..f928520d3e 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -494,14 +494,14 @@ class CRegTestParams : public CChainParams 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 527433f45e..9bd755ed27 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) { - DataStream ss{}; ss << outpoint; - ss << static_cast(coin.nHeight * 2 + coin.fCoinBase); + ss << static_cast((coin.nHeight << 1) + coin.fCoinBase); ss << coin.out; - return ss; } +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{}; + 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 54d0e4f664..c0c363a842 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 2692037273..14440571eb 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 2765509f84..9c265649d5 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -94,8 +94,10 @@ def expected_error(log_msg="", rpc_details=""): 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 @@ def expected_error(log_msg="", rpc_details=""): 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 @@ def run_test(self): 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 0f510ced89..49c01eee1d 100755 --- a/test/functional/feature_utxo_set_hash.py +++ b/test/functional/feature_utxo_set_hash.py @@ -69,7 +69,7 @@ def test_muhash_implementation(self): 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 f378878181..1ea6cf52d1 100755 --- a/test/functional/rpc_dumptxoutset.py +++ b/test/functional/rpc_dumptxoutset.py @@ -46,7 +46,7 @@ def run_test(self): '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. From cb0336817edc2b6aee2eca818f133841f613a767 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 19 Oct 2023 14:54:41 +0200 Subject: [PATCH 10/18] scripted-diff: Rename hash_serialized_2 to hash_serialized_3 -BEGIN VERIFY SCRIPT- sed -i 's/hash_serialized_2/hash_serialized_3/g' $( git grep -l 'hash_serialized_2' ./src ./contrib ./test ) -END VERIFY SCRIPT- --- contrib/devtools/utxo_snapshot.sh | 2 +- src/rpc/blockchain.cpp | 10 +++++----- test/functional/feature_coinstatsindex.py | 6 +++--- test/functional/feature_dbcrash.py | 8 ++++---- test/functional/feature_utxo_set_hash.py | 2 +- test/functional/rpc_blockchain.py | 12 ++++++------ 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/contrib/devtools/utxo_snapshot.sh b/contrib/devtools/utxo_snapshot.sh index dee25ff67b..ad2ec26651 100755 --- a/contrib/devtools/utxo_snapshot.sh +++ b/contrib/devtools/utxo_snapshot.sh @@ -34,7 +34,7 @@ ${BITCOIN_CLI_CALL} invalidateblock "${PIVOT_BLOCKHASH}" if [[ "${OUTPUT_PATH}" = "-" ]]; then (>&2 echo "Generating txoutset info...") - ${BITCOIN_CLI_CALL} gettxoutsetinfo | grep hash_serialized_2 | sed 's/^.*: "\(.\+\)\+",/\1/g' + ${BITCOIN_CLI_CALL} gettxoutsetinfo | grep hash_serialized_3 | sed 's/^.*: "\(.\+\)\+",/\1/g' else (>&2 echo "Generating UTXO snapshot...") ${BITCOIN_CLI_CALL} dumptxoutset "${OUTPUT_PATH}" diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 6d2b84cb6c..7b84747a3f 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -820,7 +820,7 @@ static RPCHelpMan pruneblockchain() CoinStatsHashType ParseHashType(const std::string& hash_type_input) { - if (hash_type_input == "hash_serialized_2") { + if (hash_type_input == "hash_serialized_3") { return CoinStatsHashType::HASH_SERIALIZED; } else if (hash_type_input == "muhash") { return CoinStatsHashType::MUHASH; @@ -867,7 +867,7 @@ static RPCHelpMan gettxoutsetinfo() "\nReturns statistics about the unspent transaction output set.\n" "Note this call may take some time if you are not using coinstatsindex.\n", { - {"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'."}, + {"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'."}, {"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).", RPCArgOptions{ .skip_type_check = true, @@ -882,7 +882,7 @@ static RPCHelpMan gettxoutsetinfo() {RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at which these statistics are calculated"}, {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs"}, {RPCResult::Type::NUM, "bogosize", "Database-independent, meaningless metric indicating the UTXO set size"}, - {RPCResult::Type::STR_HEX, "hash_serialized_2", /*optional=*/true, "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"}, + {RPCResult::Type::STR_HEX, "hash_serialized_3", /*optional=*/true, "The serialized hash (only present if 'hash_serialized_3' hash_type is chosen)"}, {RPCResult::Type::STR_HEX, "muhash", /*optional=*/true, "The serialized hash (only present if 'muhash' hash_type is chosen)"}, {RPCResult::Type::NUM, "transactions", /*optional=*/true, "The number of transactions with unspent outputs (not available when coinstatsindex is used)"}, {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() } if (hash_type == CoinStatsHashType::HASH_SERIALIZED) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_2 hash type cannot be queried for a specific block"); + throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_3 hash type cannot be queried for a specific block"); } if (!index_requested) { @@ -971,7 +971,7 @@ static RPCHelpMan gettxoutsetinfo() ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs); ret.pushKV("bogosize", (int64_t)stats.nBogoSize); if (hash_type == CoinStatsHashType::HASH_SERIALIZED) { - ret.pushKV("hash_serialized_2", stats.hashSerialized.GetHex()); + ret.pushKV("hash_serialized_3", stats.hashSerialized.GetHex()); } if (hash_type == CoinStatsHashType::MUHASH) { ret.pushKV("muhash", stats.hashSerialized.GetHex()); diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py index 2ffb182946..d6c1567e64 100755 --- a/test/functional/feature_coinstatsindex.py +++ b/test/functional/feature_coinstatsindex.py @@ -293,11 +293,11 @@ def _test_reorg_index(self): def _test_index_rejects_hash_serialized(self): self.log.info("Test that the rpc raises if the legacy hash is passed with the index") - msg = "hash_serialized_2 hash type cannot be queried for a specific block" - assert_raises_rpc_error(-8, msg, self.nodes[1].gettxoutsetinfo, hash_type='hash_serialized_2', hash_or_height=111) + msg = "hash_serialized_3 hash type cannot be queried for a specific block" + assert_raises_rpc_error(-8, msg, self.nodes[1].gettxoutsetinfo, hash_type='hash_serialized_3', hash_or_height=111) for use_index in {True, False, None}: - assert_raises_rpc_error(-8, msg, self.nodes[1].gettxoutsetinfo, hash_type='hash_serialized_2', hash_or_height=111, use_index=use_index) + assert_raises_rpc_error(-8, msg, self.nodes[1].gettxoutsetinfo, hash_type='hash_serialized_3', hash_or_height=111, use_index=use_index) def _test_init_index_after_reorg(self): self.log.info("Test a reorg while the index is deactivated") diff --git a/test/functional/feature_dbcrash.py b/test/functional/feature_dbcrash.py index 3f94bbc9d1..afd0246209 100755 --- a/test/functional/feature_dbcrash.py +++ b/test/functional/feature_dbcrash.py @@ -85,7 +85,7 @@ def restart_node(self, node_index, expected_tip): # Any of these RPC calls could throw due to node crash self.start_node(node_index) self.nodes[node_index].waitforblock(expected_tip) - utxo_hash = self.nodes[node_index].gettxoutsetinfo()['hash_serialized_2'] + utxo_hash = self.nodes[node_index].gettxoutsetinfo()['hash_serialized_3'] return utxo_hash except Exception: # An exception here should mean the node is about to crash. @@ -130,7 +130,7 @@ def sync_node3blocks(self, block_hashes): If any nodes crash while updating, we'll compare utxo hashes to ensure recovery was successful.""" - node3_utxo_hash = self.nodes[3].gettxoutsetinfo()['hash_serialized_2'] + node3_utxo_hash = self.nodes[3].gettxoutsetinfo()['hash_serialized_3'] # Retrieve all the blocks from node3 blocks = [] @@ -172,12 +172,12 @@ def verify_utxo_hash(self): """Verify that the utxo hash of each node matches node3. Restart any nodes that crash while querying.""" - node3_utxo_hash = self.nodes[3].gettxoutsetinfo()['hash_serialized_2'] + node3_utxo_hash = self.nodes[3].gettxoutsetinfo()['hash_serialized_3'] self.log.info("Verifying utxo hash matches for all nodes") for i in range(3): try: - nodei_utxo_hash = self.nodes[i].gettxoutsetinfo()['hash_serialized_2'] + nodei_utxo_hash = self.nodes[i].gettxoutsetinfo()['hash_serialized_3'] except OSError: # probably a crash on db flushing nodei_utxo_hash = self.restart_node(i, self.nodes[3].getbestblockhash()) diff --git a/test/functional/feature_utxo_set_hash.py b/test/functional/feature_utxo_set_hash.py index 49c01eee1d..ce2a5ab8ac 100755 --- a/test/functional/feature_utxo_set_hash.py +++ b/test/functional/feature_utxo_set_hash.py @@ -69,7 +69,7 @@ def test_muhash_implementation(self): assert_equal(finalized[::-1].hex(), node_muhash) self.log.info("Test deterministic UTXO set hash results") - assert_equal(node.gettxoutsetinfo()['hash_serialized_2'], "d1c7fec1c0623f6793839878cbe2a531eb968b50b27edd6e2a57077a5aed6094") + assert_equal(node.gettxoutsetinfo()['hash_serialized_3'], "d1c7fec1c0623f6793839878cbe2a531eb968b50b27edd6e2a57077a5aed6094") assert_equal(node.gettxoutsetinfo("muhash")['muhash'], "d1725b2fe3ef43e55aa4907480aea98d406fc9e0bf8f60169e2305f1fbf5961b") def run_test(self): diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index 18a0a0c6cc..53163720bb 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -340,7 +340,7 @@ def _test_gettxoutsetinfo(self): assert size > 6400 assert size < 64000 assert_equal(len(res['bestblock']), 64) - assert_equal(len(res['hash_serialized_2']), 64) + assert_equal(len(res['hash_serialized_3']), 64) self.log.info("Test gettxoutsetinfo works for blockchain with just the genesis block") b1hash = node.getblockhash(1) @@ -353,7 +353,7 @@ def _test_gettxoutsetinfo(self): assert_equal(res2['txouts'], 0) assert_equal(res2['bogosize'], 0), assert_equal(res2['bestblock'], node.getblockhash(0)) - assert_equal(len(res2['hash_serialized_2']), 64) + assert_equal(len(res2['hash_serialized_3']), 64) self.log.info("Test gettxoutsetinfo returns the same result after invalidate/reconsider block") node.reconsiderblock(b1hash) @@ -365,20 +365,20 @@ def _test_gettxoutsetinfo(self): assert_equal(res, res3) self.log.info("Test gettxoutsetinfo hash_type option") - # Adding hash_type 'hash_serialized_2', which is the default, should + # Adding hash_type 'hash_serialized_3', which is the default, should # not change the result. - res4 = node.gettxoutsetinfo(hash_type='hash_serialized_2') + res4 = node.gettxoutsetinfo(hash_type='hash_serialized_3') del res4['disk_size'] assert_equal(res, res4) # hash_type none should not return a UTXO set hash. res5 = node.gettxoutsetinfo(hash_type='none') - assert 'hash_serialized_2' not in res5 + assert 'hash_serialized_3' not in res5 # hash_type muhash should return a different UTXO set hash. res6 = node.gettxoutsetinfo(hash_type='muhash') assert 'muhash' in res6 - assert res['hash_serialized_2'] != res6['muhash'] + assert res['hash_serialized_3'] != res6['muhash'] # muhash should not be returned unless requested. for r in [res, res2, res3, res4, res5]: From 66865446a771327be9e972cdaf01154ea1bdff6d Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 19 Oct 2023 15:02:38 +0200 Subject: [PATCH 11/18] docs: Add release notes for #28685 --- doc/release-notes-28685.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 doc/release-notes-28685.md diff --git a/doc/release-notes-28685.md b/doc/release-notes-28685.md new file mode 100644 index 0000000000..6f04d8d542 --- /dev/null +++ b/doc/release-notes-28685.md @@ -0,0 +1,4 @@ +RPC +--- + +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. From f6213929c519d0e615cacd3d6f479f1517be1662 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 19 Oct 2023 15:49:04 +0200 Subject: [PATCH 12/18] assumeutxo: Check deserialized coins for out of range values --- src/validation.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 2600f0f9fe..a6cab6b095 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5399,6 +5399,11 @@ bool ChainstateManager::PopulateAndValidateSnapshot( coins_count - coins_left); return false; } + if (!MoneyRange(coin.out.nValue)) { + LogPrintf("[snapshot] bad snapshot data after deserializing %d coins - bad tx out value\n", + coins_count - coins_left); + return false; + } coins_cache.EmplaceCoinInternalDANGER(std::move(outpoint), std::move(coin)); From a503cd0f0b55736743bcf8d2c46d271064772bef Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 19 Oct 2023 14:08:19 +0200 Subject: [PATCH 13/18] chainparams, assumeutxo: Fix testnet txoutset hash Review hint: You can use devtools/utxo_snapshot.sh to validate this. ./contrib/devtools/utxo_snapshot.sh 2500000 testnet-utxo.dat ./src/bitcoin-cli --- src/kernel/chainparams.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index f928520d3e..fe47306fb5 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -269,7 +269,7 @@ class CTestNetParams : public CChainParams { m_assumeutxo_data = { { .height = 2'500'000, - .hash_serialized = AssumeutxoHash{uint256S("0x2a8fdefef3bf75fa00540ccaaaba4b5281bea94229327bdb0f7416ef1e7a645c")}, + .hash_serialized = AssumeutxoHash{uint256S("0xf841584909f68e47897952345234e37fcd9128cd818f41ee6c3ca68db8071be7")}, .nChainTx = 66484552, .blockhash = uint256S("0x0000000000000093bcb68c03a9a168ae252572d348a2eaeba2cdf9231d73206f") } From 4bfaad4eca01674a9c84a447a17594dc2b9a4c39 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 19 Oct 2023 14:10:18 +0200 Subject: [PATCH 14/18] chainparams, assumeutxo: Fix signet txoutset hash Review hint: You can use devtools/utxo_snapshot.sh to validate this. ./contrib/devtools/utxo_snapshot.sh 160000 signet-utxo.dat ./src/bitcoin-cli --- src/kernel/chainparams.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index fe47306fb5..73ba330ff0 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -378,7 +378,7 @@ class SigNetParams : public CChainParams { m_assumeutxo_data = { { .height = 160'000, - .hash_serialized = AssumeutxoHash{uint256S("0x5225141cb62dee63ab3be95f9b03d60801f264010b1816d4bd00618b2736e7be")}, + .hash_serialized = AssumeutxoHash{uint256S("0xfe0a44309b74d6b5883d246cb419c6221bcccf0b308c9b59b7d70783dbdf928a")}, .nChainTx = 2289496, .blockhash = uint256S("0x0000003ca3c99aff040f2563c2ad8f8ec88bd0fd6b8f0895cfaf1ef90353a62c") } From 3f482ac231c3a0077dd236c0ec8f5afc12b71859 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 23 Oct 2023 10:21:00 +0100 Subject: [PATCH 15/18] doc: add historical release notes for 24.2 --- doc/release-notes/release-notes-24.2.md | 76 +++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 doc/release-notes/release-notes-24.2.md diff --git a/doc/release-notes/release-notes-24.2.md b/doc/release-notes/release-notes-24.2.md new file mode 100644 index 0000000000..059ab51872 --- /dev/null +++ b/doc/release-notes/release-notes-24.2.md @@ -0,0 +1,76 @@ +24.2 Release Notes +================== + +Bitcoin Core version 24.2 is now available from: + + + +This release includes various bug fixes and performance +improvements, as well as updated translations. + +Please report bugs using the issue tracker at GitHub: + + + +To receive security and update notifications, please subscribe to: + + + +How to Upgrade +============== + +If you are running an older version, shut it down. Wait until it has completely +shut down (which might take a few minutes in some cases), then run the +installer (on Windows) or just copy over `/Applications/Bitcoin-Qt` (on macOS) +or `bitcoind`/`bitcoin-qt` (on Linux). + +Upgrading directly from a version of Bitcoin Core that has reached its EOL is +possible, but it might take some time if the data directory needs to be migrated. Old +wallet versions of Bitcoin Core are generally supported. + +Compatibility +============== + +Bitcoin Core is supported and extensively tested on operating systems +using the Linux kernel, macOS 10.15+, and Windows 7 and newer. Bitcoin +Core should also work on most other Unix-like systems but is not as +frequently tested on them. It is not recommended to use Bitcoin Core on +unsupported systems. + +### Fees + +- #27622 Fee estimation: avoid serving stale fee estimate + +### RPC and other APIs + +- #27727 rpc: Fix invalid bech32 address handling + +### Build System + +- #28097 depends: xcb-proto 1.15.2 +- #28543 build, macos: Fix qt package build with new Xcode 15 linker +- #28571 depends: fix unusable memory_resource in macos qt build + +### CI + +- #27777 ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN +- #27834 ci: Nuke Android APK task, Use credits for tsan +- #27844 ci: Use podman stop over podman kill +- #27886 ci: Switch to amd64 container in "ARM" task + +### Miscellaneous +- #28452 Do not use std::vector = {} to release memory + +Credits +======= + +Thanks to everyone who directly contributed to this release: + +- Abubakar Sadiq Ismail +- Hennadii Stepanov +- Marco Falke +- Michael Ford +- Pieter Wuille + +As well as to everyone that helped with translations on +[Transifex](https://www.transifex.com/bitcoin/bitcoin/). From f09bfab4aff04a9cc1ea157b94bfdae19f3465b1 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 23 Oct 2023 12:14:37 +0100 Subject: [PATCH 16/18] Revert "gui: provide wallet controller context to wallet actions" This reverts commit 7066e8996d0ac090535cc97cdcb54a219986460f. --- src/qt/bitcoingui.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 720f5584d6..fd71938b60 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -392,7 +392,7 @@ void BitcoinGUI::createActions() connect(usedSendingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedSendingAddresses); connect(usedReceivingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedReceivingAddresses); connect(openAction, &QAction::triggered, this, &BitcoinGUI::openClicked); - connect(m_open_wallet_menu, &QMenu::aboutToShow, m_wallet_controller, [this] { + connect(m_open_wallet_menu, &QMenu::aboutToShow, [this] { m_open_wallet_menu->clear(); for (const std::pair& i : m_wallet_controller->listWalletDir()) { const std::string& path = i.first; @@ -409,7 +409,7 @@ void BitcoinGUI::createActions() continue; } - connect(action, &QAction::triggered, m_wallet_controller, [this, path] { + connect(action, &QAction::triggered, [this, path] { auto activity = new OpenWalletActivity(m_wallet_controller, this); connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection); connect(activity, &OpenWalletActivity::opened, rpcConsole, &RPCConsole::setCurrentWallet, Qt::QueuedConnection); @@ -421,7 +421,7 @@ void BitcoinGUI::createActions() action->setEnabled(false); } }); - connect(m_restore_wallet_action, &QAction::triggered, m_wallet_controller, [this] { + connect(m_restore_wallet_action, &QAction::triggered, [this] { //: Name of the wallet data file format. QString name_data_file = tr("Wallet Data"); @@ -447,14 +447,14 @@ void BitcoinGUI::createActions() auto backup_file_path = fs::PathFromString(backup_file.toStdString()); activity->restore(backup_file_path, wallet_name.toStdString()); }); - connect(m_close_wallet_action, &QAction::triggered, m_wallet_controller, [this] { + connect(m_close_wallet_action, &QAction::triggered, [this] { m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this); }); connect(m_create_wallet_action, &QAction::triggered, this, &BitcoinGUI::createWallet); - connect(m_close_all_wallets_action, &QAction::triggered, m_wallet_controller, [this] { + connect(m_close_all_wallets_action, &QAction::triggered, [this] { m_wallet_controller->closeAllWallets(this); }); - connect(m_migrate_wallet_action, &QAction::triggered, m_wallet_controller, [this] { + connect(m_migrate_wallet_action, &QAction::triggered, [this] { auto activity = new MigrateWalletActivity(m_wallet_controller, this); connect(activity, &MigrateWalletActivity::migrated, this, &BitcoinGUI::setCurrentWallet); activity->migrate(walletFrame->currentWalletModel()); From fa6588737714cf26571657fc216552a4291376da Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 23 Oct 2023 12:09:31 +0200 Subject: [PATCH 17/18] ci: Add missing --external to podman image prune --- ci/test/02_run_container.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ci/test/02_run_container.sh b/ci/test/02_run_container.sh index c8c54267e7..edf8f2c30f 100755 --- a/ci/test/02_run_container.sh +++ b/ci/test/02_run_container.sh @@ -35,10 +35,13 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then # Still prune everything in case the filtered pruning doesn't work, or if labels were not set # on a previous run. Belt and suspenders approach, should be fine to remove in the future. + # Prune images used by --external containers (e.g. build containers) when + # using podman. echo "Prune all dangling images" - docker image prune --force + podman image prune --force --external fi echo "Prune all dangling $CI_IMAGE_LABEL images" + # When detecting podman-docker, `--external` should be added. docker image prune --force --filter "label=$CI_IMAGE_LABEL" # shellcheck disable=SC2086 From 5a0688a20d88a9641c02436abbd7b49e227f1a37 Mon Sep 17 00:00:00 2001 From: Matthew Zipkin Date: Tue, 17 Oct 2023 11:26:00 -0400 Subject: [PATCH 18/18] test: enable reindex readonly test on *BSD and macOS as root --- test/functional/feature_reindex_readonly.py | 43 +++++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/test/functional/feature_reindex_readonly.py b/test/functional/feature_reindex_readonly.py index 26531f472b..dd99c3c4fa 100755 --- a/test/functional/feature_reindex_readonly.py +++ b/test/functional/feature_reindex_readonly.py @@ -7,7 +7,6 @@ """ import os -import platform import stat import subprocess from test_framework.test_framework import BitcoinTestFramework @@ -34,11 +33,13 @@ def reindex_readonly(self): filename = self.nodes[0].chain_path / "blocks" / "blk00000.dat" filename.chmod(stat.S_IREAD) - used_chattr = False - if platform.system() == "Linux": + undo_immutable = lambda: None + # Linux + try: + subprocess.run(['chattr'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) try: subprocess.run(['chattr', '+i', filename], capture_output=True, check=True) - used_chattr = True + undo_immutable = lambda: subprocess.check_call(['chattr', '-i', filename]) self.log.info("Made file immutable with chattr") except subprocess.CalledProcessError as e: self.log.warning(str(e)) @@ -50,15 +51,33 @@ def reindex_readonly(self): self.log.warning("Return early on Linux under root, because chattr failed.") self.log.warning("This should only happen due to missing capabilities in a container.") self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.") - return - - self.log.debug("Attempt to restart and reindex the node with the unwritable block file") - with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]): - self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'], - expected_msg="Error: A fatal internal error occurred, see debug.log for details") + undo_immutable = False + except Exception: + # macOS, and *BSD + try: + subprocess.run(['chflags'], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + try: + subprocess.run(['chflags', 'uchg', filename], capture_output=True, check=True) + undo_immutable = lambda: subprocess.check_call(['chflags', 'nouchg', filename]) + self.log.info("Made file immutable with chflags") + except subprocess.CalledProcessError as e: + self.log.warning(str(e)) + if e.stdout: + self.log.warning(f"stdout: {e.stdout}") + if e.stderr: + self.log.warning(f"stderr: {e.stderr}") + if os.getuid() == 0: + self.log.warning("Return early on BSD under root, because chflags failed.") + undo_immutable = False + except Exception: + pass - if used_chattr: - subprocess.check_call(['chattr', '-i', filename]) + if undo_immutable: + self.log.info("Attempt to restart and reindex the node with the unwritable block file") + with self.nodes[0].assert_debug_log(expected_msgs=['FlushStateToDisk', 'failed to open file'], unexpected_msgs=[]): + self.nodes[0].assert_start_raises_init_error(extra_args=['-reindex', '-fastprune'], + expected_msg="Error: A fatal internal error occurred, see debug.log for details") + undo_immutable() filename.chmod(0o777)