From 813a16a463326079472366aed42f5218a57db63b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 24 Oct 2023 17:44:46 -0400 Subject: [PATCH 01/26] wallet: Add HasCryptedKeys --- src/wallet/scriptpubkeyman.cpp | 12 ++++++++++++ src/wallet/scriptpubkeyman.h | 3 +++ src/wallet/wallet.cpp | 8 ++++++++ src/wallet/wallet.h | 1 + 4 files changed, 24 insertions(+) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 46ec5dc1ac..a37981ff65 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -525,6 +525,12 @@ bool LegacyScriptPubKeyMan::HavePrivateKeys() const return !mapKeys.empty() || !mapCryptedKeys.empty(); } +bool LegacyScriptPubKeyMan::HaveCryptedKeys() const +{ + LOCK(cs_KeyStore); + return !mapCryptedKeys.empty(); +} + void LegacyScriptPubKeyMan::RewriteDB() { LOCK(cs_KeyStore); @@ -2392,6 +2398,12 @@ bool DescriptorScriptPubKeyMan::HavePrivateKeys() const return m_map_keys.size() > 0 || m_map_crypted_keys.size() > 0; } +bool DescriptorScriptPubKeyMan::HaveCryptedKeys() const +{ + LOCK(cs_desc_man); + return !m_map_crypted_keys.empty(); +} + std::optional DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const { // This is only used for getwalletinfo output and isn't relevant to descriptor wallets. diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index cf7b7eaf31..e0601785b0 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -221,6 +221,7 @@ class ScriptPubKeyMan virtual bool Upgrade(int prev_version, int new_version, bilingual_str& error) { return true; } virtual bool HavePrivateKeys() const { return false; } + virtual bool HaveCryptedKeys() const { return false; } //! The action to do when the DB needs rewrite virtual void RewriteDB() {} @@ -472,6 +473,7 @@ class LegacyScriptPubKeyMan : public LegacyDataSPKM bool Upgrade(int prev_version, int new_version, bilingual_str& error) override; bool HavePrivateKeys() const override; + bool HaveCryptedKeys() const override; void RewriteDB() override; @@ -659,6 +661,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); //! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked. std::optional GetKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + bool HaveCryptedKeys() const override; std::optional GetOldestKeyPoolTime() const override; unsigned int GetKeyPoolSize() const override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index de565102cc..bef42cbdc2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3695,6 +3695,14 @@ bool CWallet::HasEncryptionKeys() const return !mapMasterKeys.empty(); } +bool CWallet::HaveCryptedKeys() const +{ + for (const auto& spkm : GetAllScriptPubKeyMans()) { + if (spkm->HaveCryptedKeys()) return true; + } + return false; +} + void CWallet::ConnectScriptPubKeyManNotifiers() { for (const auto& spk_man : GetActiveScriptPubKeyMans()) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d3a7208b15..88e4778660 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -969,6 +969,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool WithEncryptionKey(std::function cb) const override; bool HasEncryptionKeys() const override; + bool HaveCryptedKeys() const; /** Get last block processed height */ int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) From 2b9279b50a3682ab97308888b4f272d3ae379811 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 24 Oct 2023 17:45:01 -0400 Subject: [PATCH 02/26] wallet: Remove unused encryption keys from watchonly wallets Due to a bug in earlier versions, some wallets without private keys may have an encryption key. This encryption key is unused and can lead to confusing behavior elsewhere. When such wallets are detected, those encryption keys will now be deleted from the wallet. For safety, we only do this to wallets which have private keys disabled, have encryption keys, and definitely do not have encrypted keys. --- src/wallet/walletdb.cpp | 20 ++++++++++++++++++++ src/wallet/walletdb.h | 1 + 2 files changed, 21 insertions(+) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 597a4ef9a4..82e73fc213 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -154,6 +154,11 @@ bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey) return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true); } +bool WalletBatch::EraseMasterKey(unsigned int id) +{ + return EraseIC(std::make_pair(DBKeys::MASTER_KEY, id)); +} + bool WalletBatch::WriteCScript(const uint160& hash, const CScript& redeemScript) { return WriteIC(std::make_pair(DBKeys::CSCRIPT, hash), redeemScript, false); @@ -1241,6 +1246,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) result = DBErrors::CORRUPT; } + // Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is + // such a wallet and remove the encryption key records to avoid any future issues. + // Although wallets without private keys should not have *ckey records, we should double check that. + // Removing the mkey records is only safe if there are no *ckey records. + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) { + pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n"); + for (const auto& [id, _] : pwallet->mapMasterKeys) { + if (!EraseMasterKey(id)) { + pwallet->WalletLogPrintf("Error: Unable to remove extraneous encryption key '%u'. Wallet corrupt.\n", id); + return DBErrors::CORRUPT; + } + } + pwallet->mapMasterKeys.clear(); + } + return result; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index bffcc87202..08054f42ba 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -238,6 +238,7 @@ class WalletBatch bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta); bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector& vchCryptedSecret, const CKeyMetadata &keyMeta); bool WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey); + bool EraseMasterKey(unsigned int id); bool WriteCScript(const uint160& hash, const CScript& redeemScript); From 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 24 Oct 2023 18:57:16 -0400 Subject: [PATCH 03/26] tests: Test cleanup of mkeys from wallets without privkeys --- test/functional/wallet_encryption.py | 61 ++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 5a5fb3e5be..b30a1478ab 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -5,7 +5,9 @@ """Test Wallet encryption""" import time +import subprocess +from test_framework.messages import hash256 from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_raises_rpc_error, @@ -100,6 +102,65 @@ def run_test(self): sig = self.nodes[0].signmessage(address, msg) assert self.nodes[0].verifymessage(address, sig, msg) + self.log.info("Test that wallets without private keys cannot be encrypted") + self.nodes[0].createwallet(wallet_name="noprivs", disable_private_keys=True) + noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs") + assert_raises_rpc_error(-16, "Error: wallet does not contain private keys, nothing to encrypt.", noprivs_wallet.encryptwallet, "pass") + + if self.is_wallet_tool_compiled(): + self.log.info("Test that encryption keys in wallets without privkeys are removed") + + def do_wallet_tool(*args): + proc = subprocess.Popen( + [self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}"] + list(args), + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + stdout, stderr = proc.communicate() + assert_equal(proc.poll(), 0) + assert_equal(stderr, "") + + # Since it is no longer possible to encrypt a wallet without privkeys, we need to force one into the wallet + # 1. Make a dump of the wallet + # 2. Add mkey record to the dump + # 3. Create a new wallet from the dump + + # Make the dump + noprivs_wallet.unloadwallet() + dumpfile_path = self.nodes[0].datadir_path / "noprivs.dump" + do_wallet_tool("-wallet=noprivs", f"-dumpfile={dumpfile_path}", "dump") + + # Modify the dump + with open(dumpfile_path, "r", encoding="utf-8") as f: + dump_content = f.readlines() + # Drop the checksum line + dump_content = dump_content[:-1] + # Insert a valid mkey line. This corresponds to a passphrase of "pass". + dump_content.append("046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n") + with open(dumpfile_path, "w", encoding="utf-8") as f: + contents = "".join(dump_content) + f.write(contents) + checksum = hash256(contents.encode()) + f.write(f"checksum,{checksum.hex()}\n") + + # Load the dump into a new wallet + do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "createfromdump") + # Load the wallet and make sure it is no longer encrypted + with self.nodes[0].assert_debug_log(["Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys."]): + self.nodes[0].loadwallet("noprivs_enc") + noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs_enc") + assert_raises_rpc_error(-15, "Error: running with an unencrypted wallet, but walletpassphrase was called.", noprivs_wallet.walletpassphrase, "pass", 1) + noprivs_wallet.unloadwallet() + + # Make a new dump and check that there are no mkeys + dumpfile_path = self.nodes[0].datadir_path / "noprivs_enc.dump" + do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "dump") + with open(dumpfile_path, "r", encoding="utf-8") as f: + # Check theres nothing with an 'mkey' prefix + assert_equal(all([not line.startswith("046d6b6579") for line in f]), True) + if __name__ == '__main__': WalletEncryptionTest(__file__).main() From fa397177acfa1006ea6feee0b215c53e51f284de Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 10 Dec 2024 21:48:30 +0100 Subject: [PATCH 04/26] util: Add missing types in make_secure_unique --- src/support/allocators/secure.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/support/allocators/secure.h b/src/support/allocators/secure.h index 4395567722..e7ffc9e201 100644 --- a/src/support/allocators/secure.h +++ b/src/support/allocators/secure.h @@ -1,5 +1,5 @@ // Copyright (c) 2009-2010 Satoshi Nakamoto -// Copyright (c) 2009-2021 The Bitcoin Core developers +// Copyright (c) 2009-present The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -74,7 +74,7 @@ secure_unique_ptr make_secure_unique(Args&&... as) // initialize in place, and return as secure_unique_ptr try { - return secure_unique_ptr(new (p) T(std::forward(as)...)); + return secure_unique_ptr(new (p) T(std::forward(as)...)); } catch (...) { secure_allocator().deallocate(p, 1); throw; From 589ed1a8eafe1daed379ebb383602c8f220aef19 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 10 Dec 2024 15:03:37 -0500 Subject: [PATCH 05/26] wallet: migration, avoid loading wallet after failure when it wasn't loaded before During migration failure, only load wallet back into memory when the wallet was loaded prior to migration. This fixes the case where BDB is not supported, which implies that no legacy wallet can be loaded into memory due to the lack of db writing functionality. This commit also improves migration backup related comments to better document the current workflow. Co-authored-by: Ava Chow --- src/wallet/wallet.cpp | 31 ++++++++++++++++++++--------- src/wallet/wallet.h | 2 +- test/functional/wallet_migration.py | 20 ++++++++++++++++--- 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0961aebc7c..5c597e5a26 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -490,7 +490,9 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& return wallet; } -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings) +// Re-creates wallet from the backup file by renaming and moving it into the wallet's directory. +// If 'load_after_restore=true', the wallet object will be fully initialized and appended to the context. +std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore) { DatabaseOptions options; ReadDatabaseArgs(*context.args, options); @@ -515,13 +517,17 @@ std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& b fs::copy_file(backup_file, wallet_file, fs::copy_options::none); - wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); + if (load_after_restore) { + wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings); + } } catch (const std::exception& e) { assert(!wallet); if (!error.empty()) error += Untranslated("\n"); error += Untranslated(strprintf("Unexpected exception: %s", e.what())); } - if (!wallet) { + + // Remove created wallet path only when loading fails + if (load_after_restore && !wallet) { fs::remove_all(wallet_path); } @@ -4523,7 +4529,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } if (!success) { // Migration failed, cleanup - // Copy the backup to the actual wallet dir + // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename); fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none); @@ -4560,17 +4566,24 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } // Restore the backup - DatabaseStatus status; - std::vector warnings; - if (!RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, error, warnings)) { - error += _("\nUnable to restore backup of wallet."); + // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory. + // Reload it into memory if the wallet was previously loaded. + bilingual_str restore_error; + const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded); + if (!restore_error.empty()) { + error += restore_error + _("\nUnable to restore backup of wallet."); return util::Error{error}; } - // Move the backup to the wallet dir + // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none); fs::remove(temp_backup_location); + // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null. + // This check is performed after restoration to avoid an early error before saving the backup. + bool wallet_reloaded = ptr_wallet != nullptr; + assert(was_loaded == wallet_reloaded); + return util::Error{error}; } return res; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d869f031bb..c6a45e9a14 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -95,7 +95,7 @@ std::shared_ptr GetDefaultWallet(WalletContext& context, size_t& count) std::shared_ptr GetWallet(WalletContext& context, const std::string& name); std::shared_ptr LoadWallet(WalletContext& context, const std::string& name, std::optional load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); std::shared_ptr CreateWallet(WalletContext& context, const std::string& name, std::optional load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); -std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings); +std::shared_ptr RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector& warnings, bool load_after_restore = true); std::unique_ptr HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet); void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& wallet); std::unique_ptr MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 5abb43e3b9..da816f9a84 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -894,9 +894,7 @@ def test_failed_migration_cleanup(self): shutil.copytree(self.old_node.wallets_path / "failed", self.master_node.wallets_path / "failed") assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed") - assert "failed" in self.master_node.listwallets() - assert "failed_watchonly" not in self.master_node.listwallets() - assert "failed_solvables" not in self.master_node.listwallets() + assert all(wallet not in self.master_node.listwallets() for wallet in ["failed", "failed_watchonly", "failed_solvables"]) assert not (self.master_node.wallets_path / "failed_watchonly").exists() # Since the file in failed_solvables is one that we put there, migration shouldn't touch it @@ -910,6 +908,22 @@ def test_failed_migration_cleanup(self): _, _, magic = struct.unpack("QII", data) assert_equal(magic, BTREE_MAGIC) + #################################################### + # Perform the same test with a loaded legacy wallet. + # The wallet should remain loaded after the failure. + # + # This applies only when BDB is enabled, as the user + # cannot interact with the legacy wallet database + # without BDB support. + if self.is_bdb_compiled() is not None: + # Advance time to generate a different backup name + self.master_node.setmocktime(self.master_node.getblockheader(self.master_node.getbestblockhash())['time'] + 100) + assert "failed" not in self.master_node.listwallets() + self.master_node.loadwallet("failed") + assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed") + wallets = self.master_node.listwallets() + assert "failed" in wallets and all(wallet not in wallets for wallet in ["failed_watchonly", "failed_solvables"]) + def test_blank(self): self.log.info("Test that a blank wallet is migrated") wallet = self.create_legacy_wallet("blank", blank=True) From fa9aacf614f6066ff3b5c02f0daaaeb0ebb93f33 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 6 Dec 2024 09:55:54 +0100 Subject: [PATCH 06/26] lint: Move assertion linter into lint runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On failure, this makes the output more consistent with the other linter. Each failure will be marked with an '⚠️ ' emoji and explanation, making it easier to spot. Also, add --line-number to the filesystem linter. Also, add newlines after each failing check, to visually separate different failures from each other. Can be reviewed with: "--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space" --- test/lint/lint-assertions.py | 54 ----------------------- test/lint/test_runner/src/main.rs | 71 ++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 56 deletions(-) delete mode 100755 test/lint/lint-assertions.py diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py deleted file mode 100755 index 5d01b13fd4..0000000000 --- a/test/lint/lint-assertions.py +++ /dev/null @@ -1,54 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2018-2022 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -# -# Check for assertions with obvious side effects. - -import sys -import subprocess - - -def git_grep(params: [], error_msg: ""): - try: - output = subprocess.check_output(["git", "grep", *params], text=True, encoding="utf8") - print(error_msg) - print(output) - return 1 - except subprocess.CalledProcessError as ex1: - if ex1.returncode > 1: - raise ex1 - return 0 - - -def main(): - # Aborting the whole process is undesirable for RPC code. So nonfatal - # checks should be used over assert. See: src/util/check.h - # src/rpc/server.cpp is excluded from this check since it's mostly meta-code. - exit_code = git_grep([ - "--line-number", - "--extended-regexp", - r"\<(A|a)ss(ume|ert)\(", - "--", - "src/rpc/", - "src/wallet/rpc*", - ":(exclude)src/rpc/server.cpp", - ], "CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.") - - # The `BOOST_ASSERT` macro requires to `#include boost/assert.hpp`, - # which is an unnecessary Boost dependency. - exit_code |= git_grep([ - "--line-number", - "--extended-regexp", - r"BOOST_ASSERT\(", - "--", - "*.cpp", - "*.h", - ], "BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK.") - - sys.exit(exit_code) - - -if __name__ == "__main__": - main() diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index f4fb1c36e5..ebfc3d051e 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -48,6 +48,16 @@ fn get_linter_list() -> Vec<&'static Linter> { name: "std_filesystem", lint_fn: lint_std_filesystem }, + &Linter { + description: "Check that fatal assertions are not used in RPC code", + name: "rpc_assert", + lint_fn: lint_rpc_assert + }, + &Linter { + description: "Check that boost assertions are not used", + name: "boost_assert", + lint_fn: lint_boost_assert + }, &Linter { description: "Check that release note snippets are in the right folder", name: "doc_release_note_snippets", @@ -237,7 +247,7 @@ fn lint_py_lint() -> LintResult { "F822", // undefined name name in __all__ "F823", // local variable name … referenced before assignment "F841", // local variable 'foo' is assigned to but never used - "PLE", // Pylint errors + "PLE", // Pylint errors "W191", // indentation contains tabs "W291", // trailing whitespace "W292", // no newline at end of file @@ -273,6 +283,7 @@ fn lint_std_filesystem() -> LintResult { let found = git() .args([ "grep", + "--line-number", "std::filesystem", "--", "./src/", @@ -293,6 +304,62 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted. } } +fn lint_rpc_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"\<(A|a)ss(ume|ert)\(", + "--", + "src/rpc/", + "src/wallet/rpc*", + ":(exclude)src/rpc/server.cpp", + // src/rpc/server.cpp is excluded from this check since it's mostly meta-code. + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +^^^ +CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code. + +Aborting the whole process is undesirable for RPC code. So nonfatal +checks should be used over assert. See: src/util/check.h + "# + .to_string()) + } else { + Ok(()) + } +} + +fn lint_boost_assert() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "--extended-regexp", + r"BOOST_ASSERT\(", + "--", + "*.cpp", + "*.h", + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +^^^ +BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary +include of the boost/assert.hpp dependency. + "# + .to_string()) + } else { + Ok(()) + } +} + fn lint_doc_release_note_snippets() -> LintResult { let non_release_notes = check_output(git().args([ "ls-files", @@ -593,7 +660,7 @@ fn main() -> ExitCode { "{err}\n^---- ⚠️ Failure generated from lint check '{}'!", linter.name ); - println!("{}", linter.description); + println!("{}\n\n", linter.description); test_failed = true; } } From e8f0e6efaf555a7d17bdb118464bd572bd5f3933 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Thu, 12 Dec 2024 19:47:19 +0100 Subject: [PATCH 07/26] lint: output-only - Avoid repeated arrows, trim - No empty line separating errors and arrows ("^^^"). Keeping them together signals they are related. - No empty line separating error message and linter failure line (not completely empty, it contains several spaces left over from Rust multi-line literal). - Keep the linter description on the same line as the failure line, otherwise it looks like it's a description for the following step. --- test/lint/test_runner/src/main.rs | 37 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index ebfc3d051e..8479fd2d64 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -294,10 +294,10 @@ fn lint_std_filesystem() -> LintResult { .success(); if found { Err(r#" -^^^ Direct use of std::filesystem may be dangerous and buggy. Please include and use the fs:: namespace, which has unsafe filesystem functions marked as deleted. "# + .trim() .to_string()) } else { Ok(()) @@ -322,12 +322,12 @@ fn lint_rpc_assert() -> LintResult { .success(); if found { Err(r#" -^^^ CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code. Aborting the whole process is undesirable for RPC code. So nonfatal checks should be used over assert. See: src/util/check.h "# + .trim() .to_string()) } else { Ok(()) @@ -350,10 +350,10 @@ fn lint_boost_assert() -> LintResult { .success(); if found { Err(r#" -^^^ BOOST_ASSERT must be replaced with Assert, BOOST_REQUIRE, or BOOST_CHECK to avoid an unnecessary include of the boost/assert.hpp dependency. "# + .trim() .to_string()) } else { Ok(()) @@ -370,17 +370,15 @@ fn lint_doc_release_note_snippets() -> LintResult { if non_release_notes.is_empty() { Ok(()) } else { - Err(format!( - r#" -{} -^^^ + println!("{non_release_notes}"); + Err(r#" Release note snippets and other docs must be put into the doc/ folder directly. The doc/release-notes/ folder is for archived release notes of previous releases only. Snippets are expected to follow the naming "/doc/release-notes-.md". - "#, - non_release_notes - )) + "# + .trim() + .to_string()) } } @@ -423,7 +421,6 @@ fn lint_trailing_whitespace() -> LintResult { .success(); if trailing_space { Err(r#" -^^^ Trailing whitespace (including Windows line endings [CR LF]) is problematic, because git may warn about it, or editors may remove it by default, forcing developers in the future to either undo the changes manually or spend time on review. @@ -433,6 +430,7 @@ Thus, it is best to remove the trailing space now. Please add any false positives, such as subtrees, Windows-related files, patch files, or externally sourced files to the exclude list. "# + .trim() .to_string()) } else { Ok(()) @@ -449,7 +447,6 @@ fn lint_tabs_whitespace() -> LintResult { .success(); if tabs { Err(r#" -^^^ Use of tabs in this codebase is problematic, because existing code uses spaces and tabs will cause display issues and conflict with editor settings. @@ -457,6 +454,7 @@ Please remove the tabs. Please add any false positives, such as subtrees, or externally sourced files to the exclude list. "# + .trim() .to_string()) } else { Ok(()) @@ -531,7 +529,6 @@ fn lint_includes_build_config() -> LintResult { if missing { return Err(format!( r#" -^^^ One or more files use a symbol declared in the bitcoin-build-config.h header. However, they are not including the header. This is problematic, because the header may or may not be indirectly included. If the indirect include were to be intentionally or accidentally removed, the build could @@ -547,12 +544,13 @@ include again. #include // IWYU pragma: keep "#, defines_regex - )); + ) + .trim() + .to_string()); } let redundant = print_affected_files(false); if redundant { return Err(r#" -^^^ None of the files use a symbol declared in the bitcoin-build-config.h header. However, they are including the header. Consider removing the unused include. "# @@ -605,7 +603,9 @@ Markdown link errors found: {} "#, stderr - )) + ) + .trim() + .to_string()) } Err(e) if e.kind() == ErrorKind::NotFound => { println!("`mlc` was not found in $PATH, skipping markdown lint check."); @@ -657,10 +657,9 @@ fn main() -> ExitCode { env::set_current_dir(&git_root).unwrap(); if let Err(err) = (linter.lint_fn)() { println!( - "{err}\n^---- ⚠️ Failure generated from lint check '{}'!", - linter.name + "^^^\n{err}\n^---- ⚠️ Failure generated from lint check '{}' ({})!\n\n", + linter.name, linter.description, ); - println!("{}\n\n", linter.description); test_failed = true; } } From 0a76c292ac8fa29166a7ec6efda1fafce86af0d3 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:09:49 +0000 Subject: [PATCH 08/26] doc: Install `net/py-pyzmq` port on FreeBSD for `interface_zmq.py` --- doc/build-freebsd.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/build-freebsd.md b/doc/build-freebsd.md index 2456abd57b..694224621e 100644 --- a/doc/build-freebsd.md +++ b/doc/build-freebsd.md @@ -96,7 +96,7 @@ There is an included test suite that is useful for testing code changes when dev To run the test suite (recommended), you will need to have Python 3 installed: ```bash -pkg install python3 databases/py-sqlite3 +pkg install python3 databases/py-sqlite3 net/py-pyzmq ``` --- From b0b8d96d93ea4971e7941ddca03f4393deaac293 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sat, 21 Dec 2024 17:39:19 +0000 Subject: [PATCH 09/26] depends: Update capnproto to 1.1.0 This change fixes compilation on NetBSD with GCC 14. --- depends/packages/native_capnp.mk | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/depends/packages/native_capnp.mk b/depends/packages/native_capnp.mk index d6e6b4cadb..e67b103716 100644 --- a/depends/packages/native_capnp.mk +++ b/depends/packages/native_capnp.mk @@ -1,9 +1,9 @@ package=native_capnp -$(package)_version=1.0.2 +$(package)_version=1.1.0 $(package)_download_path=https://capnproto.org/ $(package)_download_file=capnproto-c++-$($(package)_version).tar.gz $(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz -$(package)_sha256_hash=9057dbc0223366b74bbeca33a05de164a229b0377927f1b7ef3828cdd8cb1d7e +$(package)_sha256_hash=07167580e563f5e821e3b2af1c238c16ec7181612650c5901330fa9a0da50939 define $(package)_set_vars $(package)_config_opts := -DBUILD_TESTING=OFF From 34e8ee23b83eeecb1c4022b48e7b8db45a60ffdd Mon Sep 17 00:00:00 2001 From: Boris Nagaev Date: Tue, 31 Dec 2024 00:04:20 -0300 Subject: [PATCH 10/26] txmempool: fix typos in comments --- src/txmempool.cpp | 2 +- src/txmempool.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 950004044e..3a5a3fb306 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -442,7 +442,7 @@ void CTxMemPool::Apply(ChangeSet* changeset) std::optional ancestors; if (i == 0) { // Note: ChangeSet::CalculateMemPoolAncestors() will return a - // cached value if mempool ancestors for this tranaction were + // cached value if mempool ancestors for this transaction were // previously calculated. // We can only use a cached ancestor calculation for the first // transaction in a package, because in-package parents won't be diff --git a/src/txmempool.h b/src/txmempool.h index e505c87f09..10acb2aa22 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -810,7 +810,7 @@ class CTxMemPool * mempool. * * CalculateMemPoolAncestors() calculates the in-mempool (not including - * what is in the change set itself) ancestors of a given transacion. + * what is in the change set itself) ancestors of a given transaction. * * Apply() will apply the removals and additions that are staged into the * mempool. From 2bdaf52ed1259fd3bec22b680e12563fcee0a8b3 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 31 Dec 2024 10:14:02 +0000 Subject: [PATCH 11/26] doc: Update NetBSD Build Guide 1. Update the documented NetBSD version. 2. Add the optional ZeroMQ package to align the guide with other *BSD systems. 3. Update the Python version to meet the minimum requirement specified in https://github.com/bitcoin/bitcoin/pull/30527. 4. Install `net/py-zmq` package to enable the `interface_zmq.py` functional test. 5. Fix a formatting issue. --- doc/build-netbsd.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/doc/build-netbsd.md b/doc/build-netbsd.md index 63bfbd61db..988f3b93a7 100644 --- a/doc/build-netbsd.md +++ b/doc/build-netbsd.md @@ -1,6 +1,6 @@ # NetBSD Build Guide -**Updated for NetBSD [10.0](https://netbsd.org/releases/formal-10/NetBSD-10.0.html)** +**Updated for NetBSD [10.1](https://netbsd.org/releases/formal-10/NetBSD-10.1.html)** This guide describes how to build bitcoind, command-line utilities, and GUI on NetBSD. @@ -83,6 +83,13 @@ pkgin install qrencode Otherwise, if you don't need QR encoding support, use the `-DWITH_QRENCODE=OFF` option to disable this feature in order to compile the GUI. +#### Notifications +###### ZeroMQ + +Bitcoin Core can provide notifications via ZeroMQ. If the package is installed, support will be compiled in. +```bash +pkgin zeromq +``` #### Test Suite Dependencies @@ -90,10 +97,10 @@ There is an included test suite that is useful for testing code changes when dev To run the test suite (recommended), you will need to have Python 3 installed: ```bash -pkgin install python39 +pkgin install python310 py310-zmq ``` -### Building Bitcoin Core +## Building Bitcoin Core ### 1. Configuration From fa0411ee305fe04800c54d141fbbeac342eaa764 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 2 Jan 2025 13:59:04 +0100 Subject: [PATCH 12/26] ci: Run functional tests in msan task --- ci/test/00_setup_env_native_msan.sh | 1 - ci/test/01_base_install.sh | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/ci/test/00_setup_env_native_msan.sh b/ci/test/00_setup_env_native_msan.sh index c6b3d68be6..cd4ac99942 100755 --- a/ci/test/00_setup_env_native_msan.sh +++ b/ci/test/00_setup_env_native_msan.sh @@ -27,4 +27,3 @@ export BITCOIN_CONFIG="\ -DAPPEND_CPPFLAGS='-U_FORTIFY_SOURCE' \ " export USE_MEMORY_SANITIZER="true" -export RUN_FUNCTIONAL_TESTS="false" diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh index e026b919e3..0130e820c2 100755 --- a/ci/test/01_base_install.sh +++ b/ci/test/01_base_install.sh @@ -49,7 +49,7 @@ if [ -n "$PIP_PACKAGES" ]; then fi if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then - ${CI_RETRY_EXE} git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-19.1.0" /msan/llvm-project + ${CI_RETRY_EXE} git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-19.1.6" /msan/llvm-project cmake -G Ninja -B /msan/clang_build/ \ -DLLVM_ENABLE_PROJECTS="clang" \ From 04249682e381f976de6ba56bb4fb2996dfa194ab Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 3 Jan 2025 11:48:02 +0100 Subject: [PATCH 13/26] test: use Mining interface in miner_tests --- src/test/miner_tests.cpp | 195 +++++++++++++++++++++++---------------- 1 file changed, 118 insertions(+), 77 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 8a264f3fd3..d48f9cca3f 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -28,8 +29,9 @@ #include using namespace util::hex_literals; +using interfaces::BlockTemplate; +using interfaces::Mining; using node::BlockAssembler; -using node::CBlockTemplate; namespace miner_tests { struct MinerTestingSetup : public TestingSetup { @@ -54,7 +56,10 @@ struct MinerTestingSetup : public TestingSetup { Assert(error.empty()); return *m_node.mempool; } - BlockAssembler AssemblerForTest(CTxMemPool& tx_mempool, BlockAssembler::Options options); + std::unique_ptr MakeMining() + { + return interfaces::MakeMining(m_node); + } }; } // namespace miner_tests @@ -62,13 +67,6 @@ BOOST_FIXTURE_TEST_SUITE(miner_tests, MinerTestingSetup) static CFeeRate blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE); -BlockAssembler MinerTestingSetup::AssemblerForTest(CTxMemPool& tx_mempool, BlockAssembler::Options options) -{ - options.nBlockMaxWeight = MAX_BLOCK_WEIGHT; - options.blockMinFeeRate = blockMinFeeRate; - return BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}; -} - constexpr static struct { unsigned char extranonce; unsigned int nonce; @@ -106,6 +104,10 @@ static std::unique_ptr CreateBlockIndex(int nHeight, CBlockIndex* a void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const std::vector& txFirst) { CTxMemPool& tx_mempool{MakeMempool()}; + auto mining{MakeMining()}; + BlockAssembler::Options options; + options.coinbase_output_script = scriptPubKey; + LOCK(tx_mempool.cs); // Test the ancestor feerate transaction selection. TestMemPoolEntryHelper entry; @@ -135,13 +137,13 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const Txid hashHighFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - BlockAssembler::Options options; - options.coinbase_output_script = scriptPubKey; - std::unique_ptr pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4U); - BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashParentTx); - BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashHighFeeTx); - BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashMediumFeeTx); + std::unique_ptr block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; + BOOST_REQUIRE_EQUAL(block.vtx.size(), 4U); + BOOST_CHECK(block.vtx[1]->GetHash() == hashParentTx); + BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx); + BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx); // Test that a package below the block min tx fee doesn't get included tx.vin[0].prevout.hash = hashHighFeeTx; @@ -158,11 +160,13 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 1000 - 50000 - feeToUse; Txid hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); // Verify that the free tx and the low fee tx didn't get selected - for (size_t i=0; iblock.vtx.size(); ++i) { - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeTx); - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashLowFeeTx); + for (size_t i=0; iGetHash() != hashFreeTx); + BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx); } // Test that packages above the min relay fee do get included, even if one @@ -172,10 +176,12 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse + 2).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U); - BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashFreeTx); - BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashLowFeeTx); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U); + BOOST_CHECK(block.vtx[4]->GetHash() == hashFreeTx); + BOOST_CHECK(block.vtx[5]->GetHash() == hashLowFeeTx); // Test that transaction selection properly updates ancestor fee // calculations as ancestor transactions get included in a block. @@ -194,12 +200,14 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 100000000 - feeToUse; Txid hashLowFeeTx2 = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse).SpendsCoinbase(false).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); // Verify that this tx isn't selected. - for (size_t i=0; iblock.vtx.size(); ++i) { - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashFreeTx2); - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashLowFeeTx2); + for (size_t i=0; iGetHash() != hashFreeTx2); + BOOST_CHECK(block.vtx[i]->GetHash() != hashLowFeeTx2); } // This tx will be mineable, and should cause hashLowFeeTx2 to be selected @@ -207,9 +215,11 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vin[0].prevout.n = 1; tx.vout[0].nValue = 100000000 - 10000; // 10k satoshi fee AddToMempool(tx_mempool, entry.Fee(10000).FromTx(tx)); - pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 9U); - BOOST_CHECK(pblocktemplate->block.vtx[8]->GetHash() == hashLowFeeTx2); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_REQUIRE_EQUAL(block.vtx.size(), 9U); + BOOST_CHECK(block.vtx[8]->GetHash() == hashLowFeeTx2); } void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::vector& txFirst, int baseheight) @@ -225,6 +235,9 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: const CAmount HIGHFEE = COIN; const CAmount HIGHERFEE = 4 * COIN; + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; @@ -233,8 +246,9 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: LOCK(tx_mempool.cs); // Just to make sure we can still make simple blocks - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_CHECK(pblocktemplate); + auto block_template{mining->createNewBlock(options)}; + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; // block sigops > limit: 1000 CHECKMULTISIG + 1 tx.vin.resize(1); @@ -253,7 +267,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vin[0].prevout.hash = hash; } - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-blk-sigops")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-blk-sigops")); } { @@ -270,7 +284,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(spendsCoinbase).SigOpsCost(80).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); } { @@ -294,7 +308,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); } { @@ -304,7 +318,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: // orphan in tx_mempool, template creation fails hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); } { @@ -325,7 +339,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue = tx.vout[0].nValue + BLOCKSUBSIDY - HIGHERFEE; // First txn output + fresh coinbase - new txn fee hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(HIGHERFEE).Time(Now()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); } { @@ -341,7 +355,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: // give it a fee so it'll get mined AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); // Should throw bad-cb-multiple - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-cb-multiple")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-cb-multiple")); } { @@ -358,7 +372,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(HIGHFEE).Time(Now()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("bad-txns-inputs-missingorspent")); } { @@ -378,7 +392,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: next->BuildSkip(); m_node.chainman->ActiveChain().SetTip(*next); } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); // Extend to a 210000-long block chain. while (m_node.chainman->ActiveChain().Tip()->nHeight < 210000) { CBlockIndex* prev = m_node.chainman->ActiveChain().Tip(); @@ -390,7 +404,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: next->BuildSkip(); m_node.chainman->ActiveChain().SetTip(*next); } - BOOST_CHECK(AssemblerForTest(tx_mempool, options).CreateNewBlock()); + BOOST_REQUIRE(mining->createNewBlock(options)); // invalid p2sh txn in tx_mempool, template creation fails tx.vin[0].prevout.hash = txFirst[0]->GetHash(); @@ -406,7 +420,7 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vout[0].nValue -= LOWFEE; hash = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(LOWFEE).Time(Now()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK_EXCEPTION(AssemblerForTest(tx_mempool, options).CreateNewBlock(), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); + BOOST_CHECK_EXCEPTION(mining->createNewBlock(options), std::runtime_error, HasReason("mandatory-script-verify-flag-failed")); // Delete the dummy blocks again. while (m_node.chainman->ActiveChain().Tip()->nHeight > nHeight) { @@ -508,14 +522,15 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: tx.vin[0].nSequence = CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG | 1; BOOST_CHECK(!TestSequenceLocks(CTransaction{tx}, tx_mempool)); // Sequence locks fail - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_CHECK(pblocktemplate); + auto block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); // None of the of the absolute height/time locked tx should have made // it into the template because we still check IsFinalTx in CreateNewBlock, // but relative locked txs will if inconsistently added to mempool. // For now these will still generate a valid template until BIP68 soft fork - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U); + CBlock block{block_template->getBlock()}; + BOOST_CHECK_EQUAL(block.vtx.size(), 3U); // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) { CBlockIndex* ancestor{Assert(m_node.chainman->ActiveChain().Tip()->GetAncestor(m_node.chainman->ActiveChain().Tip()->nHeight - i))}; @@ -524,12 +539,17 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std:: m_node.chainman->ActiveChain().Tip()->nHeight++; SetMockTime(m_node.chainman->ActiveChain().Tip()->GetMedianTimePast() + 1); - BOOST_CHECK(pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock()); - BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 5U); + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + block = block_template->getBlock(); + BOOST_CHECK_EQUAL(block.vtx.size(), 5U); } void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const std::vector& txFirst) { + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; @@ -594,34 +614,34 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const Txid hashFreeGrandchild = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(0).SpendsCoinbase(false).FromTx(tx)); - auto pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock(); - BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 6U); - BOOST_CHECK(pblocktemplate->block.vtx[1]->GetHash() == hashFreeParent); - BOOST_CHECK(pblocktemplate->block.vtx[2]->GetHash() == hashFreePrioritisedTx); - BOOST_CHECK(pblocktemplate->block.vtx[3]->GetHash() == hashParentTx); - BOOST_CHECK(pblocktemplate->block.vtx[4]->GetHash() == hashPrioritsedChild); - BOOST_CHECK(pblocktemplate->block.vtx[5]->GetHash() == hashFreeChild); - for (size_t i=0; iblock.vtx.size(); ++i) { + auto block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + CBlock block{block_template->getBlock()}; + BOOST_REQUIRE_EQUAL(block.vtx.size(), 6U); + BOOST_CHECK(block.vtx[1]->GetHash() == hashFreeParent); + BOOST_CHECK(block.vtx[2]->GetHash() == hashFreePrioritisedTx); + BOOST_CHECK(block.vtx[3]->GetHash() == hashParentTx); + BOOST_CHECK(block.vtx[4]->GetHash() == hashPrioritsedChild); + BOOST_CHECK(block.vtx[5]->GetHash() == hashFreeChild); + for (size_t i=0; iblock.vtx[i]->GetHash() != hashFreeGrandchild); + BOOST_CHECK(block.vtx[i]->GetHash() != hashFreeGrandchild); // De-prioritised transaction should not be included. - BOOST_CHECK(pblocktemplate->block.vtx[i]->GetHash() != hashMediumFeeTx); + BOOST_CHECK(block.vtx[i]->GetHash() != hashMediumFeeTx); } } // NOTE: These tests rely on CreateNewBlock doing its own self-validation! BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { + auto mining{MakeMining()}; + BOOST_REQUIRE(mining); + // Note that by default, these tests run with size accounting enabled. CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; - std::unique_ptr pblocktemplate; - BlockAssembler::Options options; options.coinbase_output_script = scriptPubKey; - - CTxMemPool& tx_mempool{*m_node.mempool}; - // Simple block creation, nothing special yet: - BOOST_CHECK(pblocktemplate = AssemblerForTest(tx_mempool, options).CreateNewBlock()); + std::unique_ptr block_template; // We can't make transactions until we have inputs // Therefore, load 110 blocks :) @@ -629,27 +649,48 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) int baseheight = 0; std::vector txFirst; for (const auto& bi : BLOCKINFO) { - CBlock *pblock = &pblocktemplate->block; // pointer for convenience + const int current_height{mining->getTip()->height}; + + // Simple block creation, nothing special yet: + block_template = mining->createNewBlock(options); + BOOST_REQUIRE(block_template); + + CBlock block{block_template->getBlock()}; + CMutableTransaction txCoinbase(*block.vtx[0]); { LOCK(cs_main); - pblock->nVersion = VERSIONBITS_TOP_BITS; - pblock->nTime = m_node.chainman->ActiveChain().Tip()->GetMedianTimePast()+1; - CMutableTransaction txCoinbase(*pblock->vtx[0]); + block.nVersion = VERSIONBITS_TOP_BITS; + block.nTime = Assert(m_node.chainman)->ActiveChain().Tip()->GetMedianTimePast()+1; txCoinbase.version = 1; - txCoinbase.vin[0].scriptSig = CScript{} << (m_node.chainman->ActiveChain().Height() + 1) << bi.extranonce; + txCoinbase.vin[0].scriptSig = CScript{} << (current_height + 1) << bi.extranonce; txCoinbase.vout.resize(1); // Ignore the (optional) segwit commitment added by CreateNewBlock (as the hardcoded nonces don't account for this) txCoinbase.vout[0].scriptPubKey = CScript(); - pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); + block.vtx[0] = MakeTransactionRef(txCoinbase); if (txFirst.size() == 0) - baseheight = m_node.chainman->ActiveChain().Height(); + baseheight = current_height; if (txFirst.size() < 4) - txFirst.push_back(pblock->vtx[0]); - pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); - pblock->nNonce = bi.nonce; + txFirst.push_back(block.vtx[0]); + block.hashMerkleRoot = BlockMerkleRoot(block); + block.nNonce = bi.nonce; + } + std::shared_ptr shared_pblock = std::make_shared(block); + // Alternate calls between Chainman's ProcessNewBlock and submitSolution + // via the Mining interface. The former is used by net_processing as well + // as the submitblock RPC. + if (current_height % 2 == 0) { + BOOST_REQUIRE(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr)); + } else { + BOOST_REQUIRE(block_template->submitSolution(block.nVersion, block.nTime, block.nNonce, MakeTransactionRef(txCoinbase))); + } + { + LOCK(cs_main); + // The above calls don't guarantee the tip is actually updated, so + // we explictly check this. + auto maybe_new_tip{Assert(m_node.chainman)->ActiveChain().Tip()}; + BOOST_REQUIRE_EQUAL(maybe_new_tip->GetBlockHash(), block.GetHash()); } - std::shared_ptr shared_pblock = std::make_shared(*pblock); - BOOST_CHECK(Assert(m_node.chainman)->ProcessNewBlock(shared_pblock, true, true, nullptr)); - pblock->hashPrevBlock = pblock->GetHash(); + // This just adds coverage + mining->waitTipChanged(block.hashPrevBlock); } LOCK(cs_main); From 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 2 Jan 2025 18:31:16 -0500 Subject: [PATCH 14/26] doc: Clarify comments about endianness after #30526 This is a documentation-only change following up on suggestions made in the #30526 review. Motivation for this change is that I was recently reviewing #31583, which reminded me how confusing the arithmetic blob code was and made me want to write better comments. --- src/arith_uint256.h | 1 + src/policy/packages.h | 5 +++-- src/rpc/mining.cpp | 4 ++-- src/uint256.h | 29 ++++++++++++++++++++--------- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/arith_uint256.h b/src/arith_uint256.h index 38b7453034..60b371f6d3 100644 --- a/src/arith_uint256.h +++ b/src/arith_uint256.h @@ -26,6 +26,7 @@ class base_uint protected: static_assert(BITS / 32 > 0 && BITS % 32 == 0, "Template parameter BITS must be a positive multiple of 32."); static constexpr int WIDTH = BITS / 32; + /** Big integer represented with 32-bit digits, least-significant first. */ uint32_t pn[WIDTH]; public: diff --git a/src/policy/packages.h b/src/policy/packages.h index 3050320122..4b2350edac 100644 --- a/src/policy/packages.h +++ b/src/policy/packages.h @@ -89,8 +89,9 @@ bool IsChildWithParents(const Package& package); */ bool IsChildWithParentsTree(const Package& package); -/** Get the hash of these transactions' wtxids, concatenated in lexicographical order (treating the - * wtxids as little endian encoded uint256, smallest to largest). */ +/** Get the hash of the concatenated wtxids of transactions, with wtxids + * treated as a little-endian numbers and sorted in ascending numeric order. + */ uint256 GetPackageHash(const std::vector& transactions); #endif // BITCOIN_POLICY_PACKAGES_H diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index e230281240..7e5936fddf 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -633,8 +633,8 @@ static RPCHelpMan getblocktemplate() {RPCResult::Type::OBJ, "", "", { {RPCResult::Type::STR_HEX, "data", "transaction data encoded in hexadecimal (byte-for-byte)"}, - {RPCResult::Type::STR_HEX, "txid", "transaction id encoded in little-endian hexadecimal"}, - {RPCResult::Type::STR_HEX, "hash", "hash encoded in little-endian hexadecimal (including witness data)"}, + {RPCResult::Type::STR_HEX, "txid", "transaction hash excluding witness data, shown in byte-reversed hex"}, + {RPCResult::Type::STR_HEX, "hash", "transaction hash including witness data, shown in byte-reversed hex"}, {RPCResult::Type::ARR, "depends", "array of numbers", { {RPCResult::Type::NUM, "", "transactions before this one (by 1-based index in 'transactions' list) that must be present in the final block if this one is"}, diff --git a/src/uint256.h b/src/uint256.h index 8223787041..708f0c2ff1 100644 --- a/src/uint256.h +++ b/src/uint256.h @@ -69,16 +69,27 @@ class base_blob /** @name Hex representation * - * The reverse-byte hex representation is a convenient way to view the blob - * as a number, because it is consistent with the way the base_uint class - * converts blobs to numbers. + * The hex representation used by GetHex(), ToString(), FromHex() and + * SetHexDeprecated() is unusual, since it shows bytes of the base_blob in + * reverse order. For example, a 4-byte blob {0x12, 0x34, 0x56, 0x78} is + * represented as "78563412" instead of the more typical "12345678" + * representation that would be shown in a hex editor or used by typical + * byte-array / hex conversion functions like python's bytes.hex() and + * bytes.fromhex(). + * + * The nice thing about the reverse-byte representation, even though it is + * unusual, is that if a blob contains an arithmetic number in little endian + * format (with least significant bytes first, and most significant bytes + * last), the GetHex() output will match the way the number would normally + * be written in base-16 (with most significant digits first and least + * significant digits last). + * + * This means, for example, that ArithToUint256(num).GetHex() can be used to + * display an arith_uint256 num value as a number, because + * ArithToUint256() converts the number to a blob in little-endian format, + * so the arith_uint256 class doesn't need to have its own number parsing + * and formatting functions. * - * @note base_uint treats the blob as an array of bytes with the numerically - * least significant byte first and the most significant byte last. Because - * numbers are typically written with the most significant digit first and - * the least significant digit last, the reverse hex display of the blob - * corresponds to the same numeric value that base_uint interprets from the - * blob. * @{*/ std::string GetHex() const; /** Unlike FromHex this accepts any invalid input, thus it is fragile and deprecated! From 29bca9713d21916b315c2ca0e9183bf567f39351 Mon Sep 17 00:00:00 2001 From: epysqyli <49974367+epysqyli@users.noreply.github.com> Date: Sat, 4 Jan 2025 22:50:29 +0100 Subject: [PATCH 15/26] test: fix typo in mempool_ephemeral_dust --- test/functional/mempool_ephemeral_dust.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/mempool_ephemeral_dust.py b/test/functional/mempool_ephemeral_dust.py index 10cacd9539..e614a9e607 100755 --- a/test/functional/mempool_ephemeral_dust.py +++ b/test/functional/mempool_ephemeral_dust.py @@ -126,7 +126,7 @@ def test_node_restart(self): assert_equal(len(self.nodes[0].getrawmempool()), 2) assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]]) - # Node restart; doesn't allow allow ephemeral transaction back in due to individual submission + # Node restart; doesn't allow ephemeral transaction back in due to individual submission # resulting in 0-fee. Supporting re-submission of CPFP packages on restart is desired but not # yet implemented. self.restart_node(0) From fa029a78780fd00e1d3ce1bebb81a95857bfbb94 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Mon, 6 Jan 2025 10:16:50 +0100 Subject: [PATCH 16/26] doc: Clarify min macOS and Xcode version --- .github/workflows/ci.yml | 6 +++++- doc/release-notes-empty-template.md | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 740d31ae56..df1d12e86c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -76,7 +76,7 @@ jobs: macos-native-arm64: name: ${{ matrix.job-name }} - # Use latest image, but hardcode version to avoid silent upgrades (and breaks). + # Use any image to support the xcode-select below, but hardcode version to avoid silent upgrades (and breaks). # See: https://github.com/actions/runner-images#available-images. runs-on: macos-14 @@ -111,6 +111,10 @@ jobs: - name: Clang version run: | + # Use the earliest Xcode supported by the version of macOS denoted in + # doc/release-notes-empty-template.md and providing at least the + # minimum clang version denoted in doc/dependencies.md. + # See: https://developer.apple.com/documentation/xcode-release-notes/xcode-15-release-notes sudo xcode-select --switch /Applications/Xcode_15.0.app clang --version diff --git a/doc/release-notes-empty-template.md b/doc/release-notes-empty-template.md index bd4600b395..5853fc60f3 100644 --- a/doc/release-notes-empty-template.md +++ b/doc/release-notes-empty-template.md @@ -43,7 +43,7 @@ Compatibility ============== Bitcoin Core is supported and tested on operating systems using the -Linux Kernel 3.17+, macOS 13.0+, and Windows 10 and newer. Bitcoin +Linux Kernel 3.17+, macOS 13+, and Windows 10+. 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. From b537a2c02a9921235d1ecf8c3c7dc1836ec68131 Mon Sep 17 00:00:00 2001 From: Kay Date: Mon, 6 Jan 2025 12:23:11 +0000 Subject: [PATCH 17/26] doc: upgrade license to 2025. --- CMakeLists.txt | 2 +- COPYING | 4 ++-- contrib/debian/copyright | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2dba6f255d..e323686601 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,7 +22,7 @@ set(CLIENT_VERSION_MINOR 99) set(CLIENT_VERSION_BUILD 0) set(CLIENT_VERSION_RC 0) set(CLIENT_VERSION_IS_RELEASE "false") -set(COPYRIGHT_YEAR "2024") +set(COPYRIGHT_YEAR "2025") # During the enabling of the CXX and CXXOBJ languages, we modify # CMake's compiler/linker invocation strings by appending the content diff --git a/COPYING b/COPYING index e6d6e9fe57..23dc5e905b 100644 --- a/COPYING +++ b/COPYING @@ -1,7 +1,7 @@ The MIT License (MIT) -Copyright (c) 2009-2024 The Bitcoin Core developers -Copyright (c) 2009-2024 Bitcoin Developers +Copyright (c) 2009-2025 The Bitcoin Core developers +Copyright (c) 2009-2025 Bitcoin Developers Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal diff --git a/contrib/debian/copyright b/contrib/debian/copyright index dafc92f8ad..af8a1bc0c1 100644 --- a/contrib/debian/copyright +++ b/contrib/debian/copyright @@ -5,7 +5,7 @@ Upstream-Contact: Satoshi Nakamoto Source: https://github.com/bitcoin/bitcoin Files: * -Copyright: 2009-2024, Bitcoin Core Developers +Copyright: 2009-2025, Bitcoin Core Developers License: Expat Comment: The Bitcoin Core Developers encompasses all contributors to the project, listed in the release notes or the git log. From ff21870e20b2391b684cc50fdd6879805055d6a1 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Wed, 18 Dec 2024 14:44:23 +0000 Subject: [PATCH 18/26] fuzz: Add SetMockTime() to necessary targets --- src/test/fuzz/fuzz.cpp | 3 +++ src/test/fuzz/load_external_block_file.cpp | 2 ++ src/test/fuzz/mini_miner.cpp | 7 +++++++ src/test/fuzz/net.cpp | 2 ++ src/test/fuzz/p2p_headers_presync.cpp | 5 +++-- src/test/fuzz/partially_downloaded_block.cpp | 2 ++ src/test/fuzz/socks5.cpp | 2 ++ src/test/fuzz/txdownloadman.cpp | 3 +++ src/test/fuzz/utxo_snapshot.cpp | 2 ++ src/test/fuzz/utxo_total_supply.cpp | 9 +++++++-- src/test/util/setup_common.cpp | 4 +++- src/wallet/test/fuzz/notifications.cpp | 2 ++ src/wallet/test/fuzz/scriptpubkeyman.cpp | 2 ++ src/wallet/test/fuzz/spend.cpp | 2 ++ 14 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp index e4e4723c74..5b1d0cda7a 100644 --- a/src/test/fuzz/fuzz.cpp +++ b/src/test/fuzz/fuzz.cpp @@ -116,6 +116,9 @@ void initialize() // - Creating a BasicTestingSetup or derived class will switch to a random seed. SeedRandomStateForTest(SeedRand::ZEROS); + // Set time to the genesis block timestamp for deterministic initialization. + SetMockTime(1231006505); + // Terminate immediately if a fuzzing harness ever tries to create a socket. // Individual tests can override this by pointing CreateSock to a mocked alternative. CreateSock = [](int, int, int) -> std::unique_ptr { std::terminate(); }; diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp index 6460261f0f..b5293dd11a 100644 --- a/src/test/fuzz/load_external_block_file.cpp +++ b/src/test/fuzz/load_external_block_file.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -27,6 +28,7 @@ void initialize_load_external_block_file() FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_file) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); FuzzedFileProvider fuzzed_file_provider{fuzzed_data_provider}; AutoFile fuzzed_block_file{fuzzed_file_provider.open()}; if (fuzzed_block_file.IsNull()) { diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp index a5bccd103d..817bc0f18e 100644 --- a/src/test/fuzz/mini_miner.cpp +++ b/src/test/fuzz/mini_miner.cpp @@ -1,3 +1,7 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + #include #include #include @@ -13,6 +17,7 @@ #include #include #include +#include #include #include @@ -36,6 +41,7 @@ FUZZ_TARGET(mini_miner, .init = initialize_miner) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); bilingual_str error; CTxMemPool pool{CTxMemPool::Options{}, error}; Assert(error.empty()); @@ -115,6 +121,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); bilingual_str error; CTxMemPool pool{CTxMemPool::Options{}, error}; Assert(error.empty()); diff --git a/src/test/fuzz/net.cpp b/src/test/fuzz/net.cpp index 4cec96274e..1a0de7aa36 100644 --- a/src/test/fuzz/net.cpp +++ b/src/test/fuzz/net.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -79,6 +80,7 @@ FUZZ_TARGET(net, .init = initialize_net) FUZZ_TARGET(local_address, .init = initialize_net) { FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider)); CService service{ConsumeService(fuzzed_data_provider)}; CNode node{ConsumeNode(fuzzed_data_provider)}; { diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp index 873eb2b1cc..ed7041ad1f 100644 --- a/src/test/fuzz/p2p_headers_presync.cpp +++ b/src/test/fuzz/p2p_headers_presync.cpp @@ -154,14 +154,15 @@ void initialize() FUZZ_TARGET(p2p_headers_presync, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); + ChainstateManager& chainman = *g_testing_setup->m_node.chainman; LOCK(NetEventsInterface::g_msgproc_mutex); g_testing_setup->ResetAndInitialize(); - FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; - CBlockHeader base{chainman.GetParams().GenesisBlock()}; SetMockTime(base.nTime); diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index 8a42807be8..82d781cd53 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -46,6 +47,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); auto block{ConsumeDeserializable(fuzzed_data_provider, TX_WITH_WITNESS)}; if (!block || block->vtx.size() == 0 || diff --git a/src/test/fuzz/socks5.cpp b/src/test/fuzz/socks5.cpp index 17d1787586..2e6e2a94ac 100644 --- a/src/test/fuzz/socks5.cpp +++ b/src/test/fuzz/socks5.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -29,6 +30,7 @@ void initialize_socks5() FUZZ_TARGET(socks5, .init = initialize_socks5) { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); ProxyCredentials proxy_credentials; proxy_credentials.username = fuzzed_data_provider.ConsumeRandomLengthString(512); proxy_credentials.password = fuzzed_data_provider.ConsumeRandomLengthString(512); diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index 4917e8b405..bb1331c37c 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -167,6 +168,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider)); // Initialize txdownloadman bilingual_str error; @@ -297,6 +299,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider)); // Initialize a TxDownloadManagerImpl bilingual_str error; diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 9fe922cfaf..5eb6a32320 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -72,6 +73,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + SetMockTime(ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)); // regtest genesis block timestamp auto& setup{*g_setup}; bool dirty_chainman{false}; // Re-use the global chainman, but reset it when it is dirty auto& chainman{*setup.m_node.chainman}; diff --git a/src/test/fuzz/utxo_total_supply.cpp b/src/test/fuzz/utxo_total_supply.cpp index 84d82f71e2..e574c5b85c 100644 --- a/src/test/fuzz/utxo_total_supply.cpp +++ b/src/test/fuzz/utxo_total_supply.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include using node::BlockAssembler; @@ -22,18 +23,22 @@ using node::BlockAssembler; FUZZ_TARGET(utxo_total_supply) { SeedRandomStateForTest(SeedRand::ZEROS); + FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); + const auto mock_time{ConsumeTime(fuzzed_data_provider, /*min=*/1296688602)}; // regtest genesis block timestamp /** The testing setup that creates a chainman only (no chainstate) */ ChainTestingSetup test_setup{ ChainType::REGTEST, { - .extra_args = {"-testactivationheight=bip34@2"}, + .extra_args = { + "-testactivationheight=bip34@2", + strprintf("-mocktime=%d", mock_time).c_str() + }, }, }; // Create chainstate test_setup.LoadVerifyActivateChainstate(); auto& node{test_setup.m_node}; auto& chainman{*Assert(test_setup.m_node.chainman)}; - FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); const auto ActiveHeight = [&]() { LOCK(chainman.GetMutex()); diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp index 3f8c6f41ba..1ebc3464d8 100644 --- a/src/test/util/setup_common.cpp +++ b/src/test/util/setup_common.cpp @@ -199,7 +199,9 @@ BasicTestingSetup::~BasicTestingSetup() { m_node.ecc_context.reset(); m_node.kernel.reset(); - SetMockTime(0s); // Reset mocktime for following tests + if constexpr (!G_FUZZING) { + SetMockTime(0s); // Reset mocktime for following tests + } LogInstance().DisconnectTestLogger(); if (m_has_custom_datadir) { // Only remove the lock file, preserve the data directory. diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp index 7b85fb26fc..b1302e3b35 100644 --- a/src/wallet/test/fuzz/notifications.cpp +++ b/src/wallet/test/fuzz/notifications.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -58,6 +59,7 @@ FUZZ_TARGET(wallet_notifications, .init = initialize_setup) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); // The total amount, to be distributed to the wallets a and b in txs // without fee. Thus, the balance of the wallets should always equal the // total amount. diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index b0d8628a91..0c17a6bf7a 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -87,6 +88,7 @@ FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); const auto& node{g_setup->m_node}; Chainstate& chainstate{node.chainman->ActiveChainstate()}; std::unique_ptr wallet_ptr{std::make_unique(node.chain.get(), "", CreateMockableWalletDatabase())}; diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp index c4c04bce4b..552364a667 100644 --- a/src/wallet/test/fuzz/spend.cpp +++ b/src/wallet/test/fuzz/spend.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup) { SeedRandomStateForTest(SeedRand::ZEROS); FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + SetMockTime(ConsumeTime(fuzzed_data_provider)); const auto& node = g_setup->m_node; Chainstate& chainstate{node.chainman->ActiveChainstate()}; ArgsManager& args = *node.args; From a96b84cb1b76e65a639e62f0224f534f89858c18 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Fri, 20 Dec 2024 13:11:14 +0000 Subject: [PATCH 19/26] fuzz: Abort when calling system time without setting mock time --- src/test/fuzz/util/check_globals.cpp | 16 ++++++++++++++++ src/test/fuzz/util/check_globals.h | 3 +++ src/util/time.cpp | 4 ++++ 3 files changed, 23 insertions(+) diff --git a/src/test/fuzz/util/check_globals.cpp b/src/test/fuzz/util/check_globals.cpp index fbc5a55598..f91a965afc 100644 --- a/src/test/fuzz/util/check_globals.cpp +++ b/src/test/fuzz/util/check_globals.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -16,6 +17,8 @@ struct CheckGlobalsImpl { { g_used_g_prng = false; g_seeded_g_prng_zero = false; + g_used_system_time = false; + SetMockTime(0s); } ~CheckGlobalsImpl() { @@ -34,6 +37,19 @@ struct CheckGlobalsImpl { << std::endl; std::abort(); // Abort, because AFL may try to recover from a std::exit } + + if (g_used_system_time) { + std::cerr << "\n\n" + "The current fuzz target accessed system time.\n\n" + + "This is acceptable, but requires the fuzz target to call \n" + "SetMockTime() at the beginning of processing the fuzz input.\n\n" + + "Without setting mock time, time-dependent behavior can lead \n" + "to non-reproducible bugs or inefficient fuzzing.\n\n" + << std::endl; + std::abort(); + } } }; diff --git a/src/test/fuzz/util/check_globals.h b/src/test/fuzz/util/check_globals.h index 79f247535a..12d39c2daf 100644 --- a/src/test/fuzz/util/check_globals.h +++ b/src/test/fuzz/util/check_globals.h @@ -5,10 +5,13 @@ #ifndef BITCOIN_TEST_FUZZ_UTIL_CHECK_GLOBALS_H #define BITCOIN_TEST_FUZZ_UTIL_CHECK_GLOBALS_H +#include #include #include #include +extern std::atomic g_used_system_time; + struct CheckGlobalsImpl; struct CheckGlobals { CheckGlobals(); diff --git a/src/util/time.cpp b/src/util/time.cpp index e20f30a474..00f0f47392 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -20,10 +20,14 @@ void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); } static std::atomic g_mock_time{}; //!< For testing +std::atomic g_used_system_time{false}; NodeClock::time_point NodeClock::now() noexcept { const auto mocktime{g_mock_time.load(std::memory_order_relaxed)}; + if (!mocktime.count()) { + g_used_system_time = true; + } const auto ret{ mocktime.count() ? mocktime : From 1ea7e45a1f445d32a2b690d52befb2e63418653b Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 10 Dec 2024 19:25:22 +0100 Subject: [PATCH 20/26] test: raise explicit error if any of the needed release binaries is missing If the `releases` directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like "AssertionError: [node 0] Error: no RPC connection") and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike. Improve this by checking and printing out *all* of the missing release binaries and failing with an explicit error in this case. Also add an info on how to download previous releases binaries. Noticed while testing #30328. Can be tested by e.g. $ ./test/get_previous_releases.py -b $ rm -rf ./releases/v28.0/ $ ./build/test/functional/wallet_migration.py --- test/functional/test_framework/test_framework.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 7e8c40cf16..16069e522f 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -526,6 +526,15 @@ def get_bin_from_version(version, bin_name, bin_default): binary = [get_bin_from_version(v, 'bitcoind', self.options.bitcoind) for v in versions] if binary_cli is None: binary_cli = [get_bin_from_version(v, 'bitcoin-cli', self.options.bitcoincli) for v in versions] + # Fail test if any of the needed release binaries is missing + bins_missing = False + for bin_path in binary + binary_cli: + if shutil.which(bin_path) is None: + self.log.error(f"Binary not found: {bin_path}") + bins_missing = True + if bins_missing: + raise AssertionError("At least one release binary is missing. " + "Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.") assert_equal(len(extra_confs), num_nodes) assert_equal(len(extra_args), num_nodes) assert_equal(len(versions), num_nodes) From fb37acd932b06c2386d07efe88a65ee3ac9aaa24 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 6 Jan 2025 15:22:15 +0100 Subject: [PATCH 21/26] ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_* For the task `MSan, depends (Cirrus CI)` we build a custom libc++ for which we already use `-DLIBCXX_HARDENING_MODE=debug`. Compile it also with `_LIBCPP_ABI_BOUNDED_*` to enable further checks. Docs at: https://libcxx.llvm.org/Hardening.html#abi-options --- ci/test/01_base_install.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/test/01_base_install.sh b/ci/test/01_base_install.sh index 0130e820c2..a46f9ffabb 100755 --- a/ci/test/01_base_install.sh +++ b/ci/test/01_base_install.sh @@ -74,6 +74,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then -DLLVM_TARGETS_TO_BUILD=Native \ -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF \ -DLIBCXXABI_USE_LLVM_UNWINDER=OFF \ + -DLIBCXX_ABI_DEFINES="_LIBCPP_ABI_BOUNDED_ITERATORS;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STD_ARRAY;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_STRING;_LIBCPP_ABI_BOUNDED_ITERATORS_IN_VECTOR;_LIBCPP_ABI_BOUNDED_UNIQUE_PTR" \ -DLIBCXX_HARDENING_MODE=debug \ -S /msan/llvm-project/runtimes From fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 7 Jan 2025 15:50:39 +0000 Subject: [PATCH 22/26] build, test: Build `db_tests.cpp` regardless of `USE_BDB` While some tests are specific to BDB, `db_tests` as a whole are not limited to BDB. --- src/wallet/test/CMakeLists.txt | 7 +------ src/wallet/test/db_tests.cpp | 2 ++ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/wallet/test/CMakeLists.txt b/src/wallet/test/CMakeLists.txt index 8b442b262b..f14a78ca1d 100644 --- a/src/wallet/test/CMakeLists.txt +++ b/src/wallet/test/CMakeLists.txt @@ -8,6 +8,7 @@ target_sources(test_bitcoin PRIVATE init_test_fixture.cpp wallet_test_fixture.cpp + db_tests.cpp coinselector_tests.cpp feebumper_tests.cpp group_outputs_tests.cpp @@ -22,10 +23,4 @@ target_sources(test_bitcoin walletdb_tests.cpp walletload_tests.cpp ) -if(USE_BDB) - target_sources(test_bitcoin - PRIVATE - db_tests.cpp - ) -endif() target_link_libraries(test_bitcoin bitcoin_wallet) diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp index 41951e84c8..56a39c8d5f 100644 --- a/src/wallet/test/db_tests.cpp +++ b/src/wallet/test/db_tests.cpp @@ -62,6 +62,7 @@ static void CheckPrefix(DatabaseBatch& batch, Span prefix, Mock BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup) +#ifdef USE_BDB static std::shared_ptr GetWalletEnv(const fs::path& path, fs::path& database_filename) { fs::path data_file = BDBDataFile(path); @@ -124,6 +125,7 @@ BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance) BOOST_CHECK(env_1_a != env_1_b); BOOST_CHECK(env_2_a == env_2_b); } +#endif static std::vector> TestDatabases(const fs::path& path_root) { From 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Tue, 7 Jan 2025 14:57:39 -0500 Subject: [PATCH 23/26] miner: add package feerate vector to CBlockTemplate - The package feerates are ordered by the sequence in which packages are selected for inclusion in the block template. - The commit also tests this new behaviour. Co-authored-by: willcl-ark --- src/node/miner.cpp | 1 + src/node/miner.h | 4 ++++ src/test/miner_tests.cpp | 26 +++++++++++++++++++++++--- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 5d7304b597..0ff5a1eade 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -421,6 +421,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda } ++nPackagesSelected; + pblocktemplate->m_package_feerates.emplace_back(packageFees, static_cast(packageSize)); // Update transactions that depend on each of these nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx); diff --git a/src/node/miner.h b/src/node/miner.h index f6461a8d55..3c4c66b0ba 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -39,6 +40,9 @@ struct CBlockTemplate std::vector vTxFees; std::vector vTxSigOpsCost; std::vector vchCoinbaseCommitment; + /* A vector of package fee rates, ordered by the sequence in which + * packages are selected for inclusion in the block template.*/ + std::vector m_package_feerates; }; // Container for tracking updates to ancestor feerate as we include (parent) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index d48f9cca3f..d7bc790d0e 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -25,6 +26,7 @@ #include #include +#include #include @@ -123,19 +125,22 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const tx.vout[0].nValue = 5000000000LL - 1000; // This tx has a low fee: 1000 satoshis Txid hashParentTx = tx.GetHash(); // save this txid for later use - AddToMempool(tx_mempool, entry.Fee(1000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); + const auto parent_tx{entry.Fee(1000).Time(Now()).SpendsCoinbase(true).FromTx(tx)}; + AddToMempool(tx_mempool, parent_tx); // This tx has a medium fee: 10000 satoshis tx.vin[0].prevout.hash = txFirst[1]->GetHash(); tx.vout[0].nValue = 5000000000LL - 10000; Txid hashMediumFeeTx = tx.GetHash(); - AddToMempool(tx_mempool, entry.Fee(10000).Time(Now()).SpendsCoinbase(true).FromTx(tx)); + const auto medium_fee_tx{entry.Fee(10000).Time(Now()).SpendsCoinbase(true).FromTx(tx)}; + AddToMempool(tx_mempool, medium_fee_tx); // This tx has a high fee, but depends on the first transaction tx.vin[0].prevout.hash = hashParentTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 50k satoshi fee Txid hashHighFeeTx = tx.GetHash(); - AddToMempool(tx_mempool, entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)); + const auto high_fee_tx{entry.Fee(50000).Time(Now()).SpendsCoinbase(false).FromTx(tx)}; + AddToMempool(tx_mempool, high_fee_tx); std::unique_ptr block_template = mining->createNewBlock(options); BOOST_REQUIRE(block_template); @@ -145,6 +150,21 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const BOOST_CHECK(block.vtx[2]->GetHash() == hashHighFeeTx); BOOST_CHECK(block.vtx[3]->GetHash() == hashMediumFeeTx); + // Test the inclusion of package feerates in the block template and ensure they are sequential. + const auto block_package_feerates = BlockAssembler{m_node.chainman->ActiveChainstate(), &tx_mempool, options}.CreateNewBlock()->m_package_feerates; + BOOST_CHECK(block_package_feerates.size() == 2); + + // parent_tx and high_fee_tx are added to the block as a package. + const auto combined_txs_fee = parent_tx.GetFee() + high_fee_tx.GetFee(); + const auto combined_txs_size = parent_tx.GetTxSize() + high_fee_tx.GetTxSize(); + FeeFrac package_feefrac{combined_txs_fee, combined_txs_size}; + // The package should be added first. + BOOST_CHECK(block_package_feerates[0] == package_feefrac); + + // The medium_fee_tx should be added next. + FeeFrac medium_tx_feefrac{medium_fee_tx.GetFee(), medium_fee_tx.GetTxSize()}; + BOOST_CHECK(block_package_feerates[1] == medium_tx_feefrac); + // Test that a package below the block min tx fee doesn't get included tx.vin[0].prevout.hash = hashHighFeeTx; tx.vout[0].nValue = 5000000000LL - 1000 - 50000; // 0 fee From f93f0c93961bbce413101c2a92300a7a29277506 Mon Sep 17 00:00:00 2001 From: 0xb10c Date: Thu, 9 Jan 2025 06:42:05 +0000 Subject: [PATCH 24/26] tracing: Rename the `MIN` macro to `_TRACEPOINT_TEST_MIN` in log_raw_p2p_msgs Inspired by: 00c1dbd26ddb816e5541c5724397015a92a3d06b (#31419) --- contrib/tracing/log_raw_p2p_msgs.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contrib/tracing/log_raw_p2p_msgs.py b/contrib/tracing/log_raw_p2p_msgs.py index c0ab704106..67b92ce7b8 100755 --- a/contrib/tracing/log_raw_p2p_msgs.py +++ b/contrib/tracing/log_raw_p2p_msgs.py @@ -41,7 +41,8 @@ program = """ #include -#define MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; }) +// A min() macro. Prefixed with _TRACEPOINT_TEST to avoid collision with other MIN macros. +#define _TRACEPOINT_TEST_MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; }) // Maximum possible allocation size // from include/linux/percpu.h in the Linux kernel @@ -88,7 +89,7 @@ bpf_usdt_readarg_p(3, ctx, &msg->peer_conn_type, MAX_PEER_CONN_TYPE_LENGTH); bpf_usdt_readarg_p(4, ctx, &msg->msg_type, MAX_MSG_TYPE_LENGTH); bpf_usdt_readarg(5, ctx, &msg->msg_size); - bpf_usdt_readarg_p(6, ctx, &msg->msg, MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); + bpf_usdt_readarg_p(6, ctx, &msg->msg, _TRACEPOINT_TEST_MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); inbound_messages.perf_submit(ctx, msg, sizeof(*msg)); return 0; @@ -108,7 +109,7 @@ bpf_usdt_readarg_p(3, ctx, &msg->peer_conn_type, MAX_PEER_CONN_TYPE_LENGTH); bpf_usdt_readarg_p(4, ctx, &msg->msg_type, MAX_MSG_TYPE_LENGTH); bpf_usdt_readarg(5, ctx, &msg->msg_size); - bpf_usdt_readarg_p(6, ctx, &msg->msg, MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); + bpf_usdt_readarg_p(6, ctx, &msg->msg, _TRACEPOINT_TEST_MIN(msg->msg_size, MAX_MSG_DATA_LENGTH)); outbound_messages.perf_submit(ctx, msg, sizeof(*msg)); return 0; From e04be3731f4921cd51d25b1d6210eace7600fea4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 7 Jan 2025 15:49:19 +0100 Subject: [PATCH 25/26] init,log: Unify block index and chainstate loading log line Example logs before the change: ``` 2025-01-07T11:58:33Z Verification progress: 99% 2025-01-07T11:58:33Z Verification: No coin database inconsistencies in last 6 blocks (18905 transactions) 2025-01-07T11:58:33Z block index 31892ms 2025-01-07T11:58:33Z Setting NODE_NETWORK on non-prune mode 2025-01-07T11:58:33Z block tree size = 878086 2025-01-07T11:58:33Z nBestHeight = 878085 ``` Removed redundant duration as well since it can be recovered from the timestamps. Co-authored-by: TheCharlatan Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> --- src/init.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index b5adcf6476..cb4ff733a5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1245,7 +1245,6 @@ static ChainstateLoadResult InitAndLoadChainstate( "", CClientUIInterface::MSG_ERROR); }; uiInterface.InitMessage(_("Loading block index…").translated); - const auto load_block_index_start_time{SteadyClock::now()}; auto catch_exceptions = [](auto&& f) { try { return f(); @@ -1263,7 +1262,7 @@ static ChainstateLoadResult InitAndLoadChainstate( } std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); }); if (status == node::ChainstateLoadStatus::SUCCESS) { - LogPrintf(" block index %15dms\n", Ticks(SteadyClock::now() - load_block_index_start_time)); + LogInfo("Block index and chainstate loaded"); } } return {status, error}; From 8a46286da667d19414c30350df48ebf245589e32 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 9 Jan 2025 13:36:19 +0000 Subject: [PATCH 26/26] depends: Fix spacing issue This change resolves an issue where a missing space caused the value of the `build_AR` variable to be concatenated with the "NM=" string. This resulted in subsequent calls to `${AR}` and `${NM}` failing. --- depends/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/depends/Makefile b/depends/Makefile index a2a9f6823e..9ba8213a92 100644 --- a/depends/Makefile +++ b/depends/Makefile @@ -141,7 +141,7 @@ include packages/packages.mk # 2. Before including packages/*.mk (excluding packages/packages.mk), since # they rely on the build_id variables # -build_id:=$(shell env CC='$(build_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(build_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(build_AR) 'NM='$(build_NM)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') +build_id:=$(shell env CC='$(build_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(build_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(build_AR)' NM='$(build_NM)' RANLIB='$(build_RANLIB)' STRIP='$(build_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(BUILD_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') $(host_arch)_$(host_os)_id:=$(shell env CC='$(host_CC)' C_STANDARD='$(C_STANDARD)' CXX='$(host_CXX)' CXX_STANDARD='$(CXX_STANDARD)' AR='$(host_AR)' NM='$(host_NM)' RANLIB='$(host_RANLIB)' STRIP='$(host_STRIP)' SHA256SUM='$(build_SHA256SUM)' DEBUG='$(DEBUG)' LTO='$(LTO)' NO_HARDEN='$(NO_HARDEN)' ./gen_id '$(HOST_ID_SALT)' 'GUIX_ENVIRONMENT=$(realpath $(GUIX_ENVIRONMENT))') boost_packages_$(NO_BOOST) = $(boost_packages)