Skip to content

fix: conflict ProReg collateral reuse with in-mempool masternode updates - #63

Draft
PastaPastaPasta wants to merge 10 commits into
developfrom
sec/v054
Draft

fix: conflict ProReg collateral reuse with in-mempool masternode updates#63
PastaPastaPasta wants to merge 10 commits into
developfrom
sec/v054

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Audit finding V054 (high, CONFIRMED — upgraded from medium).

Issue

A ProRegTx that reuses an external collateral currently backing a live masternode replaces that MN, so the MN ceases to exist. Mempool conflict detection did not account for this: removeProTxSpentCollateralConflicts only covers collateral spends, not reuse.

The result is an in-block ordering hazard. If a replacement ProRegTx and an update targeting the replaced MN's proTxHash are mined in the same block, BuildNewListFromBlock fails with bad-protx-hash — aborting block assembly.

Fix

  • src/txmempool.cpp: in removeProTxConflicts, when a ProRegTx reuses an external collateral, look up the MN that collateral currently backs and drop any mempool transaction still referencing its proTxHash — such an update can never be mined afterwards.
  • existsProviderTxConflict gains the matching acceptance-time check, so the conflicting update is rejected at entry rather than only evicted later.
  • The duplicated removeSpentCollateralConflict lambda is extracted to a CTxMemPool::removeProTxReferences member, since both call sites now need it. The body is unchanged.

Tests

test: prove ProReg replacement conflicts with same-MN updates precedes the fix and fails without it. The review follow-up adds coverage for the replacement-eviction path specifically.

Review notes

  • The extraction commit is a pure refactor — the lambda body moved verbatim, including its LogPrint on the "should not happen" branch. Reviewing it separately from the behavior change should be quick.
  • Based on dashpay/dash develop @ 6d04c60ef36. Not rebase-tested against a newer tip; full functional suite not run.

🤖 Generated with Claude Code

thepastaclaw and others added 2 commits June 27, 2026 10:26
@thepastaclaw @claude
fix: gracefully handle HD chain type mismatch on wallet load
8fbae2b 
Replace the assert in ReadKeyValue that compared the DB record's key
type against CHDChain::IsCrypted() with a wallet load error. A wallet
whose record type and payload disagree is corrupt, not a programmer
bug, and should not abort the process.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thepastaclaw
fix: prune ChainLock seen cache with hysteresis
64cf27d 
dashpay#7424 bounded ChainlockHandler::seenChainLocks by switching it to
an unordered_limitedmap constructed as `seenChainLocks{MAX_SEEN_CHAINLOCKS}`.
unordered_limitedmap defaults nPruneAfterSize to nMaxSize, so prune() runs on
every unique insertion past 1024 entries: it allocates a vector of all 1025
iterators, std::sort()s it in full, and erases a single element, all while
ChainlockHandler::cs is held.

ProcessNewChainLock records the CLSIG hash before both the stale-height early
return and VerifyChainLock, and stale-height CLSIGs carry no misbehavior
penalty, so a peer could turn a stream of unique, unverified CLSIG hashes into
a stream of O(n log n) sorts under cs -- CPU amplification on the ChainLocks
relay path.

Construct the cache with an explicit prune-after size of twice the retained
size instead. The map now grows to 2048 entries and is pruned back to 1024 in
one batch, amortising each sort over 1024 evictions rather than one, at the
cost of a larger transient cache. The 2x ratio matches the default already used
by unordered_lru_cache. Note that pruning less often also reduces the chance
that an entry is evicted by the same prune that inserted it, since entries
inserted within one second share a timestamp and tie under prune()'s
comparator.

Stale CLSIG duplicate suppression is unchanged: hashes are still recorded
before the stale-height return, and the 24h time-based Cleanup() is unaffected.

Naming now distinguishes the retained size from the temporary prune threshold
(MAX_SEEN_CHAINLOCKS -> SEEN_CHAINLOCKS_RETAINED_SIZE plus a new
SEEN_CHAINLOCKS_PRUNE_AFTER_SIZE), and the constructor's defaulted
nPruneAfterSize is documented as the footgun it is.

Tests: two new limitedmap cases prove the generic container semantics -- no
prune at retained max+1, growth bounded by the trigger, and a batch prune back
to the retained size on crossing it -- plus the default-threshold behavior.
The ChainLock handler test no longer asserts a strict instantaneous 1024 cap;
it now verifies how the handler wires the cache up and that growth past the
retained size is retained until the trigger is crossed. Verified the handler
test fails when the fix is reverted.
@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d6f41d35-6c19-4e82-887d-53b3938bc155

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sec/v054

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 and others added 8 commits July 29, 2026 15:14
@thepastaclaw
ci: reject @mentions in pull request descriptions
9bc7ed5 
Flag GitHub @username mentions in PR bodies so they are not copied into
merge commits and re-notify people on merge, rebase, or backport.

Use a dedicated Python helper with unit tests, preserve the existing
required-check name, and allow complete email addresses.
@PastaPastaPasta
Merge dashpay#7482: fix(net): amortize ChainLock seen-cache pruning
dcc8a7b 
64cf27d fix: prune ChainLock seen cache with hysteresis (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  Dash dashpay#7424 bounded `ChainlockHandler::seenChainLocks` with `unordered_limitedmap`, but the single-argument constructor makes the prune trigger equal to the 1,024-entry retained size. After the cache fills, every unique insertion creates and sorts a 1,025-element iterator vector and removes one entry while holding `ChainlockHandler::cs`.

  `ProcessNewChainLock()` records the hash before its stale-height return and before signature verification. A peer can therefore submit unlimited unique stale-height CLSIGs, receive no misbehavior penalty, and trigger the full sort on every message.

  ## What was done?

  - Constructed `seenChainLocks` with a 1,024-entry retained size and a 2,048-entry prune trigger, so one sort removes a batch of 1,025 entries instead of one.
  - Renamed the constants and test accessors to distinguish the retained size from the temporary prune threshold.
  - Documented `unordered_limitedmap`'s default prune behavior and exposed its configured threshold for focused tests.
  - Updated the generic limited-map and ChainLock tests to verify growth through the hysteresis window, batch pruning back to the retained size, stale CLSIG duplicate suppression, and best-ChainLock lookup after eviction.

  Stale CLSIG hashes are still remembered before the early return, and the existing 24-hour cleanup behavior is unchanged.

  ## How Has This Been Tested?

  Tested locally on macOS against current `develop`:

  - `./autogen.sh` and `./configure` with the existing `aarch64-apple-darwin` depends prefix
  - `make -C src test/test_dash`
  - `./src/test/test_dash --run_test=limitedmap_tests,llmq_chainlock_tests`
  - `./src/test/test_dash --run_test=limitedmap_tests,llmq_chainlock_tests,llmq_signing_tests,llmq_dkg_tests,evo_islock_tests`
  - `git diff --check upstream/develop..HEAD`
  - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py`
  - `test/lint/lint-circular-dependencies.py`
  - `test/lint/lint-include-guards.py`
  - Scoped clang-format verification for the changed ChainLock files
  - Negative control: restoring the single-argument constructor makes the new handler regression test fail; restoring the fix passes it
  - Exact-head pre-PR code-review gate: `ship`, zero findings

  ## Breaking Changes

  None.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 64cf27d

Tree-SHA512: 90f014827163c1e898d39b90d7c88bd238d30d4cf9dfac15717dc4f260c5e1a1049f626895bdcf55ba0c74625535001fa602ba9403e11af79eeec375159328b1
@thepastaclaw
ci: drop unit tests for PR description mention check
9cde0fa 
Reviewers asked to keep this check simple; the helper itself is
enough without a dedicated CI unit-test file.
@PastaPastaPasta
Merge dashpay#7383: fix: reject mismatched HD chain wallet records
62325fe 
8fbae2b fix: gracefully handle HD chain type mismatch on wallet load (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented
  Malformed wallet database records can currently trip a process assertion while
  loading HD chain state. A wallet database whose HD chain key type disagrees with
  the serialized chain encryption flag is corrupt input and should be reported as
  a wallet load error instead of aborting the process.

  ## What was done?
  Changed `ReadKeyValue` to validate `HDCHAIN` / `CRYPTED_HDCHAIN` records with a
  normal conditional check. When the key type and serialized `CHDChain` state do
  not match, wallet loading now returns a descriptive database error.

  Added focused unit coverage for both mismatch directions.

  ## How Has This Been Tested?
  Ran:

  ```bash
  git diff --check upstream/develop...HEAD
  src/test/test_dash --run_test=walletdb_hdchain_tests
  code-review dashpay/dash upstream/develop e85b88f74882b2ecf437cbce56441177e1955e11 "Reject mismatched HDCHAIN/CRYPTED_HDCHAIN wallet records gracefully instead of asserting during wallet load"
  ```

  The pre-PR review gate returned `ship`.

  ## Breaking Changes
  None.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  thepastaclaw:
    @PastaPastaPasta Ready for merge consideration on head `8fbae2b3`.

- @UdjinM6 utACK'd this exact head on 2026-07-21.
- Branch is mergeable against `develop`; PastaClaw gate and CodeRabbit are clean on this SHA.
- The only remaining red jobs are the known unrelated functional-test flakes (`linux64-test` / `linux64_tsan-test`), already scoped as out-of-diff in earlier comments — no branch change needed for those.

Happy to handle any follow-up if you want a different landing path.
  UdjinM6:
    Makes sense, utACK 8fbae2b

Tree-SHA512: fdccad171b07c0878e5185a9acd09d8549d982bc637a4c025daa0cda86e37575f4996359b7402b135253a3af26bbd33c7a764febd528441203a13151ba13231e
@PastaPastaPasta
Merge dashpay#7496: ci: reject @mentions in pull request descriptions
35e9dac 
9cde0fa ci: drop unit tests for PR description mention check (PastaClaw)
9bc7ed5 ci: reject @mentions in pull request descriptions (PastaClaw)

Pull request description:

  ## Issue being fixed or feature implemented

  PR descriptions that include GitHub username mentions get copied into merge commits. After that, those people get notified again whenever the PR is merged, rebased, or backported. See dashpay#7493 for the motivating case ("Thanks knst" / similar mentions in a release PR description).

  This change makes CI fail when a PR description contains those mentions.

  ## What was done?

  Extended the existing semantic PR title workflow (`.github/workflows/semantic-pull-request.yml`) so it also validates the PR description:

  - Renamed the workflow/job to cover title and description
  - Kept the existing conventional-commit title check (`amannn/action-semantic-pull-request@v6`) unchanged
  - Added a step that reads `github.event.pull_request.body` via env on `pull_request_target` (`opened` / `edited` / `synchronize`), writes it to a file, and fails on GitHub-style `@username` mentions
  - Email addresses and empty descriptions are allowed
  - Failure message explains the merge/rebase/backport notification problem

  ## How Has This Been Tested?

  - YAML parses cleanly
  - `git diff --check upstream/develop...HEAD` clean
  - Local regex checks (same pattern as the workflow step):
    - `Thanks @knst.` → fail
    - `@PastaPastaPasta please look` → fail
    - `contact dev@example.com` → pass
    - `no mentions here` / empty body / bare `@` / `@@` diff markers → pass

  ## Breaking Changes

  None. Existing PRs with `@username` mentions in their description will fail this check on the next `edited` / `synchronize` event until the mentions are removed.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: 0fa3cb00d87f59e162add5ad62d0fd7aba2373dd4e4c01e8f4745b8e4e951d4830878ca9b7760f133fe6a5915b49ebef9967e46cc05620417a78e3d49ca26429
@PastaPastaPasta
test: prove ProReg replacement conflicts with same-MN updates
fee2296 
A ProRegTx that reuses a confirmed external collateral deletes the live MN mid-block. The same-block update for that proTxHash then fails BuildNewListFromBlock with bad-protx-hash and aborts CreateNewBlock. Assert the in-block hazard and that existsProviderTxConflict must reject either ordering in the mempool.
@PastaPastaPasta
fix: conflict ProReg collateral reuse with mempool MN updates
64f6836 
existsProviderTxConflict now links a replacement ProRegTx that reuses a live external collateral to any in-mempool ProUpServ/ProUpReg/ProUpRev for the MN being replaced, so both cannot coexist and CreateNewBlock cannot package the bad-protx-hash ordering. removeProTxConflicts also drops those updates when such a replacement is mined. Consensus block acceptance is unchanged; only mempool packaging/eviction is tightened.
@PastaPastaPasta
refactor: extract removeProTxReferences and cover replacement eviction
1ad32c4 
The earlier fix inlined a copy of removeProTxSpentCollateralConflicts' inner loop into removeProTxConflicts, minus the diagnostic log on the should-never-happen branch. Hoist that loop into a named CTxMemPool::removeProTxReferences helper and call it from both sites, so the two paths that drop TXs naming a vanished MN cannot drift apart.

Also add a removeForBlock assertion to the new test. existsProviderTxConflict only gates our own acceptance and cannot stop an attacker from mining the replacement ProRegTx themselves; the eviction hunk in removeProTxConflicts is what keeps the orphaned update from stalling our block assembly afterwards, and it previously had no coverage. Verified as a negative control: the new assertion fails (1 != 0) with that hunk disabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PastaPastaPasta @thepastaclaw