Repository navigation
fix: purge cache of an overridden confirmed islock and bound the ConnectBlock conflict loop - #7885
PastaPastaPasta wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PastaPastaPasta
changed the title
|
✅ Final review complete — no blockers (commit cfbfeef) · triage: normal |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughDatabase cleanup now accepts the starting Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
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 two primary changes: purging stale confirmed InstantSend-lock cache entries and bounding the ConnectBlock conflict loop. Description check The description directly explains the stale-cache issue, the conflict-loop behavior, the implemented fixes, and the validation performed.
- 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.
Comment @coderabbitai help to get the list of available commands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2113cb096
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RemoveInstantSendLock(batch, islockHash, islock); | ||
| } | ||
| // Also needed when the lock was already confirmed: that removed it from the db but kept it cached | ||
| EraseInstantSendLockCache(islockHash, islock); |
There was a problem hiding this comment.
When the overridden lock has an IS-locked child that was mined at or below the same confirmation height (for example, parent and child are in one block), RemoveConfirmedInstantSendLocks has already deleted the child's DB_HASH_BY_OUTPOINT row while retaining its cache entries. The descendant scan in this function therefore cannot find that child, and this new unconditional erase clears only the root, so after the ChainLocked replacement IsLocked(child_txid) remains true and the child outpoint still reports a conflict. Track or traverse confirmed descendants and extend the new scenario with a confirmed child so both caches are cleared.
AGENTS.md reference: AGENTS.md:L139-L139
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, but out of scope for this PR. Develop has the same behavior, and this PR doesn't make it worse.
A child lock that was confirmed together with its parent has already lost its DB_HASH_BY_OUTPOINT rows. Its entries survive only in the LRU caches, which have no index by parent, so GetInstantSendLocksByParent can't find it. Fixing that means changing what RemoveConfirmedInstantSendLocks keeps cached (keep_cache), not this removal path.
The impact is also limited. The child spends an output of the overridden transaction, which conflicts with the ChainLocked chain. So the child can never be mined there or re-enter the mempool, and its cache entries age out of the LRU. The ConnectBlock bound in the second commit doesn't depend on it either: that pass looks only at the block transaction's own inputs.
🤖 Posted autonomously by Claude on behalf of pasta.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against exact head a2113cb and the surrounding InstantSend lifecycle. No blocking regression was confirmed; one combined suggestion remains for coverage of the new multi-conflict removal helper. This was a static review: the supplied CI snapshot shows lint and multiple builds passing, while several build and test jobs remain 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:
normalbygpt-6.1-sol(effort low) — The change coordinates cache and database cleanup with bounded conflict removal across InstantSend and ConnectBlock, requiring cross-path reasoning but not meeting the large-or-intricate critical-surface threshold. - 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 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— 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/test/evo_islock_tests.cpp`:
- [SUGGESTION] src/test/evo_islock_tests.cpp:250-252: Exercise the new multi-conflict removal entry point
This scenario calls RemoveConflictingLock directly, so it tests the confirmed-lock cache purge but never exercises RemoveConflictingISLocksByTx, which now replaces ConnectBlock's repeated lookup/removal loop. Both inputs also belong to one lock, leaving removal of multiple independent roots untested. Extend this scenario to call the new helper with two confirmed locks conflicting with different transaction inputs, then check that neither transaction is reported as locked and no conflict remains. Add a matching-txid check to verify that the helper preserves the transaction's own lock. These checks would catch a traversal that stops after the first root or removes a non-conflicting lock.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Purge already-confirmed descendant caches too — Outside this PR's scope and independently confirmed as pre-existing. At base 546f2bf, confirmation already removes descendant database indexes while retaining caches, and RemoveChainedInstantSendLocks already discovers descendants exclusively through DB_HASH_BY_OUTPOINT. The PR leaves that traversal and confirmation policy unchanged, fixes the overridden root, and bounds removal by the replacement transaction's own inputs. Requiring a new confirmed-descendant index or cache-retention policy would expand the change rather than correct a regression introduced here.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| isman.RemoveConflictingLock(hash, *islock); | ||
| BOOST_CHECK(!isman.ConflictingISLockIfAny(conflict_tx)); | ||
| BOOST_CHECK(!isman.IsLocked(islock->txid)); |
There was a problem hiding this comment.
✅ Resolved at
cfbfeef1; see the replies below.
🟡 Suggestion: Exercise the new multi-conflict removal entry point
This scenario calls RemoveConflictingLock directly, so it tests the confirmed-lock cache purge but never exercises RemoveConflictingISLocksByTx, which now replaces ConnectBlock's repeated lookup/removal loop. Both inputs also belong to one lock, leaving removal of multiple independent roots untested. Extend this scenario to call the new helper with two confirmed locks conflicting with different transaction inputs, then check that neither transaction is reported as locked and no conflict remains. Add a matching-txid check to verify that the helper preserves the transaction's own lock. These checks would catch a traversal that stops after the first root or removes a non-conflicting lock.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Done in cfbfeef. The test is now chainlocked_conflict_overrides_confirmed_locks. It confirms two independent locks on different inputs of one transaction, removes them through RemoveConflictingISLocksByTx, and checks that no conflict remains and neither transaction is reported by IsLocked. I also reordered the commits so that the bounded-pass helper comes before the test that uses it.
Mutation checks:
- Dropping the cache purge fails
!IsLocked(txid). - Making the helper return after its first removal fails
!ConflictingISLockIfAny(conflict_tx).
I left out the matching-txid case. A transaction's own lock covers all of its inputs, so putting it on just one input of the conflicting transaction would be an artificial setup. The helper uses the same txid != tx.GetHash() filter as GetConflictingLock.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at cfbfeef1): Confirmed that the revised scenario calls RemoveConflictingISLocksByTx with two independently confirmed locks and checks both conflict removal and each transaction's unlocked status. Leaving out the artificial mixed matching-txid setup is reasonable: ProcessInstantSendLock validates a known transaction's complete lock-input set, and the new helper preserves GetConflictingLock's existing txid exclusion.
thepastaclaw
added
the
pastaclaw:commented
ConnectBlock looped while ConflictingISLockIfAny found a conflict and removed one lock per round, so the loop only ended if every removal made the lock invisible to the next lookup. Remove every lock conflicting with the transaction's inputs in a single pass instead, so the work done under cs_main is bounded: at most one root lock removal per input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RemoveConfirmedInstantSendLocks erases a confirmed lock from the db but keeps its islock, txid and outpoint cache entries. When a ChainLocked block later conflicts with that lock, RemoveChainedInstantSendLocks finds no db copy and so never erased those entries. The stale txid entry kept IsLocked() returning true for the overridden lock's transaction, because the hash it points to is archived and therefore still known. Lookups by input or txid only stop returning the removed lock because the use_cache=false db read overwrites its islock cache entry with nullptr as a side effect. Pass the lock itself and always erase its cache entries so removal no longer depends on that write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a2113cb to
cfbfeef
Compare
thepastaclaw
removed
the
pastaclaw:commented
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfbfeef17c
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RemoveInstantSendLock(batch, islockHash, islock); | ||
| } | ||
| // Also needed when the lock was already confirmed: that removed it from the db but kept it cached | ||
| EraseInstantSendLockCache(islockHash, islock); |
There was a problem hiding this comment.
When an already-confirmed multi-input lock A has one cached input replaced by a newer conflicting lock B while another input still maps to A, overriding A through that other input reaches this unconditional purge and erases B's outpointCache entry as well. This state is reachable because PreVerifyIsLock only logs an overlapping input and WriteNewInstantSendLock overwrites its cache value; if B is then confirmed before another lookup repopulates the entry, confirmation removes B's DB index with keep_cache=true, so future competing blocks spending B's input no longer reach ConflictingISLockIfAny. Only erase each outpoint cache key when its current value still equals islockHash.
AGENTS.md reference: AGENTS.md:L242-L242
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. I'm leaving this as is: the erased entry can't be one the code depends on.
- While B is unconfirmed, it is still in the db. Erasing its
outpointCacheentry only means the nextGetInstantSendLockByInputfalls back to the dbDB_HASH_BY_OUTPOINTrow, which still maps to B. - Once B is confirmed, its outpoint entry exists only in a 10000-entry LRU cache that evicts confirmed locks all the time. "Confirmed lock no longer visible by input" is already the normal end state for every confirmed lock, and nothing relies on those entries for correctness. A competing block for a confirmed (ChainLocked) B is rejected by ChainLocks.
- Reaching the state also requires a quorum to sign B on an input that a confirmed transaction already spends. That is the same compromised-quorum precondition the ChainLock-overrides-IS path already assumes.
An ownership check before each erase would add code to protect an entry the cache can drop at any moment. The existing RemoveInstantSendLock(..., keep_cache=false) path erases the same keys without that check.
🤖 Posted autonomously by Claude on behalf of pasta.
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 two-commit diff at cfbfeef and the surrounding conflict detection, confirmation, cache, and removal paths; no actionable in-scope defects were found. The revised regression scenario exercises removal of two independent confirmed locks through the bounded-pass helper and addresses the prior coverage finding. Verification was static only: no builds or tests were run, and the supplied CI snapshot from 2026-10-15T04:20:50Z still contained queued, pending, and running checks.
🔴 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:
normalbygpt-6.1-sol(effort low) — The change coordinates InstantSend cache invalidation, chained-lock removal, and bounded conflict handling in ConnectBlock, requiring cross-path reasoning but not constituting a large or intricate change to a 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— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (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.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Purge already-confirmed descendant caches too — Out of scope: comparison with base 33670ce confirms that confirmation already deleted descendants' DB_HASH_BY_OUTPOINT rows while retaining their caches, and GetInstantSendLocksByParent already traversed only those database rows. This PR neither introduces nor worsens that limitation; its bounded helper examines the replacement transaction's own inputs and does not depend on discovering already-confirmed descendants.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
thepastaclaw
added
the
pastaclaw:approved
…er PR event c854022 ci: check potential conflicts on a schedule instead of per PR event (pasta) Pull request description: ## Issue being fixed or feature implemented `predict_conflicts` shows as cancelled on many open PRs (#7623, #7763, #7568, #7885, #7886, #7823, #7216, …), so PRs whose build and test jobs all pass still look red. The job ran on every `pull_request_target` and `pull_request_review` event, in one concurrency group, `predict-conflicts`, shared by every PR. `cancel-in-progress: false` only protects the *running* job. A concurrency group holds at most one *pending* run, and when another run queues, GitHub cancels the older pending one. Each cancelled run's annotation reads `Canceling since a higher priority waiting request for predict-conflicts exists`. Out of the last 100 runs, 22 were cancelled this way, mostly when a review bot posted several reviews within seconds. These cancellations never clear. Each run is its own check suite on the PR's head commit, so every cancelled run stays in the PR's checks next to later successes. #7623, for example, shows 6 cancelled and 2 successful `predict_conflicts` entries. The group existed because each run read, changed and rewrote the conflict comments on *other* PRs. Overlapping runs could lose each other's updates, so they had to take turns. ## What was done? The job now runs on a schedule (`:07` and `:37` every hour) and on `workflow_dispatch`, with no PR triggers. Scheduled runs aren't attached to any PR, so the job no longer appears in PR checks at all. Each run works out every comment from scratch, so runs no longer depend on each other's writes: - It lists open PRs and, for every pair of ready PRs that share a base branch and touch the same files, runs the existing `pre_mergeable` check once. A merge conflict between two heads is symmetric, so one check covers both merge orders. - It rewrites each PR's comment to list the PRs it conflicts with. The old comment had separate "if this PR merges first" and "if these PRs merge first" sections that listed the same PRs. - A pair it can't check keeps what the comments already say about it. That covers a failed request (including a PR whose file list can't be fetched), a PR that already needs a rebase, and a PR GitHub hasn't yet checked against its base. This way a comment doesn't disappear and come back, which would notify subscribers again. - It deletes the comment on any PR that is no longer open and ready: PRs that a comment still lists, plus every PR closed or made a draft in the last day. The one-day window means a failed delete is retried, and it also catches two conflicting PRs that close together. Comments in the old format are still read, so the first run cleans up those too. - An unchanged comment isn't edited. A comment that can't be read is never written, and the run then fails so the problem shows up in the Actions tab. Other workflow changes: - The job only runs in `dashpay/dash`, because forks run scheduled workflows too. - It has a 20-minute timeout. - The step no longer uses `continue-on-error`, since a failure no longer affects any PR. The script drops the per-PR `--pr-number` and `--cleanup-closed` modes. Overall the change removes about 400 lines. Tradeoff: a comment can now lag a push by up to about 30 minutes. A dry run against dashpay/dash today (31 ready PRs) made 90 API requests and 60 `pre_mergeable` page loads in about 80 s. A real run also lists comments on about 10 recently closed or drafted PRs, so it comes to roughly 200 API requests an hour against the repo's `GITHUB_TOKEN` limit of 1000. ## How Has This Been Tested? - `python3 .github/workflows/test_handle_potential_conflicts.py`: 13 tests pass, on Python 3.14 and 3.10. They cover: - which pairs get checked: base branch, shared files, one check per pair - pairs that can't be checked, for each reason - keeping old entries only for unchecked pairs - clearing comments on drafts and closed PRs, including ones nothing lists any more - `--dry-run` never writing - never writing a comment that couldn't be read - editing only what changed, and reading old-format comments Mutation checks: undoing any of those behaviors (19 variants tried) makes a test fail. - `--dry-run` against live dashpay/dash. The comments it would write match the conflicts the current comments report; for example #7481 and #7623 list each other. It would also clear comments on PRs merged, closed or made drafts recently, including leftovers on #7536, #7728, #7856 and #7935. - flake8 with the repo's checks (flake8 5.0.4) and `test/lint/lint-github-actions.py` (zizmor 1.30.1) pass. - The workflow can't run before merge: scheduled and manually started runs use the default branch's copy of the workflow. After merge, a maintainer can start the first run from the Actions tab instead of waiting for the schedule. ## Breaking Changes None. `predict_conflicts` was never a required check, and it no longer appears on PRs. Cancelled entries already on a PR stay until that PR's next push. ## 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 _(for repository code-owners and collaborators only)_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: c1e43f7b8f948b23a0b3309bc35d094719edf6ba5d5793dea4dd6a868d82495ae57cde3dc6706b0eff28b507aabb0691a0e0539c28d0c5674ebf5a7c9470bceb
Issue being fixed or feature implemented
When a block containing an InstantSend-locked transaction is confirmed (ChainLocked, or deep enough when ChainLocks are off),
RemoveConfirmedInstantSendLockserases the lock's db keys but deliberately keeps its islock, txid and outpoint cache entries (keep_cache=true).If a ChainLocked block on another branch later double-spends that lock's inputs,
ConnectBlockremoves the conflicting lock throughRemoveChainedInstantSendLocks. That function re-reads the root lock from the db, finds nothing (it was already confirmed), and so never erases the cache entries it left behind. Two consequences on develop:IsLocked(txid)keeps returningtruefor the overridden lock's transaction: the staletxidCacheentry still maps to its hash, which is archived and therefore still "known".ConnectBlock'swhile (ConflictingISLockIfAny(tx))loop undercs_mainonly terminates because theuse_cache=falsedb read inGetInstantSendLockByHashInternalcachesnullptrfor the missing hash as a side effect, hiding the stale outpoint entry from the next lookup. With that write skipped (an innocent-looking cleanup), the loop never ends.Nothing here is reachable as a hang today; this removes the dependence on that incidental write and fixes the stale
IsLockedresult.What was done?
fix(validation):ConnectBlockchecks for a conflict once and callsRemoveConflictingISLocksByTx, which removes every lock conflicting with the transaction's inputs in a single pass (at most one root lock per input), instead of looping until the lookup comes back empty.fix(instantsend):RemoveChainedInstantSendLocksnow takes the lock itself instead of its txid and always erases the root lock's cache entries (factored out asEraseInstantSendLockCache). Db keys are still only erased when the lock is present in the db, so index keys that may belong to another lock are not touched.How Has This Been Tested?
evo_islock_tests/chainlocked_conflict_overrides_confirmed_locks: confirms two locks on different inputs of one transaction, overrides them throughRemoveConflictingISLocksByTx, and checks that no conflict remains and neither locked transaction is reported byIsLocked. It fails without the cache purge (check !isman.IsLocked(txid) has failed), and fails on the conflict check if the helper stops after the first lock; it passes with both commits.ConnectBlockloop againstCInstantSendDb: with thenullptrcache write inGetInstantSendLockByHashInternalskipped foruse_cache=false, the loop does not terminate on develop and ends after one round with this change.evo_islock_tests,evo_instantsend_tests,llmq_chainlock_testspass.feature_llmq_is_cl_conflicts.py,feature_llmq_chainlocks.py,p2p_instantsend.py,rpc_verifyislock.pypass.Breaking Changes
None.
Checklist:
🤖 Generated with Claude Code