From ff3b4cc6e2ecdf770871d068638f5262264f4b55 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 19 Oct 2026 15:30:34 +0700 Subject: [PATCH 1/9] test: expect v0.17 HD wallet warnings in wallet_backwards_compatibility without BDB The upgrade path restarts the v0.17 node with -usehd=1 and creates an HD wallet, so v0.17 prints its encryption warning on stderr twice. The node was only stopped with the matching expected_stderr inside the BDB-only block, so builds without BDB reached the final stop_nodes() with unexpected stderr and failed. Stop v0.17 with the right expectation on both paths. --- test/functional/wallet_backwards_compatibility.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index 2826495de08d..cb3b4aaa5816 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -287,7 +287,9 @@ def run_test(self): wallet = node_v19.get_wallet_rpc("w1_v19") assert wallet.getaddressinfo(address_18075)["solvable"] - self.stop_node(node_v17.index, expected_stderr=("Warning: Make sure to encrypt your wallet and delete all non-encrypted backups after you have verified that the wallet works!\n"*3)[:-1]) + # v0.17 warns on every start of a non-encrypted HD wallet + hd_warnings = 3 if self.is_bdb_compiled() else 2 + self.stop_node(node_v17.index, expected_stderr=("Warning: Make sure to encrypt your wallet and delete all non-encrypted backups after you have verified that the wallet works!\n"*hd_warnings)[:-1]) if __name__ == '__main__': BackwardsCompatibilityTest().main() From 774a9a8a80a0208bf5a85839a30a6a6e77bcf454 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 16 Mar 2023 14:36:34 -0400 Subject: [PATCH 2/9] Merge bitcoin/bitcoin#27200: test: psbt: check non-witness UTXO removal for segwit v1 input 3dd2f6461b4bb28b2b212c691a3df28ac793ad91 test: psbt: check non-witness UTXO removal for segwit v1 input (Sebastian Falbesoner) dd78e3fa439d57e148a2a5e312021da962c4a394 test: speedup rpc_psbt.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner) e194e3e93dd0665181bafeb162bf4c9f3621d6f1 test: PSBT: eliminate magic numbers for global unsigned tx key (0) (Sebastian Falbesoner) Pull request description: This PR adds missing test coverage for dropping non-witness UTXOs from PSBTs for segwit v1+ inputs (see commit 103c6fd2791f7e73eeab7f3900fbedd5b550211d). The formerly [disabled](https://github.com/bitcoin/bitcoin/commit/46004790588c24174a0bec49b540d158ce163ffd) method `test_utxo_conversion` is re-enabled and adapted to spend a Taproot (`bech32m`) instead of a wrapped SegWit (`p2sh-segwit`) output. Note that in contrast to the original test, we have to add the non-witness UTXO manually here using the test framework's PSBT module, since the constructing node knows that the output is segwit v1 and hence doesn't add the non-witness UTXO in the first place (see also [BIP371]( https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki#user-content-UTXO_Types)). I strongly assume that most wallets would behave the same as Bitcoin Core here and wouldn't create PSBTs with non-witness UTXOs for Taproot inputs, but it's still good to test everything works as expected if it's still done and that the non-witness UTXO is simply dropped in that case. The first two commits contain a small refactor (magic number elimination in PSBT module) and test speedup of ~2-3x (using whitelisting peers / immediate tx relay). ACKs for top commit: achow101: ACK 3dd2f6461b4bb28b2b212c691a3df28ac793ad91 instagibbs: ACK 3dd2f6461b4bb28b2b212c691a3df28ac793ad91 Tree-SHA512: b8d7f7ea5d7d21def024b70dfca61991cc96a4193be8857018b4d7cf3ca1465d185619fd4a77623803d9da309aa489c53273e9b7683d970ce12e2399b5b50031 Co-authored-by: Andrew Chow --- test/functional/test_framework/psbt.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/functional/test_framework/psbt.py b/test/functional/test_framework/psbt.py index 68945e7e8435..cc2fab531587 100644 --- a/test/functional/test_framework/psbt.py +++ b/test/functional/test_framework/psbt.py @@ -105,8 +105,8 @@ def __init__(self, *, g=None, i=None, o=None): def deserialize(self, f): assert f.read(5) == b"psbt\xff" self.g = from_binary(PSBTMap, f) - assert 0 in self.g.map - self.tx = from_binary(CTransaction, self.g.map[0]) + assert PSBT_GLOBAL_UNSIGNED_TX in self.g.map + self.tx = from_binary(CTransaction, self.g.map[PSBT_GLOBAL_UNSIGNED_TX]) self.i = [from_binary(PSBTMap, f) for _ in self.tx.vin] self.o = [from_binary(PSBTMap, f) for _ in self.tx.vout] return self @@ -115,8 +115,8 @@ def serialize(self): assert isinstance(self.g, PSBTMap) assert isinstance(self.i, list) and all(isinstance(x, PSBTMap) for x in self.i) assert isinstance(self.o, list) and all(isinstance(x, PSBTMap) for x in self.o) - assert 0 in self.g.map - tx = from_binary(CTransaction, self.g.map[0]) + assert PSBT_GLOBAL_UNSIGNED_TX in self.g.map + tx = from_binary(CTransaction, self.g.map[PSBT_GLOBAL_UNSIGNED_TX]) assert len(tx.vin) == len(self.i) assert len(tx.vout) == len(self.o) From f863b45826822ad95145753e1a89f1a7d1c84158 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 12 Feb 2024 13:33:08 -0500 Subject: [PATCH 3/9] Merge bitcoin/bitcoin#28987: wallet: simplify and batch zap wallet txes process 9a3c5c8697659e34d0476103af942a4615818f4e scripted-diff: rename ZapSelectTx to RemoveTxs (furszy) 83b762845f5804f23b63526d403b2f327fe99632 wallet: batch and simplify ZapSelectTx process (furszy) 595d50a1032ad7ffa9945464c86aa57f16665e93 wallet: migration, remove extra NotifyTransactionChanged call (furszy) a2b071f9920c2f4893afcc43a152f593c03966bf wallet: ZapSelectTx, remove db rewrite code (furszy) Pull request description: Work decoupled from #28574. Brother of #28894. Includes two different, yet interconnected, performance and code improvements to the zap wallet transactions process. 1) As the goal of the `ZapSelectTx` function is to erase tx records that match any of the inputted hashes. There is no need to traverse the whole database record by record. We could just check if the tx exist, and remove it directly by calling `EraseTx()`. 2) Instead of performing single write operations per removed tx record, this PR batches them all within a single atomic db txn. Moreover, these changes will enable us to consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future. ACKs for top commit: achow101: ACK 9a3c5c8697659e34d0476103af942a4615818f4e josibake: ACK https://github.com/bitcoin/bitcoin/pull/28987/commits/9a3c5c8697659e34d0476103af942a4615818f4e Tree-SHA512: fb2ecc48224c400ab3b1fbb32e174b5b13bf03794717727f80f01f55fb183883b067a68c0a127b2de8885564da15425d021a96541953bf38a72becc2e9929ccf Co-authored-by: Ava Chow --- src/wallet/rpc/backup.cpp | 10 +-- src/wallet/rpc/wallet.cpp | 11 ++- src/wallet/test/wallet_tests.cpp | 6 +- src/wallet/wallet.cpp | 69 +++++++++-------- src/wallet/wallet.h | 4 +- src/wallet/walletdb.cpp | 84 --------------------- src/wallet/walletdb.h | 2 - src/wallet/wallettool.cpp | 7 +- test/functional/wallet_importprunedfunds.py | 2 +- 9 files changed, 55 insertions(+), 140 deletions(-) diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index acd32c39f8df..2e634aa0ce46 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -398,14 +398,8 @@ RPCHelpMan removeprunedfunds() uint256 hash(ParseHashV(request.params[0], "txid")); std::vector vHash; vHash.push_back(hash); - std::vector vHashOut; - - if (pwallet->ZapSelectTx(vHash, vHashOut) != DBErrors::LOAD_OK) { - throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete the transaction."); - } - - if(vHashOut.empty()) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction does not exist in wallet."); + if (auto res = pwallet->RemoveTxs(vHash); !res) { + throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original); } return UniValue::VNULL; diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index a27d70ef2a1b..f2a4918906ac 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -836,7 +836,6 @@ static RPCHelpMan wipewallettxes() for (size_t progress = 0; progress < STEPS; ++progress) { std::vector vHashIn; - std::vector vHashOut; size_t count{0}; for (auto& [txid, wtx] : pwallet->mapWallet) { @@ -845,13 +844,13 @@ static RPCHelpMan wipewallettxes() vHashIn.push_back(txid); } - if (vHashIn.size() > 0 && pwallet->ZapSelectTx(vHashIn, vHashOut) != DBErrors::LOAD_OK) { - pwallet->ShowProgress(strprintf("%s " + _("Wiping wallet transactions…").translated, pwallet->GetDisplayName()), 100); - throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete transactions."); + if (vHashIn.size() > 0) { + if (auto res = pwallet->RemoveTxs(vHashIn); !res) { + pwallet->ShowProgress(strprintf("%s " + _("Wiping wallet transactions…").translated, pwallet->GetDisplayName()), 100); + throw JSONRPCError(RPC_WALLET_ERROR, util::ErrorString(res).original); + } } - CHECK_NONFATAL(vHashOut.size() == vHashIn.size()); - if (pwallet->IsAbortingRescan() || pwallet->chain().shutdownRequested()) { pwallet->ShowProgress(strprintf("%s " + _("Wiping wallet transactions…").translated, pwallet->GetDisplayName()), 100); throw JSONRPCError(RPC_MISC_ERROR, "Wiping was aborted by user."); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index e623b8e85401..6daddd99be71 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -1073,7 +1073,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletWithoutChain, BasicTestingSetup) UnloadWallet(std::move(wallet)); } -BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) +BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) { m_args.ForceSetArg("-unsafesqlitesync", "1"); WalletContext context; @@ -1098,8 +1098,8 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) BOOST_CHECK(wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 1u); - std::vector vHashIn{ block_hash }, vHashOut; - BOOST_CHECK_EQUAL(wallet->ZapSelectTx(vHashIn, vHashOut), DBErrors::LOAD_OK); + std::vector vHashIn{ block_hash }; + BOOST_CHECK(wallet->RemoveTxs(vHashIn)); BOOST_CHECK(!wallet->HasWalletSpend(prev_tx)); BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_hash), 0u); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0fb7fcf1a801..f110619eaf1c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2685,15 +2685,44 @@ void CWallet::AutoLockMasternodeCollaterals() LockProTxCoins(candidates, &batch); } -DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) +util::Result CWallet::RemoveTxs(std::vector& txs_to_remove) { AssertLockHeld(cs_wallet); - WalletLogPrintf("ZapSelectTx started for %d transactions...\n", vHashIn.size()); + WalletLogPrintf("RemoveTxs started for %d transactions...\n", txs_to_remove.size()); - DBErrors nZapSelectTxRet = WalletBatch(GetDatabase()).ZapSelectTx(vHashIn, vHashOut); - for (const uint256& hash : vHashOut) { - const auto& it = mapWallet.find(hash); + WalletBatch batch(GetDatabase()); + if (!batch.TxnBegin()) return util::Error{_("Error starting db txn for wallet transactions removal")}; + + // Check for transaction existence and remove entries from disk + using TxIterator = std::unordered_map::const_iterator; + std::vector erased_txs; + bilingual_str str_err; + for (const uint256& hash : txs_to_remove) { + auto it_wtx = mapWallet.find(hash); + if (it_wtx == mapWallet.end()) { + str_err = strprintf(_("Transaction %s does not belong to this wallet"), hash.GetHex()); + break; + } + if (!batch.EraseTx(hash)) { + str_err = strprintf(_("Failure removing transaction: %s"), hash.GetHex()); + break; + } + erased_txs.emplace_back(it_wtx); + } + + // Roll back removals in case of an error + if (!str_err.empty()) { + batch.TxnAbort(); + return util::Error{str_err}; + } + + // Dump changes to disk + if (!batch.TxnCommit()) return util::Error{_("Error committing db txn for wallet transactions removal")}; + + // Update the in-memory state and notify upper layers about the removals + for (const auto& it : erased_txs) { + const uint256 hash{it->first}; wtxOrdered.erase(it->second.m_it_wtxOrdered); for (const auto& txin : it->second.tx->vin) mapTxSpends.erase(txin.prevout); @@ -2701,23 +2730,10 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vectorRewriteDB(); - } - } - } - - if (nZapSelectTxRet != DBErrors::LOAD_OK) - return nZapSelectTxRet; - MarkDirty(); - WalletLogPrintf("ZapSelectTx completed for %d transactions.\n", vHashOut.size()); - return DBErrors::LOAD_OK; + WalletLogPrintf("RemoveTxs completed for %d transactions.\n", txs_to_remove.size()); + return {}; // all good } bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& address, const std::string& strName, const std::optional& new_purpose) @@ -5045,19 +5061,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) watchonly_batch.reset(); // Flush // Do the removes if (txids_to_delete.size() > 0) { - std::vector deleted_txids; - if (ZapSelectTx(txids_to_delete, deleted_txids) != DBErrors::LOAD_OK) { - error = _("Error: Could not delete watchonly transactions"); + if (auto res = RemoveTxs(txids_to_delete); !res) { + error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); return false; } - if (deleted_txids != txids_to_delete) { - error = _("Error: Not all watchonly txs could be deleted"); - return false; - } - // Tell the GUI of each tx - for (const uint256& txid : deleted_txids) { - NotifyTransactionChanged(txid, CT_UPDATED); - } } // Pair external wallets with their corresponding db handler diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 51f167e355f7..27a594039554 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -973,7 +973,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati DBErrors LoadWallet(); void AutoLockMasternodeCollaterals(); - DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + + /** Erases the provided transactions from the wallet. */ + util::Result RemoveTxs(std::vector& txs_to_remove) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional& purpose); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 7e1e0dbaa653..ccc5b09c6a7b 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1086,90 +1086,6 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -DBErrors WalletBatch::FindWalletTxHashes(std::vector& tx_hashes) -{ - DBErrors result = DBErrors::LOAD_OK; - - try { - int nMinVersion = 0; - if (m_batch->Read(DBKeys::MINVERSION, nMinVersion)) { - if (nMinVersion > FEATURE_LATEST) - return DBErrors::TOO_NEW; - } - - // Get cursor - std::unique_ptr cursor = m_batch->GetNewCursor(); - if (!cursor) - { - LogPrintf("Error getting wallet database cursor\n"); - return DBErrors::CORRUPT; - } - - while (true) - { - // Read next record - DataStream ssKey{}; - DataStream ssValue{}; - DatabaseCursor::Status status = cursor->Next(ssKey, ssValue); - if (status == DatabaseCursor::Status::DONE) { - break; - } else if (status == DatabaseCursor::Status::FAIL) { - LogPrintf("Error reading next record from wallet database\n"); - return DBErrors::CORRUPT; - } - - std::string strType; - ssKey >> strType; - if (strType == DBKeys::TX) { - uint256 hash; - ssKey >> hash; - tx_hashes.push_back(hash); - } - } - } catch (...) { - result = DBErrors::CORRUPT; - } - - return result; -} - -DBErrors WalletBatch::ZapSelectTx(std::vector& vTxHashIn, std::vector& vTxHashOut) -{ - // build list of wallet TX hashes - std::vector vTxHash; - DBErrors err = FindWalletTxHashes(vTxHash); - if (err != DBErrors::LOAD_OK) { - return err; - } - - std::sort(vTxHash.begin(), vTxHash.end()); - std::sort(vTxHashIn.begin(), vTxHashIn.end()); - - // erase each matching wallet TX - bool delerror = false; - std::vector::iterator it = vTxHashIn.begin(); - for (const uint256& hash : vTxHash) { - while (it < vTxHashIn.end() && (*it) < hash) { - it++; - } - if (it == vTxHashIn.end()) { - break; - } - else if ((*it) == hash) { - if(!EraseTx(hash)) { - LogPrint(BCLog::WALLETDB, "Transaction was found for deletion but returned database error: %s\n", hash.GetHex()); - delerror = true; - } - vTxHashOut.push_back(hash); - } - } - - if (delerror) { - return DBErrors::CORRUPT; - } - return DBErrors::LOAD_OK; -} - void MaybeCompactWalletDB(WalletContext& context) { static std::atomic fOneThread(false); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 4d050e108e37..d2d9ae08deab 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -257,8 +257,6 @@ class WalletBatch bool EraseActiveScriptPubKeyMan(bool internal); DBErrors LoadWallet(CWallet* pwallet); - DBErrors FindWalletTxHashes(std::vector& tx_hashes); - DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut); /* Function to determine if a certain KV/key-type is a key (cryptographical key) type */ static bool IsKeyType(const std::string& strType); diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index d06ffad83266..9eb99208735e 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -210,7 +210,6 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) if (wallet_instance == nullptr) return false; std::vector vHash; - std::vector vHashOut; LOCK(wallet_instance->cs_wallet); @@ -218,14 +217,14 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) vHash.push_back(txid); } - if (wallet_instance->ZapSelectTx(vHash, vHashOut) != DBErrors::LOAD_OK) { - tfm::format(std::cerr, "Could not properly delete transactions"); + if (auto res = wallet_instance->RemoveTxs(vHash); !res) { + tfm::format(std::cerr, "Could not properly delete transactions: %s", util::ErrorString(res).original); wallet_instance->Close(); return false; } wallet_instance->Close(); - return vHashOut.size() == vHash.size(); + return true; #else tfm::format(std::cerr, "Wipetxes command is not available as BDB support is not compiled"); return false; diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py index 984ec467bba7..d7ef3f106f70 100755 --- a/test/functional/wallet_importprunedfunds.py +++ b/test/functional/wallet_importprunedfunds.py @@ -125,7 +125,7 @@ def run_test(self): assert_equal(address_info['ismine'], True) # Remove transactions - assert_raises_rpc_error(-8, "Transaction does not exist in wallet.", w1.removeprunedfunds, txnid1) + assert_raises_rpc_error(-4, f'Transaction {txnid1} does not belong to this wallet', w1.removeprunedfunds, txnid1) assert not [tx for tx in w1.listtransactions(include_watchonly=True) if tx['txid'] == txnid1] wwatch.removeprunedfunds(txnid2) From 6adb460bf9d4d0102eb5eef3176160dc16d4df2a Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 30 May 2023 15:15:03 +0100 Subject: [PATCH 4/9] Merge bitcoin/bitcoin#27774: refactor: Add [[nodiscard]] where ignoring a Result return type is an error fa5680b7520c340952239c4e25ebe505d16c7c39 fix includes for touched header files (iwyu) (MarcoFalke) dddde27f6fbcff7cdb31f7138efc5d8363537b03 Add [[nodiscard]] where ignoring a Result return type is an error (MarcoFalke) Pull request description: Only add it for those where it is an error to ignore. Also, fix the gcc compile warning https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1564350880. Also, fix iwyu for touched header files. ACKs for top commit: TheCharlatan: ACK fa5680b7520c340952239c4e25ebe505d16c7c39 stickies-v: ACK fa5680b7520c340952239c4e25ebe505d16c7c39 Tree-SHA512: c3509103bfeae246e2cf565bc561fcd68d8118515bac5431ba5304c3a63c8254b9c4f40e268b6f6d6b79405675c5a960db9b4eb3bdd14aedca333dc1c9e76415 Co-authored-by: fanquake --- src/addrdb.cpp | 3 ++- src/addrdb.h | 5 +---- src/kernel/checks.h | 4 ++-- src/node/blockmanager_args.h | 2 +- src/node/chainstatemanager_args.h | 2 +- src/txdb.h | 5 ++++- src/wallet/wallet.h | 2 +- 7 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index cac7c380f9f1..87690501f7fc 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -226,7 +226,8 @@ util::Result> LoadAddrman(const NetGroupManager& netgro return util::Error{strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."), e.what(), PACKAGE_BUGREPORT, fs::quoted(fs::PathToString(path_addr)))}; } - return addrman; // std::move should be unneccessary but is temporarily needed to work around clang bug (https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092) + return {std::move(addrman)}; // std::move should be unneccessary but is temporarily needed to work around clang bug + // (https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092) } void DumpAnchors(const fs::path& anchors_db_path, const std::vector& anchors) diff --git a/src/addrdb.h b/src/addrdb.h index 164191633abb..cc3014dce29b 100644 --- a/src/addrdb.h +++ b/src/addrdb.h @@ -6,13 +6,11 @@ #ifndef BITCOIN_ADDRDB_H #define BITCOIN_ADDRDB_H -#include // For banmap_t -#include +#include #include #include #include -#include #include class ArgsManager; @@ -20,7 +18,6 @@ class AddrMan; class CAddress; class DataStream; class NetGroupManager; -struct bilingual_str; /** Only used by tests. */ void ReadFromStream(AddrMan& addr, DataStream& ssPeers); diff --git a/src/kernel/checks.h b/src/kernel/checks.h index 3eb14824fb6f..82807329f24f 100644 --- a/src/kernel/checks.h +++ b/src/kernel/checks.h @@ -16,8 +16,8 @@ struct Context; /** * Ensure a usable environment with all necessary library support. */ -std::optional SanityChecks(const Context&); +[[nodiscard]] std::optional SanityChecks(const Context&); -} +} // namespace kernel #endif // BITCOIN_KERNEL_CHECKS_H diff --git a/src/node/blockmanager_args.h b/src/node/blockmanager_args.h index e657c6bb4568..a3e04916d105 100644 --- a/src/node/blockmanager_args.h +++ b/src/node/blockmanager_args.h @@ -14,7 +14,7 @@ class ArgsManager; struct bilingual_str; namespace node { -std::optional ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts); +[[nodiscard]] std::optional ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts); } // namespace node #endif // BITCOIN_NODE_BLOCKMANAGER_ARGS_H diff --git a/src/node/chainstatemanager_args.h b/src/node/chainstatemanager_args.h index 6c46b998f213..dd6771f5232d 100644 --- a/src/node/chainstatemanager_args.h +++ b/src/node/chainstatemanager_args.h @@ -13,7 +13,7 @@ class ArgsManager; struct bilingual_str; namespace node { -std::optional ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts); +[[nodiscard]] std::optional ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts); } // namespace node #endif // BITCOIN_NODE_CHAINSTATEMANAGER_ARGS_H diff --git a/src/txdb.h b/src/txdb.h index b4ea64a07147..aa2b0e98e38c 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -13,6 +13,9 @@ #include #include +#include +#include +#include #include #include #include @@ -21,11 +24,11 @@ class CBlockFileInfo; class CBlockIndex; +class COutPoint; class uint256; namespace Consensus { struct Params; }; -struct bilingual_str; //! -dbcache default (MiB) static const int64_t nDefaultDbCache = 300; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 27a594039554..4a852a18955e 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1339,7 +1339,7 @@ struct MigrationResult { }; //! Do all steps to migrate a legacy wallet to a descriptor wallet -util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context); +[[nodiscard]] util::Result MigrateLegacyToDescriptor(const std::string& wallet_name, const SecureString& passphrase, WalletContext& context); } // namespace wallet #endif // BITCOIN_WALLET_WALLET_H From 57cc86f1c6f2f86db88b6f37e930bb065bdf77fb Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 13 Jun 2023 16:26:58 -0400 Subject: [PATCH 5/9] Merge bitcoin/bitcoin#27876: test: (refactor) Use datadir from options in chainstatemanager test d54819d74e04e6105c1f0362755f5bcfa845eefd scripted-diff: Use datadir from options in chainstatemanager test (TheCharlatan) Pull request description: This should make the test less reliant on argument state from the test setup. This is a follow-up PR as requested in https://github.com/bitcoin/bitcoin/pull/27576#discussion_r1224638890. ACKs for top commit: achow101: ACK d54819d74e04e6105c1f0362755f5bcfa845eefd MarcoFalke: lgtm ACK d54819d74e04e6105c1f0362755f5bcfa845eefd kevkevinpal: ACK https://github.com/bitcoin/bitcoin/commit/d54819d74e04e6105c1f0362755f5bcfa845eefd ryanofsky: Code review ACK d54819d74e04e6105c1f0362755f5bcfa845eefd Tree-SHA512: 939fde2505c5585d993545a3d05d3a00caec40f860c74fa002caebdf4c1b70e774cfb028a8a8f780525f8968844157d2c568d9f2c8dd5ec32b093173d8644c34 Co-authored-by: Andrew Chow --- src/test/validation_chainstatemanager_tests.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp index 304386d600dc..21840f5f88fd 100644 --- a/src/test/validation_chainstatemanager_tests.cpp +++ b/src/test/validation_chainstatemanager_tests.cpp @@ -232,7 +232,7 @@ struct SnapshotTestSetup : TestChain100Setup { { LOCK(::cs_main); BOOST_CHECK(!chainman.IsSnapshotValidated()); - BOOST_CHECK(!node::FindSnapshotChainstateDir(m_args.GetDataDirNet())); + BOOST_CHECK(!node::FindSnapshotChainstateDir(chainman.m_options.datadir)); } size_t initial_size; @@ -282,7 +282,7 @@ struct SnapshotTestSetup : TestChain100Setup { auto_infile >> coin; })); - BOOST_CHECK(!node::FindSnapshotChainstateDir(m_args.GetDataDirNet())); + BOOST_CHECK(!node::FindSnapshotChainstateDir(chainman.m_options.datadir)); BOOST_REQUIRE(!CreateAndActivateUTXOSnapshot( this, [](AutoFile& auto_infile, SnapshotMetadata& metadata) { @@ -306,7 +306,7 @@ struct SnapshotTestSetup : TestChain100Setup { })); BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(this)); - BOOST_CHECK(fs::exists(*node::FindSnapshotChainstateDir(m_args.GetDataDirNet()))); + BOOST_CHECK(fs::exists(*node::FindSnapshotChainstateDir(chainman.m_options.datadir))); // Ensure our active chain is the snapshot chainstate. BOOST_CHECK(!chainman.ActiveChainstate().m_from_snapshot_blockhash->IsNull()); @@ -328,7 +328,7 @@ struct SnapshotTestSetup : TestChain100Setup { { LOCK(::cs_main); - fs::path found = *node::FindSnapshotChainstateDir(m_args.GetDataDirNet()); + fs::path found = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); // Note: WriteSnapshotBaseBlockhash() is implicitly tested above. BOOST_CHECK_EQUAL( @@ -444,7 +444,7 @@ struct SnapshotTestSetup : TestChain100Setup { m_node.notifications = std::make_unique(); const ChainstateManager::Options chainman_opts{ .chainparams = ::Params(), - .datadir = m_args.GetDataDirNet(), + .datadir = chainman.m_options.datadir, .superblock_status = [&sbman = *m_node.sbman](const CDeterministicMNList& mn_list, int height) { return sbman.GetStatus(mn_list, height); }, .mn_sync = m_node.mn_sync.get(), .notifications = *m_node.notifications, @@ -629,7 +629,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_init, SnapshotTestSetup) this->SetupSnapshot(); - fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(m_args.GetDataDirNet()); + fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); @@ -926,7 +926,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_snapshot_completion, SnapshotTestSetup SnapshotCompletionResult res; auto mock_shutdown = [](bilingual_str msg) {}; - fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(m_args.GetDataDirNet()); + fs::path snapshot_chainstate_dir = *node::FindSnapshotChainstateDir(chainman.m_options.datadir); BOOST_CHECK(fs::exists(snapshot_chainstate_dir)); BOOST_CHECK_EQUAL(snapshot_chainstate_dir, gArgs.GetDataDirNet() / "chainstate_snapshot"); From 6dc0079b65fe5ba55161cff97c90d937f3d68e62 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 21 Jun 2023 10:45:21 +0100 Subject: [PATCH 6/9] Merge bitcoin/bitcoin#27733: test: refactor: introduce `generate_keypair` helper with WIF support 1a572ce7d6e2b8282c6ad457cf8ecd2cf5ab7fd6 test: refactor: introduce `generate_keypair` helper with WIF support (Sebastian Falbesoner) Pull request description: In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.: privkey = ECKey() privkey.generate() privkey_wif = bytes_to_wif(privkey.get_bytes()) pubkey = privkey.get_pubkey().get_bytes() Simplify this by providing a new `generate_keypair` helper function that returns the private key either as `ECKey` object or as WIF-string (depending on the boolean `wif` parameter) and the public key as byte-string; these formats are what we mostly need (currently we don't use `ECPubKey` objects from generated keypairs anywhere). With this, most of the affected code blocks following the pattern above can be replaced by one-liners, e.g.: privkey, pubkey = generate_keypair(wif=True) Note that after this commit, the only direct uses of `ECKey` remain in situations where we want to set the private key explicitly, e.g. in MiniWallet (test/functional/test_framework/wallet.py) or the test for the signet miner script (test/functional/tool_signet_miner.py). ACKs for top commit: instagibbs: ACK https://github.com/bitcoin/bitcoin/pull/27733/commits/1a572ce7d6e2b8282c6ad457cf8ecd2cf5ab7fd6 kevkevinpal: reACK [1a572ce](https://github.com/bitcoin/bitcoin/pull/27733/commits/1a572ce7d6e2b8282c6ad457cf8ecd2cf5ab7fd6) stratospher: ACK 1a572ce7. neat to have this since keypair generation is done in lots of places. Tree-SHA512: ceb695ba7b34dc9f65357b55be03e67609e7e13a178083d405284eff4d8d3c5cea4fb0b6632658604a533f38ebfefc33e0c375995cc21ebc7843442ad764287b Co-authored-by: fanquake --- test/functional/feature_asset_locks.py | 8 ++----- test/functional/feature_assumevalid.py | 11 +++++---- test/functional/feature_block.py | 6 ++--- test/functional/feature_masternode_shares.py | 9 ++++---- test/functional/feature_mnehf.py | 6 ++--- test/functional/feature_nulldummy.py | 10 +++----- test/functional/mempool_accept.py | 6 ++--- test/functional/mempool_dust.py | 9 +++----- test/functional/p2p_blockfilters.py | 6 ++--- test/functional/rpc_createmultisig.py | 11 ++++----- test/functional/rpc_psbt.py | 9 +++----- test/functional/test_framework/wallet.py | 7 +++--- test/functional/test_framework/wallet_util.py | 23 +++++++++++-------- test/functional/wallet_blank.py | 14 ++++------- test/functional/wallet_createwallet.py | 12 ++++------ test/functional/wallet_fundrawtransaction.py | 9 ++------ test/functional/wallet_importprunedfunds.py | 9 +++----- test/functional/wallet_listsinceblock.py | 9 +++----- test/functional/wallet_send.py | 8 +++---- 19 files changed, 70 insertions(+), 112 deletions(-) diff --git a/test/functional/feature_asset_locks.py b/test/functional/feature_asset_locks.py index 4e2d06f86827..597331caf900 100755 --- a/test/functional/feature_asset_locks.py +++ b/test/functional/feature_asset_locks.py @@ -16,7 +16,6 @@ create_coinbase, ) from test_framework.authproxy import JSONRPCException -from test_framework.key import ECKey from test_framework.messages import ( CAssetLockTx, CAssetUnlockTx, @@ -53,7 +52,7 @@ softfork_active, ) from test_framework.segwit_addr import encode_platform_p2pkh -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair llmq_type_test = 106 # LLMQType::LLMQ_TEST_PLATFORM MNEHF_SIGNAL_TX_TYPE = 7 # TRANSACTION_MNHF_SIGNAL @@ -342,11 +341,8 @@ def run_test(self): self.mempool_size = 0 - key = ECKey() - key.generate() - privkey = bytes_to_wif(key.get_bytes()) + privkey, pubkey = generate_keypair(wif=True) node_wallet.importprivkey(privkey) - pubkey = key.get_pubkey().get_bytes() self.test_asset_locks(node_wallet, node, pubkey) self.test_asset_unlocks(node_wallet, node, pubkey) diff --git a/test/functional/feature_assumevalid.py b/test/functional/feature_assumevalid.py index 72582fb7be6c..92989bcd550f 100755 --- a/test/functional/feature_assumevalid.py +++ b/test/functional/feature_assumevalid.py @@ -35,7 +35,6 @@ create_block, create_coinbase, ) -from test_framework.key import ECKey from test_framework.messages import ( CBlockHeader, COutPoint, @@ -46,9 +45,13 @@ msg_headers, ) from test_framework.p2p import P2PInterface -from test_framework.script import (CScript, OP_TRUE) +from test_framework.script import ( + CScript, + OP_TRUE, +) from test_framework.test_framework import BitcoinTestFramework from test_framework.util import (assert_equal, set_node_times) +from test_framework.wallet_util import generate_keypair class BaseNode(P2PInterface): @@ -96,9 +99,7 @@ def run_test(self): self.blocks = [] # Get a pubkey for the coinbase TXO - coinbase_key = ECKey() - coinbase_key.generate() - coinbase_pubkey = coinbase_key.get_pubkey().get_bytes() + _, coinbase_pubkey = generate_keypair() # Create the first block with a coinbase output to our key height = 1 diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 406ba0af9bb0..65bf750c4eb8 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -13,7 +13,6 @@ create_tx_with_script, get_legacy_sigopcount_block, ) -from test_framework.key import ECKey from test_framework.messages import ( CBlock, COIN, @@ -53,6 +52,7 @@ assert_equal, assert_greater_than, ) +from test_framework.wallet_util import generate_keypair from data import invalid_txs # This functional test assumes DIP0001 is disabled @@ -130,9 +130,7 @@ def run_test(self): self.bootstrap_p2p() # Add one p2p connection to the node self.block_heights = {} - self.coinbase_key = ECKey() - self.coinbase_key.generate() - self.coinbase_pubkey = self.coinbase_key.get_pubkey().get_bytes() + self.coinbase_key, self.coinbase_pubkey = generate_keypair() self.tip = None self.blocks = {} self.genesis_hash = int(self.nodes[0].getbestblockhash(), 16) diff --git a/test/functional/feature_masternode_shares.py b/test/functional/feature_masternode_shares.py index b153581e3ffa..f0e60d7fb483 100755 --- a/test/functional/feature_masternode_shares.py +++ b/test/functional/feature_masternode_shares.py @@ -12,7 +12,7 @@ from test_framework.descriptors import descsum_create from test_framework.address import key_to_p2pkh from test_framework.crypto import secp256k1 -from test_framework.key import ECKey, ORDER, rfc6979_nonce +from test_framework.key import ORDER, rfc6979_nonce from test_framework.messages import ( COIN, CBlock, @@ -34,7 +34,7 @@ get_bip9_details, softfork_active, ) -from test_framework.wallet_util import bytes_to_wif, get_generate_key +from test_framework.wallet_util import bytes_to_wif, generate_keypair, get_generate_key # Keep the earliest activation height above the ~119 blocks the framework setup mines, so # run_test starts with v24 locked in but not yet active and can exercise pre-activation rules @@ -477,8 +477,7 @@ def test_separate_participant_wallets(self): wallet.importprivkey(key.privkey) refund = wallet.addmultisigaddress(2, [key.pubkey for key in keys])["address"] # The test keeps the owner key too, to sign dissolutions the wallet refuses to - owner_key = ECKey() - owner_key.generate() + owner_key, owner_pubkey = generate_keypair() owner_wif = bytes_to_wif(owner_key.get_bytes()) if self.options.descriptors: result, = wallet.importdescriptors([{"desc": descsum_create(f"pkh({owner_wif})"), "timestamp": "now"}]) @@ -487,7 +486,7 @@ def test_separate_participant_wallets(self): wallet.importprivkey(owner_wif, "", False) owner_keys.append(owner_key) shares.append({"amount": amount * COIN, "refundAddress": refund, - "ownerAddress": key_to_p2pkh(owner_key.get_pubkey().get_bytes().hex()), + "ownerAddress": key_to_p2pkh(owner_pubkey.hex()), "rewardAddress": common_reward}) funding_addresses.append(wallet.getnewaddress()) change_addresses.append(wallet.getnewaddress()) diff --git a/test/functional/feature_mnehf.py b/test/functional/feature_mnehf.py index 10995fc43c2a..851607a8e5bb 100755 --- a/test/functional/feature_mnehf.py +++ b/test/functional/feature_mnehf.py @@ -7,7 +7,6 @@ import struct from io import BytesIO -from test_framework.key import ECKey from test_framework.messages import ( CMnEhf, CTransaction, @@ -22,6 +21,7 @@ assert_equal, get_bip9_details, ) +from test_framework.wallet_util import generate_keypair class MnehfTest(DashTestFramework): def add_options(self, parser): @@ -141,9 +141,7 @@ def run_test(self): self.test_no_signing_outside_window() self.check_fork('defined') - key = ECKey() - key.generate() - pubkey = key.get_pubkey().get_bytes() + _, pubkey = generate_keypair() ehf_tx = self.create_mnehf(28, pubkey) ehf_unknown_tx = self.create_mnehf(27, pubkey) diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py index 29c54e571693..df36b1a55f2a 100755 --- a/test/functional/feature_nulldummy.py +++ b/test/functional/feature_nulldummy.py @@ -33,8 +33,7 @@ assert_raises_rpc_error, ) from test_framework.wallet import getnewdestination -from test_framework.key import ECKey -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair NULLDUMMY_ERROR = "mandatory-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)" @@ -68,11 +67,8 @@ def create_transaction(self, *, txid, input_details=None, addr, amount, privkey) return tx_from_hex(signedtx["hex"]) def run_test(self): - eckey = ECKey() - eckey.generate() - self.privkey = bytes_to_wif(eckey.get_bytes()) - self.pubkey = eckey.get_pubkey().get_bytes().hex() - cms = self.nodes[0].createmultisig(1, [self.pubkey]) + self.privkey, self.pubkey = generate_keypair(wif=True) + cms = self.nodes[0].createmultisig(1, [self.pubkey.hex()]) self.ms_address = cms["address"] ms_unlock_details = {"scriptPubKey": address_to_scriptpubkey(self.ms_address).hex(), "redeemScript": cms["redeemScript"]} diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 55c3e60ee984..1fe8488f73ef 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -9,7 +9,6 @@ import math from test_framework.test_framework import BitcoinTestFramework -from test_framework.key import ECKey from test_framework.messages import ( BIP125_SEQUENCE_NUMBER, COIN, @@ -39,6 +38,7 @@ assert_raises_rpc_error, ) from test_framework.wallet import MiniWallet +from test_framework.wallet_util import generate_keypair class MempoolAcceptanceTest(BitcoinTestFramework): @@ -297,9 +297,7 @@ def run_test(self): rawtxs=[tx.serialize().hex()], ) tx = tx_from_hex(raw_tx_reference) - key = ECKey() - key.generate() - pubkey = key.get_pubkey().get_bytes() + _, pubkey = generate_keypair() tx.vout[0].scriptPubKey = keys_to_multisig_script([pubkey] * 3, k=2) # Some bare multisig script (2-of-3) self.check_mempool_result( result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'bare-multisig'}], diff --git a/test/functional/mempool_dust.py b/test/functional/mempool_dust.py index 1339b1cba99d..bdc1bb84ad34 100755 --- a/test/functional/mempool_dust.py +++ b/test/functional/mempool_dust.py @@ -5,7 +5,6 @@ """Test dust limit mempool policy (`-dustrelayfee` parameter)""" from decimal import Decimal -from test_framework.key import ECKey from test_framework.messages import ( COIN, CTxOut, @@ -28,6 +27,7 @@ get_fee, ) from test_framework.wallet import MiniWallet +from test_framework.wallet_util import generate_keypair DUST_RELAY_TX_FEE = 3000 # default setting [sat/kvB] @@ -70,11 +70,8 @@ def run_test(self): self.wallet = MiniWallet(self.nodes[0]) # prepare output scripts of each standard type - key = ECKey() - key.generate(compressed=False) - uncompressed_pubkey = key.get_pubkey().get_bytes() - key.generate(compressed=True) - pubkey = key.get_pubkey().get_bytes() + _, uncompressed_pubkey = generate_keypair(compressed=False) + _, pubkey = generate_keypair(compressed=True) output_scripts = ( (key_to_p2pk_script(uncompressed_pubkey), "P2PK (uncompressed)"), diff --git a/test/functional/p2p_blockfilters.py b/test/functional/p2p_blockfilters.py index b28bd45e635d..8dfc82cb484a 100755 --- a/test/functional/p2p_blockfilters.py +++ b/test/functional/p2p_blockfilters.py @@ -38,7 +38,7 @@ assert_equal, assert_greater_than, ) -from test_framework.key import ECKey +from test_framework.wallet_util import generate_keypair from decimal import Decimal class FiltersClient(P2PInterface): @@ -309,9 +309,7 @@ def create_simple_assetlock(self, base_tx, base_tx_value, amount_locked_1, amoun continue # Create a key for the credit outputs - key = ECKey() - key.generate() - pubkey = key.get_pubkey().get_bytes() + _, pubkey = generate_keypair() credit_outputs.append(CTxOut(int(amount * COIN), key_to_p2pkh_script(pubkey))) # AssetLock payload diff --git a/test/functional/rpc_createmultisig.py b/test/functional/rpc_createmultisig.py index bd83b446a91f..7d5bceb40ce1 100755 --- a/test/functional/rpc_createmultisig.py +++ b/test/functional/rpc_createmultisig.py @@ -11,13 +11,13 @@ from test_framework.address import address_to_scriptpubkey from test_framework.authproxy import JSONRPCException from test_framework.descriptors import descsum_create, drop_origins -from test_framework.key import ECPubKey, ECKey +from test_framework.key import ECPubKey from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_raises_rpc_error, assert_equal, ) -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair from test_framework.wallet import ( MiniWallet, getnewdestination, @@ -37,10 +37,9 @@ def get_keys(self): self.priv = [] node0, node1, node2 = self.nodes for _ in range(self.nkeys): - k = ECKey() - k.generate() - self.pub.append(k.get_pubkey().get_bytes().hex()) - self.priv.append(bytes_to_wif(k.get_bytes(), k.is_compressed)) + privkey, pubkey = generate_keypair(wif=True) + self.pub.append(pubkey.hex()) + self.priv.append(privkey) if self.is_bdb_compiled(): self.final = node2.getnewaddress() else: diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 1e3063ae743e..2b24ddf36c02 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -9,7 +9,6 @@ from itertools import product from test_framework.descriptors import descsum_create -from test_framework.key import ECKey from test_framework.messages import ( COutPoint, CTransaction, @@ -38,8 +37,8 @@ random_bytes, ) from test_framework.wallet_util import ( - bytes_to_wif, - get_generate_key + generate_keypair, + get_generate_key, ) import json @@ -589,9 +588,7 @@ def test_psbt_input_keys(psbt_input, keys): self.log.info("Test that we can fund psbts with external inputs specified") - eckey = ECKey() - eckey.generate() - privkey = bytes_to_wif(eckey.get_bytes()) + privkey, _ = generate_keypair(wif=True) self.nodes[1].createwallet("extfund") wallet = self.nodes[1].get_wallet_rpc("extfund") diff --git a/test/functional/test_framework/wallet.py b/test/functional/test_framework/wallet.py index cfdf89412948..da59bf3b4940 100644 --- a/test/functional/test_framework/wallet.py +++ b/test/functional/test_framework/wallet.py @@ -17,6 +17,7 @@ key_to_p2pkh, ADDRESS_BCRT1_P2SH_OP_TRUE, ) +from test_framework.blocktools import COINBASE_MATURITY from test_framework.descriptors import descsum_create from test_framework.key import ECKey from test_framework.messages import ( @@ -41,7 +42,7 @@ assert_equal, assert_greater_than_or_equal, ) -from test_framework.blocktools import COINBASE_MATURITY +from test_framework.wallet_util import generate_keypair DEFAULT_FEE = Decimal("0.0001") @@ -378,9 +379,7 @@ def getnewdestination(address_type='legacy'): 'legacy'. Can be used when a random destination is needed, but no compiled wallet is available (e.g. as replacement to the getnewaddress/getaddressinfo RPCs).""" - key = ECKey() - key.generate() - pubkey = key.get_pubkey().get_bytes() + key, pubkey = generate_keypair() if address_type == 'legacy': scriptpubkey = key_to_p2pkh_script(pubkey) address = key_to_p2pkh(pubkey) diff --git a/test/functional/test_framework/wallet_util.py b/test/functional/test_framework/wallet_util.py index 64b6c593312e..c758966ef148 100755 --- a/test/functional/test_framework/wallet_util.py +++ b/test/functional/test_framework/wallet_util.py @@ -43,12 +43,9 @@ def get_generate_key(): """Generate a fresh key Returns a named tuple of privkey, pubkey and all address and scripts.""" - eckey = ECKey() - eckey.generate() - privkey = bytes_to_wif(eckey.get_bytes()) - pubkey = eckey.get_pubkey().get_bytes().hex() + privkey, pubkey = generate_keypair(wif=True) return Key(privkey=privkey, - pubkey=pubkey, + pubkey=pubkey.hex(), p2pkh_script=key_to_p2pkh_script(pubkey).hex(), p2pkh_addr=key_to_p2pkh(pubkey)) @@ -84,8 +81,14 @@ def bytes_to_wif(b, compressed=True): b += b'\x01' return byte_to_base58(b, 239) -def generate_wif_key(): - # Makes a WIF privkey for imports - k = ECKey() - k.generate() - return bytes_to_wif(k.get_bytes(), k.is_compressed) +def generate_keypair(compressed=True, wif=False): + """Generate a new random keypair and return the corresponding ECKey / + bytes objects. The private key can also be provided as WIF (wallet + import format) string instead, which is often useful for wallet RPC + interaction.""" + privkey = ECKey() + privkey.generate(compressed) + pubkey = privkey.get_pubkey().get_bytes() + if wif: + privkey = bytes_to_wif(privkey.get_bytes(), compressed) + return privkey, pubkey diff --git a/test/functional/wallet_blank.py b/test/functional/wallet_blank.py index 69927dcc3074..dc466d3ab91e 100755 --- a/test/functional/wallet_blank.py +++ b/test/functional/wallet_blank.py @@ -10,11 +10,10 @@ ADDRESS_BCRT1_UNSPENDABLE, ADDRESS_BCRT1_UNSPENDABLE_DESCRIPTOR, ) -from test_framework.key import ECKey from test_framework.util import ( assert_equal, ) -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair class WalletBlankTest(BitcoinTestFramework): @@ -50,10 +49,8 @@ def test_importpubkey(self): assert_equal(info["descriptors"], False) assert_equal(info["blank"], True) - eckey = ECKey() - eckey.generate(compressed=comp) - - wallet.importpubkey(eckey.get_pubkey().get_bytes().hex()) + _, pubkey = generate_keypair(compressed=comp) + wallet.importpubkey(pubkey.hex()) assert_equal(wallet.getwalletinfo()["blank"], False) def test_importprivkey(self): @@ -67,10 +64,7 @@ def test_importprivkey(self): assert_equal(info["descriptors"], False) assert_equal(info["blank"], True) - eckey = ECKey() - eckey.generate(compressed=comp) - wif = bytes_to_wif(eckey.get_bytes(), eckey.is_compressed) - + wif, _ = generate_keypair(compressed=comp, wif=True) wallet.importprivkey(wif) assert_equal(wallet.getwalletinfo()["blank"], False) diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py index 03844d880e7b..eda81f184c0a 100755 --- a/test/functional/wallet_createwallet.py +++ b/test/functional/wallet_createwallet.py @@ -7,13 +7,13 @@ from test_framework.address import key_to_p2pkh from test_framework.descriptors import descsum_create -from test_framework.key import ECKey from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_raises_rpc_error, ) -from test_framework.wallet_util import bytes_to_wif, generate_wif_key +from test_framework.wallet_util import generate_keypair + EMPTY_PASSPHRASE_MSG = "Empty string given as passphrase, wallet will not be encrypted." @@ -50,14 +50,12 @@ def run_test(self): w1.importpubkey(w0.getaddressinfo(address1)['pubkey']) self.log.info('Test that private keys cannot be imported') - eckey = ECKey() - eckey.generate() - privkey = bytes_to_wif(eckey.get_bytes()) + privkey, pubkey = generate_keypair(wif=True) assert_raises_rpc_error(-4, 'Cannot import private keys to a wallet with private keys disabled', w1.importprivkey, privkey) if self.options.descriptors: result = w1.importdescriptors([{'desc': descsum_create('pkh(' + privkey + ')'), 'timestamp': 'now'}]) else: - result = w1.importmulti([{'scriptPubKey': {'address': key_to_p2pkh(eckey.get_pubkey().get_bytes())}, 'timestamp': 'now', 'keys': [privkey]}]) + result = w1.importmulti([{'scriptPubKey': {'address': key_to_p2pkh(pubkey)}, 'timestamp': 'now', 'keys': [privkey]}]) assert not result[0]['success'] assert 'warnings' not in result[0] assert_equal(result[0]['error']['code'], -4) @@ -77,7 +75,7 @@ def run_test(self): assert_raises_rpc_error(-4, "Error: This wallet has no available keys", w3.getnewaddress) assert_raises_rpc_error(-4, "Error: This wallet has no available keys", w3.getrawchangeaddress) # Import private key - w3.importprivkey(generate_wif_key()) + w3.importprivkey(generate_keypair(wif=True)[0]) # Imported private keys are currently ignored by the keypool assert_equal(w3.getwalletinfo()['keypoolsize'], 0) if self.options.descriptors: diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index ca2a8efb161b..06922aad6d7a 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -9,7 +9,6 @@ from itertools import product from test_framework.descriptors import descsum_create -from test_framework.key import ECKey from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_approx, @@ -22,7 +21,7 @@ get_fee, satoshi_round, ) -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair ERR_NOT_ENOUGH_PRESET_INPUTS = "The preselected coins total amount does not cover the transaction target. " \ "Please allow other inputs to be automatically selected or include more coins manually" @@ -958,11 +957,7 @@ def test_subtract_fee_with_presets(self): def test_external_inputs(self): self.log.info("Test funding with external inputs") - - eckey = ECKey() - eckey.generate() - privkey = bytes_to_wif(eckey.get_bytes()) - + privkey, _ = generate_keypair(wif=True) self.nodes[2].createwallet("extfund") wallet = self.nodes[2].get_wallet_rpc("extfund") diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py index d7ef3f106f70..7bfcc2c253da 100755 --- a/test/functional/wallet_importprunedfunds.py +++ b/test/functional/wallet_importprunedfunds.py @@ -7,7 +7,6 @@ from test_framework.address import key_to_p2pkh from test_framework.blocktools import COINBASE_MATURITY -from test_framework.key import ECKey from test_framework.messages import ( CMerkleBlock, from_hex, @@ -17,7 +16,7 @@ assert_equal, assert_raises_rpc_error, ) -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair class ImportPrunedFundsTest(BitcoinTestFramework): @@ -42,10 +41,8 @@ def run_test(self): # pubkey address2 = self.nodes[0].getnewaddress() # privkey - eckey = ECKey() - eckey.generate() - address3_privkey = bytes_to_wif(eckey.get_bytes()) - address3 = key_to_p2pkh(eckey.get_pubkey().get_bytes()) + address3_privkey, address3_pubkey = generate_keypair(wif=True) + address3 = key_to_p2pkh(address3_pubkey) self.nodes[0].importprivkey(address3_privkey) # Check only one address diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py index 9a263ed81d52..218b96f160d6 100755 --- a/test/functional/wallet_listsinceblock.py +++ b/test/functional/wallet_listsinceblock.py @@ -7,7 +7,6 @@ from test_framework.address import key_to_p2pkh from test_framework.blocktools import COINBASE_MATURITY from test_framework.descriptors import descsum_create -from test_framework.key import ECKey from test_framework.test_framework import BitcoinTestFramework from test_framework.messages import BIP125_SEQUENCE_NUMBER from test_framework.util import ( @@ -15,7 +14,7 @@ assert_equal, assert_raises_rpc_error, ) -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair from decimal import Decimal @@ -202,10 +201,8 @@ def test_double_spend(self): self.sync_all() # share utxo between nodes[1] and nodes[2] - eckey = ECKey() - eckey.generate() - privkey = bytes_to_wif(eckey.get_bytes()) - address = key_to_p2pkh(eckey.get_pubkey().get_bytes()) + privkey, pubkey = generate_keypair(wif=True) + address = key_to_p2pkh(pubkey) self.nodes[2].sendtoaddress(address, 10) self.generate(self.nodes[2], 6) self.nodes[2].importprivkey(privkey) diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index e1c2d8011e0c..c974ad6d77c0 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -9,7 +9,6 @@ from test_framework.authproxy import JSONRPCException from test_framework.descriptors import descsum_create -from test_framework.key import ECKey from test_framework.messages import ( ser_compact_size, ) @@ -20,7 +19,8 @@ assert_greater_than, assert_raises_rpc_error, ) -from test_framework.wallet_util import bytes_to_wif +from test_framework.wallet_util import generate_keypair + class WalletSendTest(BitcoinTestFramework): def add_options(self, parser): @@ -484,9 +484,7 @@ def run_test(self): assert res["complete"] self.log.info("External outputs") - eckey = ECKey() - eckey.generate() - privkey = bytes_to_wif(eckey.get_bytes()) + privkey, _ = generate_keypair(wif=True) self.nodes[1].createwallet("extsend") ext_wallet = self.nodes[1].get_wallet_rpc("extsend") From 1a11c222678941e107a92e3ff3a44460b8694f16 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 30 Jun 2023 16:19:16 +0100 Subject: [PATCH 7/9] Merge bitcoin/bitcoin#28009: script, test: python typing and linter updates 6c97757a480b6e71a0750330d69ff18ac7cc6127 script: appease spelling linter (Jon Atack) 1316119ce7ba3de4135bbf1e5ac28c9ea26f62e1 script: update ignored-words.txt (Jon Atack) 146c861da2e4236997bee3eed6110a5016a8b86b script: update linter dependencies (Jon Atack) 92408224a4cb2f454465d5ff8445c247f2c4318a test: fix PEP484 no implicit optional argument types errors (Jon Atack) f86a3014338de6a2204bbdda10795b75ef6654c0 script, test: add missing python type annotations (Jon Atack) Pull request description: With these updates, `./test/lint/lint-python.py` and `./test/lint/lint-spelling.py` should be green again for developers using relatively recent Python dependencies, in particular mypy 0.991 (released 11/2022) and later. Please see the commit messages for details. ACKs for top commit: fanquake: ACK 6c97757a480b6e71a0750330d69ff18ac7cc6127 Tree-SHA512: 8a46a4d36d5978affdcecf4f2ace20ca1b52d483e098304911a2169afe60ccb9b042fa90c04b762d94f3ce53d2cafe6f24476ae839867a770c7f31e7e7242d99 Co-authored-by: fanquake --- src/addrdb.cpp | 2 +- test/functional/interface_rest.py | 3 ++- test/functional/p2p_message_capture.py | 2 +- test/functional/test_framework/coverage.py | 3 ++- test/functional/test_framework/test_framework.py | 8 ++++---- test/functional/test_framework/util.py | 2 +- test/lint/spelling.ignore-words.txt | 2 ++ 7 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/addrdb.cpp b/src/addrdb.cpp index 87690501f7fc..a1a536ea2b4f 100644 --- a/src/addrdb.cpp +++ b/src/addrdb.cpp @@ -226,7 +226,7 @@ util::Result> LoadAddrman(const NetGroupManager& netgro return util::Error{strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."), e.what(), PACKAGE_BUGREPORT, fs::quoted(fs::PathToString(path_addr)))}; } - return {std::move(addrman)}; // std::move should be unneccessary but is temporarily needed to work around clang bug + return {std::move(addrman)}; // std::move should be unnecessary but is temporarily needed to work around clang bug // (https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1561270092) } diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py index 37e47ed9575c..43af348815b1 100755 --- a/test/functional/interface_rest.py +++ b/test/functional/interface_rest.py @@ -26,6 +26,7 @@ MiniWallet, getnewdestination, ) +from typing import Optional INVALID_PARAM = "abc" @@ -63,7 +64,7 @@ def test_rest_request( body: str = '', status: int = 200, ret_type: RetType = RetType.JSON, - query_params: typing.Dict[str, typing.Any] = None, + query_params: Optional[typing.Dict[str, typing.Any]] = None, ) -> typing.Union[http.client.HTTPResponse, bytes, str, None]: rest_uri = '/rest' + uri if req_type in ReqType: diff --git a/test/functional/p2p_message_capture.py b/test/functional/p2p_message_capture.py index 62ff4da8fd15..691a0b6409c8 100755 --- a/test/functional/p2p_message_capture.py +++ b/test/functional/p2p_message_capture.py @@ -19,7 +19,7 @@ LENGTH_SIZE = 4 MSGTYPE_SIZE = 12 -def mini_parser(dat_file): +def mini_parser(dat_file: str) -> None: """Parse a data file created by CaptureMessageToFile. From the data file we'll only check the structure. diff --git a/test/functional/test_framework/coverage.py b/test/functional/test_framework/coverage.py index 72d7902880e8..e2bfd30ec26d 100644 --- a/test/functional/test_framework/coverage.py +++ b/test/functional/test_framework/coverage.py @@ -11,6 +11,7 @@ import os from .authproxy import AuthServiceProxy +from typing import Optional REFERENCE_FILENAME = 'rpc_interface.txt' @@ -20,7 +21,7 @@ class AuthServiceProxyWrapper(): An object that wraps AuthServiceProxy to record specific RPC calls. """ - def __init__(self, auth_service_proxy_instance: AuthServiceProxy, rpc_url: str, coverage_logfile: str=None): + def __init__(self, auth_service_proxy_instance: AuthServiceProxy, rpc_url: str, coverage_logfile: Optional[str]=None): """ Kwargs: auth_service_proxy_instance: the instance being wrapped. diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 6c27bffc28b6..9ed5fca5b641 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -23,7 +23,7 @@ import time from concurrent.futures import ThreadPoolExecutor -from typing import List, Optional, Union +from typing import Any, List, Optional, Union from .address import ADDRESS_BCRT1_P2SH_OP_TRUE from .authproxy import JSONRPCException from test_framework.masternodes import check_banned, check_punished @@ -118,14 +118,14 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): chain = None # type: str setup_clean_chain = None # type: bool - def __init__(self): + def __init__(self) -> None: """Sets test framework defaults. Do not override this method. Instead, override the set_test_params() method""" self.chain: str = 'regtest' self.setup_clean_chain: bool = False self.disable_mocktime: bool = False self.noban_tx_relay: bool = False self.nodes: List[TestNode] = [] - self.extra_args = None + self.extra_args: Any = None self.network_thread = None self.mocktime = 0 self.rpc_timeout = 60 # Wait for up to 60 seconds for the RPC server to respond @@ -134,7 +134,7 @@ def __init__(self): self.parse_args() self.default_wallet_name = "default_wallet" if self.options.descriptors else "" self.wallet_data_filename = "wallet.dat" - self.extra_args_from_options = [] + self.extra_args_from_options: List[str] = [] # Optional list of wallet names that can be set in set_test_params to # create and import keys to. If unset, default is len(nodes) * # [default_wallet_name]. If wallet names are None, wallet creation is diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 4c6d134018dc..eb095994f333 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -341,7 +341,7 @@ class PortSeed: n = None -def get_rpc_proxy(url: str, node_number: int, *, timeout: int=None, coveragedir: str=None) -> coverage.AuthServiceProxyWrapper: +def get_rpc_proxy(url: str, node_number: int, *, timeout: Optional[int]=None, coveragedir: Optional[str]=None) -> coverage.AuthServiceProxyWrapper: """ Args: url: URL of the RPC server to call diff --git a/test/lint/spelling.ignore-words.txt b/test/lint/spelling.ignore-words.txt index 708d27c5366d..e2fff2836987 100644 --- a/test/lint/spelling.ignore-words.txt +++ b/test/lint/spelling.ignore-words.txt @@ -1,3 +1,4 @@ +afile asend ba blockin @@ -17,6 +18,7 @@ keypair mor nd nin +requestor ser setban siz From ee3cdf060e8f44d0f25ca610cd6921769644bc66 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 16 Feb 2023 09:55:21 +0000 Subject: [PATCH 8/9] partial Merge bitcoin/bitcoin#24149: Signing support for Miniscript Descriptors BACKPORT NOTE: partial due to missing taproot; missing bitcoin#24148 ----- 6c7a17a8e0eec377f83ed1399f003ae70b898270 psbt: support externally provided preimages for Miniscript satisfaction (Antoine Poinsot) 840a396029316896beda46600aec3c1af09a899c qa: add a "smart" Miniscript fuzz target (Antoine Poinsot) 17e3547241d593bc92c5c6b36c54284d9d9f3feb qa: add a fuzz target generating random nodes from a binary encoding (Antoine Poinsot) 611e12502a5887ffb751bb92fadaa334d484824b qa: functional test Miniscript signing with key and timelocks (Antoine Poinsot) d57b7f2021d2369f6e88cdf0f562aab27c51beaf refactor: make descriptors in Miniscript functional test more readable (Antoine Poinsot) 0a8fc9e200b5018c1efd6f9126eb405ca0beeea3 wallet: check solvability using descriptor in AvailableCoins (Antoine Poinsot) 560e62b1e221832ae99ff8684559a7b8f9df84a7 script/sign: signing support for Miniscripts with hash preimage challenges (Antoine Poinsot) a2f81b6a8f1ff3b0750711409c7538812a52ef40 script/sign: signing support for Miniscript with timelocks (Antoine Poinsot) 61c6d1a8440db09c44d7fd367a6f2c641ea93d40 script/sign: basic signing support for Miniscript descriptors (Antoine Poinsot) 4242c1c52127df3a24be0c15b88d4fc463af04fc Align 'e' property of or_d and andor with website spec (Pieter Wuille) f5deb417804b9f267830bd40177677987df4526d Various additional explanations of the satisfaction logic from Pieter (Pieter Wuille) 22c5b00345063bdeb8b6d3da8b5692d18f92bfb7 miniscript: satisfaction support (Antoine Poinsot) Pull request description: This makes the Miniscript descriptors solvable. Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR. ACKs for top commit: achow101: ACK 6c7a17a8e0eec377f83ed1399f003ae70b898270 sipa: utACK 6c7a17a8e0eec377f83ed1399f003ae70b898270 (to the extent that it's not my own code). Tree-SHA512: a71ec002aaf66bd429012caa338fc58384067bcd2f453a46e21d381ed1bacc8e57afb9db57c0fb4bf40de43b30808815e9ebc0ae1fbd9e61df0e7b91a17771cc Co-authored-by: fanquake --- src/script/miniscript.cpp | 76 ++++++++++++++++++- src/script/miniscript.h | 73 ++++++++++++++++++ src/script/sign.cpp | 98 +++++++++++++++++++++++-- src/script/sign.h | 25 ++++++- src/test/descriptor_tests.cpp | 30 ++++++-- src/test/evo_deterministicmns_tests.cpp | 3 +- src/test/fuzz/script_sign.cpp | 6 +- src/test/multisig_tests.cpp | 3 +- src/test/orphanage_tests.cpp | 6 +- src/test/script_p2sh_tests.cpp | 15 ++-- src/test/script_tests.cpp | 15 ++-- src/test/txvalidation_tests.cpp | 3 +- src/wallet/spend.cpp | 4 +- 13 files changed, 319 insertions(+), 38 deletions(-) diff --git a/src/script/miniscript.cpp b/src/script/miniscript.cpp index 5e471cbe89e8..3937638cf80a 100644 --- a/src/script/miniscript.cpp +++ b/src/script/miniscript.cpp @@ -172,8 +172,8 @@ Type ComputeType(Fragment fragment, Type x, Type y, Type z, const std::vector::max(); + has_sig = false; + malleable = false; + non_canon = false; + } + return *this; +} + +InputStack& InputStack::SetWithSig() { + has_sig = true; + return *this; +} + +InputStack& InputStack::SetNonCanon() { + non_canon = true; + return *this; +} + +InputStack& InputStack::SetMalleable(bool x) { + malleable = x; + return *this; +} + +InputStack operator+(InputStack a, InputStack b) { + a.stack = Cat(std::move(a.stack), std::move(b.stack)); + if (a.available != Availability::NO && b.available != Availability::NO) a.size += b.size; + a.has_sig |= b.has_sig; + a.malleable |= b.malleable; + a.non_canon |= b.non_canon; + if (a.available == Availability::NO || b.available == Availability::NO) { + a.SetAvailable(Availability::NO); + } else if (a.available == Availability::MAYBE || b.available == Availability::MAYBE) { + a.SetAvailable(Availability::MAYBE); + } + return a; +} + +InputStack operator|(InputStack a, InputStack b) { + // If only one is invalid, pick the other one. If both are invalid, pick an arbitrary one. + if (a.available == Availability::NO) return b; + if (b.available == Availability::NO) return a; + // If only one of the solutions has a signature, we must pick the other one. + if (!a.has_sig && b.has_sig) return a; + if (!b.has_sig && a.has_sig) return b; + if (!a.has_sig && !b.has_sig) { + // If neither solution requires a signature, the result is inevitably malleable. + a.malleable = true; + b.malleable = true; + } else { + // If both options require a signature, prefer the non-malleable one. + if (b.malleable && !a.malleable) return a; + if (a.malleable && !b.malleable) return b; + } + // Between two malleable or two non-malleable solutions, pick the smaller one between + // YESes, and the bigger ones between MAYBEs. Prefer YES over MAYBE. + if (a.available == Availability::YES && b.available == Availability::YES) { + return std::move(a.size <= b.size ? a : b); + } else if (a.available == Availability::MAYBE && b.available == Availability::MAYBE) { + return std::move(a.size >= b.size ? a : b); + } else if (a.available == Availability::YES) { + return a; + } else { + return b; + } +} + std::optional> DecomposeScript(const CScript& script) { std::vector out; diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 21755d76838c..603b18174aca 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -223,6 +223,11 @@ enum class Fragment { // WRAP_U(X) is represented as OR_I(X,0) }; +enum class Availability { + NO, + YES, + MAYBE, +}; namespace internal { @@ -235,6 +240,62 @@ size_t ComputeScriptLen(Fragment fragment, Type sub0typ, size_t subsize, uint32_ //! A helper sanitizer/checker for the output of CalcType. Type SanitizeType(Type x); +//! An object representing a sequence of witness stack elements. +struct InputStack { + /** Whether this stack is valid for its intended purpose (satisfaction or dissatisfaction of a Node). + * The MAYBE value is used for size estimation, when keys/preimages may actually be unavailable, + * but may be available at signing time. This makes the InputStack structure and signing logic, + * filled with dummy signatures/preimages usable for witness size estimation. + */ + Availability available = Availability::YES; + //! Whether this stack contains a digital signature. + bool has_sig = false; + //! Whether this stack is malleable (can be turned into an equally valid other stack by a third party). + bool malleable = false; + //! Whether this stack is non-canonical (using a construction known to be unnecessary for satisfaction). + //! Note that this flag does not affect the satisfaction algorithm; it is only used for sanity checking. + bool non_canon = false; + //! Serialized witness size. + size_t size = 0; + //! Data elements. + std::vector> stack; + //! Construct an empty stack (valid). + InputStack() {} + //! Construct a valid single-element stack (with an element up to 75 bytes). + InputStack(std::vector in) : size(in.size() + 1), stack(Vector(std::move(in))) {} + //! Change availability + InputStack& SetAvailable(Availability avail); + //! Mark this input stack as having a signature. + InputStack& SetWithSig(); + //! Mark this input stack as non-canonical (known to not be necessary in non-malleable satisfactions). + InputStack& SetNonCanon(); + //! Mark this input stack as malleable. + InputStack& SetMalleable(bool x = true); + //! Concatenate two input stacks. + friend InputStack operator+(InputStack a, InputStack b); + //! Choose between two potential input stacks. + friend InputStack operator|(InputStack a, InputStack b); +}; + +/** A stack consisting of a single zero-length element (interpreted as 0 by the script interpreter in numeric context). */ +static const auto ZERO = InputStack(std::vector()); +/** A stack consisting of a single malleable 32-byte 0x0000...0000 element (for dissatisfying hash challenges). */ +static const auto ZERO32 = InputStack(std::vector(32, 0)).SetMalleable(); +/** A stack consisting of a single 0x01 element (interpreted as 1 by the script interpreted in numeric context). */ +static const auto ONE = InputStack(Vector((unsigned char)1)); +/** The empty stack. */ +static const auto EMPTY = InputStack(); +/** A stack representing the lack of any (dis)satisfactions. */ +static const auto INVALID = InputStack().SetAvailable(Availability::NO); + +//! A pair of a satisfaction and a dissatisfaction InputStack. +struct InputResult { + InputStack nsat, sat; + + template + InputResult(A&& in_nsat, B&& in_sat) : nsat(std::forward(in_nsat)), sat(std::forward(in_sat)) {} +}; + //! Class whose objects represent the maximum of a list of integers. template struct MaxInt { @@ -842,6 +903,18 @@ struct Node { //! Check whether this node is safe as a script on its own. bool IsSane() const { return IsValidTopLevel() && IsSaneSubexpression() && NeedsSignature(); } + //! Produce a witness for this script, if possible and given the information available in the context. + //! The non-malleable satisfaction is guaranteed to be valid if it exists, and ValidSatisfaction() + //! is true. If IsSane() holds, this satisfaction is guaranteed to succeed in case the node's + //! conditions are satisfied (private keys and hash preimages available, locktimes satsified). + template + Availability Satisfy(const Ctx& ctx, std::vector>& stack, bool nonmalleable = true) const { + auto ret = ProduceInput(ctx); + if (nonmalleable && (ret.sat.malleable || !ret.sat.has_sig)) return Availability::NO; + stack = std::move(ret.sat.stack); + return ret.sat.available; + } + //! Equality testing. bool operator==(const Node& arg) const { return Compare(*this, arg) == 0; } diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 8df3235a24d8..3993702e839b 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -10,6 +10,7 @@ #include #include #include