Skip to content

test: cover retroactive provider tx refusal, fix wording - #7895

Merged
PastaPastaPasta merged 2 commits into
dashpay:developfrom
UdjinM6:test/no-islock-provider-txs-retroactive
Oct 16, 2026
Merged

PastaPastaPasta merged 2 commits into
dashpay:developfrom
UdjinM6:test/no-islock-provider-txs-retroactive

Conversation

@UdjinM6

@UdjinM6 UdjinM6 commented Oct 15, 2026 •

Copy link
Copy Markdown

Issue being fixed or feature implemented

Follow-up to #7858, which stopped masternodes from InstantSend-locking provider transactions. test_provider_tx_not_locked had gaps:

  • The mempool case checked that the ProTx had no lock right after an unrelated later transaction was locked. That is a timing proxy: a regression where the ProTx lock arrives slightly later than the other lock would still pass.
  • A provider transaction that signers first see in a block (NetInstantSend::BlockConnected → ProcessTx(tx, /*fRetroactive=*/true)) was not covered. The existing test passed a mutation that disabled the provider checks only for retroactive processing.
  • The release note said transactions spending provider outputs can be locked once the provider transaction is mined. A mined parent whose block is not ChainLocked is only accepted after nInstantSendConfirmationsRequired confirmations, and its children are retried when its block is ChainLocked, so "once the block that includes the provider transaction is ChainLocked" is the accurate condition. Nothing tested this.

What was done?

doc/release-notes-7858.md: children of provider transactions can be locked once the block that includes the parent is ChainLocked, not once the parent is mined.

Test changes are in test_provider_tx_not_locked in p2p_instantsend.py:

  • Mempool case: the ProTx is built with submit=False and broadcast inside assert_debug_log on the masternode, which waits for the txid-specific CheckCanLock -- txid=…: provider txs are not locked and ProcessTx -- txid=…: CheckCanLock returned false lines. These confirm that ProcessTx returned before input signing, rather than inferring it from another transaction's lock. A redundant sync_mempools() is removed.
  • New case: node 0 is isolated, a ProUpServTx and a normal transaction are mined there, and node 0 then reconnects, so the masternode first sees both in a block. The same log barrier, taken around the reconnect, proves the retroactive path refused the ProTx. The test then waits for the normal transaction's retroactive instantlock_internal as a positive control, and checks that the ProTx has no lock and the block is not yet ChainLocked.
  • Child check, in the same case: while that block is mined (one confirmation) but not ChainLocked, a child spending the ProTx output is broadcast inside a log barrier waiting for CheckCanLock -- txid=<child>: outpoint <protx>-0 too new and not ChainLocked, and has no instantlock_internal on any node. Mocktime is then advanced so the same block is ChainLocked without mining another one, and the child must get instantlock_internal on all nodes while the tip is unchanged. This covers both the refusal before the ChainLock and the retry of pending children when it arrives.

The normal-transaction and child checks poll instantlock_internal to require an InstantSend lock; instantlock is also true when the transaction's block is ChainLocked.

How Has This Been Tested?

  • p2p_instantsend.py passed repeatedly on macOS (arm64) with an --enable-debug build, on top of current develop. A full GitHub CI run passed on an earlier revision of this branch.
  • Mutation checks, each with a rebuilt dashd:
    • Skip the provider refusal only when fRetroactive is set and drop the TrySignInstantSendLock guard: the retroactive case fails at its log barrier. The original test passed under this mutation.
    • Drop the provider refusal in CheckCanLock(tx) and the TrySignInstantSendLock guard entirely: the mempool case fails at its log barrier.
    • Don't retry children when the parent's block is ChainLocked (RemoveNonLockedTx(txid, false) in RemoveConfirmedInstantSendLocks): the child never gets locked and the test fails.
    • Drop the "too new and not ChainLocked" refusal in CheckCanLock(outpoint): the child check fails at its log barrier.
  • The child check would also pass before fix: do not InstantSend-lock provider transactions #7858. It guards against breaking the descendants of provider transactions, not against reverting the refusal.
  • These tests don't exercise the TrySignInstantSendLock provider guard on its own; CheckCanLock(tx) refuses the provider transaction before input signing.

Breaking Changes

None. Test and release-note changes only.

Checklist:

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

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 15, 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: 76561efa-9606-44b3-9103-60a420950777
📥 Commits

Reviewing files that changed from the base of the PR and between 3c82adf and 52cf9ae.

📒 Files selected for processing: 1
  • test/functional/p2p_instantsend.py

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 release note states that a spending transaction can be locked after the block containing its provider transaction is ChainLocked. The functional test covers provider transactions submitted from the mempool and first observed in a block. It checks debug logs, InstantLock status, and ChainLock status across these cases.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Merge Risk: ⚪ Minimal · up to 52cf9

No actionable merge-blocking risk is established; the change is ready for 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 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 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 summarizes the main changes: expanded provider transaction refusal coverage and corrected release-note wording.
Description check Passed The description directly explains the test coverage, release-note correction, validation results, and scope of the changes.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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 commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit 52cf9ae) · triage: normal

@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: 3c82adf2c8

ℹ️ 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 test/functional/p2p_instantsend.py Outdated
self.log.info("Children of a provider tx are locked once its block is ChainLocked")
protx_out = node.getrawtransaction(protx_id, True)["vout"][0]
child_raw = node.createrawtransaction([{"txid": protx_id, "vout": 0}], {node.getnewaddress(): protx_out["value"] - Decimal("0.001")})
child_id = node.sendrawtransaction(node.signrawtransactionwithwallet(child_raw)["hex"])

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 Exercise child refusal before the parent ChainLock

When a regression permits children of a one-confirmation provider transaction to lock before the parent block is ChainLocked, or breaks the pending-child retry when that ChainLock arrives, this check still passes because the child is first broadcast only after wait_for_chainlocked_block has completed. Broadcast it before that ChainLock, verify instantlock_internal stays false, and then ChainLock the block and wait for the same child to lock; otherwise the test does not protect the boundary described by the release-note change.

AGENTS.md reference: AGENTS.md:L139-L139

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 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-15T19:34:47.580543Z 52cf9ae 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 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 supplied reviewer evidence against head 3c82adf. The refusal log barriers, retroactive positive control, and release-note correction match the implementation; one non-blocking coverage gap remains around a child's transition across the parent ChainLock boundary. This was a static review: no builds or tests were run, and the supplied CI snapshot shows successful lint checks with builds still queued and functional-test results pending.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (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: 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 functional-test changes require reasoning about asynchronous log barriers, isolated-node reconnection, retroactive InstantSend processing, and ChainLock timing, but do not change production critical surfaces.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 77% left, 5h 12% left), glm-5.3-flash (not used above high effort; tier asks max)
  • 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 — 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 `test/functional/p2p_instantsend.py`:
- [SUGGESTION] test/functional/p2p_instantsend.py:199-204: Cover the child's transition from waiting to locked
  The child is first broadcast after `wait_for_chainlocked_block` completes, so this verifies the successful ChainLock exception but cannot detect premature locking of children with shallow, non-ChainLocked parents or a broken pending-child retry. Both paths are directly relevant to the boundary described by the release-note correction: `CheckCanLock(outpoint)` rejects the shallow parent, and `RemoveConfirmedInstantSendLocks` schedules its children for retry when the ChainLock arrives. Extend the new isolated-node scenario by submitting a child after reconnecting, while the provider's block is still not ChainLocked. Wait for the child's txid-specific refusal and check that `instantlock_internal` is false, then require that same child to acquire an internal lock after the parent's block becomes ChainLocked, keeping the child unmined during the transition.

Comment thread test/functional/p2p_instantsend.py Outdated
Comment on lines +199 to +204
self.log.info("Children of a provider tx are locked once its block is ChainLocked")
protx_out = node.getrawtransaction(protx_id, True)["vout"][0]
child_raw = node.createrawtransaction([{"txid": protx_id, "vout": 0}], {node.getnewaddress(): protx_out["value"] - Decimal("0.001")})
child_id = node.sendrawtransaction(node.signrawtransactionwithwallet(child_raw)["hex"])
self.sync_mempools()
self.wait_until(lambda: all(n.getrawtransaction(child_id, True)["instantlock_internal"] for n in self.nodes))

@thepastaclaw thepastaclaw Oct 15, 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.

✅ Resolved at 52cf9aea; see the replies below.

🟡 Suggestion: Cover the child's transition from waiting to locked

The child is first broadcast after wait_for_chainlocked_block completes, so this verifies the successful ChainLock exception but cannot detect premature locking of children with shallow, non-ChainLocked parents or a broken pending-child retry. Both paths are directly relevant to the boundary described by the release-note correction: CheckCanLock(outpoint) rejects the shallow parent, and RemoveConfirmedInstantSendLocks schedules its children for retry when the ChainLock arrives. Extend the new isolated-node scenario by submitting a child after reconnecting, while the provider's block is still not ChainLocked. Wait for the child's txid-specific refusal and check that instantlock_internal is false, then require that same child to acquire an internal lock after the parent's block becomes ChainLocked, keeping the child unmined during the transition.

source: gpt-6.1-sol (phase2-reviewer: general)

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.

Resolved (re-reviewed at 52cf9aea): You now broadcast the child before its parent's block is ChainLocked, wait for its txid-specific 'too new and not ChainLocked' refusal, and check that it has no internal InstantSend lock on every node. After advancing mocktime, you require the same child to obtain an internal InstantSend lock on every node while the tip remains unchanged, covering both the refusal boundary and the pending-child retry.

@thepastaclaw thepastaclaw added the pastaclaw:commented thepastaclaw's latest review was comment-only label Oct 15, 2026
Wait for txid-specific signer refusal logs before checking that
provider transactions have no InstantSend lock. The mempool check
previously relied on an unrelated transaction locking first.

Add coverage for a provider transaction first seen by the signer in
a block. Also check that a child spending it is refused while that
block is not ChainLocked, and is locked once the block is ChainLocked
without being mined itself.

Follow-up to dashpay#7858.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@UdjinM6
UdjinM6 force-pushed the test/no-islock-provider-txs-retroactive branch from 3c82adf to 52cf9ae Compare October 15, 2026 19:29
@thepastaclaw thepastaclaw removed the pastaclaw:commented thepastaclaw's latest review was comment-only label Oct 15, 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 — Final validation — Phase 1 + Phase 2

Independently reviewed the complete diff at 52cf9ae and verified the reviewer conclusions against the InstantSend, ChainLock, RPC, and test-helper implementations; no actionable issues were found. The prior coverage finding is fixed: the same child is refused before its parent's ChainLock and must obtain an internal InstantSend lock afterward without another block being mined. Validation was static only; the supplied CI snapshot at 2026-10-15T20:00:30Z still had build checks queued and lint running, so it does not establish a completed test pass for this head.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (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: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: 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 adds multi-stage functional coverage involving peer isolation, retroactive processing, ChainLock timing, and child retries, but changes no production critical surface.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 78% left, 5h 13% left), glm-5.3-flash (not used above high effort; tier asks max)
  • 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 — 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 — 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 15, 2026
@PastaPastaPasta
PastaPastaPasta merged commit 4461504 into dashpay:develop Oct 16, 2026
48 of 52 checks passed
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 16, 2026
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Oct 16, 2026
…wording

52cf9ae test: cover retroactive refusal of provider tx InstantSend locks (UdjinM6)
95fed01 doc: fix when children of provider txs become lockable (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Follow-up to dashpay#7858, which stopped masternodes from InstantSend-locking provider transactions. `test_provider_tx_not_locked` had gaps:

  - The mempool case checked that the ProTx had no lock right after an unrelated later transaction was locked. That is a timing proxy: a regression where the ProTx lock arrives slightly later than the other lock would still pass.
  - A provider transaction that signers first see in a block (`NetInstantSend::BlockConnected` → `ProcessTx(tx, /*fRetroactive=*/true)`) was not covered. The existing test passed a mutation that disabled the provider checks only for retroactive processing.
  - The release note said transactions spending provider outputs can be locked once the provider transaction is mined. A mined parent whose block is not ChainLocked is only accepted after `nInstantSendConfirmationsRequired` confirmations, and its children are retried when its block is ChainLocked, so "once the block that includes the provider transaction is ChainLocked" is the accurate condition. Nothing tested this.

  ## What was done?
  `doc/release-notes-7858.md`: children of provider transactions can be locked once the block that includes the parent is ChainLocked, not once the parent is mined.

  Test changes are in `test_provider_tx_not_locked` in `p2p_instantsend.py`:

  - Mempool case: the ProTx is built with `submit=False` and broadcast inside `assert_debug_log` on the masternode, which waits for the txid-specific `CheckCanLock -- txid=…: provider txs are not locked` and `ProcessTx -- txid=…: CheckCanLock returned false` lines. These confirm that `ProcessTx` returned before input signing, rather than inferring it from another transaction's lock. A redundant `sync_mempools()` is removed.
  - New case: node 0 is isolated, a ProUpServTx and a normal transaction are mined there, and node 0 then reconnects, so the masternode first sees both in a block. The same log barrier, taken around the reconnect, proves the retroactive path refused the ProTx. The test then waits for the normal transaction's retroactive `instantlock_internal` as a positive control, and checks that the ProTx has no lock and the block is not yet ChainLocked.
  - Child check, in the same case: while that block is mined (one confirmation) but not ChainLocked, a child spending the ProTx output is broadcast inside a log barrier waiting for `CheckCanLock -- txid=<child>: outpoint <protx>-0 too new and not ChainLocked`, and has no `instantlock_internal` on any node. Mocktime is then advanced so the same block is ChainLocked without mining another one, and the child must get `instantlock_internal` on all nodes while the tip is unchanged. This covers both the refusal before the ChainLock and the retry of pending children when it arrives.

  The normal-transaction and child checks poll `instantlock_internal` to require an InstantSend lock; `instantlock` is also true when the transaction's block is ChainLocked.

  ## How Has This Been Tested?
  - `p2p_instantsend.py` passed repeatedly on macOS (arm64) with an `--enable-debug` build, on top of current develop. A full GitHub CI run passed on an earlier revision of this branch.
  - Mutation checks, each with a rebuilt `dashd`:
    - Skip the provider refusal only when `fRetroactive` is set and drop the `TrySignInstantSendLock` guard: the retroactive case fails at its log barrier. The original test passed under this mutation.
    - Drop the provider refusal in `CheckCanLock(tx)` and the `TrySignInstantSendLock` guard entirely: the mempool case fails at its log barrier.
    - Don't retry children when the parent's block is ChainLocked (`RemoveNonLockedTx(txid, false)` in `RemoveConfirmedInstantSendLocks`): the child never gets locked and the test fails.
    - Drop the "too new and not ChainLocked" refusal in `CheckCanLock(outpoint)`: the child check fails at its log barrier.
  - The child check would also pass before dashpay#7858. It guards against breaking the descendants of provider transactions, not against reverting the refusal.
  - These tests don't exercise the `TrySignInstantSendLock` provider guard on its own; `CheckCanLock(tx)` refuses the provider transaction before input signing.

  ## Breaking Changes
  None. Test and release-note changes only.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

  🤖 Generated with [Claude Code](https://claude.com/claude-code)

Top commit has no ACKs.

Tree-SHA512: 056ab842e7163e42973e62237c248d65e51023496c75def7434536cc2579b2029d66d98e3f52053ec1aa8af1d05afb1be27ee6fb9b50d75ab4cdb8153c46e6b5
(cherry picked from commit 4461504)
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Oct 16, 2026
…wording

52cf9ae test: cover retroactive refusal of provider tx InstantSend locks (UdjinM6)
95fed01 doc: fix when children of provider txs become lockable (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Follow-up to dashpay#7858, which stopped masternodes from InstantSend-locking provider transactions. `test_provider_tx_not_locked` had gaps:

  - The mempool case checked that the ProTx had no lock right after an unrelated later transaction was locked. That is a timing proxy: a regression where the ProTx lock arrives slightly later than the other lock would still pass.
  - A provider transaction that signers first see in a block (`NetInstantSend::BlockConnected` → `ProcessTx(tx, /*fRetroactive=*/true)`) was not covered. The existing test passed a mutation that disabled the provider checks only for retroactive processing.
  - The release note said transactions spending provider outputs can be locked once the provider transaction is mined. A mined parent whose block is not ChainLocked is only accepted after `nInstantSendConfirmationsRequired` confirmations, and its children are retried when its block is ChainLocked, so "once the block that includes the provider transaction is ChainLocked" is the accurate condition. Nothing tested this.

  ## What was done?
  `doc/release-notes-7858.md`: children of provider transactions can be locked once the block that includes the parent is ChainLocked, not once the parent is mined.

  Test changes are in `test_provider_tx_not_locked` in `p2p_instantsend.py`:

  - Mempool case: the ProTx is built with `submit=False` and broadcast inside `assert_debug_log` on the masternode, which waits for the txid-specific `CheckCanLock -- txid=…: provider txs are not locked` and `ProcessTx -- txid=…: CheckCanLock returned false` lines. These confirm that `ProcessTx` returned before input signing, rather than inferring it from another transaction's lock. A redundant `sync_mempools()` is removed.
  - New case: node 0 is isolated, a ProUpServTx and a normal transaction are mined there, and node 0 then reconnects, so the masternode first sees both in a block. The same log barrier, taken around the reconnect, proves the retroactive path refused the ProTx. The test then waits for the normal transaction's retroactive `instantlock_internal` as a positive control, and checks that the ProTx has no lock and the block is not yet ChainLocked.
  - Child check, in the same case: while that block is mined (one confirmation) but not ChainLocked, a child spending the ProTx output is broadcast inside a log barrier waiting for `CheckCanLock -- txid=<child>: outpoint <protx>-0 too new and not ChainLocked`, and has no `instantlock_internal` on any node. Mocktime is then advanced so the same block is ChainLocked without mining another one, and the child must get `instantlock_internal` on all nodes while the tip is unchanged. This covers both the refusal before the ChainLock and the retry of pending children when it arrives.

  The normal-transaction and child checks poll `instantlock_internal` to require an InstantSend lock; `instantlock` is also true when the transaction's block is ChainLocked.

  ## How Has This Been Tested?
  - `p2p_instantsend.py` passed repeatedly on macOS (arm64) with an `--enable-debug` build, on top of current develop. A full GitHub CI run passed on an earlier revision of this branch.
  - Mutation checks, each with a rebuilt `dashd`:
    - Skip the provider refusal only when `fRetroactive` is set and drop the `TrySignInstantSendLock` guard: the retroactive case fails at its log barrier. The original test passed under this mutation.
    - Drop the provider refusal in `CheckCanLock(tx)` and the `TrySignInstantSendLock` guard entirely: the mempool case fails at its log barrier.
    - Don't retry children when the parent's block is ChainLocked (`RemoveNonLockedTx(txid, false)` in `RemoveConfirmedInstantSendLocks`): the child never gets locked and the test fails.
    - Drop the "too new and not ChainLocked" refusal in `CheckCanLock(outpoint)`: the child check fails at its log barrier.
  - The child check would also pass before dashpay#7858. It guards against breaking the descendants of provider transactions, not against reverting the refusal.
  - These tests don't exercise the `TrySignInstantSendLock` provider guard on its own; `CheckCanLock(tx)` refuses the provider transaction before input signing.

  ## Breaking Changes
  None. Test and release-note changes only.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

  🤖 Generated with [Claude Code](https://claude.com/claude-code)

Top commit has no ACKs.

Tree-SHA512: 056ab842e7163e42973e62237c248d65e51023496c75def7434536cc2579b2029d66d98e3f52053ec1aa8af1d05afb1be27ee6fb9b50d75ab4cdb8153c46e6b5
(cherry picked from commit 4461504)
PastaPastaPasta added a commit that referenced this pull request Oct 17, 2026
6c02b2b ci: let DEPENDS_ARCH override the machine arch for multiprocess and tsan (pasta)
ce5dbf3 Merge #7888: fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding (pasta)
54cba5c Merge #7537: fix: data races and check-then-act races in the CoinJoin server (pasta)
08d8bca Merge #7909: feat(mining): build the masternode list incrementally during block assembly (pasta)
5633761 Merge #7895: test: cover retroactive provider tx refusal, fix wording (pasta)
cbb9500 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta)
8efdf5e Merge #7857: fix: mine a pending ProUpServTx before a key change of its masternode (pasta)
e56e6ae Merge #7842: fix(wallet): reject mnemonic passphrases that seed derivation truncates (pasta)
79480ff test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario (Konstantin Akimov)
a56ed47 fix(net): announce sporks to new peers instead of requesting them during mnsync (pasta)
cdaea5f Merge #7894: fix: due to logical conflicts feature_pruning.py had been broken while (pasta)
98d92f6 Merge #7817: backport: partial bitcoin#34156 restore cleanup ownership (pasta)
35e6d18 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta)
904d73d Merge #7881: fix: refuse to sign shared registrar updates that move the funder's coins (pasta)
95982f5 Merge #7782: fix(wallet): persist pending-observation coin locks once the record becomes readable (pasta)
cdf65d0 Merge #7836: backport: partial bitcoin#25812, bitcoin#29855 (pasta)
cc5716f Merge #7824: fix: retain initialized DKG sessions during inventory reads (pasta)
588e458 Merge #7827: backport: partial bitcoin#35984 (pasta)
795c6f1 Merge #7839: fix: release InstantSend tracking after mempool eviction (pasta)
dc1a6b2 Merge #7875: docs(rpc): clarify getchainlockbyheight availability (pasta)
e4798ff Merge #7876: fix(consensus): capture operator key scheme changes with unchanged bytes in state diffs (pasta)
fe68ecb Merge #7871: backport!: Merge bitcoin#27217: wallet: Replace use of purpose strings with an enum (pasta)
8348d22 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta)
861a579 Merge #7866: fix: stop `masternode payments` at the genesis block (pasta)
efcef71 Merge #7521: fix: update masternode rate limit on failed governance trigger path (pasta)
0623182 Merge #7831: fix: bound governance object signatures and reset the governance cache (pasta)
b230657 Merge #7841: fix: authenticate cached InstantSend proof bytes (pasta)
a1f37ea Merge #7820: fix: hold chainstate lock during CoinJoin input validation (pasta)
50a5617 Merge #7829: fix: preserve wallet permissions when changing passphrases (pasta)
2ff0103 Merge #7818: fix: return REST errors for unavailable deployment data (pasta)
cb39369 Merge #7834: fix: validate Electrum import input boundaries (pasta)
e7e3daa Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta)
2fea45f Merge #7792: fix: treat missing snapshots during evodb repair as repair errors (pasta)
3c5c039 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta)
0bf80f8 Merge #7810: fix(rpc): handle unavailable masternode payment inputs (pasta)
bcefee4 Merge #7822: fix: reject malformed detached quorum share batches (pasta)
86a7737 Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta)
886d5c4 Merge #7816: fix: check credit pool amount accumulation (pasta)
00fc3b9 Merge #7851: fix(rpc): correct masternode status help for top-level service field (pasta)
0e75c22 Merge #7814: fix(qt): restore HTML escaping of send confirmation recipients (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  Backports for v24.0.0-rc.4, on top of `v24.0.x` at the v24.0.0-rc.3 tag (0364ed6, #7808).

  ## What was done?

  Cherry-picked (`git cherry-pick -m1 -x`) every develop merge since rc.3 that carries `backport-candidate-24.0.x`, plus eight unlabeled fixes picked after an audit of every other develop merge since rc.3 (marked † below). They are in develop merge order:

  - #7814 fix(qt): restore HTML escaping of send confirmation recipients †
  - #7851 fix(rpc): correct masternode status help for top-level service field
  - #7816 fix: check credit pool amount accumulation
  - #7828 fix: omit recovery words from invalid mnemonic errors †
  - #7822 fix: reject malformed detached quorum share batches †
  - #7810 fix(rpc): handle unavailable masternode payment inputs †
  - #7838 fix(wallet): authenticate unlock deadline extensions
  - #7792 fix: treat missing snapshots during evodb repair as repair errors
  - #7821 fix: relock encrypted wallets after HD upgrade failures
  - #7834 fix: validate Electrum import input boundaries
  - #7818 fix: return REST errors for unavailable deployment data
  - #7829 fix: preserve wallet permissions when changing passphrases
  - #7820 fix: hold chainstate lock during CoinJoin input validation
  - #7841 fix: authenticate cached InstantSend proof bytes
  - #7831 fix: bound governance object signatures and reset the governance cache
  - #7521 fix: update masternode rate limit on failed governance trigger path
  - #7866 fix: stop `masternode payments` at the genesis block
  - #7867 fix: refuse masternode list RPCs for blocks outside the active chain †
  - #7871 backport!: bitcoin#27217: wallet: Replace use of purpose strings with an enum
  - #7876 fix(consensus): capture operator key scheme changes with unchanged bytes in state diffs
  - #7875 docs(rpc): clarify getchainlockbyheight availability †
  - #7839 fix: release InstantSend tracking after mempool eviction
  - #7827 backport: partial bitcoin#35984
  - #7824 fix: retain initialized DKG sessions during inventory reads
  - #7836 backport: partial bitcoin#25812, bitcoin#29855
  - #7782 fix(wallet): persist pending-observation coin locks once the record becomes readable
  - #7881 fix: refuse to sign shared registrar updates that move the funder's coins †
  - #7858 fix: do not InstantSend-lock provider transactions
  - #7817 backport: partial bitcoin#34156 restore cleanup ownership
  - #7894 test: fix feature_pruning.py restart arguments † (needed because #7792 makes the evodb repair fatal, which the old restart arguments trip)
  - #7893 fix(net): announce sporks to new peers instead of requesting them during mnsync
  - 4a19742 (from #7859) test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario. Test-only, picked as its commit. Without it, #7821's `test_failed_upgrade()` expects RPC error -1 for a 257-byte mnemonic passphrase, which #7842's new precheck rejects with -8 first, so `wallet_upgradetohd.py --legacy-wallet` fails. On develop, #7859 replaced that scenario before #7842 merged. The rest of #7859 is test cleanup and docs and is left out.
  - #7842 fix(wallet): reject mnemonic passphrases that seed derivation truncates
  - #7857 fix: mine a pending ProUpServTx before a key change of its masternode
  - #7846 fix: treat pending owner keys and Platform node IDs as one conflict namespace
  - #7895 test: cover retroactive provider tx refusal, fix wording
  - #7909 feat(mining): build the masternode list incrementally during block assembly
  - #7537 fix: data races and check-then-act races in the CoinJoin server
  - #7888 fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding

  34 of the 39 picks change exactly the same lines as their develop merges. #7888 and #7782 needed test-only changes (see testing), and three others needed adaptation, described in their commit messages:

  - **#7893** was merged to develop twice: f99212b with an earlier revision of its commit, then 72a5997 with the final one, so develop now announces sporks twice in the VERACK handler (a develop fix is open separately). Here the PR's final commit 45acdf3 is picked once instead of either merge. In `src/node/sync_manager.cpp`, this branch still sent `GETSPORKS` from the mnsync tick on every network, because #7847, which limited it to test chains, is not included. The request is removed entirely, as on develop. Mainnet spork values are hardcoded in `CSporkManager::GetSporkValue`, so the mainnet request had no effect.
  - **#7857** and **#7909**: develop folded `CChainstateHelper` into `Chainstate` (d5c1bd7), which is not on this branch, so the new code goes through `ChainHelper().special_tx` / `m_chain_helper.special_tx` instead of `m_special_tx`. The neighbouring develop-only `CSpecialTxProcessor::GetDeterministicMNListHash` is left out.

  #7888 tightens consensus rules for ExtAddr provider transactions behind V24, so I checked it against testnet. V24 is not active there yet (the tip's CbTx is still version 3), so the chain has no ExtAddr ProTxs the new rules could reject. rc.3's own serializer always writes canonical, sorted purposes, so every ProTx it builds passes the new rules. Mixed rc.3/rc.4 networks could only split on a hand-crafted non-canonical ExtAddr ProTx mined after V24 activates. So testnet miners and masternodes should move to rc.4 before V24 is signalled.

  Unlabeled develop fixes since rc.3 that were audited and left out:
  - Worth considering before the final release: #7869 (startup block replay after an unclean shutdown asserts on a quorum commitment, so recovery needs `-reindex-chainstate`; not a v24 regression), #7833 (gate signing intake before quorum retrieval, DoS hardening), #7682 (intermittent `relic_conf.h` parallel-build failure).
  - Left for 24.1: #7812, #7826, #7844, #7847, #7850, #7868, plus the routine upstream backports (#7780, #7806, #7830, #7855, #7856, #7862, #7863, #7865, #7883, #7884, #7898, #7906), refactors (#7805, #7878, #7880), and CI, docs and test-only changes.

  One CI-only commit comes last. It takes only the `ci/test` hunks of 6f71fa5 (#7901), which make `ci/test/00_setup_env_native_{multiprocess,tsan}.sh` honor `DEPENDS_ARCH`. Pull requests into `v24.0.x` run develop's workflow files (`pull_request_target`), and since #7901 those check the depends cache on x86_64 runners and pass the aarch64 build arch in `DEPENDS_ARCH`. Without the hunks, `linux64_multiprocess / Build depends` fails on any depends cache miss (`Build runner computes HOST=aarch64-linux-gnu but check-cache used x86_64-pc-linux-gnu`). Without `DEPENDS_ARCH` the scripts still use `uname -m`, so this branch's own push CI is unchanged.

  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.4` tag.

  ## How Has This Been Tested?

  Built and tested at 6c02b2b on macOS arm64 against depends, with `--enable-debug --enable-werror --disable-hardening`.

  - `make check`: all unit test suites pass, including `test_dash-qt`.
  - Functional tests, all passing: `feature_evo_shares_activation.py`, `feature_governance_objects.py`, `feature_llmq_signing.py` (plain and `--spork21`), `feature_masternode_shares.py`, `feature_sporks.py`, `interface_rest.py`, `p2p_instantsend.py`, `p2p_quorum_data.py`, `rpc_masternode.py`, `rpc_psbt.py`, `rpc_quorum.py`, `wallet_backup.py`, `wallet_dump.py`, `wallet_encryption.py`, `wallet_labels.py` and `wallet_upgradetohd.py`, each with `--descriptors` and `--legacy-wallet` where it supports them.

  - Each pick's changed lines were compared with its develop merge (or, for #7893, its PR commit). They are identical except for the #7893, #7857 and #7909 adaptations above, and the #7888 test adaptation below.
  - A local macOS arm64 debug build (`--enable-debug --enable-werror`) compiled most of the tree and stopped on a single error in `src/test/evo_netinfo_tests.cpp`. #7888's new test used `CNetAddr::V2()`, the serialization-parameter API from bitcoin#25284, which this branch lacks. The pick now writes that entry through an `OverrideStream` with `ADDRV2_FORMAT`, the way `NetInfoEntry::Serialize` does on this branch (see its commit message).
  - #7782's new unit test reloads the wallet with develop's three-argument `CWallet` constructor. On this branch the constructor still takes the CoinJoin loader and an `ArgsManager`, so the test passes them the way its fixture already does (see the commit message).
  - With both test fixes, a full local `make` (`dashd`, `dash-qt`, `test_dash`, `test_dash-qt`) completes with no errors.
  - `feature_pruning.py` is an extended test that CI does not run. #7894 is included because #7792 makes its restart path fail.

  ## Breaking Changes

  Carried over from the picked PRs:

  - #7876: rc.1 to rc.3 nodes that connected an affected block stored a wrong masternode list diff for it. Those nodes need `-reindex`; `-reindex-chainstate` is not enough for nodes that already marked the following block invalid.
  - #7888: stricter ExtAddr `netInfo` decoding and validation behind V24 (see above). The RFC1918 check now applies even with `-allowprivatenet`.
  - #7858: provider transactions no longer get InstantSend locks. Software that waits for an islock on a ProTx should wait for a ChainLock or confirmations instead.
  - #7857: a `ProUpServTx` spending an output of a pending operator key change, asset lock, asset unlock or MNHF signal is rejected from the mempool until that transaction confirms.
  - #7831: governance object signatures over 96 bytes are rejected, and the governance cache is discarded and re-synced on first start (`doc/release-notes-7831.md`).
  - #7871: address purposes are limited to `send`, `receive` and `refund`. Wallets with other purpose strings load with warnings, and `listlabels` errors on an unknown purpose (`doc/release-notes-27217.md`).
  - #7842: creating an HD wallet with a mnemonic passphrase over 248 bytes fails unless `-allow-deprecated-long-mnemonics` is set, and over 256 bytes always fails (`doc/release-notes-7842.md`).
  - #7792: a node whose evodb is missing a masternode list snapshot now stops at startup and asks for `-reindex`, and `evodb repair` reports it in `repairErrors`.
  - #7867: `protx info <proTxHash> <blockHash>`, `protx listdiff` and `masternode payments <blockHash>` return an error for blocks outside the active chain.
  - #7881: `protx shared_combine` on a `ProUpSharedRegTx` is refused unless its inputs are unlocked coins of a single address of the calling wallet, change returns to that address, and the fee is within `-maxtxfee`.
  - #7836: PSBTs referencing nonexistent previous outputs fail to deserialize (RPC error -22).
  - #7834: negative Electrum-import rescan indices are rejected.
  - #7893: on this branch, mainnet nodes no longer send `GETSPORKS` during mnsync (no effect, mainnet spork values are hardcoded).

  ## 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: 7de9535ca51620f4ddf5a46df2c3341a5e5fdd2263606f83f7db6b576c31ff7b82e1b285bf66988ac4b2729a9060513c45710135f61d7fd5c147b670d5f405dc
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.

3 participants