Skip to content

backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695 - #7916

Merged
knst merged 4 commits into
dashpay:developfrom
thepastaclaw:backport-0.26-b186-misc
Oct 18, 2026
Merged

knst merged 4 commits into
dashpay:developfrom
thepastaclaw:backport-0.26-b186-misc

Conversation

@DCG-Claude

@DCG-Claude DCG-Claude commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

🔎 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 backport: v0.26 bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695 thepastaclaw/dash#100 at 8cfc3c4522 — no passing CI record at this head

Gates that did not come back clean — please weigh these:

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

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
  • 🔄 Rebase onto current develop
  • ❌ Close — abandon this batch and release its items

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T03:49:35.848954Z 8cfc3c4 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@thepastaclaw

thepastaclaw commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 8cfc3c4) · triage: normal

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 81c0e605-c13c-4800-850c-513583767b09
📥 Commits

Reviewing files that changed from the base of the PR and between 6d5ffe8 and 1af9327.

📒 Files selected for processing: 15
  • src/flat-database.h
  • src/governance/net_governance.cpp
  • src/llmq/dkgsessionmgr.cpp
  • src/llmq/net_quorum.cpp
  • src/llmq/quorums.cpp
  • src/llmq/quorumsman.cpp
  • src/streams.h
  • src/wallet/dump.cpp
  • src/wallet/scriptpubkeyman.cpp
  • src/wallet/scriptpubkeyman.h
  • src/wallet/test/util.cpp
  • src/wallet/wallet.cpp
  • test/functional/p2p_leak_tx.py
  • test/functional/wallet_fundrawtransaction.py
  • test/functional/wallet_migration.py
💤 Files with no reviewable changes: 1
  • src/streams.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Walkthrough

The patch replaces direct stream writes and message payloads with span-based byte access, and removes DataStream::Serialize. Legacy wallet handling now filters script sets, tracks destinations from non-owned scripts during migration, removes matching address-book entries, and warns when loading wallets without the descriptor flag. Functional tests add transaction upload and nested P2SH migration coverage, and adjust fee calculations.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to 1af93

No identified issue needs resolution before merge; proceed with normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 14 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly identifies the pull request as a backport and lists the four upstream Bitcoin Core changes represented in the changeset.
Description check Passed The description directly explains the backported changes, affected tests, adaptations, and validation results. It is relevant to the changeset.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw thepastaclaw left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: normal by gpt-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); agent phase1-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; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-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.

Comment thread src/wallet/wallet.cpp Outdated
Comment on lines +295 to +298
// 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."));
}

@thepastaclaw thepastaclaw Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thepastaclaw added the pastaclaw:commented thepastaclaw's latest review was comment-only label Oct 16, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thepastaclaw added pastaclaw:approved thepastaclaw's latest review approved this PR and removed pastaclaw:commented thepastaclaw's latest review was comment-only labels Oct 16, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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)

@thepastaclaw thepastaclaw added pastaclaw:commented thepastaclaw's latest review was comment-only and removed pastaclaw:approved thepastaclaw's latest review approved this PR labels Oct 16, 2026
PastaPastaPasta added a commit to thepastaclaw/review-system that referenced this pull request Oct 16, 2026
…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>
@DCG-Claude

Copy link
Copy Markdown
Collaborator Author

CI failed at 1af9327 on dashpay/dash: linux64_asan-test / Test source, linux64_multiprocess / Build depends

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 thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: normal by gpt-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); agent phase1-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; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

@thepastaclaw thepastaclaw added pastaclaw:approved thepastaclaw's latest review approved this PR and removed pastaclaw:commented thepastaclaw's latest review was comment-only labels Oct 16, 2026
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm failure on this test; it worths to backport to 24.0

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@knst knst left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

27800: utACK
28125: tACK
27869: to remove
28232: utACK
27695: utACK

…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
@DCG-Claude
DCG-Claude force-pushed the backport-0.26-b186-misc branch from 1af9327 to 8cfc3c4 Compare October 18, 2026 03:43
@DCG-Claude DCG-Claude changed the title backport: bitcoin#27800, bitcoin#28125, bitcoin#27869, bitcoin#28232, bitcoin#27695 backport: bitcoin#27800, bitcoin#28125, bitcoin#28232, bitcoin#27695 Oct 18, 2026
@DCG-Claude

Copy link
Copy Markdown
Collaborator Author

Branch rewritten at 8cfc3c4522 (4 backport(s)).


🤖 backportsys, on behalf of the Dash backport pipeline.

@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 18, 2026
@DCG-Claude

Copy link
Copy Markdown
Collaborator Author

27800: utACK

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/wallet/wallet.cpp
return false;
}

// Get all invalid or non-watched scripts that will not be migrated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@knst knst added this to the 24.1 milestone Oct 18, 2026

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 8cfc3c4

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: normal by gpt-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); agent phase1-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; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-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.

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 18, 2026
@knst
knst merged commit fe232f9 into dashpay:develop Oct 18, 2026
55 checks passed
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 18, 2026
@UdjinM6 UdjinM6 modified the milestones: 24.1, 24 Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants