fix: validate ProUpServTx nType at mempool acceptance - #60
Draft
PastaPastaPasta wants to merge 10 commits into
Draft
fix: validate ProUpServTx nType at mempool acceptance#60PastaPastaPasta wants to merge 10 commits into
PastaPastaPasta wants to merge 10 commits into
Conversation
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>
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.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
PastaPastaPasta
force-pushed
the
sec/v051
branch
from
July 27, 2026 16:20
8673afd to
e5587dc
Compare
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.
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
Reviewers asked to keep this check simple; the helper itself is enough without a dedicated CI unit-test file.
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
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
Add unit coverage for the mempool-vs-block consensus divergence: CProUpServTx::IsTriviallyValid and CheckProUpServTx must reject out-of-range and mismatched nType the same way BuildNewListFromBlock already does.
Mirror CProRegTx and BuildNewListFromBlock: reject out-of-range nType in CProUpServTx::IsTriviallyValid, and reject type mismatches in CheckProUpServTx. Closes the mempool/block divergence that let a single ProUpServTx stall getblocktemplate.
PastaPastaPasta
force-pushed
the
sec/v051
branch
from
August 1, 2026 18:07
e5587dc to
51846b0
Compare
Follow-up to the ProUpServTx nType mempool/block divergence fix. - Move the IsValidMnType check in RebuildListFromBlock above the netInfo loop. In its old position it was unreachable: dmn->nType is always in range, so by the time control reached it an out-of-range nType had already been rejected as bad-protx-type-mismatch. Block acceptance is unchanged - the same transactions are rejected either way - but the reject reason is now the accurate one, and it matches what IsTriviallyValid reports for the same payload. - Drop the IsValidMnType check from CheckProUpServTx. IsTriviallyValid, reached via GetValidatedPayload, already rejects an out-of-range nType before the body runs, so the check could never fire. - Cover both block-layer branches: an out-of-range nType must report bad-protx-type and a valid-but-wrong nType bad-protx-type-mismatch. Also pin the TxValidationResult for the mismatch case, so the peer punishment level cannot change unnoticed. - Add a positive case asserting that a matching nType still reaches the mempool, guarding against over-rejection. - Drop an unused util/check.h include. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit finding V051. A ProUpServTx carrying an invalid or mismatched
nTypecould be accepted into the mempool and then break block production.Issue
CProUpServTx::IsTriviallyValidnever checkedIsValidMnType(nType), andCheckProUpServTxnever checked thatnTypematched the registered masternode.BuildNewListFromBlockdoes enforce both.The consequence is a mempool/consensus mismatch: a transaction passes mempool acceptance, then
CreateNewBlockfails when the same transaction is evaluated against the deterministic MN list. A miner selecting that transaction cannot produce a block until it is evicted.Fix
src/evo/providertx.cpp: reject!IsValidMnType(nType)inIsTriviallyValidwithbad-protx-type.src/evo/specialtxman.cpp: inCheckProUpServTx, rejectnType != dmn->nType(bad-protx-type-mismatch) and!IsValidMnType(bad-protx-type), mirroringBuildNewListFromBlock.Test
test: reject ProUpServTx with invalid nTypeprecedes the fix, covering both the trivial-validation and the contextual-check paths.Review notes
TX_CONSENSUS, matching the surrounding rejections in the same functions.dashpay/dashdevelop @6d04c60ef36. Not rebase-tested against any newer tip; full functional suite not run.🤖 Generated with Claude Code