Repository navigation
fix: check hash and node ID sizes when writing masternode list diffs - #328
PastaPastaPasta wants to merge 1 commit into
Conversation
SimplifiedMNListDiff.toBuffer() wrote baseBlockHash, blockHash, deletedMNs entries and deleted quorum hashes without checking their size, so a diff a caller built by hand could serialize to a message with misaligned fields. SimplifiedMNListEntry only checked that an evonode's platformNodeID was a string. Reject anything other than 32-byte hashes and a 20-byte node ID. Parsed diffs always pass; only hand-built objects are affected. Part of #324. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Diff hash validation lib/deterministicmnlist/SimplifiedMNListDiff.js, test/deterministicmnlist/SimplifiedMNListDiff.js |
toBuffer() checks that block hashes, deleted masternode hashes, and deleted quorum hashes are 32-byte hexadecimal strings. Tests cover invalid values and entries. |
Platform node ID validation lib/deterministicmnlist/SimplifiedMNListEntry.js, test/deterministicmnlist/SimplifiedMNListEntry.js |
validate() requires platformNodeID to be a 20-byte hexadecimal string. Tests verify that toP2PBuffer() rejects malformed values. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
- Issue 324 covers diff-writer hash checks and
platformNodeIDvalidation. Its separate BLS and IPv6 follow-ups are not addressed here.
Possibly related PRs
- dashpay/dashcore-lib#323: Adds the serialization paths that this PR validates.
- dashpay/dashcore-lib#319: Adds evonode fields, including
platformNodeID, which this PR validates. - dashpay/dashcore-lib#320: Changes SML entry version-3 validation, where this PR tightens the
platformNodeIDcheck.
Suggested reviewers: antouhou
Merge Risk: 
bf39c
-
No actionable merge-blocking risk is established for this validation change.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding hash and node ID size checks when writing masternode list diffs. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… 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. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 4 files.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- 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.
|
✅ Final review complete — no blockers (commit bf39c51) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the complete four-file diff at bf39c51 and found no actionable in-scope issues. The checks enforce the intended hexadecimal field widths at the existing serialization and validation boundaries without changing valid byte encodings. This was a static review; no local builds or tests were run, and both the supplied exact-head CI snapshot and current PR checks report the library test job successful, covering lint, type, Node, and browser tests.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The change modifies serialization of peer-facing masternode list diff messages in SimplifiedMNListDiff.toBuffer() and validation of platform node IDs, a critical network boundary where malformed lengths can misalign subsequent fields. - 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 quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
SimplifiedMNListDiff.toBuffer()wrotebaseBlockHash,blockHash,deletedMNsentries and deleted quorum hashes without checking their length, andSimplifiedMNListEntryonly checked that an evonode'splatformNodeIDwas a string. A diff built by hand with a short or long value serialized to anmnlistdiffmessage whose later fields were misaligned, instead of failing.The writer now rejects anything other than 32-byte hashes and a 20-byte
platformNodeID. Parsed diffs always pass these checks, so only hand-built objects can be affected.This is the first item of #324. The other two items stay open there on purpose. Checking BLS encodings while parsing would need the synchronous parsers to load BLS. The IPv6/CJDNS ambiguity in the address-string API matches Core's own JSON and only shows up on regtest and devnets.
Tested: 10 new tests fail without the fix and pass with it. Lint, type tests and the full node suite (3,976 passing) are green.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests