From 65b2bf8260d9fb87e699b1b75731c142212d91f9 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Sun, 20 Sep 2026 10:20:51 -0500 Subject: [PATCH 1/5] 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. --- 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 37646c0ce987b7ba23654642d13c2ff4ff767f66 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Sun, 20 Sep 2026 10:24:54 -0500 Subject: [PATCH 2/5] Merge bitcoin/bitcoin#28232: test: locked_wallet, skip default fee estimation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5364dd8666ae1afa35536f9b4fc0170eeaf37888 test: locked_wallet, skip default fee estimation (furszy) Pull request description: Coming from https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1284563239. No test case in this file is meant to exercise fee estimation. All default wallets have a custom tx fee set [here](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/test/functional/wallet_fundrawtransaction.py#L100). The only one missing is the one created for `locked_wallet`. ACKs for top commit: theStack: ACK 5364dd8666ae1afa35536f9b4fc0170eeaf37888 Tree-SHA512: 514c02708081d18330d759d10e306cee16c6350de243c68f0973777d2582f5d81968a237393c1f59aba245297e03f3f98d3ae5249a042469d0d016255f568719 Dash adaptations: - test/functional/wallet_fundrawtransaction.py: import conflict resolved by keeping Dash's existing `satoshi_round` (still used at the outputs-value assertion in test_change_position) alongside upstream's new `get_fee`, in alphabetical order - test/functional/wallet_fundrawtransaction.py: tx_size changed from upstream's 110 (p2wpkh->p2wpkh) to 192 (p2pkh->p2pkh) since Dash has no segwit — 4 version + 1 vin count + 148 input (36 prevout + 1 + 107 scriptSig + 4 sequence) + 1 vout count + 34 output + 4 locktime; 107-byte scriptSig follows from DUMMY_MAXIMUM_SIGNATURE_CREATOR(33,32) -> 72-byte DER sig plus a 33-byte compressed pubkey - test/functional/wallet_fundrawtransaction.py: dropped the Dash-only comment explaining the hardcoded 0.00000500 constant, since upstream replaces that constant with an exact get_fee() computation and the comment no longer describes the code --- test/functional/wallet_fundrawtransaction.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 655a421787d0..e8a8571f860a 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 = 192 # Total tx size: 192 bytes, p2pkh -> p2pkh. Input 148 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 From c708657cc83bfd75ba1bd6f46606116c7b276353 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Tue, 22 Sep 2026 17:43:53 -0500 Subject: [PATCH 3/5] Merge bitcoin/bitcoin#27866: blockstorage: Return on fatal flush errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit d8041d4e042957660827313951b18c8dd9a99a16 blockstorage: Return on fatal undo file flush error (TheCharlatan) f0207e00303a1030eca795ede231e3c0d94df061 blockstorage: Return on fatal block file flush error (TheCharlatan) 5671c15f4520c6dc20e0805fd0b06157ff94bcd7 blockstorage: Mark FindBlockPos as nodiscard (TheCharlatan) Pull request description: The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site. Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent database when crashing. Add `[[nodiscard]]` annotations to them such that they are not ignored in future. Functions that call either `FlushUndoFile` or `FlushBlockFile`, need to handle these extra abort cases properly. Since `Chainstate::FlushStateToDisk` already produces an abort error in case of `WriteBlockIndexDB` failing, no extra logic for functions calling `Chainstate::FlushStateToDisk` is required. Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called by `FindBlockPos`, while `FlushUndoFile` is only called by `FlushBlockFile` and `WriteUndoDataForBlock`. For both these cases, the flush error is not further bubbled up. Instead, the error is logged and a comment is provided why bubbling up an error would be less desirable in these cases. --- This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site. Further work for enforcing returns when `AbortNode` is called is done in https://github.com/bitcoin/bitcoin/pull/27862. ACKs for top commit: stickies-v: re-ACK d8041d4 ryanofsky: Code review ACK d8041d4e042957660827313951b18c8dd9a99a16 Tree-SHA512: 47ade9b873b15e567c8f60ca538d5a0daf32163e1031be3212a3a45eb492b866664b225f2787c9e40f3e0c089140157d8fd1039abc00c7bdfeec1b52ecd7e219 Dash adaptations: - src/node/blockstorage.cpp: FlushUndoFile/FlushBlockFile keep Dash's AbortNode("Flushing ... failed...") instead of upstream's m_opts.notifications.flushError(...) — Dash has not backported the kernel notifications interface; only the added `return false;`/`success = false;` return-code handling from this PR was applied around it --- src/node/blockstorage.cpp | 40 +++++++++++++++++++++++++++++++++------ src/node/blockstorage.h | 11 ++++++++--- src/validation.cpp | 6 +++++- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index 1f2680fa0a6c..c5922b0e7a30 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -574,16 +574,19 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex) return true; } -void BlockManager::FlushUndoFile(int block_file, bool finalize) +bool BlockManager::FlushUndoFile(int block_file, bool finalize) { FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize); if (!UndoFileSeq().Flush(undo_pos_old, finalize)) { AbortNode("Flushing undo file to disk failed. This is likely the result of an I/O error."); + return false; } + return true; } -void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) +bool BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) { + bool success = true; LOCK(cs_LastBlockFile); if (m_blockfile_info.size() < 1) { @@ -591,17 +594,23 @@ void BlockManager::FlushBlockFile(bool fFinalize, bool finalize_undo) // chainstate init, when we call ChainstateManager::MaybeRebalanceCaches() (which // then calls FlushStateToDisk()), resulting in a call to this function before we // have populated `m_blockfile_info` via LoadBlockIndexDB(). - return; + return true; } assert(static_cast(m_blockfile_info.size()) > m_last_blockfile); FlatFilePos block_pos_old(m_last_blockfile, m_blockfile_info[m_last_blockfile].nSize); if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) { AbortNode("Flushing block file to disk failed. This is likely the result of an I/O error."); + success = false; } // we do not always flush the undo file, as the chain tip may be lagging behind the incoming blocks, // e.g. during IBD or a sync after a node going offline - if (!fFinalize || finalize_undo) FlushUndoFile(m_last_blockfile, finalize_undo); + if (!fFinalize || finalize_undo) { + if (!FlushUndoFile(m_last_blockfile, finalize_undo)) { + success = false; + } + } + return success; } uint64_t BlockManager::CalculateCurrentUsage() @@ -696,7 +705,19 @@ bool BlockManager::FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigne if (!fKnown) { LogPrint(BCLog::BLOCKSTORAGE, "Leaving block file %i: %s\n", m_last_blockfile, m_blockfile_info[m_last_blockfile].ToString()); } - FlushBlockFile(!fKnown, finalize_undo); + + // Do not propagate the return code. The flush concerns a previous block + // and undo file that has already been written to. If a flush fails + // here, and we crash, there is no expected additional block data + // inconsistency arising from the flush failure here. However, the undo + // data may be inconsistent after a crash if the flush is called during + // a reindex. A flush error might also leave some of the data files + // untrimmed. + if (!FlushBlockFile(!fKnown, finalize_undo)) { + LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, /* Continued */ + "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new block file %05i\n", + m_last_blockfile, !fKnown, finalize_undo, nFile); + } m_last_blockfile = nFile; m_undo_height_in_last_blockfile = 0; // No undo data yet in the new file, so reset our undo-height tracking. } @@ -789,7 +810,14 @@ bool BlockManager::WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValid // with the block writes (usually when a synced up node is getting newly mined blocks) -- this case is caught in // the FindBlockPos function if (_pos.nFile < m_last_blockfile && static_cast(block.nHeight) == m_blockfile_info[_pos.nFile].nHeightLast) { - FlushUndoFile(_pos.nFile, true); + // Do not propagate the return code, a failed flush here should not + // be an indication for a failed write. If it were propagated here, + // the caller would assume the undo data not to be written, when in + // fact it is. Note though, that a failed flush might leave the data + // file untrimmed. + if (!FlushUndoFile(_pos.nFile, true)) { + LogPrintLevel(BCLog::BLOCKSTORAGE, BCLog::Level::Warning, "Failed to flush undo file %05i\n", _pos.nFile); + } } else if (_pos.nFile == m_last_blockfile && static_cast(block.nHeight) > m_undo_height_in_last_blockfile) { m_undo_height_in_last_blockfile = block.nHeight; } diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index 048139db25c4..7d03bb542fda 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -98,9 +98,14 @@ class BlockManager */ bool LoadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(cs_main); - void FlushBlockFile(bool fFinalize = false, bool finalize_undo = false); - void FlushUndoFile(int block_file, bool finalize = false); - bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown); + + /** Return false if block file or undo file flushing fails. */ + [[nodiscard]] bool FlushBlockFile(bool fFinalize = false, bool finalize_undo = false); + + /** Return false if undo file flushing fails. */ + [[nodiscard]] bool FlushUndoFile(int block_file, bool finalize = false); + + [[nodiscard]] bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, uint64_t nTime, bool fKnown); bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize); /* Calculate the block/rev files to delete based on height specified by user with RPC command pruneblockchain */ diff --git a/src/validation.cpp b/src/validation.cpp index d9e8fd69c5e4..b00a5114c9fb 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3020,7 +3020,11 @@ bool Chainstate::FlushStateToDisk( LOG_TIME_MILLIS_WITH_CATEGORY("write block and undo data to disk", BCLog::BENCHMARK); // First make sure all block and undo data is flushed to disk. - m_blockman.FlushBlockFile(); + // TODO: Handle return error, or add detailed comment why it is + // safe to not return an error upon failure. + if (!m_blockman.FlushBlockFile()) { + LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Warning, "%s: Failed to flush block file.\n", __func__); + } } // Then update all block file information (which may refer to block and undo files). From 83c7b0ae9cf5cee452042a5bb08a8e432ef28949 Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Tue, 22 Sep 2026 17:43:54 -0500 Subject: [PATCH 4/5] Merge bitcoin/bitcoin#28038: wallet: address book migration bug fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9 test: wallet, add coverage for addressbook migration (furszy) a277f8357ad8b0eb26f33fc36f919d868c06847b wallet: migration bugfix, persist empty labels (furszy) 1b64f6498c394a143df196172a14204fe3b8a744 wallet: migration bugfix, clone 'send' record label to all wallets (furszy) Pull request description: Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it. Bug 1: Non-Cloning of External 'Send' Records The external 'send' records were not being correctly cloned to all wallets. Bug 2: Persistence of Empty Labels As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookData ::IsChange()` return true), we must persist empty labels during the migration process. The user might have called `setlabel` with an "" string for an external address and that must be retained during migration. ACKs for top commit: achow101: ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9 Tree-SHA512: b8a8483a4178a37c49af11eb7ba8a82ca95e54a6cd799e155e33f9fbe7f37b259e28372c77d6944d46b6765f9eaca6b8ca8d1cdd9d223120a3653e4e41d0b6b7 Dash adaptations: - src/wallet/wallet.cpp: kept Dash's string-based purpose line `if (purpose != "unknown") batch.WritePurpose(address, purpose);` plus its `auto purpose{...}` local instead of upstream's `if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose));` — that line is unchanged context in the upstream diff and reflects the AddressPurpose enum conversion Dash has not backported (CAddressBookData::purpose is still std::string defaulting to "unknown"); only the label change from this PR was applied Replayed onto a newer base. Dash adaptations: - test/functional/wallet_migration.py: upstream inserts test_addressbook directly after test_direct_file; Dash's develop now has test_migrate_raw_p2sh and test_hybrid_pubkey occupying that spot (from later backports), so test_addressbook is appended after them instead — both the method definition and the run_test() call. Position within the class is cosmetic; no behavior differs. - src/wallet/wallet.cpp: the persist_address_book lambda keeps Dash's string-valued purpose (`auto purpose{addr_book_data.purpose}` / `if (purpose != "unknown")`) instead of upstream's std::optional purpose + PurposeToString, since Dash has not backported the purpose-to-enum change; only the label half of the hunk (the IsChange()-gated std::optional) is taken, as in the prior reviewed backport. --- src/wallet/wallet.cpp | 10 +- test/functional/wallet_migration.py | 136 ++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0ae127473cd2..d09e660287ad 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4803,7 +4803,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) return false; } } else { - // Labels for everything else (send) should be cloned to all + // Labels for everything else ("send") should be cloned to all if (data.watchonly_wallet) { LOCK(data.watchonly_wallet->cs_wallet); // Add to the watchonly. Preserve the labels, purpose, and change-ness @@ -4815,7 +4815,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) if (!addr_pair.second.IsChange()) { data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label); } - continue; } if (data.solvable_wallet) { LOCK(data.solvable_wallet->cs_wallet); @@ -4828,7 +4827,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) if (!addr_pair.second.IsChange()) { data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label); } - continue; } } } @@ -4840,10 +4838,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) for (const auto& [destination, addr_book_data] : wallet.m_address_book) { auto address{EncodeDestination(destination)}; auto purpose{addr_book_data.purpose}; - auto label{addr_book_data.GetLabel()}; - // don't bother writing default values (unknown purpose, empty label) + std::optional label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel()); + // don't bother writing default values (unknown purpose) if (purpose != "unknown") batch.WritePurpose(address, purpose); - if (!label.empty()) batch.WriteName(address, label); + if (label) batch.WriteName(address, *label); } }; if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 98da6442a467..2036bb1a789d 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -69,6 +69,15 @@ def assert_list_txs_equal(self, received_list_txs, expected_list_txs): del d["parent_descs"] assert_equal(received_list_txs, expected_list_txs) + def check_address(self, wallet, addr, is_mine, is_change, label): + addr_info = wallet.getaddressinfo(addr) + assert_equal(addr_info['ismine'], is_mine) + assert_equal(addr_info['ischange'], is_change) + if label is not None: + assert_equal(addr_info['labels'], [label]), + else: + assert_equal(addr_info['labels'], []), + def test_basic(self): default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) @@ -650,6 +659,132 @@ def test_hybrid_pubkey(self): wallet.unloadwallet() + def test_addressbook(self): + df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + self.log.info("Test migration of address book data") + wallet = self.create_legacy_wallet("legacy_addrbook") + df_wallet.sendtoaddress(wallet.getnewaddress(), 3) + + # Import watch-only script to create a watch-only wallet after migration + watch_addr = df_wallet.getnewaddress() + wallet.importaddress(watch_addr) + df_wallet.sendtoaddress(watch_addr, 2) + + # Import solvable script + multi_addr1 = wallet.getnewaddress() + multi_addr2 = wallet.getnewaddress() + multi_addr3 = df_wallet.getnewaddress() + wallet.importpubkey(df_wallet.getaddressinfo(multi_addr3)["pubkey"]) + ms_addr_info = wallet.addmultisigaddress(2, [multi_addr1, multi_addr2, multi_addr3]) + + self.generate(self.nodes[0], 1) + + # Test vectors + addr_external = { + "addr": df_wallet.getnewaddress(), + "is_mine": False, + "is_change": False, + "label": "" + } + addr_external_with_label = { + "addr": df_wallet.getnewaddress(), + "is_mine": False, + "is_change": False, + "label": "external" + } + addr_internal = { + "addr": wallet.getnewaddress(), + "is_mine": True, + "is_change": False, + "label": "" + } + addr_internal_with_label = { + "addr": wallet.getnewaddress(), + "is_mine": True, + "is_change": False, + "label": "internal" + } + change_address = { + "addr": wallet.getrawchangeaddress(), + "is_mine": True, + "is_change": True, + "label": None + } + watch_only_addr = { + "addr": watch_addr, + "is_mine": False, + "is_change": False, + "label": "imported" + } + ms_addr = { + "addr": ms_addr_info['address'], + "is_mine": False, + "is_change": False, + "label": "multisig" + } + + # To store the change address in the addressbook need to send coins to it + wallet.send(outputs=[{wallet.getnewaddress(): 2}], options={"change_address": change_address['addr']}) + self.generate(self.nodes[0], 1) + + # Util wrapper func for 'addr_info' + def check(info, node): + self.check_address(node, info['addr'], info['is_mine'], info['is_change'], info["label"]) + + # Pre-migration: set label and perform initial checks + for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, watch_only_addr, ms_addr]: + if not addr_info['is_change']: + wallet.setlabel(addr_info['addr'], addr_info["label"]) + check(addr_info, wallet) + + # Migrate wallet + info_migration = wallet.migratewallet() + wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) + wallet_solvables = self.nodes[0].get_wallet_rpc(info_migration["solvables_name"]) + + ######################### + # Post migration checks # + ######################### + + # First check the main wallet + for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]: + check(addr_info, wallet) + + # Watch-only wallet will contain the watch-only entry (with 'is_mine=True') and all external addresses ('send') + self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"]) + for addr_info in [addr_external, addr_external_with_label, ms_addr]: + check(addr_info, wallet_wo) + + # Solvables wallet will contain the multisig entry (with 'is_mine=True') and all external addresses ('send') + self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"]) + for addr_info in [addr_external, addr_external_with_label]: + check(addr_info, wallet_solvables) + + ######################################################################################## + # Now restart migrated wallets and verify that the addressbook entries are still there # + ######################################################################################## + + # First the main wallet + self.nodes[0].unloadwallet("legacy_addrbook") + self.nodes[0].loadwallet("legacy_addrbook") + for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]: + check(addr_info, wallet) + + # Watch-only wallet + self.nodes[0].unloadwallet(info_migration["watchonly_name"]) + self.nodes[0].loadwallet(info_migration["watchonly_name"]) + self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"]) + for addr_info in [addr_external, addr_external_with_label, ms_addr]: + check(addr_info, wallet_wo) + + # Solvables wallet + self.nodes[0].unloadwallet(info_migration["solvables_name"]) + self.nodes[0].loadwallet(info_migration["solvables_name"]) + self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"]) + for addr_info in [addr_external, addr_external_with_label]: + check(addr_info, wallet_solvables) + def run_test(self): self.generate(self.nodes[0], 101) @@ -667,6 +802,7 @@ def run_test(self): self.test_direct_file() self.test_migrate_raw_p2sh() self.test_hybrid_pubkey() + self.test_addressbook() if __name__ == '__main__': WalletMigrationTest().main() From 7b94941ceaaf0a30fb8f1d28747ba62da1fbc30d Mon Sep 17 00:00:00 2001 From: DCG-Claude Date: Tue, 22 Sep 2026 17:43:54 -0500 Subject: [PATCH 5/5] Merge bitcoin/bitcoin#28039: wallet: don't include bdb files from our headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8b5397c00e821d7eaab22f512e9d71a1a0392ebf wallet: bdb: include bdb header from our implementation files only (Cory Fields) 6e010626af7ed51f1748323ece2f46335e145f2f wallet: bdb: don't use bdb define in header (Cory Fields) 004b184b027520a4f9019d1432a816e6ec891fe3 wallet: bdb: move BerkeleyDatabase constructor to cpp file (Cory Fields) b3582baa3a2f84db7d2fb5a681121a5f2d6de3a1 wallet: bdb: move SafeDbt to cpp file (Cory Fields) e5e5aa1da261633c8f73b97d5aefe5dc450a7db9 wallet: bdb: move SpanFromDbt to below SafeDbt's implementation (Cory Fields) 4216f69250937b1ca4650dc0c21678a8444c6650 wallet: bdb: move TxnBegin to cpp file since it uses a bdb function (Cory Fields) 43369f37060a1b4c987672707c500d35c9a27c1d wallet: bdb: drop default parameter (Cory Fields) Pull request description: Only `#include` upstream bdb headers from our cpp files. It's generally good practice to avoid including 3rd party deps in headers as otherwise they tend to sneak into new compilation units. IMO this makes for a nice cleanup. There's a good bit of code movement here, but each commit is small and _should_ be obviously correct. Note: in the future, the buildsystem can add the bdb include path for `bdb.cpp` and `salvage.cpp` only, rather than all wallet sources. ACKs for top commit: achow101: reACK 8b5397c00e821d7eaab22f512e9d71a1a0392ebf hebasto: ACK 8b5397c00e821d7eaab22f512e9d71a1a0392ebf Tree-SHA512: 0ef6e8a9c4c6e2d1e5d6a3534495f91900e4175143911a5848258c56da54535b85fad67b6d573da5f7b96e7881299b5a8ca2327e708f305b317b9a3e85038d66 Dash adaptations: - src/wallet/bdb.h: upstream removes a namespace-scope `SafeDbt` class; Dash still has it nested as `BerkeleyBatch::SafeDbt` (Dash predates the upstream refactor that hoisted it out). Since its `Dbt m_dbt;` member needs the complete BDB type, the nested class had to be removed from the header too — it is now defined at `wallet` namespace scope in bdb.cpp exactly as upstream's final state. - src/wallet/bdb.cpp: the `BerkeleyBatch::SafeDbt::` out-of-line member definitions are unqualified to `SafeDbt::` to match the new namespace-scope class; all call sites are inside `BerkeleyBatch` members in this TU, so unqualified `SafeDbt` still resolves. - src/wallet/bdb.cpp: the anonymous-namespace `SpanFromDbt` took `const BerkeleyBatch::SafeDbt&` in Dash; removed there and re-added as upstream's `static ... SpanFromDbt(const SafeDbt&)` after the class definition. - src/wallet/bdb.h: the conflict hunk that deletes the old `SafeDbt` carried upstream's `BerkeleyCursor` class as context, which Dash does not have (Dash still uses `BerkeleyBatch::StartCursor/ReadAtCursor/CloseCursor`). Kept Dash's `BerkeleyDatabase::SupportsAutoBackup()` override and applied only the deletion the hunk actually makes — no BerkeleyCursor was introduced. - src/wallet/salvage.cpp: took upstream's `env->TxnBegin(DB_TXN_WRITE_NOSYNC)` but kept Dash's `CWallet dummyWallet(/*chain=*/nullptr, /*coinjoin_loader=*/nullptr, "", gArgs, CreateDummyWalletDatabase())` plus `SetupLegacyScriptPubKeyMan()` — upstream's `DummyDatabase` one-liner is an unrelated, newer constructor signature. - src/wallet/bdb.h: kept Dash's `namespace wallet {` / `struct WalletDatabaseFileId` adjacency (no blank line) rather than upstream's, purely cosmetic. --- src/wallet/bdb.cpp | 62 ++++++++++++++++++++++++++++++++++-------- src/wallet/bdb.h | 48 ++++++++------------------------ src/wallet/salvage.cpp | 4 ++- 3 files changed, 65 insertions(+), 49 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index 54fdbce21990..a0104f24e820 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -14,6 +14,7 @@ #include +#include #include // Windows may not define S_IRUSR or S_IWUSR. We define both @@ -25,12 +26,10 @@ #endif #endif +static_assert(BDB_DB_FILE_ID_LEN == DB_FILE_ID_LEN, "DB_FILE_ID_LEN should be 20."); + namespace wallet { namespace { -Span SpanFromDbt(const BerkeleyBatch::SafeDbt& dbt) -{ - return {reinterpret_cast(dbt.get_data()), dbt.get_size()}; -} //! Make sure database has a unique fileid within the environment. If it //! doesn't, throw an error. BDB caches do not work properly when more than one @@ -232,17 +231,37 @@ BerkeleyEnvironment::BerkeleyEnvironment() : m_use_shared_memory(false) fMockDb = true; } -BerkeleyBatch::SafeDbt::SafeDbt() +/** RAII class that automatically cleanses its data on destruction */ +class SafeDbt final +{ + Dbt m_dbt; + +public: + // construct Dbt with internally-managed data + SafeDbt(); + // construct Dbt with provided data + SafeDbt(void* data, size_t size); + ~SafeDbt(); + + // delegate to Dbt + const void* get_data() const; + uint32_t get_size() const; + + // conversion operator to access the underlying Dbt + operator Dbt*(); +}; + +SafeDbt::SafeDbt() { m_dbt.set_flags(DB_DBT_MALLOC); } -BerkeleyBatch::SafeDbt::SafeDbt(void* data, size_t size) +SafeDbt::SafeDbt(void* data, size_t size) : m_dbt(data, size) { } -BerkeleyBatch::SafeDbt::~SafeDbt() +SafeDbt::~SafeDbt() { if (m_dbt.get_data() != nullptr) { // Clear memory, e.g. in case it was a private key @@ -256,21 +275,33 @@ BerkeleyBatch::SafeDbt::~SafeDbt() } } -const void* BerkeleyBatch::SafeDbt::get_data() const +const void* SafeDbt::get_data() const { return m_dbt.get_data(); } -uint32_t BerkeleyBatch::SafeDbt::get_size() const +uint32_t SafeDbt::get_size() const { return m_dbt.get_size(); } -BerkeleyBatch::SafeDbt::operator Dbt*() +SafeDbt::operator Dbt*() { return &m_dbt; } +static Span SpanFromDbt(const SafeDbt& dbt) +{ + return {reinterpret_cast(dbt.get_data()), dbt.get_size()}; +} + +BerkeleyDatabase::BerkeleyDatabase(std::shared_ptr env, fs::path filename, const DatabaseOptions& options) : + WalletDatabase(), env(std::move(env)), m_filename(std::move(filename)), m_max_log_mb(options.max_log_mb) +{ + auto inserted = this->env->m_databases.emplace(m_filename, std::ref(*this)); + assert(inserted.second); +} + bool BerkeleyDatabase::Verify(bilingual_str& errorStr) { fs::path walletDir = env->Directory(); @@ -459,6 +490,15 @@ void BerkeleyEnvironment::ReloadDbEnv() Open(open_err); } +DbTxn* BerkeleyEnvironment::TxnBegin(int flags) +{ + DbTxn* ptxn = nullptr; + int ret = dbenv->txn_begin(nullptr, &ptxn, flags); + if (!ptxn || ret != 0) + return nullptr; + return ptxn; +} + bool BerkeleyDatabase::Rewrite(const char* pszSkip) { while (true) { @@ -715,7 +755,7 @@ bool BerkeleyBatch::TxnBegin() { if (!pdb || activeTxn) return false; - DbTxn* ptxn = env->TxnBegin(); + DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC); if (!ptxn) return false; activeTxn = ptxn; diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 9389e70f629b..b141fd46efa4 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -21,11 +21,18 @@ struct bilingual_str; -#include +class DbEnv; +class DbTxn; +class Db; +class Dbc; + +// This constant was introduced in BDB 4.0.14 and has never changed, but there +// is a belt-and-suspenders check in the cpp file just in case. +#define BDB_DB_FILE_ID_LEN 20 /* Unique file ID length. */ namespace wallet { struct WalletDatabaseFileId { - uint8_t value[DB_FILE_ID_LEN]; + uint8_t value[BDB_DB_FILE_ID_LEN]; bool operator==(const WalletDatabaseFileId& rhs) const; }; @@ -64,14 +71,7 @@ class BerkeleyEnvironment void CloseDb(const fs::path& filename); void ReloadDbEnv(); - DbTxn* TxnBegin(int flags = DB_TXN_WRITE_NOSYNC) - { - DbTxn* ptxn = nullptr; - int ret = dbenv->txn_begin(nullptr, &ptxn, flags); - if (!ptxn || ret != 0) - return nullptr; - return ptxn; - } + DbTxn* TxnBegin(int flags); }; /** Get BerkeleyEnvironment given a directory path. */ @@ -88,12 +88,7 @@ class BerkeleyDatabase : public WalletDatabase BerkeleyDatabase() = delete; /** Create DB handle to real database */ - BerkeleyDatabase(std::shared_ptr env, fs::path filename, const DatabaseOptions& options) : - WalletDatabase(), env(std::move(env)), m_filename(std::move(filename)), m_max_log_mb(options.max_log_mb) - { - auto inserted = this->env->m_databases.emplace(m_filename, std::ref(*this)); - assert(inserted.second); - } + BerkeleyDatabase(std::shared_ptr env, fs::path filename, const DatabaseOptions& options); ~BerkeleyDatabase() override; @@ -161,27 +156,6 @@ class BerkeleyDatabase : public WalletDatabase /** RAII class that provides access to a Berkeley database */ class BerkeleyBatch : public DatabaseBatch { -public: - /** RAII class that automatically cleanses its data on destruction */ - class SafeDbt final - { - Dbt m_dbt; - - public: - // construct Dbt with internally-managed data - SafeDbt(); - // construct Dbt with provided data - SafeDbt(void* data, size_t size); - ~SafeDbt(); - - // delegate to Dbt - const void* get_data() const; - uint32_t get_size() const; - - // conversion operator to access the underlying Dbt - operator Dbt*(); - }; - private: bool ReadKey(CDataStream&& key, CDataStream& value) override; bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite = true) override; diff --git a/src/wallet/salvage.cpp b/src/wallet/salvage.cpp index fdae721b7667..ff8bb918a41c 100644 --- a/src/wallet/salvage.cpp +++ b/src/wallet/salvage.cpp @@ -11,6 +11,8 @@ #include #include +#include + namespace wallet { /* End of headers, beginning of key/value data */ static const char *HEADER_END = "HEADER=END"; @@ -134,7 +136,7 @@ bool RecoverDatabaseFile(const ArgsManager& args, const fs::path& file_path, bil return false; } - DbTxn* ptxn = env->TxnBegin(); + DbTxn* ptxn = env->TxnBegin(DB_TXN_WRITE_NOSYNC); CWallet dummyWallet(/*chain=*/nullptr, /*coinjoin_loader=*/nullptr, "", gArgs, CreateDummyWalletDatabase()); dummyWallet.SetupLegacyScriptPubKeyMan(); for (KeyValPair& row : salvagedData)