fix: reject invalid extended masternode net info#7385
Conversation
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 3341988) |
|
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⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/evo_netinfo_tests.cpp (1)
500-561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
CheckServicereject mapping.These cases validate
ExtNetInfo::Validate(), but the behavior change insrc/evo/specialtxman.cppis theTX_BAD_SPECIAL/reason-string translation. A typo or missing case there would still pass this suite, so please add one validation test that feeds deserialized bad netinfo through the provider-tx path and asserts the new reject reason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/evo_netinfo_tests.cpp` around lines 500 - 561, The current `extnetinfo_validate_deser` test only checks `ExtNetInfo::Validate()` and does not cover the `CheckService` reject mapping in the provider-tx path. Add one focused test that deserializes an invalid `ExtNetInfo`, sends it through the special-tx validation flow in `specialtxman`/`CheckService`, and asserts the `TX_BAD_SPECIAL` reject reason string. Use the existing `ExtNetInfo`, `Validate()`, and provider-tx validation symbols to locate the right path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/evo_netinfo_tests.cpp`:
- Around line 500-561: The current `extnetinfo_validate_deser` test only checks
`ExtNetInfo::Validate()` and does not cover the `CheckService` reject mapping in
the provider-tx path. Add one focused test that deserializes an invalid
`ExtNetInfo`, sends it through the special-tx validation flow in
`specialtxman`/`CheckService`, and asserts the `TX_BAD_SPECIAL` reject reason
string. Use the existing `ExtNetInfo`, `Validate()`, and provider-tx validation
symbols to locate the right path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0bd68d96-c614-44c3-a829-1adc6c815bee
📒 Files selected for processing (3)
src/evo/netinfo.cppsrc/evo/specialtxman.cppsrc/test/evo_netinfo_tests.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped consensus-safety fix. Three previously unreachable assert(false) arms in CheckService are converted into stable TX_BAD_SPECIAL rejections, and ExtNetInfo::Validate() now enforces the per-purpose MAX_ENTRIES_EXTNETINFO cap that the in-memory builder enforces at insertion. Unit coverage exercises the three deserialization paths (duplicate entries, oversized list, domain on non-HTTPS purpose). No in-scope issues.
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/evo_deterministicmns_tests.cpp (1)
724-733: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert
TX_BAD_SPECIALin this helper.Right now this only checks
state.GetRejectReason(). A regression that keeps the same string but changes the validation class would still pass, even though this PR is also locking down theTX_BAD_SPECIALmapping.Suggested test tightening
auto check_reject_reason = [&](std::shared_ptr<NetInfoInterface> net_info, const std::string& reject_reason) { TxValidationState state; { LOCK(cs_main); BOOST_CHECK(!CheckProRegTx(BuildExtNetInfoProRegTx(std::move(net_info)), chainman.ActiveChain().Tip(), dmnman, chainman.ActiveChainstate().CoinsTip(), chainman, state, /*check_sigs=*/false)); } + BOOST_CHECK_EQUAL(state.GetResult(), TxValidationResult::TX_BAD_SPECIAL); BOOST_CHECK_EQUAL(state.GetRejectReason(), reject_reason); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/evo_deterministicmns_tests.cpp` around lines 724 - 733, The test helper check_reject_reason currently only verifies state.GetRejectReason(), so it can miss regressions where the reject string stays the same but the validation class changes. Update this helper in evo_deterministicmns_tests.cpp to also assert the TxValidationState maps to TX_BAD_SPECIAL, using the existing state object from CheckProRegTx alongside the reject reason check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 724-733: The test helper check_reject_reason currently only
verifies state.GetRejectReason(), so it can miss regressions where the reject
string stays the same but the validation class changes. Update this helper in
evo_deterministicmns_tests.cpp to also assert the TxValidationState maps to
TX_BAD_SPECIAL, using the existing state object from CheckProRegTx alongside the
reject reason check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c42647b0-4c12-4901-9713-b4f014910ca6
📒 Files selected for processing (1)
src/test/evo_deterministicmns_tests.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only delta (0f5fd825) adds CheckProRegTx-level regression coverage for the three new TX_BAD_SPECIAL reject paths introduced by 5ba9ace (BadInput, Duplicate, MaxLimit deserialization escapes). Implementation and test coverage are correct and in scope. Only a minor test-hardening nit from CodeRabbit is worth surfacing; no carried-forward prior findings (prior PastaClaw review was clean).
💬 1 nitpick(s)
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT while preserving the verified nonblocking finding.
| auto check_reject_reason = [&](std::shared_ptr<NetInfoInterface> net_info, const std::string& reject_reason) { | ||
| TxValidationState state; | ||
| { | ||
| LOCK(cs_main); | ||
| BOOST_CHECK(!CheckProRegTx(BuildExtNetInfoProRegTx(std::move(net_info)), chainman.ActiveChain().Tip(), | ||
| dmnman, chainman.ActiveChainstate().CoinsTip(), chainman, state, | ||
| /*check_sigs=*/false)); | ||
| } | ||
| BOOST_CHECK_EQUAL(state.GetRejectReason(), reject_reason); | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: Also assert TxValidationResult::TX_BAD_SPECIAL in check_reject_reason
The fix in 5ba9ace deliberately maps BadInput/Duplicate/MaxLimit to TX_BAD_SPECIAL with stable reason strings. The new helper only checks the reject reason, so a future regression that keeps the same reason string but degrades the result class (e.g. TX_CONSENSUS or TX_RESULT_UNSET) would slip through. Since this test exists specifically to lock down both halves of that mapping, adding a result-class assertion costs one line and tightens the guarantee.
| auto check_reject_reason = [&](std::shared_ptr<NetInfoInterface> net_info, const std::string& reject_reason) { | |
| TxValidationState state; | |
| { | |
| LOCK(cs_main); | |
| BOOST_CHECK(!CheckProRegTx(BuildExtNetInfoProRegTx(std::move(net_info)), chainman.ActiveChain().Tip(), | |
| dmnman, chainman.ActiveChainstate().CoinsTip(), chainman, state, | |
| /*check_sigs=*/false)); | |
| } | |
| BOOST_CHECK_EQUAL(state.GetRejectReason(), reject_reason); | |
| }; | |
| auto check_reject_reason = [&](std::shared_ptr<NetInfoInterface> net_info, const std::string& reject_reason) { | |
| TxValidationState state; | |
| { | |
| LOCK(cs_main); | |
| BOOST_CHECK(!CheckProRegTx(BuildExtNetInfoProRegTx(std::move(net_info)), chainman.ActiveChain().Tip(), | |
| dmnman, chainman.ActiveChainstate().CoinsTip(), chainman, state, | |
| /*check_sigs=*/false)); | |
| } | |
| BOOST_CHECK_EQUAL(state.GetResult(), TxValidationResult::TX_BAD_SPECIAL); | |
| BOOST_CHECK_EQUAL(state.GetRejectReason(), reject_reason); | |
| }; |
source: ['coderabbit']
thepastaclaw
force-pushed
the
fix/extnetinfo-validation
branch
from
0f5fd82 to
d818922
Compare
|
Addressed the latest CodeRabbit test-hardening nit by adding the Validation:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #7385 at head d818922. The single prior nitpick (assert TxValidationResult::TX_BAD_SPECIAL in check_reject_reason) is resolved at src/test/evo_deterministicmns_tests.cpp:732. No new findings in the latest delta; the change is a one-line test hardening that strengthens coverage of deserialized ExtNetInfo rejection paths.
Note: GitHub does not allow me to approve my own PR, so this is posted as a COMMENT while preserving the verified clean result.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/evo_deterministicmns_tests.cpp (1)
671-708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHand-rolled serialization format lacks documentation.
DeserializeCoreP2PExtNetInfomanually replicatesExtNetInfo's wire format (version byte, map size, purpose key, entry-vector size) with unexplained magic values (uint8_t{1},WriteCompactSize(ds, 1)). This is presumably intentional to bypass validation-gated construction, but a brief comment noting what each write represents would help future maintainers keep this helper in sync if the real serialization format changes.✏️ Suggested comment additions
static std::shared_ptr<NetInfoInterface> DeserializeCoreP2PExtNetInfo(const std::vector<NetInfoEntry>& entries) { CDataStream ds(SER_DISK, CLIENT_VERSION); - ds << uint8_t{1}; - WriteCompactSize(ds, 1); + ds << uint8_t{1}; // ExtNetInfo serialization version + WriteCompactSize(ds, 1); // number of purpose entries in the map ds << NetInfoPurpose::CORE_P2P; - WriteCompactSize(ds, entries.size()); + WriteCompactSize(ds, entries.size()); // number of NetInfoEntry values for this purpose for (const auto& entry : entries) { ds << entry; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/evo_deterministicmns_tests.cpp` around lines 671 - 708, The helper DeserializeCoreP2PExtNetInfo manually reconstructs ExtNetInfo wire serialization with unexplained fixed values, so add a brief inline comment around the CDataStream writes to document what the version byte, map size, purpose key, and entry count represent. Make it clear that this intentionally mirrors ExtNetInfo serialization so future changes to ExtNetInfo or NetInfoPurpose are kept in sync, and reference DeserializeCoreP2PExtNetInfo and BuildExtNetInfoProRegTx when locating the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 671-708: The helper DeserializeCoreP2PExtNetInfo manually
reconstructs ExtNetInfo wire serialization with unexplained fixed values, so add
a brief inline comment around the CDataStream writes to document what the
version byte, map size, purpose key, and entry count represent. Make it clear
that this intentionally mirrors ExtNetInfo serialization so future changes to
ExtNetInfo or NetInfoPurpose are kept in sync, and reference
DeserializeCoreP2PExtNetInfo and BuildExtNetInfoProRegTx when locating the
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67c5c483-9f64-431e-bea6-387fe6ec3355
📒 Files selected for processing (1)
src/test/evo_deterministicmns_tests.cpp
Address CodeRabbit nitpick on PR dashpay#7385: annotate the version byte, map purpose count, purpose key and entry count written manually in DeserializeCoreP2PExtNetInfo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
PR #7385 is a clean defense-in-depth fix that converts three previously-asserted ExtNetInfo states into graceful TX_BAD_SPECIAL rejections and adds a missing per-purpose MAX_ENTRIES_EXTNETINFO check to Validate(). Coverage at both primitive and transaction levels is thorough. Prior review had zero still-valid findings and no new code issues were found in the latest delta. One convergent commit-history suggestion: fold the comment-only review-nit commit 9c26643 into d818922 before merge so it does not land as permanent history noise.
🟡 1 suggestion(s)
Prior reconciliation: no carried-forward prior findings from d8189221; the previous exact-SHA review had zero still-valid findings.
New latest-delta findings: one commit-history suggestion on the comment-only 9c26643a19 follow-up commit.
Source: reviewers opus, claude-sonnet-5, and gpt-5.5 for general and dash-core-commit-history; verifier opus.
🤖 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 `<commit:9c26643a19>`:
- [SUGGESTION] <commit:9c26643a19>:1: Squash comment-only CodeRabbit-nitpick fixup into d8189221b1 before merge
Commit 9c26643a19 ("docs: document hand-rolled ExtNetInfo serialization in test helper") is a 6-line comment-only change to `DeserializeCoreP2PExtNetInfo`, a helper introduced two commits earlier in the same PR stack by d8189221b1 ("test: cover invalid deserialized ExtNetInfo tx validation"). Its own commit message states it exists purely to "Address CodeRabbit nitpick on PR #7385". Because dashpay/dash merges PR stacks without squashing, this commit will land in develop as a permanent standalone entry — adding clutter to `git log`, an extra step for `git bisect`, and misleading `git blame` on the helper's comments (blame will point at 9c26643a instead of the commit that introduced the code being annotated). Recommend folding it into d8189221b1 via interactive rebase before merge so the helper is introduced in a single commit with its documenting comments already in place. Not blocking — purely history-hygiene.
9c26643 to
24ade24
Compare
|
Folded the comment-only CodeRabbit follow-up commit into the test coverage commit as suggested. The resulting tree is unchanged from the previously reviewed head Validation:
|
There was a problem hiding this comment.
Code Review
Prior finding prior-1 (squash comment-only fixup 9c26643 into the test commit) is FIXED: the stack is now a clean two-commit series (5ba9ace fix, 24ade24 test) and git diff 9c26643a19..24ade24eef is empty, confirming content parity. All six agents converge with zero new findings. No blocking issues, no suggestions, no nitpicks remain.
Source: reviewers opus, claude-sonnet-5, and gpt-5.5 for general and dash-core-commit-history; verifier opus.
|
@coderabbitai review |
✅ Action performedReview finished.
|
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-dup-netinfo-entry"); | ||
| case NetInfoStatus::MaxLimit: | ||
| assert(false); | ||
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-toomany"); |
There was a problem hiding this comment.
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-toomany"); | |
| return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-protx-netinfo-maxlimit"); |
`ExtNetInfo::Validate()` can return `BadInput`, `Duplicate`, and `MaxLimit` for states reachable via deserialization that bypass the in-memory builder's per-entry checks. `CheckService` previously asserted on those statuses; map them to graceful `TX_BAD_SPECIAL` rejections with stable reason strings instead. Also enforce the per-purpose `MAX_ENTRIES_EXTNETINFO` cap inside `ExtNetInfo::Validate()` so an oversized deserialized list is rejected rather than silently accepted. Adds unit coverage for the three deserialization paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/extnetinfo-validation
branch
from
24ade24 to
3341988
Compare
|
Addressed kwvg’s suggestion by folding the MaxLimit reject reason rename into the existing stack: Validation:
|
There was a problem hiding this comment.
Code Review
All six agent reviews converge on zero findings for the current head 334198886a. The latest delta is a consistent reject-reason rename (bad-protx-netinfo-toomany -> bad-protx-netinfo-maxlimit) applied in place across both the fix commit (44a536d581) and the tests commit (334198886a), preserving a clean two-commit stack with matching production and test assertions.
Prior reconciliation: the previous 24ade24e review had zero open findings. The older squash/comment-only fixup finding remains FIXED; that fixup commit is not in the current stack.
Carried-forward prior findings: none.
New findings in the latest delta: none.
Source: reviewers opus, claude-sonnet-5, and gpt-5.5 for general and dash-core-commit-history; verifier opus.
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Summary
Issue being fixed or feature implemented
Extended masternode net info loaded from serialized transaction payloads can
contain semantic states that the in-memory builder rejects during normal
construction. Validation should reject those states explicitly instead of
relying on internal assertions.
What was done?
validation failures.
per-purpose lists, and domain entries under non-HTTPS purposes.
CheckProRegTxregression coverage for the sameinvalid deserialized net info states.
How Has This Been Tested?
Tested locally on macOS:
Pre-PR automated code review was also run for
upstream/develop..fix/extnetinfo-validationafter the transaction-levelcoverage was added and returned
ship.Breaking Changes
None.
Checklist
(for repository code-owners and collaborators only)