Repository navigation
backport: v0.26 bitcoin#28139, bitcoin#28232, bitcoin#27866, bitcoin#28038, bitcoin#28039 - #81
Open
DCG-Claude wants to merge 5 commits into
Open
DCG-Claude wants to merge 5 commits into
DCG-Claude wants to merge 5 commits into
Conversation
…let case c648bdb test: create wallet specific for test_locked_wallet case (furszy) Pull request description: Coming from bitcoin#28089 (comment). 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 (bitcoin#28089), where other test cases fail due the wallet re-locking itself after the 100ms. ACKs for top commit: MarcoFalke: lgtm ACK c648bdb ishaanam: utACK c648bdb 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.
5364dd8 test: locked_wallet, skip default fee estimation (furszy) Pull request description: Coming from bitcoin#28139 (comment). 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 5364dd8 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
d8041d4 blockstorage: Return on fatal undo file flush error (TheCharlatan) f0207e0 blockstorage: Return on fatal block file flush error (TheCharlatan) 5671c15 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](bitcoin#27711 (comment)). 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 bitcoin#27862. ACKs for top commit: stickies-v: re-ACK d8041d4 ryanofsky: Code review ACK d8041d4 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
7ecc29a test: wallet, add coverage for addressbook migration (furszy) a277f83 wallet: migration bugfix, persist empty labels (furszy) 1b64f64 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 7ecc29a 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.
8b5397c wallet: bdb: include bdb header from our implementation files only (Cory Fields) 6e01062 wallet: bdb: don't use bdb define in header (Cory Fields) 004b184 wallet: bdb: move BerkeleyDatabase constructor to cpp file (Cory Fields) b3582ba wallet: bdb: move SafeDbt to cpp file (Cory Fields) e5e5aa1 wallet: bdb: move SpanFromDbt to below SafeDbt's implementation (Cory Fields) 4216f69 wallet: bdb: move TxnBegin to cpp file since it uses a bdb function (Cory Fields) 43369f3 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 8b5397c hebasto: ACK 8b5397c 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.
|
This pull request has conflicts, please rebase. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated Bitcoin Core v0.26 backports, batch
backport-0.26-b061-misc.65b2bf8260locked_walletand resto37646c0ce9c708657cc883c7b0ae9c7b94941ceaProvenance
Each commit passed: cherry-pick (adapted by an Opus lane only where conflicts existed), build, touched tests, a mechanical diff-of-diffs check (every upstream hunk landed; no added line without an upstream counterpart), and an independent Opus verification lane where anything was adapted. Gate rows and lane artifacts are in the backportsys DB.