Repository navigation
backport: v0.26 bitcoin#27866, bitcoin#28038, bitcoin#28039 - #77
Closed
DCG-Claude wants to merge 3 commits into
Closed
DCG-Claude wants to merge 3 commits into
DCG-Claude wants to merge 3 commits into
Conversation
DCG-Claude
force-pushed
the
backport-0.26-b057-misc
branch
from
September 22, 2026 12:09
283a9c8 to
e8bf8e0
Compare
Collaborator
Author
The lint break comes from bitcoin#27632's translated -loglevel message, which uses positional format specifiers (%1$s ... %4$s); Dash still runs test/lint/lint-format-strings.py (upstream deleted it), and its count_format_specifiers() counts 5 specifiers against 4 arguments, so src/init/common.cpp fails the linter. I left the upstream string untouched and added the call to the existing FALSE_POSITIVES list in test/lint/run-lint-format-strings.py (a Dash-only lint script), which is the mechanism Dash already uses for calls this parser cannot handle. (folded into the 🤖 backportsys, on behalf of the Dash backport pipeline. |
|
This pull request has conflicts, please rebase. |
DCG-Claude
changed the title
DCG-Claude
force-pushed
the
backport-0.26-b057-misc
branch
from
September 22, 2026 15:20
e8bf8e0 to
9922e78
Compare
Collaborator
Author
|
Branch rewritten at
🤖 backportsys, on behalf of the Dash backport pipeline. |
This was referenced Sep 22, 2026
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.
DCG-Claude
force-pushed
the
backport-0.26-b057-misc
branch
from
September 22, 2026 22:48
9922e78 to
e6bcbab
Compare
Collaborator
Author
The quoted log only shows job cleanup, so I worked out the cause from the diff rather than from the log. The CI run is at the branch tip, but the likely cause is in bitcoin#27866: its multi-line 🤖 backportsys, on behalf of the Dash backport pipeline. |
Collaborator
Author
|
folded into backport-0.26-b061-misc: its commits ship there |
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-b057-misc.0e5821c7ce279f74d533e6bcbab3aeProvenance
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.