From f55ad1d127638776b6479fd402329779c7070866 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Tue, 13 Oct 2026 14:21:33 -0500 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#28139: test: create wallet specific for test_locked_wallet case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit c648bdbda21c7ae90c6b40e506ca4ed62b1dbb6c test: create wallet specific for test_locked_wallet case (furszy) Pull request description: Coming from https://github.com/bitcoin/bitcoin/pull/28089#discussion_r1265478128. Several test cases are relying on the node1 default wallet, which thanks to 'test_locked_wallet' is encrypted. And can be only accessed within a specific timeframe (100ms), a duration internally set by the same test. This situation introduces a potential race condition, where other tests must complete their operations within the specified 100ms window to pass (otherwise the wallet gets re-locked and they fail). This can be seen running the test in valgrind (https://github.com/bitcoin/bitcoin/pull/28089), where other test cases fail due the wallet re-locking itself after the 100ms. ACKs for top commit: MarcoFalke: lgtm ACK c648bdbda21c7ae90c6b40e506ca4ed62b1dbb6c ishaanam: utACK c648bdbda21c7ae90c6b40e506ca4ed62b1dbb6c Tree-SHA512: 01cde5a4a0cb3405adb9ea3c1f73841f3fa237d1162268ed06f0d49ca38541006b423a029e0b5e5955e1aa7e018c4600d894e555a68cf17ff60a4b8be58f4aa9 Dash adaptations: - test/functional/wallet_fundrawtransaction.py: kept Dash's `pkh(...)` descriptors instead of upstream's `wpkh(...)` (Dash has no segwit) - test/functional/wallet_fundrawtransaction.py: kept Dash's `walletpassphrase(..., 999000)` timeouts instead of upstream's 10/100/600 — Dash already carries the later bitcoin#28403 which bumped these, so restoring the small values would regress it - test/functional/wallet_fundrawtransaction.py: kept Dash's amounts (sendtoaddress 12, output 11, balance delta 511.0000000) rather than upstream's 1.2/1.1/51.10000000 — Dash's block subsidy and this test's amounts are 10x - test/functional/wallet_fundrawtransaction.py: did not add upstream's `wallet.getrawchangeaddress()` (keypool drain) or `wallet.keypoolrefill(8)` lines — those lines do not exist in Dash's copy of this test (they were never introduced in Dash history); only the self.nodes[1]->wallet rename part of those upstream hunks applies - test/functional/wallet_fundrawtransaction.py: changeless-tx fee deduction is `0.00000500` instead of upstream's `0.00002200`. Upstream's constant is exactly the fee of a 110-vB segwit tx at Bitcoin's test fallbackfee of 0.0002/kB; Dash's test fallbackfee/min relay fee is 0.00001/kB (1 duff/byte) and the tx is a legacy 1-in/1-out ~192 bytes, so the required fee is ~192 duffs. With 2200 deducted the 2008-duff remainder exceeds min_viable_change (discard fee 10000/kB * 148 B + 1 = 1481 duffs), so the wallet would demand a change output and `fundrawtransaction` would fail with -4 on the locked wallet instead of returning changepos -1. 500 duffs covers the fee and leaves 308 < 1481, giving the changeless tx upstream intends. Dash's old 2200 (with its stale 'bnb doesn't work same way as in bitcoin' comment) was sized for the previous multi-input selection and no longer applies now that the wallet holds exactly one UTXO; replaced by upstream's comments plus a one-line note about the value. (cherry picked from commit 5df4af4c3bb41f9e042b72528f23aa531c7e7bc4) --- test/functional/wallet_fundrawtransaction.py | 62 +++++++++++++------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index f1e6749dccec..655a421787d0 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -547,11 +547,20 @@ def test_spend_2of2(self): def test_locked_wallet(self): self.log.info("Test fundrawtxn with locked wallet and hardened derivation") - self.nodes[1].encryptwallet("test") + df_wallet = self.nodes[1].get_wallet_rpc(self.default_wallet_name) + self.nodes[1].createwallet(wallet_name="locked_wallet", descriptors=self.options.descriptors) + wallet = self.nodes[1].get_wallet_rpc("locked_wallet") + + # Add some balance to the wallet (this will be reverted at the end of the test) + df_wallet.sendall(recipients=[wallet.getnewaddress()]) + self.generate(self.nodes[1], 1) + + # Encrypt wallet and import descriptors + wallet.encryptwallet("test") if self.options.descriptors: - self.nodes[1].walletpassphrase('test', 999000) - self.nodes[1].importdescriptors([{ + wallet.walletpassphrase('test', 999000) + wallet.importdescriptors([{ 'desc': descsum_create('pkh(tprv8ZgxMBicQKsPdYeeZbPSKd2KYLmeVKtcFA7kqCxDvDR13MQ6us8HopUR2wLcS2ZKPhLyKsqpDL2FtL73LMHcgoCL7DXsciA8eX8nbjCR2eG/0h/*h)'), 'timestamp': 'now', 'active': True @@ -562,48 +571,57 @@ def test_locked_wallet(self): 'active': True, 'internal': True }]) - self.nodes[1].walletlock() + wallet.walletlock() # Drain the keypool. - self.nodes[1].getnewaddress() - - # Choose 2 inputs - # bnb doesn't work same way as in bitcoin, so, `value` is also calculated by different way - inputs = self.nodes[1].listunspent() - value = sum(inp["amount"] for inp in inputs) - Decimal("0.00002200") + wallet.getnewaddress() + + # Choose input + inputs = wallet.listunspent() + # Deduce fee to produce a changeless transaction + # (500 duffs is above the fee of this 1-in/1-out tx at the test feerate, + # while the remainder stays below the cost of creating a change output) + value = inputs[0]["amount"] - Decimal("0.00000500") outputs = {self.nodes[0].getnewaddress():value} - rawtx = self.nodes[1].createrawtransaction(inputs, outputs) + rawtx = wallet.createrawtransaction(inputs, outputs) # fund a transaction that does not require a new key for the change output - self.nodes[1].fundrawtransaction(rawtx) + funded_tx = wallet.fundrawtransaction(rawtx) + assert_equal(funded_tx["changepos"], -1) # fund a transaction that requires a new key for the change output # creating the key must be impossible because the wallet is locked outputs = {self.nodes[0].getnewaddress():value - Decimal("0.1")} - rawtx = self.nodes[1].createrawtransaction(inputs, outputs) - assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", self.nodes[1].fundrawtransaction, rawtx) + rawtx = wallet.createrawtransaction(inputs, outputs) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", wallet.fundrawtransaction, rawtx) # Refill the keypool. - self.nodes[1].walletpassphrase("test", 999000) - self.nodes[1].walletlock() + wallet.walletpassphrase("test", 999000) + wallet.walletlock() - assert_raises_rpc_error(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 12) + assert_raises_rpc_error(-13, "walletpassphrase", wallet.sendtoaddress, self.nodes[0].getnewaddress(), 12) oldBalance = self.nodes[0].getbalance() inputs = [] outputs = {self.nodes[0].getnewaddress():11} - rawtx = self.nodes[1].createrawtransaction(inputs, outputs) - fundedTx = self.nodes[1].fundrawtransaction(rawtx) + rawtx = wallet.createrawtransaction(inputs, outputs) + fundedTx = wallet.fundrawtransaction(rawtx) + assert fundedTx["changepos"] != -1 # Now we need to unlock. - self.nodes[1].walletpassphrase("test", 999000) - signedTx = self.nodes[1].signrawtransactionwithwallet(fundedTx['hex']) - self.nodes[1].sendrawtransaction(signedTx['hex']) + wallet.walletpassphrase("test", 999000) + signedTx = wallet.signrawtransactionwithwallet(fundedTx['hex']) + wallet.sendrawtransaction(signedTx['hex']) self.generate(self.nodes[1], 1) # Make sure funds are received at node1. assert_equal(oldBalance+Decimal('511.0000000'), self.nodes[0].getbalance()) + # Restore pre-test wallet state + wallet.sendall(recipients=[df_wallet.getnewaddress(), df_wallet.getnewaddress(), df_wallet.getnewaddress()]) + wallet.unloadwallet() + self.generate(self.nodes[1], 1) + def test_many_inputs_fee(self): """Multiple (~19) inputs tx test | Compare fee.""" self.log.info("Test fundrawtxn fee with many inputs") From d885ecd02e98adcc0def113b71591eb97ee2d71f Mon Sep 17 00:00:00 2001 From: merge-script Date: Sun, 18 Oct 2026 10:57:35 +0700 Subject: [PATCH 2/2] Merge dashpay/dash#7916: backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8cfc3c4522cac08214eacc6255d9025f6663d2fd Merge bitcoin/bitcoin#27695: test: Add test to check tx in the last block can be downloaded (DCG-Claude) 3e2077c3808617917cc3700f48de9d0935afb386 Merge bitcoin/bitcoin#28232: test: locked_wallet, skip default fee estimation (DCG-Claude) a579befd06f70089dc62921acfdc74a1824a93d2 Merge bitcoin/bitcoin#28125: wallet: bugfix, disallow migration of invalid scripts (DCG-Claude) 2d7b1557844663913550b307658181bb0371602e Merge bitcoin/bitcoin#27800: streams: Drop confusing DataStream::Serialize method and << operator (DCG-Claude) Pull request description: > 🔎 **Needs a careful look.** Commits are ordered by how much they were adapted: 🔴 heavy first, then 🟡 light (at most two invented lines, or up to six lines counting edits of upstream functional-test or doc lines), then 🟢 faithful (identical to upstream apart from context). ## Issue being fixed or feature implemented Backports 4 Bitcoin Core v0.26 pull request(s) that the Dash queue selected, including any discovered prerequisites: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695. ## What was done? | upstream | commit | review | gates | notes | |---|---|---|---|---| | bitcoin/bitcoin#27800 | `2d7b155784` | 🔴 heavy | build:pass mech:warn pick:pass tests:pass tree:pass verify:pass | Backports bitcoin#27800: removes DataStream::Serialize and updates the upstream call sites in wallet/dump.cpp | | bitcoin/bitcoin#28232 | `3e2077c380` | 🔴 heavy | build:pass mech:warn pick:pass tests:pass tree:pass verify:pass | Backports bitcoin#28232: the locked_wallet test now sets a fixed tx fee so it no longer depends on fee estimat | | bitcoin/bitcoin#27695 | `8cfc3c4522` | 🔴 heavy | build:pass ci_upstream:warn mech:warn pick:pass tests:pass tree:pass verify:pass | Backports bitcoin#27695 in full: p2p_leak_tx.py is split into test_tx_in_block and test_notfound_on_unannounce | | bitcoin/bitcoin#28125 | `a579befd06` | 🟡 light | build:pass mech:warn pick:pass tests:pass tree:pass verify:pass | Backports bitcoin#28125 in full: legacy-wallet migration now skips invalid or non-watched watch-only scripts ( | Each commit keeps the upstream subject (`partial Merge …` only where a hunk is deferred to a prerequisite still to be backported, named in the commit body; a hunk Dash intentionally never wants is recorded in the commit body or the reviewer's note above and does not make the backport partial). Conflicts were resolved commit by commit; commits that needed no resolution were cherry-picked unchanged. ## How Has This Been Tested? Recorded per commit, at that commit's own sha, not once for the branch: - built at each commit, reusing the worktree build cache - the test suites touched by each diff run at that commit, where the diff selected one - a mechanical diff-of-diffs against the upstream patch (every upstream hunk present, no added line without an upstream counterpart, no Dash-specific line dropped) - an independent verification pass on 4 commit(s) that needed adaptation - fork gate PR thepastaclaw/dash#100 at `8cfc3c4522` — **no passing CI record at this head** Gates that did not come back clean — please weigh these: - bitcoin#27800 `mech`: **warn** — 9 invented line(s); 1 Dash-feature line(s) removed; 1 partial/prereq/low-risk finding(s) - bitcoin#28125 `mech`: **warn** — 1 edited line(s) - bitcoin#28232 `mech`: **warn** — 1 invented line(s); 1 partial/prereq/low-risk finding(s) - bitcoin#27695 `mech`: **warn** — 5 invented line(s); 1 partial/prereq/low-risk finding(s) ## Breaking Changes None beyond the upstream changes themselves. ## Checklist: _Left for the reviewer; backportsys does not tick boxes on its own behalf._ - [ ] I have performed a self-review of my own code - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ --- ### Maintainer controls Tick a box and backportsys acts on it within a few minutes, then clears the box. For anything else — a hunk to drop, a resolution to redo, a question — just leave a review comment; nothing here needs a box. - [ ] 🔒 **Hands off** — stop every automated update to this branch - [ ] 🔄 **Rebase** onto current `develop` - [ ] ❌ **Close** — abandon this batch and release its items ACKs for top commit: knst: utACK 8cfc3c4522cac08214eacc6255d9025f6663d2fd Tree-SHA512: da34d35ae7071be1d25f9bf7e8b06f94e3f81f50c40d1c278230be3af6ee37d74e86417be89409911dffc4b990eb5c35c704ffaa09d51847b6abfc307180be5f (cherry picked from commit fe232f95d6ef19c1ea2c4d3d822ffe3b67a38947) --- src/flat-database.h | 2 +- src/governance/net_governance.cpp | 4 +-- src/llmq/dkgsessionmgr.cpp | 2 +- src/llmq/net_quorum.cpp | 4 +-- src/llmq/quorums.cpp | 2 +- src/llmq/quorumsman.cpp | 2 +- src/streams.h | 8 ----- src/wallet/dump.cpp | 6 +--- src/wallet/scriptpubkeyman.cpp | 17 ++++++++++- src/wallet/scriptpubkeyman.h | 6 ++++ src/wallet/test/util.cpp | 2 +- src/wallet/wallet.cpp | 18 +++++++++-- test/functional/p2p_leak_tx.py | 32 +++++++++++++++++--- test/functional/wallet_fundrawtransaction.py | 12 +++++--- test/functional/wallet_migration.py | 31 ++++++++++++++++++- 15 files changed, 112 insertions(+), 36 deletions(-) diff --git a/src/flat-database.h b/src/flat-database.h index a55231900545..2c484e3c37e7 100644 --- a/src/flat-database.h +++ b/src/flat-database.h @@ -59,7 +59,7 @@ class CFlatDB // Write and commit header, data try { - fileout << ssObj; + fileout << Span{ssObj}; } catch (std::exception &e) { LogError("%s: Serialize or I/O error - %s\n", __func__, e.what()); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 2e8b5da79140..56d193d9ad62 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -288,7 +288,7 @@ bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgM CDataStream ss(SER_NETWORK, pfrom.GetCommonVersion()); ss.reserve(1000); if (!m_gov_manager.SerializeObjectForHash(inv.hash, ss)) return false; - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECT, ss)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECT, Span{ss})); return true; } if (inv.type == MSG_GOVERNANCE_OBJECT_VOTE) { @@ -296,7 +296,7 @@ bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgM CDataStream ss(SER_NETWORK, pfrom.GetCommonVersion()); ss.reserve(1000); if (!m_gov_manager.SerializeVoteForHash(inv.hash, ss)) return false; - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECTVOTE, ss)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECTVOTE, Span{ss})); return true; } return false; diff --git a/src/llmq/dkgsessionmgr.cpp b/src/llmq/dkgsessionmgr.cpp index 6cb65165e795..5b68ffb0059d 100644 --- a/src/llmq/dkgsessionmgr.cpp +++ b/src/llmq/dkgsessionmgr.cpp @@ -129,7 +129,7 @@ void CDKGSessionManager::WriteVerifiedVvecContribution(Consensus::LLMQType llmqT for (auto& pubkey : *vvec) { s << CBLSPublicKeyVersionWrapper(pubkey, false); } - db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), s); + db->Write(std::make_tuple(DB_VVEC, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), Span{s}); } void CDKGSessionManager::WriteVerifiedSkContribution(Consensus::LLMQType llmqType, const CBlockIndex* pQuorumBaseBlockIndex, const uint256& proTxHash, const CBLSSecretKey& skContribution) diff --git a/src/llmq/net_quorum.cpp b/src/llmq/net_quorum.cpp index 98bd4441def3..3a901b69e8d0 100644 --- a/src/llmq/net_quorum.cpp +++ b/src/llmq/net_quorum.cpp @@ -113,8 +113,8 @@ void NetQuorum::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataS } request.SetError(nError); CDataStream ssResponse{SER_NETWORK, pfrom.GetCommonVersion()}; - ssResponse << request << body; - m_connman.PushMessage(&pfrom, CNetMsgMaker(pfrom.GetCommonVersion()).Make(NetMsgType::QDATA, ssResponse)); + ssResponse << request << Span{body}; + m_connman.PushMessage(&pfrom, CNetMsgMaker(pfrom.GetCommonVersion()).Make(NetMsgType::QDATA, Span{ssResponse})); return misbehave; }; diff --git a/src/llmq/quorums.cpp b/src/llmq/quorums.cpp index 69b2100bd778..4d804c115d8f 100644 --- a/src/llmq/quorums.cpp +++ b/src/llmq/quorums.cpp @@ -200,7 +200,7 @@ void CQuorum::WriteContributions(CDBWrapper& db) const for (auto& pubkey : *quorumVvec) { s << CBLSPublicKeyVersionWrapper(pubkey, false); } - db.Write(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), s); + db.Write(std::make_pair(DB_QUORUM_QUORUM_VVEC, dbKey), Span{s}); } if (skShare.IsValid()) { db.Write(std::make_pair(DB_QUORUM_SK_SHARE, dbKey), skShare); diff --git a/src/llmq/quorumsman.cpp b/src/llmq/quorumsman.cpp index 985aa583bbc8..f529e15664fb 100644 --- a/src/llmq/quorumsman.cpp +++ b/src/llmq/quorumsman.cpp @@ -676,7 +676,7 @@ void CQuorumManager::MigrateOldQuorumDB(CEvoDB& evoDb) const if (!evoDb.GetRawDB().ReadDataStream(k, s)) { break; } - batch.Write(k, s); + batch.Write(k, Span{s}); } if (prefix == DB_QUORUM_SK_SHARE) { if (!pcursor->GetValue(sk)) { diff --git a/src/streams.h b/src/streams.h index b1d261b29935..87b16ff996ca 100644 --- a/src/streams.h +++ b/src/streams.h @@ -316,14 +316,6 @@ class DataStream vch.insert(vch.end(), src.begin(), src.end()); } - template - void Serialize(Stream& s) const - { - // Special case: stream << stream concatenates like stream += stream - if (!vch.empty()) - s.write(MakeByteSpan(vch)); - } - template DataStream& operator<<(const T& obj) { diff --git a/src/wallet/dump.cpp b/src/wallet/dump.cpp index 95b669f9382e..2b3ac280f31d 100644 --- a/src/wallet/dump.cpp +++ b/src/wallet/dump.cpp @@ -251,11 +251,7 @@ bool CreateFromDump(const ArgsManager& args, const std::string& name, const fs:: std::vector k = ParseHex(key); std::vector v = ParseHex(value); - - DataStream ss_key{k}; - DataStream ss_value{v}; - - if (!batch->Write(ss_key, ss_value)) { + if (!batch->Write(Span{k}, Span{v})) { error = strprintf(_("Error: Unable to write record to new wallet")); ret = false; break; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index ac7d41afa17a..7a99e075fa4d 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1872,8 +1872,23 @@ std::unordered_set LegacyScriptPubKeyMan::GetScriptPub } // All watchonly scripts are raw - spks.insert(setWatchOnly.begin(), setWatchOnly.end()); + for (const CScript& script : setWatchOnly) { + // As the legacy wallet allowed to import any script, we need to verify the validity here. + // LegacyScriptPubKeyMan::IsMine() return 'ISMINE_NO' for invalid or not watched scripts (IsMineResult::INVALID or IsMineResult::NO). + // e.g. a "sh(sh(pkh()))" which legacy wallets allowed to import!. + if (IsMine(script) != ISMINE_NO) spks.insert(script); + } + + return spks; +} +std::unordered_set LegacyScriptPubKeyMan::GetNotMineScriptPubKeys() const +{ + LOCK(cs_KeyStore); + std::unordered_set spks; + for (const CScript& script : setWatchOnly) { + if (IsMine(script) == ISMINE_NO) spks.insert(script); + } return spks; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index b40bdfaabb63..e074f545e1fd 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -526,6 +526,12 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv std::set GetKeys() const override; std::unordered_set GetScriptPubKeys() const override; + /** + * Retrieves scripts that were imported by bugs into the legacy spkm and are + * simply invalid, such as a sh(sh(pkh())) script, or not watched. + */ + std::unordered_set GetNotMineScriptPubKeys() const; + /** Get the DescriptorScriptPubKeyMans (with private keys) that have the same scriptPubKeys as this LegacyScriptPubKeyMan. * Does not modify this ScriptPubKeyMan. */ std::optional MigrateToDescriptor(); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 59e397f0a81b..d7402f73f283 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -75,7 +75,7 @@ std::unique_ptr DuplicateMockDatabase(WalletDatabase& database, DatabaseCursor::Status status = cursor->Next(key, value); assert(status != DatabaseCursor::Status::FAIL); if (status == DatabaseCursor::Status::DONE) break; - new_batch->Write(key, value); + new_batch->Write(Span{key}, Span{value}); } return new_database; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9dc23d31ea7f..c3f765b92df1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4655,9 +4655,7 @@ bool CWallet::MigrateToSQLite(bilingual_str& error) bool began = batch->TxnBegin(); assert(began); // This is a critical error, the new db could not be written to. The original db exists as a backup, but we should not continue execution. for (const auto& [key, value] : records) { - DataStream ss_key{key}; - DataStream ss_value{value}; - if (!batch->Write(ss_key, ss_value)) { + if (!batch->Write(MakeUCharSpan(key), MakeUCharSpan(value))) { batch->TxnAbort(); m_database->Close(); fs::remove(m_database->Filename()); @@ -4694,6 +4692,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } + // Get all invalid or non-watched scripts that will not be migrated + std::set not_migrated_dests; + for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) { + CTxDestination dest; + if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest); + } + for (auto& desc_spkm : data.desc_spkms) { if (m_spk_managers.count(desc_spkm->GetID()) > 0) { error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted."); @@ -4817,6 +4822,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) continue; } } + + // Skip invalid/non-watched scripts that will not be migrated + if (not_migrated_dests.count(addr_pair.first) > 0) { + dests_to_delete.push_back(addr_pair.first); + continue; + } + // Not ours, not in watchonly wallet, and not in solvable error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); return false; diff --git a/test/functional/p2p_leak_tx.py b/test/functional/p2p_leak_tx.py index 2ebfec9c8b86..69b6246c30c2 100755 --- a/test/functional/p2p_leak_tx.py +++ b/test/functional/p2p_leak_tx.py @@ -2,7 +2,7 @@ # Copyright (c) 2017-2020 The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. -"""Test that we don't leak txs to inbound peers that we haven't yet announced to""" +"""Test transaction upload""" from test_framework.messages import msg_getdata, CInv, MSG_TX from test_framework.p2p import p2p_lock, P2PDataStore @@ -23,16 +23,38 @@ def set_test_params(self): self.num_nodes = 1 def run_test(self): - gen_node = self.nodes[0] # The block and tx generating node - miniwallet = MiniWallet(gen_node) + self.gen_node = self.nodes[0] # The block and tx generating node + self.miniwallet = MiniWallet(self.gen_node) - inbound_peer = self.nodes[0].add_p2p_connection(P2PNode()) # An "attacking" inbound peer + self.test_tx_in_block() + self.test_notfound_on_unannounced_tx() + + def test_tx_in_block(self): + self.log.info("Check that a transaction in the last block is uploaded (beneficial for compact block relay)") + inbound_peer = self.gen_node.add_p2p_connection(P2PNode()) + + self.log.debug("Generate transaction and block") + inbound_peer.last_message.pop("inv", None) + txid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["txid"] + self.bump_mocktime(60) + inbound_peer.wait_until(lambda: "inv" in inbound_peer.last_message and inbound_peer.last_message.get("inv").inv[0].hash == int(txid, 16)) + want_tx = msg_getdata(inv=inbound_peer.last_message.get("inv").inv) + self.generate(self.gen_node, 1) + + self.log.debug("Request transaction") + inbound_peer.last_message.pop("tx", None) + inbound_peer.send_and_ping(want_tx) + assert_equal(inbound_peer.last_message.get("tx").tx.rehash(), txid) + + def test_notfound_on_unannounced_tx(self): + self.log.info("Check that we don't leak txs to inbound peers that we haven't yet announced to") + inbound_peer = self.gen_node.add_p2p_connection(P2PNode()) # An "attacking" inbound peer MAX_REPEATS = 100 self.log.info("Running test up to {} times.".format(MAX_REPEATS)) for i in range(MAX_REPEATS): self.log.info('Run repeat {}'.format(i + 1)) - txid = miniwallet.send_self_transfer(from_node=gen_node)['txid'] + txid = self.miniwallet.send_self_transfer(from_node=self.gen_node)["txid"] want_tx = msg_getdata() want_tx.inv.append(CInv(t=MSG_TX, h=int(txid, 16))) diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 655a421787d0..e024cc455efe 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -20,6 +20,7 @@ assert_raises_rpc_error, count_bytes, find_vout_for_address, + get_fee, satoshi_round, ) from test_framework.wallet_util import bytes_to_wif @@ -550,6 +551,8 @@ def test_locked_wallet(self): df_wallet = self.nodes[1].get_wallet_rpc(self.default_wallet_name) self.nodes[1].createwallet(wallet_name="locked_wallet", descriptors=self.options.descriptors) wallet = self.nodes[1].get_wallet_rpc("locked_wallet") + # This test is not meant to exercise fee estimation. Making sure all txs are sent at a consistent fee rate. + wallet.settxfee(self.min_relay_tx_fee) # Add some balance to the wallet (this will be reverted at the end of the test) df_wallet.sendall(recipients=[wallet.getnewaddress()]) @@ -578,10 +581,11 @@ def test_locked_wallet(self): # Choose input inputs = wallet.listunspent() - # Deduce fee to produce a changeless transaction - # (500 duffs is above the fee of this 1-in/1-out tx at the test feerate, - # while the remainder stays below the cost of creating a change output) - value = inputs[0]["amount"] - Decimal("0.00000500") + + # Deduce exact fee to produce a changeless transaction + tx_size = 191 # Total tx size: 191 bytes, p2pkh -> p2pkh. Input 147 bytes + rest of tx is 44 bytes. + value = inputs[0]["amount"] - get_fee(tx_size, self.min_relay_tx_fee) + outputs = {self.nodes[0].getnewaddress():value} rawtx = wallet.createrawtransaction(inputs, outputs) # fund a transaction that does not require a new key for the change output diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index af516448b2b5..f1716b0aa103 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -7,7 +7,7 @@ import os import random import shutil -from test_framework.address import key_to_p2pkh +from test_framework.address import key_to_p2pkh, script_to_p2sh from test_framework.descriptors import descsum_create from test_framework.key import ECPubKey from test_framework.test_framework import BitcoinTestFramework @@ -611,6 +611,21 @@ def send_to_script(script, amount): wallet.rpc.importaddress(address=script_pkh.hex(), label="raw_spk", rescan=True, p2sh=True) assert_equal(wallet.getbalances()['watchonly']['trusted'], 2) + # Import sh(pkh()) script, by using importaddress(), with the p2sh flag enabled. + # This will wrap the script under another sh level, which is invalid!, and store it inside the wallet. + # The migration process must skip the invalid scripts and the addressbook records linked to them. + # They are not being watched by the current wallet, nor should be watched by the migrated one. + label_sh_pkh = "raw_sh_pkh" + script_pkh = key_to_p2pkh_script(df_wallet.getaddressinfo(df_wallet.getnewaddress())["pubkey"]) + script_sh_pkh = script_to_p2sh_script(script_pkh) + addy_script_sh_pkh = script_to_p2sh(script_pkh) # valid script address + addy_script_double_sh_pkh = script_to_p2sh(script_sh_pkh) # invalid script address + + # Note: 'importaddress()' will add two scripts, a valid one sh(pkh()) and an invalid one 'sh(sh(pkh()))'. + # Both of them will be stored with the same addressbook label. And only the latter one should + # be discarded during migration. The first one must be migrated. + wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True) + # Migrate wallet and re-check balance info_migration = self.migrate_wallet(wallet) wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) @@ -620,6 +635,20 @@ def send_to_script(script, amount): # The watch-only scripts are no longer part of the main wallet assert_equal(wallet.getbalances()['mine']['trusted'], 0) + # The invalid sh(sh(pk())) script label must not be part of the main wallet anymore + assert label_sh_pkh not in wallet.listlabels() + # But, the standard sh(pkh()) script should be part of the watch-only wallet. + addrs_by_label = wallet_wo.getaddressesbylabel(label_sh_pkh) + assert addy_script_sh_pkh in addrs_by_label + assert addy_script_double_sh_pkh not in addrs_by_label + + # Also, the watch-only wallet should have the descriptor for the standard sh(pkh()) + desc = descsum_create(f"addr({addy_script_sh_pkh})") + assert next(it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc) + # And doesn't have a descriptor for the invalid one + desc_invalid = descsum_create(f"addr({addy_script_double_sh_pkh})") + assert_equal(next((it['desc'] for it in wallet_wo.listdescriptors()['descriptors'] if it['desc'] == desc_invalid), None), None) + # Just in case, also verify wallet restart self.nodes[0].unloadwallet(info_migration["watchonly_name"]) self.nodes[0].loadwallet(info_migration["watchonly_name"])