Repository navigation
backport: bitcoin#26177 - #7806
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This pull request has conflicts, please rebase. |
|
✅ Final review complete — no blockers (commit 153aa3c) · triage: normal |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughChain-parameter declarations and implementations move into the kernel layer. Devnet and regtest argument parsing now collects settings into option structures that the network factories use. Build lists and consumers are updated to use the kernel chain-parameter interface. Snapshot validation looks up assumeutxo data through the chain parameters stored in its options. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CreateChainParams
participant ReadRegTestArgs
participant ArgsManager
participant CChainParams
CreateChainParams->>ReadRegTestArgs: Collect regtest options
ReadRegTestArgs->>ArgsManager: Read command-line settings
ArgsManager-->>ReadRegTestArgs: Return setting values
ReadRegTestArgs-->>CreateChainParams: Return RegTestOptions
CreateChainParams->>CChainParams: Construct regtest parameters from options
CChainParams-->>CreateChainParams: Return chain parameters
Merge Risk: ⚪ Minimal · up to This change moves chain parameters into the kernel layer and keeps network behavior the same. The earlier problem with repeated -vbparams overrides has been fixed, and no other concrete regressions were found. Adding regression tests for repeated overrides is still recommended, but it does not block the merge. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 24.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 27 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the main change as a backport of bitcoin#26177 and is concise. Description check The description directly explains the backported change, testing, adaptations, and known review warnings.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autopilot · 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.
Comment @coderabbitai help to get the list of available commands.
8c6e8c4 to
f36ceda
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified both supplied findings against head f36ceda. Enabling production submitpackage exposes a confirmed Dash-specific provider-conflict gap that can prevent block-template creation; the header-hygiene nitpick is dropped as unchanged upstream style with no current compilation defect. Verification was static only: the supplied CI snapshot shows successful Linux, SQLite, no-wallet, fuzz and ARM builds plus lint, with other builds and test jobs still pending.
🔴 1 blocking
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: 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 large chain-parameter relocation and submitpackage RPC/topology changes warrant ordinary cross-cutting review, but the diff does not clearly change consensus rules, funds movement, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - 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/rpc/mempool.cpp`:
- [BLOCKING] src/rpc/mempool.cpp:806-808: Validate intra-package ProTx conflicts before enabling production submission
The child-with-parents restriction permits two independently funded ProRegTx parents claiming the same operator key and a child spending both parents' change outputs. Give each parent a fee above minimum relay but below the current mempool minimum, and give the child enough fee to lift the package above that minimum. AcceptPackage then defers both parents to AcceptMultipleTransactions, which runs every PreChecks before inserting any transaction. Both provider-conflict checks inspect only the existing mempool, and CheckSpecialTx checks the chain-tip masternode list, so neither parent sees the sibling's claim. The package checks reject input double-spends and duplicate asset-unlock indexes, but not provider conflicts; SubmitPackage subsequently inserts both through Finalize/addUnchecked without rechecking them. The later BroadcastTransaction calls also skip admission checks for transactions already in the mempool.
BlockAssembler validates each special transaction against the tip rather than a cumulatively updated masternode list, so it can select this ancestor package. BuildNewListFromBlock then rejects the second registration with bad-protx-dup-key, causing CreateNewBlock to throw while the conflicting entries remain eligible. Removing the regtest-only guard exposes this failure on production networks. Validate provider conflicts across the package before batch insertion and add a regression covering conflicting low-fee ProRegTx parents with a fee-bumping child.
| if (!IsChildWithParentsTree(txns)) { | ||
| throw JSONRPCTransactionError(TransactionError::INVALID_PACKAGE, "package topology disallowed. not child-with-parents or parents depend on each other."); | ||
| } |
There was a problem hiding this comment.
✅ No longer applies at
e4d54cd7; see the replies below.
🔴 Blocking: Validate intra-package ProTx conflicts before enabling production submission
The child-with-parents restriction permits two independently funded ProRegTx parents claiming the same operator key and a child spending both parents' change outputs. Give each parent a fee above minimum relay but below the current mempool minimum, and give the child enough fee to lift the package above that minimum. AcceptPackage then defers both parents to AcceptMultipleTransactions, which runs every PreChecks before inserting any transaction. Both provider-conflict checks inspect only the existing mempool, and CheckSpecialTx checks the chain-tip masternode list, so neither parent sees the sibling's claim. The package checks reject input double-spends and duplicate asset-unlock indexes, but not provider conflicts; SubmitPackage subsequently inserts both through Finalize/addUnchecked without rechecking them. The later BroadcastTransaction calls also skip admission checks for transactions already in the mempool.
BlockAssembler validates each special transaction against the tip rather than a cumulatively updated masternode list, so it can select this ancestor package. BuildNewListFromBlock then rejects the second registration with bad-protx-dup-key, causing CreateNewBlock to throw while the conflicting entries remain eligible. Removing the regtest-only guard exposes this failure on production networks. Validate provider conflicts across the package before batch insertion and add a regression covering conflicting low-fee ProRegTx parents with a fee-bumping child.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Confirmed: AcceptMultipleTransactions only rejects input double-spends and duplicate asset-unlock indexes within a package. Provider conflicts are checked only against the existing mempool and tip MN list (existsProviderTxConflict, CheckSpecialTx), so lifting the regtest guard as bitcoin#27609 does exposes sibling ProRegTx conflicts on production networks. The fix is a new Dash-specific package-level provider-conflict check plus a regression test, not a backport adaptation, so bitcoin#27609 should wait until that Dash prerequisite lands. Dropping bitcoin#27609 from this branch; it is being rewritten without it.
🤖 backportsys, on behalf of the Dash backport pipeline.
There was a problem hiding this comment.
I agree that dropping bitcoin#27609 from this batch resolves the production-exposure concern: the preceding commit still has the IsMockableChain() guard in submitpackage. Keeping the package-level provider-conflict validation and regression in a separate Dash prerequisite makes sense. The removal is not present in the checked-out head yet, so this remains pending the rewritten branch.
The finding stands on this commit until that change is pushed; the push gets a full review.
There was a problem hiding this comment.
Resolved (re-reviewed at 9ab1778b): Dropping bitcoin#27609 addresses the production-enablement issue: the current submitpackage handler rejects non-mockable chains before decoding or admitting transactions, and only regtest is mockable. This resolves the finding for this PR without claiming that intra-package ProTx conflict validation has been implemented.
There was a problem hiding this comment.
No longer applies (re-reviewed at e4d54cd7): Your removal of bitcoin#27609 is reflected in the current range, which contains no submitpackage changes. I verified that the non-regtest rejection remains in the handler, so this production-exposure finding no longer applies to this PR.
thepastaclaw
added
the
pastaclaw:changes-requested
|
Same issue as the inline finding: removing the submitpackage regtest guard exposes Dash's missing intra-package ProTx conflict validation on production networks. That needs new Dash code first, so bitcoin#27609 should be dropped from this batch until it exists. Dropping bitcoin#27609 from this branch; it is being rewritten without it. 🤖 backportsys, on behalf of the Dash backport pipeline. |
f36ceda to
9ab1778
Compare
thepastaclaw
removed
the
pastaclaw:changes-requested
DCG-Claude
changed the title
|
Branch rewritten at
🤖 backportsys, on behalf of the Dash backport pipeline. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 9ab1778 and its base. One blocking Dash-specific regression remains: repeated -vbparams arguments discard previously supplied optional activation settings; the header-include nitpick is unchanged upstream code and is dropped under the backport review policy. The prior production submitpackage exposure is fixed by restoring the regtest-only guard; verification was static, and the supplied CI snapshot shows successful builds and lint checks with tests and several builds still pending.
🔴 1 blocking
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: 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 cross-cutting refactor moves CChainParams into src/kernel/chainparams.{h,cpp} and restructures argument-driven construction without clearly changing consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - 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/chainparams.cpp`:
- [BLOCKING] src/chainparams.cpp:89-92: Preserve optional Dash settings across repeated vbparams overrides
Replacing the entire deployment-map entry discards optional Dash settings supplied by an earlier -vbparams argument for the same deployment. Before this refactor, each argument immediately called UpdateVersionBitsParameters, whose optional fields left the current value unchanged when omitted or set to -1. The new parser initializes those fields to -1 and overwrites the previous entry; the kernel then applies only that final entry, leaving network defaults instead of the earlier overrides. For example, `-vbparams=v24:0:999999999999:0:10:8:6:5:0` followed by `-vbparams=v24:1:999999999999` previously retained the 10-block window, thresholds 8/6, and disabled EHF while changing the start time. At this head it instead uses the default 250-block window, thresholds 200/150, and enabled EHF. Merge optional fields per deployment, preserving earlier values whenever the later value is -1, while retaining last-argument behavior for start time, timeout, and minimum activation height. Add regression coverage for repeated overrides with both omitted and explicit -1 optional fields.
| for (int j=0; j < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) { | ||
| if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) { | ||
| UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout, min_activation_height, nWindowSize, nThresholdStart, nThresholdMin, nFalloffCoeff, nUseEHF); | ||
| options.version_bits_parameters[Consensus::DeploymentPos(j)] = vbparams; | ||
| found = true; |
There was a problem hiding this comment.
✅ Resolved at
57982c01; see the replies below.
🔴 Blocking: Preserve optional Dash settings across repeated vbparams overrides
Replacing the entire deployment-map entry discards optional Dash settings supplied by an earlier -vbparams argument for the same deployment. Before this refactor, each argument immediately called UpdateVersionBitsParameters, whose optional fields left the current value unchanged when omitted or set to -1. The new parser initializes those fields to -1 and overwrites the previous entry; the kernel then applies only that final entry, leaving network defaults instead of the earlier overrides. For example, -vbparams=v24:0:999999999999:0:10:8:6:5:0 followed by -vbparams=v24:1:999999999999 previously retained the 10-block window, thresholds 8/6, and disabled EHF while changing the start time. At this head it instead uses the default 250-block window, thresholds 200/150, and enabled EHF. Merge optional fields per deployment, preserving earlier values whenever the later value is -1, while retaining last-argument behavior for start time, timeout, and minimum activation height. Add regression coverage for repeated overrides with both omitted and explicit -1 optional fields.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Same issue as the summary review: optional Dash -vbparams fields left at -1 now keep the value from an earlier -vbparams for the same deployment, which restores how UpdateVersionBitsParameters behaved before the refactor. (folded into the bitcoin#26177 commit)
🤖 backportsys, on behalf of the Dash backport pipeline.
There was a problem hiding this comment.
Resolved (re-reviewed at 57982c01): Verified your folded-in fix: all five optional Dash fields preserve the earlier entry when a later argument omits them or supplies -1, and kernel construction applies the merged values with the original sentinel semantics. The blocking behavior regression is fixed; the separate suggestion concerns missing regression coverage.
thepastaclaw
added
the
pastaclaw:changes-requested
|
This pull request has conflicts, please rebase. |
9ab1778 to
57982c0
Compare
|
Confirmed: this is a regression caused by the Dash adaptation, not by upstream. Upstream -vbparams has no optional fields, so last-wins is harmless there, but the old Dash code kept earlier window/threshold/falloff/useEHF values when a later argument passed -1 or left them out. ReadRegTestActivationArgs now carries those optional fields over from the existing map entry when the new value is -1, while start/timeout/min_activation_height still follow the last argument, as before; no new regression test was added. (folded into the 🤖 backportsys, on behalf of the Dash backport pipeline. |
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 57982c0 found no remaining blocking defects, and both prior findings are addressed. One in-scope suggestion remains: regression coverage for the new repeated -vbparams merge logic. No local builds or tests were run; current CI shows macOS and fuzz builds passing, other source builds and lint pending, and check_merge failing on a src/chainparams.cpp conflict with develop.
🟡 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: 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 large cross-cutting refactor relocates chain parameters and separates argument parsing from their construction in src/chainparams.cpp and src/kernel/chainparams.cpp, without clearly changing consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% 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; 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 each finding against the current code and only fix it if needed.
In `src/chainparams.cpp`:
- [SUGGESTION] src/chainparams.cpp:93-99: Add regression coverage for repeated Dash vbparams overrides
The new merge logic correctly preserves earlier optional Dash values, but the behavior that regressed during this extraction has no regression coverage. The override cases in src/test/versionbits_tests.cpp each supply a single three- or four-field argument; other existing tests do not exercise repeated overrides for the same deployment. Extend the existing tests through CreateChainParams with repeated arguments whose later override omits the optional fields or explicitly supplies -1, checking the resulting window, thresholds, falloff coefficient, and useEHF. Also cover later explicit values replacing earlier ones and an override without earlier optional values retaining network defaults. Verify that start time, timeout, and minimum activation height remain last-argument-wins. These cases would protect the Dash-specific adaptation through its public construction API rather than private parser state.
| auto& entry = options.version_bits_parameters[Consensus::DeploymentPos(j)]; | ||
| if (vbparams.window_size == -1) vbparams.window_size = entry.window_size; | ||
| if (vbparams.threshold_start == -1) vbparams.threshold_start = entry.threshold_start; | ||
| if (vbparams.threshold_min == -1) vbparams.threshold_min = entry.threshold_min; | ||
| if (vbparams.falloff_coeff == -1) vbparams.falloff_coeff = entry.falloff_coeff; | ||
| if (vbparams.use_ehf == -1) vbparams.use_ehf = entry.use_ehf; | ||
| entry = vbparams; |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for repeated Dash vbparams overrides
The new merge logic correctly preserves earlier optional Dash values, but the behavior that regressed during this extraction has no regression coverage. The override cases in src/test/versionbits_tests.cpp each supply a single three- or four-field argument; other existing tests do not exercise repeated overrides for the same deployment. Extend the existing tests through CreateChainParams with repeated arguments whose later override omits the optional fields or explicitly supplies -1, checking the resulting window, thresholds, falloff coefficient, and useEHF. Also cover later explicit values replacing earlier ones and an override without earlier optional values retaining network defaults. Verify that start time, timeout, and minimum activation height remain last-argument-wins. These cases would protect the Dash-specific adaptation through its public construction API rather than private parser state.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
The finding agrees the backported merge logic is correct: optional Dash fields left at -1 keep the value from an earlier -vbparams for the same deployment, as UpdateVersionBitsParameters() did before. It asks for new tests that upstream bitcoin#26177 doesn't ship and that aren't fixing anything the backport got wrong, so they belong in a separate Dash PR, not this commit.
🤖 backportsys, on behalf of the Dash backport pipeline.
There was a problem hiding this comment.
Still applies (re-reviewed at e4d54cd7): You are right that upstream bitcoin#26177 supplies no such test, so this is not a backport-completeness requirement. I retain it only as a non-blocking suggestion because a repeated-argument test would specifically exercise the map-composition behavior introduced by this extraction, which none of the existing single-override cases observes.
thepastaclaw
added
the
pastaclaw:commented
57982c0 to
e4d54cd
Compare
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at e4d54cd and compared the applicable transformations with Bitcoin Core bitcoin#26177. Dash's network parameters and extensions are preserved, and both prior blocking issues are addressed; one non-blocking regression-coverage suggestion remains. Validation was static only: the supplied CI snapshot has the main validation job and several supporting checks queued, so passing build/test validation at this head remains unconfirmed.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: 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 large, cross-cutting refactor moves chain parameters into src/kernel/chainparams.{h,cpp} and decouples argument parsing through options structs, but does not clearly change consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% 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; 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 each finding against the current code and only fix it if needed.
In `src/chainparams.cpp`:
- [SUGGESTION] src/chainparams.cpp:93-99: Add regression coverage for repeated Dash vbparams overrides
(existing thread: https://github.com/dashpay/dash/pull/7806#discussion_r4201301892)
The merge correctly preserves earlier optional Dash settings when a later override omits them or supplies -1. However, this extraction changes their composition from immediate updates to an accumulated deployment map, and the existing tests do not exercise repeated overrides for the same deployment: src/test/versionbits_tests.cpp supplies only single three- or four-field overrides, while the Dash-specific cases elsewhere supply one override per deployment. Add a focused table-driven test through CreateChainParams covering later omitted fields, explicit -1 retaining earlier values, explicit values replacing earlier ones, and network defaults remaining intact when no earlier optional value exists. Check the window, both thresholds, falloff coefficient, and useEHF, alongside last-argument behavior for start time, timeout, and minimum activation height. This protects the Dash adaptation introduced here; it is not an omitted upstream test or a remaining correctness blocker.
e4d54cd to
d45005f
Compare
d45005f to
d5fea03
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at d5fea03 and found no remaining in-scope correctness or backport-completeness issues. The repeated-vbparams repair preserves the original Dash semantics, production submitpackage exposure is absent, and additional repeated-override regression coverage remains intentionally deferred. This was static verification only: the supplied head-specific CI snapshot reports successful platform builds, lint, and nowallet tests, while standard Linux and SQLite tests and sanitizer and multiprocess builds remain pending.
🔴 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: 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) — This is a large, cross-cutting refactor moving chain-parameter construction into the kernel and adapting configuration and callers, rather than changing consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% 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; 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.
thepastaclaw
added
pastaclaw:approved
…nctionality to kernel b3e78dc refactor: Don't use global chainparams in chainstatemanager method (TheCharlatan) 382b692 Split non/kernel chainparams (Carl Dong) edabbc7 Add factory functions for Main/Test/Sig/Reg chainparams (Carl Dong) d938098 Remove UpdateVersionBitsParameters (Carl Dong) 84b8578 Decouple RegTestChainParams from ArgsManager (Carl Dong) 76cd4e7 Decouple SigNetChainParams from ArgsManager (Carl Dong) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin#24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only. #### Context The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on bitcoind's argument parsing logic. Instead, its interfaces should accept control and options structs that control the kernel library's desired configuration. Similar work towards decoupling the `ArgsManager` from the kernel has been done in bitcoin#25290, bitcoin#25487, bitcoin#25527 and bitcoin#25862. #### Changes By moving the `CChainParams` class definition into the kernel and giving it new factory functions `CChainParams::{RegTest,SigNet,Main,TestNet}`it can be constructed without an `ArgsManager` reference, unlike the current factory function `CreateChainParams`. The first few commits remove uses of `ArgsManager` within `CChainParams`. Then the `CChainParams` definition is moved to a new file in the `kernel/` subdirectory. ACKs for top commit: MarcoFalke: re-ACK b3e78dc 🛁 ryanofsky: Code review ACK b3e78dc. Only changes since last review were recent review suggestions. ajtowns: ACK b3e78dc Tree-SHA512: 3835aca1d3e3c75cc3303dd584bab3a77e58f6c678724a5e359fe4b0e17e0763a00931ee6191f516b9fde50496f59cc691f0709c0254206db3863bbf7ab2cacd Dash adaptations: - src/kernel/chainparams.{h,cpp}: moves Dash's CChainParams and network classes (all DIP, LLMQ, masternode and spork parameters), not Bitcoin's - src/kernel/chainparams.{h,cpp}: adds CChainParams::DevNet(DevNetOptions) next to Main()/TestNet()/RegTest() for Dash's fourth network - src/kernel/chainparams.h: VersionBitsParameters, RegTestOptions and DevNetOptions carry Dash's extra -vbparams fields and regtest/devnet overrides - src/kernel/chainparams.cpp: LLMQ overrides arrive as quorum names and are resolved in the kernel constructors, which own the per-chain LLMQ list - src/kernel/chainparams.cpp: the devnet name comes from DevNetOptions::name instead of gArgs.GetDevNetName(), keeping the kernel gArgs-free - src/kernel/chainparams.cpp: the moved classes keep develop's current values (EvoSnapshotHash, V24 bit 13, EVO_SHARES; no fRequireStandard after bitcoin#28354) - src/deploymentinfo.cpp: GetBuriedDeployment() maps the 12 names -testactivationheight already accepted (bip147 ... mn_rr) instead of upstream's segwit - src/chainparams.cpp: Dash's arg readers stay split into ReadRegTest*/ReadDevNet* helpers, called from ReadRegTestArgs()/ReadDevNetArgs() - src/chainparams.cpp: -minimumdifficultyblocks/-highsubsidyblocks/-highsubsidyfactor are read as separate optionals, so they log three lines instead of one - src/validation.h: upstream's <fs.h> is not added; Dash uses util/fs.h - src/active/dkgsession.cpp, src/chainlock/signing.cpp, src/instantsend/net_instantsend.cpp and Dash tests: include <chainparams.h>, which validation.h no longer provides Not applicable to Dash (intentionally omitted): - src/kernel/chainparams.cpp: CSigNetParams and CChainParams::SigNet(); Dash has no signet - src/kernel/chainparams.h: SigNetOptions and the SigNet() declaration; Dash has no signet - src/chainparams.cpp: ReadSigNetArgs() and the SIGNET branch of CreateChainParams(); Dash has no signet - src/kernel/chainparams.cpp: segwit/taproot fields, script_flag_exceptions and bech32_hrp of Bitcoin's chain classes; Dash's classes have none - src/deploymentinfo.cpp: the "segwit" -> DEPLOYMENT_SEGWIT mapping; Dash has no segwit deployment
d5fea03 to
153aa3c
Compare
thepastaclaw
removed
the
pastaclaw:approved
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 153aa3c, static review of the complete diff found no remaining blocking correctness defects; Dash network parameters and repeated override semantics are preserved. One non-blocking suggestion remains for regression coverage of the new Dash-specific override aggregation. No local builds or tests were run; the supplied CI snapshot shows successful lint and several platform builds, with test jobs and additional builds still pending.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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: 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) — This is a large, cross-cutting chain-parameter relocation and argument-dependency refactor, but the diff does not clearly change consensus rules or another qualifying critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% 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; 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 each finding against the current code and only fix it if needed.
In `src/chainparams.cpp`:
- [SUGGESTION] src/chainparams.cpp:93-99: Add regression coverage for repeated Dash vbparams overrides
(existing thread: https://github.com/dashpay/dash/pull/7806#discussion_r4201301892)
The current merge is correct, but this extraction replaces sequential consensus updates with Dash-specific map aggregation whose preservation branches are not exercised by existing tests. A nine-field override followed by a six-field override is particularly useful: window_size and threshold_start must take the later values, while threshold_min, falloff_coeff, and use_ehf must retain the earlier values. The single three- and four-field overrides in src/test/versionbits_tests.cpp cannot distinguish this merge from whole-entry replacement, and evo_shares_follows_v24 checks defaults rather than repeated overrides. Add a small table-driven test through CreateChainParams covering this mixed-length case, explicit -1 preservation, explicit replacement, and default retention; also check that start_time, timeout, and min_activation_height follow the final argument. This protects the adaptation introduced here, rather than requesting an omitted upstream test or changing upstream behavior.
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
Issue being fixed or feature implemented
Backports 1 Bitcoin Core v0.26 pull request(s) that the Dash queue selected, including any discovered prerequisites: bitcoin#26177.
What was done?
d5fea03a6cEach 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:
d5fea03a6c— no passing CI record at this headGates that did not come back clean — please weigh these:
mech: warn — 878 invented line(s); 194 Dash-feature line(s) removed; 6 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