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.
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