Repository navigation
backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695 - #7916
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Final review complete — no blockers (commit 8cfc3c4) · triage: normal |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe patch replaces direct stream writes and message payloads with span-based byte access, and removes Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the five-commit range ending at 1af9327 and retained one non-blocking test-coverage suggestion; no blocking defects were confirmed. This was a static review: the supplied CI snapshot shows successful platform builds and lint, with Linux/SQLite tests and the ASan build pending; the failed multiprocess depends job independently confirms a runner architecture mismatch. The supplied base-drift changes do not duplicate or supersede this patch, and no merge conflicts are reported.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff spans stream serialization call sites, legacy-wallet migration safeguards and warnings, and functional tests, but is not sufficiently large or intricate on a critical surface to warrant critical effort. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/wallet.cpp`:
- [SUGGESTION] src/wallet/wallet.cpp:295-298: Preserve bitcoin#27869's legacy-load warning assertion
bitcoin#27869 also updates wallet_backwards_compatibility.py to verify that loading an older legacy wallet returns exactly this deprecation warning. This backport adds the production warning without an equivalent assertion, leaving the new RPC-visible behavior untested. Commit c3d1214d91 explicitly documents omitting the upstream u1_v16 assertion, so this is a non-blocking coverage gap, not an undisclosed prerequisite omission. The removed scenario does not need to be restored: capture the result of the existing node_master.loadwallet("u1_v17") call at test/functional/wallet_backwards_compatibility.py:251 and assert that its warnings array contains exactly the expected legacy-wallet deprecation warning.
| // Legacy wallets are being deprecated, warn if the loaded wallet is legacy | ||
| if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { | ||
| warnings.push_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet.")); | ||
| } |
There was a problem hiding this comment.
✅ No longer applies at
8cfc3c45; see the replies below.
🟡 Suggestion: Preserve bitcoin#27869's legacy-load warning assertion
bitcoin#27869 also updates wallet_backwards_compatibility.py to verify that loading an older legacy wallet returns exactly this deprecation warning. This backport adds the production warning without an equivalent assertion, leaving the new RPC-visible behavior untested. Commit c3d1214 explicitly documents omitting the upstream u1_v16 assertion, so this is a non-blocking coverage gap, not an undisclosed prerequisite omission. The removed scenario does not need to be restored: capture the result of the existing node_master.loadwallet("u1_v17") call at test/functional/wallet_backwards_compatibility.py:251 and assert that its warnings array contains exactly the expected legacy-wallet deprecation warning.
source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)
There was a problem hiding this comment.
The upstream test change only rewrote the existing u1_v16 loadwallet assertion, a block Dash removed in 25b82ab; the commit message records this. Adding a new warnings assertion to the u1_v17 load would be new test coverage that upstream doesn't ship, not a fix to what the backport got wrong, so it's better as a separate follow-up. It also can't be checked here, because the test needs previous-release binaries.
🤖 backportsys, on behalf of the Dash backport pipeline.
There was a problem hiding this comment.
I checked the upstream diff: it updates an existing u1_v16 warnings assertion, and that load scenario is absent from Dash’s test. The omission is explicitly documented, so my suggestion went beyond preserving the upstream test and asked for additional Dash-specific coverage. I’m withdrawing it; an assertion on u1_v17 would be an optional follow-up, not a correction this backport needs.
Withdrawing this finding.
There was a problem hiding this comment.
Withdrawn (re-reviewed at 1af9327f): I checked the upstream diff: it changes only the existing u1_v16 warning assertion, and c3d1214 explicitly excludes that hunk under this PR's documented adaptation policy. I withdraw the request to add an assertion to u1_v17; that would expand the intentionally excluded coverage, rather than correct an error in this backport, regardless of previous-release binary availability.
There was a problem hiding this comment.
27869: that could go to only 25.x branch; it's a breaking change to make legacy wallets indeed legacy; as descriptor wallets are enabled by default in 24.0 only. please drop it from PR.
There was a problem hiding this comment.
Agreed: bitcoin#27869 (deprecation warning when loading legacy wallets) will be removed from this PR and kept for the 25.x line. Dropping bitcoin#27869 from this branch; it is being rewritten without it.
🤖 backportsys, on behalf of the Dash backport pipeline.
There was a problem hiding this comment.
No longer applies (re-reviewed at 8cfc3c45): Confirmed that your rewrite removed bitcoin#27869: the current title, four commits, and complete diff no longer include the legacy-wallet load warning. The warning-assertion request therefore does not apply to this PR.
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit 1af9327
Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.
Approved: the discussion withdrew or deferred every finding the full review of 1af9327f (run 5351) raised, and the approval rests on that review.
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed
thepastaclaw
left a comment
There was a problem hiding this comment.
CI failed after approval — commit 1af9327
The standing review approved this commit; this follow-up moves the verdict to COMMENT: checks that failed after the review may relate to this change, and the evidence does not rule it out.
Failing checks:
linux64_multiprocess / Build depends(failure)
Why it is unclear: The Build depends log is unavailable, so the actual failure cannot be verified. The approving review mentions a runner architecture mismatch, but no diagnostic output is provided to substantiate that explanation.
Nothing was re-reviewed: the findings of the standing review are unchanged. The next push gets a full review; a re-run that passes does not restore the approval.
Review provenance
Source: CI-check lane gpt-6.1-sol (agent: triage); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because CI failed on this commit after the review; failure logs read: 0 GitHub Actions job(s)
…ing step (#47) gh api refuses to print a response with terminal escape sequences unless --allow-escape-sequences is passed, and Actions job logs always carry colour codes, so every CI-check lane saw no log and answered 'unclear'. On dashpay/dash#7916 that moved an APPROVE to COMMENT over a runner-architecture failure the log names plainly. The log window now also ends at the last ##[error] annotation instead of the job's cleanup steps. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
linux64_multiprocess failed before building: the job ran on an aarch64 runner while the depends cache check expected x86_64-pc-linux-gnu, which is a runner/CI setup problem. In linux64_asan the only real failure was p2p_blocksonly.py, which none of these backports touch (no change to the test, net_processing or test_framework). The node log shows a race in the test: bump_mocktime(60) runs before the node processes the tx, so the announcement to second_peer is timed from the advanced clock and never goes out; slow ASAN timing seems to hit this every attempt, and feature_timestampindex.py passed on retry. 🤖 backportsys, on behalf of the Dash backport pipeline. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit 1af9327
Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Static verification of the complete diff at 1af9327 found no actionable in-scope defects; the legacy-warning test omission is explicitly documented and intentionally excluded. The overlapping base changes neither duplicate nor supersede these backports, and the supplied drift evidence reports no merge conflicts. No local builds or tests were run: current CI passes lint, platform builds, and standard Linux tests, but multiprocess dependency setup fails an architecture check and ASan testing fails waiting for transaction relay in unchanged p2p_blocksonly.py; neither failure establishes a defect in this PR.
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cross-cutting stream serialization cleanup, wallet migration fix, legacy-wallet warning, and test updates warrant ordinary review, but the diff is not sufficiently large or intricate on a critical surface to qualify as critical. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
| 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. |
There was a problem hiding this comment.
I confirm failure on this test; it worths to backport to 24.0
There was a problem hiding this comment.
No change needed on this branch: line 623 is part of the bitcoin#28125 test this branch adds, and the comment confirms the bug it fixes. Backporting it to the 24.0 release branch is a separate backport outside this PR.
🤖 backportsys, on behalf of the Dash backport pipeline.
…thod and << operator 5cd0717 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky) Pull request description: DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method. ACKs for top commit: furszy: lgtm ACK 5cd0717 MarcoFalke: lgtm ACK 5cd0717 🌙 Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2 Dash adaptations: - src/net_processing.cpp: final-alert hunk not applied; Dash's ProcessMessage has no legacy final-alert block, so there is nothing to convert - src/flat-database.h: Dash-only `fileout << ssObj` becomes `fileout << Span{ssObj}` since DataStream::Serialize is removed - src/llmq/net_quorum.cpp: QDATA error reply streams `Span{body}` and passes `Span{ssResponse}` to Make(), as upstream did for the alert message - src/governance/net_governance.cpp: MNGOVERNANCEOBJECT/MNGOVERNANCEOBJECTVOTE replies pass `Span{ss}` to Make() instead of the CDataStream itself
…ripts 8e7e3e6 test: wallet, verify migration doesn't crash for an invalid script (furszy) 1de8a23 wallet: disallow migration of invalid or not-watched scripts (furszy) Pull request description: Fixing bitcoin#28057. The legacy wallet allows to import any raw script (bitcoin#28126), without checking if it was valid or not. Appending it to the watch-only set. This causes a crash in the migration process because we are only expecting to find valid scripts inside the legacy spkm. These stored scripts internally map to `ISMINE_NO` (same as if they weren't stored at all..). So we need to check for these special case, and take into account that the legacy spkm could be storing invalid not watched scripts. Which, in code words, means `IsMineInner()` returning `IsMineResult::INVALID` for them. Note: To verify this, can run the test commit on top of master. `wallet_migration.py` will crash without the bugfix commit. ACKs for top commit: achow101: ACK 8e7e3e6 Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7 Dash adaptations: - test/functional/wallet_migration.py: new sh(sh(pkh())) import and asserts go into Dash's existing test_migrate_raw_p2sh, which has no wsh step and expects balance 2, not 5 - test/functional/wallet_migration.py: script_to_p2sh import added next to Dash's existing key_to_p2pkh import on the same line
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: keeps the Dash-only `satoshi_round` import next to the new `get_fee` import (still used at line 208). - test/functional/wallet_fundrawtransaction.py: the exact-fee size is 191 bytes for Dash's p2pkh->p2pkh tx (147-byte input + 44 bytes), not upstream's 110 vbytes for p2wpkh; Dash has no segwit. - test/functional/wallet_fundrawtransaction.py: replaces Dash's earlier fixed 500-duff fee deduction (and its comment) with upstream's exact get_fee() calculation.
… be downloaded fa4c16b test: Add test to check tx in the last block can be downloaded (MarcoFalke) fadc849 test: Split up test_notfound_on_unannounced_tx test case (MarcoFalke) Pull request description: If a peer received an `inv` about a transaction, which was included in a block before receiving the corresponding `getdata`, it can be beneficial to send this transaction to the peer to aid compact block relay. Add a test for this to avoid breaking it in the future. ACKs for top commit: sdaftuar: ACK fa4c16b instagibbs: ACK fa4c16b Tree-SHA512: 1ec16dcc216dd29c849928e753158d45c409612e9ac528db16e5af465bb5b69cd42241ac6f67e5a4583b8e558eeba49fa0a0889a4247b3fb0053b2758eec9490 Dash adaptations: - test/functional/p2p_leak_tx.py: uses send_self_transfer()["txid"] instead of ["wtxid"]; Dash has no segwit so MiniWallet returns no wtxid - test/functional/p2p_leak_tx.py: test_tx_in_block checks tx.rehash() == txid instead of tx.getwtxid() == wtxid, since Dash's CTransaction has no getwtxid() - test/functional/p2p_leak_tx.py: test_tx_in_block calls self.bump_mocktime(60) before waiting for the inv; Dash's framework freezes mocktime, so the inbound inv timer would never fire
1af9327 to
8cfc3c4
Compare
|
Branch rewritten at
🤖 backportsys, on behalf of the Dash backport pipeline. |
Thanks for the ACKs. bitcoin#27869 has already been removed: the branch now has only bitcoin#27800, bitcoin#28125, bitcoin#28232 and bitcoin#27695 on top of #7861, so nothing needed changing. 🤖 backportsys, on behalf of the Dash backport pipeline. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cfc3c4522
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return false; | ||
| } | ||
|
|
||
| // Get all invalid or non-watched scripts that will not be migrated |
There was a problem hiding this comment.
Backport or drop bitcoin#27869 from the batch
Although this hunk adds the bitcoin#28125 migration fix, CWallet::Create's existing-wallet branch at src/wallet/wallet.cpp:3390-3412 still appends no warning for a non-descriptor wallet, and a repo-wide search found no legacy-wallet load warning elsewhere. Consequently users do not receive the behavior attributed to bitcoin#27869 in this commit's title/table; either apply that upstream hunk and its test or remove bitcoin#27869 from the advertised batch so the backport history is accurate.
AGENTS.md reference: AGENTS.md:L193-L198
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This batch doesn't include bitcoin#27869: the commit messages list only bitcoin#27800, bitcoin#28125, bitcoin#28232 and bitcoin#27695, and the bitcoin#28125 commit claims only its own migration fix. The legacy-wallet load warning is upstream behaviour from a PR outside this batch, so there's nothing to backport or drop here.
🤖 backportsys, on behalf of the Dash backport pipeline.
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently inspected the complete 15-file diff at 8cfc3c4 and compared the four advertised backports with their upstream diffs; no actionable correctness or Dash-adaptation defects were found. bitcoin#27869 is absent from the current scope and implementation, so neither the prior warning-coverage request nor the external claim that this batch still advertises it applies. This was static review only: current GitHub checks show lint and ClangFormat passing and no failed code checks, but source-build jobs remain pending, so build and test success at this head is not established.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff combines cross-cutting DataStream call-site updates, a contained wallet migration fix, and test changes, but does not meet the large-or-intricate threshold for critical. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
…itcoin#27695 8cfc3c4 Merge bitcoin#27695: test: Add test to check tx in the last block can be downloaded (DCG-Claude) 3e2077c Merge bitcoin#28232: test: locked_wallet, skip default fee estimation (DCG-Claude) a579bef Merge bitcoin#28125: wallet: bugfix, disallow migration of invalid scripts (DCG-Claude) 2d7b155 Merge bitcoin#27800: streams: Drop confusing DataStream::Serialize method and << operator (DCG-Claude) Pull request description: <!-- backportsys v1 --> > 🔎 **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#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#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#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#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#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 <!-- backportsys 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 <!-- bps:hands_off --> - [ ] 🔄 **Rebase** onto current `develop` <!-- bps:rebase --> - [ ] ❌ **Close** — abandon this batch and release its items <!-- bps:close --> <!-- /backportsys controls --> <!-- human notes below; the bot never edits this section --> ACKs for top commit: knst: utACK 8cfc3c4 Tree-SHA512: da34d35ae7071be1d25f9bf7e8b06f94e3f81f50c40d1c278230be3af6ee37d74e86417be89409911dffc4b990eb5c35c704ffaa09d51847b6abfc307180be5f (cherry picked from commit fe232f9)
…labeled: #7916) d885ecd Merge #7916: backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695 (merge-script) f55ad1d Merge bitcoin#28139: test: create wallet specific for test_locked_wallet case (DCG-Claude) Pull request description: ## Issue being fixed or feature implemented Second v24.0.0-rc.5 backport PR, on top of `v24.0.x` at v24.0.0-rc.4 (0e7382d) and independent of #7965. It brings over the last merged `backport-candidate-24.0.x` PR that `v24.0.x` does not have. All 49 merged PRs with that label were checked against `v24.0.x`. 46 were picked for rc.3 (#7808) or rc.4 (#7917), and #7893 was picked there as its PR commit. #7947 is in #7965. That leaves #7916. #7638, also labeled, is still open and is not included. ## What was done? Cherry-picked with `git cherry-pick -x` (`-m1` for the merge), in develop order: - 5df4af4 bitcoin#28139: test: create wallet specific for test_locked_wallet case. This is a test-only prerequisite, picked as its commit from #7863. bitcoin#28232 in #7916 edits the `test_locked_wallet` scenario that bitcoin#28139 rewrote. Without it, `wallet_fundrawtransaction.py` conflicts. The rest of #7863 (bitcoin#27866, bitcoin#28038, bitcoin#28039) is routine and left out. - #7916 backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695. The label is there for bitcoin#28125 (`wallet: bugfix, disallow migration of invalid scripts`); the other three come along as one merge. Both picks change exactly the same lines as their develop commits. ## How Has This Been Tested? Linux x86_64 against depends, `--enable-debug --enable-werror --disable-hardening --without-gui`: build is clean. - Unit suites, passing: `wallet_tests`, `scriptpubkeyman_tests`, `streams_tests`, `serialize_tests`, `llmq_dkg_tests`, `governance_inv_tests`. - Functional tests, passing: `wallet_fundrawtransaction.py` (`--legacy-wallet`, `--legacy-wallet --nohd`, `--descriptors`), `wallet_migration.py`, `p2p_leak_tx.py` (v1/v2), `wallet_dump.py`, `feature_governance_objects.py`, `p2p_quorum_data.py`. ## Breaking Changes Only bitcoin#28125: during legacy-wallet migration, invalid or unwatched watch-only scripts are skipped instead of being migrated. ## Checklist: - [x] 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 89d59df1a643cba15ae39a1acee9cf4d2122b663a9b8dc2d0ac7fdab31853e855f46bba467dffc58cadbc9527d9dcc85ceed6825e9d83a9b18f270f5829ca943
…ixes: #7682, #7869, #7861, #7951) 1ba2609 Merge #7951: fix(net): stop peers lingering after disconnect under edge-triggered epoll (pasta) 696d45f Merge #7861: feat: keep at least 4-days worth blocks after pruning (merge-script) 9cd1c72 Merge #7869: fix: let startup block replay handle quorum commitments and asset unlocks (pasta) 73e7254 Merge #7682: build: finish relic_conf.h regeneration before compiling BLS consumers (pasta) Pull request description: ## Issue being fixed or feature implemented Third v24.0.0-rc.5 backport PR, on top of `v24.0.x` at v24.0.0-rc.4 (0e7382d). It is independent of #7965 (v23.1.9 fixes and #7947) and #7967 (#7916). rc.5 is meant to be ready to become the final release, so every develop merge since the `v24.0.x` branch point that is neither picked nor labeled was audited again. These four unlabeled fixes are needed for a final v24.0.0. #7869 was already listed in #7917 as "worth considering before the final release". ## What was done? Cherry-picked with `git cherry-pick -m1 -x`, in develop merge order: - #7682 build: finish relic_conf.h regeneration before compiling BLS consumers. Fixes an intermittent parallel-build failure. - #7869 fix: let startup block replay handle quorum commitments and asset unlocks. After an unclean shutdown, block replay asserted on a quorum commitment, so the node needed `-reindex-chainstate` to recover. - #7861 feat: keep at least 4-days worth blocks after pruning. A pruned node could prune blocks that a credit-pool rebuild, or the work block of a rotated LLMQ cycle, still needs. It then rejected a canonical block after restart, or computed different quorum members than the network. - #7951 fix(net): stop peers lingering after disconnect under edge-triggered epoll #7682 and #7951 change exactly the same lines as their develop merges. Two picks needed adaptation, described in their commit messages: - **#7869**: develop folded `CChainstateHelper` into `Chainstate` (d5c1bd7), which is not on this branch, so `RollforwardBlock()` keeps calling `m_chain_helper->special_tx`. - **#7861**: chain parameters live in `src/chainparams.{h,cpp}` here (develop moved them to `src/kernel/`), and regtest's `nPruneAfterHeight` still reads `-fastprune` through `ArgsManager`. No caller of the old `MIN_BLOCKS_TO_KEEP` is left on this branch. `doc/release-notes-7861.md` comes with the pick and needs folding into the final release notes. Unlabeled develop merges since the branch point that were audited and left out: - #7918: not needed. The duplicate spork announcement it removes was never on this branch, because #7917 picked #7893's final commit once. - Left for 24.1: - perf work (#7934, #7935, #7936), because #7934 reworks MN list construction and payee validation; - #7847, which changes spork signing and stops using sporks on mainnet; - #7868 (GETMNLISTDIFF/GETQRINFO rate limit), #7844 and #7812; - #7260 (Qt feature) and its follow-up #7804; - #7937 (maintainer script); - the routine upstream backports and CI/docs/test-only changes. ## How Has This Been Tested? Linux x86_64 against depends, `--enable-debug --enable-werror --disable-hardening --without-gui`: build is clean. - Unit suites, passing: `evo_assetlocks_tests` (including #7861's new case), `blockmanager_tests`, `net_tests`, `denialofservice_tests`. - Functional tests, passing: `feature_asset_locks.py` (including #7869's replay case), `feature_pruning.py` (extended), `feature_index_prune.py`, `p2p_node_network_limited.py` (v1/v2), `rpc_blockchain.py` (v1/v2), `p2p_addr_relay.py`, `p2p_disconnect_ban.py` (v1/v2). - `feature_asset_locks.py` failed once in a parallel run, and the log was not kept. It then passed 5 of 5 runs, 4 of them in parallel. - An independent review compared each adapted pick with its develop merge and checked v24.0.x for remaining users of the old pruning constant. ## Breaking Changes - #7861: pruned nodes keep at least 2304 blocks (four days) instead of 288 on mainnet, testnet and devnet, so they use slightly more disk. On testnet and devnet, pruning starts after height 3000 instead of 1000. Regtest is unchanged. ## Checklist: - [x] 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: a8e39c6025e1737bcd55e7aa50c1419633a67dd82a0585f0586a63b8fb14f4770e9e24b7e7d2605da44c12e6c98768a893fccf6972aa71c1eaab23eec7e5b59f
…xes, #7947) 4722ad9 chore: prepare v23.1.9 release (pasta) 71b3f0c Merge #7556: fix: gate BLS vote-signature failure log behind gobject category (pasta) 14dddb1 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta) 5f73355 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta) 0d2e579 fix(evo): keep -allowprivatenet out of masternode address validation (Konstantin Akimov) b4cd3fa fix(evo): only write credit pool snapshots inside a block-scoped EvoDB transaction (#7549) (pasta) d215280 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta) db9ec2b Merge #7826: fix: skip repeated authenticated Platform bans (pasta) dcc9596 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta) 197b14a Merge #7866: fix: stop `masternode payments` at the genesis block (pasta) 3055a19 Merge #7850: perf: let CHECKDATASIG use the signature cache (pasta) 4da0099 Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta) c7393c0 Merge #7833: fix: gate signing intake before quorum retrieval (pasta) fdf40fe Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta) 8888af8 Merge #7753: fix: bound masternode list caches by size instead of by new block (pasta) 457af6d Merge #7738: refactor: avoid double lookups and default-inserting map access in governance (pasta) be3b55e Merge #7662: fix: isolate missing quorums in InstantSend batches (pasta) 9bc1e77 Merge #7489: fix: conflict ProRegTx collateral reuse with in-mempool masternode updates (pasta) 5278ce8 Merge #7583: fix: bound pending DKG message retention per proTxHash (pasta) 9ebf78f Merge #7522: fix(net): bound verified inbound connections per proRegTxHash (pasta) 9f2946e Merge #7487: fix: bound DomainPort string deserialization and stop swallowing deser failures (pasta) c077d32 Merge #7527: fix: require a valid masternode signature before caching an orphan governance vote (pasta) dfa7db1 Merge #7518: fix: memoize governance vote signature checks to bound govsync cost (pasta) bb09c49 Merge #7532: fix: bound DYNBITSET allocation against remaining stream size (pasta) 9739f35 Merge #7488: fix: validate ProUpServTx nType at mempool acceptance (pasta) 4687550 Merge #7385: fix: reject invalid extended masternode net info (Pasta) 58b9bce fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites (Konstantin Akimov) 7a21752 Merge bitcoin#36667: net: enforce timeout interval for paused send peers (Konstantin Akimov) 58abb28 Merge bitcoin#36665: sock: align exception monitoring between poll() and select() backends (Konstantin Akimov) 8030deb Merge bitcoin#36669: refactor: store positions as a 64 bits integer in flatfiles (Konstantin Akimov) b1178f9 fix(llmq): judge every sig-share sender by its own verified shares (pasta) 33ec74e fix(governance): cap the orphan parent objects re-requested per tick (pasta) 7fc0edb doc(rpc): mark provider payload network fields optional in the help (pasta) 2aa3a87 fix(rpc): don't dereference a missing netInfo when decoding provider payloads (pasta) 120b5db fix(llmq): verify DKG contributions individually when a batch vvec check fails (pasta) 808b105 Merge #7486: fix: guard VerifyContributionShares against an empty verification-vector set (pasta) 8351db9 fix(llmq): reject invalid decrypted DKG shares before aggregation (pasta) 07494dd fix(consensus): reject negative transaction nVersion after DIP3 (pasta) 11ea306 Merge #7947: fix: stop quorum key warming from stalling DKG on 2-core masternodes (pasta) 25d7c85 Merge #7826: fix: skip repeated authenticated Platform bans (pasta) 18073dd Merge #7850: perf: let CHECKDATASIG use the signature cache (pasta) 3759397 Merge #7833: fix: gate signing intake before quorum retrieval (pasta) db98ee2 fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites (Konstantin Akimov) 54c9738 Merge bitcoin#36667: net: enforce timeout interval for paused send peers (Konstantin Akimov) f1adb2c Merge bitcoin#36665: sock: align exception monitoring between poll() and select() backends (Konstantin Akimov) 8becf6a Merge bitcoin#36669: refactor: store positions as a 64 bits integer in flatfiles (Konstantin Akimov) 4c0cbd6 fix(llmq): judge every sig-share sender by its own verified shares (pasta) 649259f fix(governance): cap the orphan parent objects re-requested per tick (pasta) 418f5ee fix(llmq): verify DKG contributions individually when a batch vvec check fails (pasta) abf2eb0 fix(llmq): reject invalid decrypted DKG shares before aggregation (pasta) 39bda8f fix(consensus): reject negative transaction nVersion after DIP3 (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for v24.0.0-rc.5, on top of `v24.0.x` at the v24.0.0-rc.4 tag (0e7382d, #7917). This PR holds the must-haves for rc.5: the v23.1.9 fixes that v24.0.x does not have yet, and #7947 (DKG stalls that get 2-core masternodes PoSe-banned). Without the v23.1.9 part, rc.5 would be missing fixes that the already published v23.1.9 ships, and it would accept transactions that v23.1.9 nodes reject (see Breaking Changes). The remaining `backport-candidate-24.0.x` work (#7916 and its test prerequisite) will follow in a separate PR so this one stays reviewable. ## What was done? ### v23.1.9 fixes not yet on v24.0.x v23.1.9 (tag 67c106f) contains 46 commits since v23.1.8. Each one was checked by content against `v24.0.x`. Most are backports of develop PRs that `v24.0.x` already has, either from before its branch point or picked for rc.3/rc.4. The rest are picked here with `git cherry-pick -x` from `v23.1.x`, in v23.1.9 merge order: - `fix(consensus): reject negative transaction nVersion after DIP3` (07494dd), **consensus**. Context-only conflict in `primitives/transaction.h`. - `fix(llmq): reject invalid decrypted DKG shares before aggregation` (8351db9). The QDATA path that v23 has in `src/active/quorums.cpp` is `src/llmq/net_quorum.cpp` on this branch. The new `feature_llmq_dkgerrors.py` case passes numeric `dkgsimerror` rates. - `fix(llmq): verify DKG contributions individually when a batch vvec check fails` (120b5db). The new `bls_tests` case calls `CBLSWorker::Start(4)`, since `Start()` takes an explicit worker count here. - `fix(governance): cap the orphan parent objects re-requested per tick` (33ec74e). The test helper uses this branch's `governance::OrphanVote`. - `fix(llmq): judge every sig-share sender by its own verified shares` (b1178f9). Ported into `NetSigning::ProcessPendingSigShares` (`llmq/net_signing.cpp`). The second half of the v23 fix (malformed share → `badSources`) is already on this branch. - bitcoin#36669 (8030deb). The `util/system` hunks land in `util/fs_helpers.{h,cpp}`. Two upstream hunks that the v23 port dropped are restored here, because this branch has their equivalents: `BlockInfo::data_pos` becomes `uint64_t` (in `interfaces/chain.h`), and `AutoFile`'s XOR read/write paths use `FileTell64`. - bitcoin#36665 (58abb28), bitcoin#36667 (7a21752). Clean. - `fix: keep flat-file positions 64-bit clean at the remaining raw seek/tell sites` (58b9bce). Also covers `BlockFilterIndex::LookupFilterRange`, which only this branch has. - #7833 fix: gate signing intake before quorum retrieval - #7850 perf: let CHECKDATASIG use the signature cache - #7826 fix: skip repeated authenticated Platform bans The last three are develop merges, picked with `-m1 -x` from develop; they change exactly the same lines as their develop merges. Every adaptation above is described in its commit message, and each matches how the open v23.1.9 → develop merge (#7961) resolves the same commit. Already on `v24.0.x` in another form, so not picked: - The credit-pool snapshot fix for #7549 (b4cd3fa) is a v23 hand-port of develop's 8503c5d / 0325195 (#7639), which this branch already has. - `fix(evo): keep -allowprivatenet out of masternode address validation` (0d2e579) is already covered by #7888. - `fix(rpc): don't dereference a missing netInfo when decoding provider payloads` (2aa3a87) and its help-text follow-up (7fc0edb). Here those accesses go through `CHECK_NONFATAL`, so an unknown-version ProTx payload gets an RPC error instead of a crash. The v23 commit message names this as the develop equivalent. ### Merge of v23.1.9 The last commit merges the v23.1.9 tag (67c106f, now `master`) with the tree unchanged (`-s ours`). `master` moved to v23.1.9 today, and `check_merge` requires `master` to fast-forward to `v24.0.x` plus the PR. The final v24.0.0 also has to contain every v23.1.9 commit. All of v23.1.9's content is already on the branch through the picks above, so every commit resolves to the `v24.0.x` side, as #7961 did on develop. Release bookkeeping (version, v23.1.9 release notes, man pages, flatpak entry) stays with v24's own release process. A trial real merge confirmed this: its only clean additions were those docs and three duplicate re-insertions of code the branch already has. ### #7947 fix: stop quorum key warming from stalling DKG on 2-core masternodes Picked with `-m1 -x`, before the merge. It changes exactly the same lines as its develop merge, and `v24.0.x` has 756fc4f (#7659), the commit it reverts. No version change is needed. `configure.ac` is already 24.0.0 with `CLIENT_VERSION_IS_RELEASE` false, so the build takes its version string from the `v24.0.0-rc.5` tag. ## How Has This Been Tested? Linux x86_64 against depends, `--enable-debug --enable-werror --disable-hardening --without-gui`: `dashd` and `test_dash` build cleanly. - Unit suites, all passing (149 cases): `transaction_tests`, `txvalidation_tests`, `sighash_tests`, `bls_tests`, `governance_inv_tests`, `llmq_utils_tests`, `llmq_dkg_tests`, `quorum_proofs_tests`, `flatfile_tests`, `fs_tests`, `net_tests`, `sock_tests`, `evo_netinfo_tests`, `evo_assetlocks_tests`, `blockfilter_index_tests`, `txindex_tests`, `blockmanager_tests`, `checkdatasig_tests`. - Functional tests, all passing: `feature_llmq_dkgerrors.py` (including the new contribution-invalid case), `feature_llmq_signing.py` (plain and `--spork21`), `feature_llmq_simplepose.py` (plain and `--disable-spork23`), `feature_llmq_rotation.py`, `feature_llmq_data_recovery.py`, `p2p_timeouts.py` (v1/v2), `mempool_accept.py`, `p2p_platform_ban.py`. In the first parallel run, `feature_llmq_data_recovery.py` (known flaky, see #7947), one `feature_llmq_signing.py --spork21` run and `p2p_platform_ban.py` failed while another build was loading the machine. All three pass when re-run on their own. - Each port was compared with its v23.1.9 commit and with #7961's develop resolution. The per-file changes are identical apart from the branch's own API differences listed above. A separate review checked v24-only call sites for second copies of each bug: - other `SigShareKey` batch verifiers; - other decrypt-then-aggregate paths; - other orphan re-request loops; - other tx version packing sites; - other raw `FlatFilePos` seeks. It also checked that the batch-vvec fallback cannot deadlock on a single-thread BLS pool; it runs on the DKG phase threads, not on a BLS worker. ## Breaking Changes - Consensus: once DIP3 is active, transactions with a negative `nVersion` are rejected (`bad-txns-version`). Every v23.1.9 node already enforces this, so mixed v23.1.9/rc.4 networks could disagree until rc.5 is deployed. Valid transactions and their txids are unchanged. - From #7947: quorum key share warming runs on its own `q-cache` thread again, which is one more thread per node. ## Checklist: - [x] 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: eaf46d1446c5f6752f2ac5d7aeaa37234cb6da4430db0a1a432320cfe61242dc3bb90c42e0a364238c2817ce0b1b1654dcc0cdd1ed9570f3b35cd2af5330e98e
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?
2d7b1557843e2077c3808cfc3c4522a579befd06Each 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:
8cfc3c4522— no passing CI record at this headGates that did not come back clean — please weigh these:
mech: warn — 9 invented line(s); 1 Dash-feature line(s) removed; 1 partial/prereq/low-risk finding(s)mech: warn — 1 edited line(s)mech: warn — 1 invented line(s); 1 partial/prereq/low-risk finding(s)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.
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.
develop