fix: guard deprecated service netInfo access#7344
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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 (2)
WalkthroughThis PR refactors deprecated service field generation in core_write.cpp by introducing a centralized helper function Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/evo/core_write.cppsrc/evo/core_write.cpp:5:10: fatal error: 'evo/assetlocktx.h' file not found ... [truncated 1071 characters] ... lang/install/lib/clang/18/include" 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 |
|
✅ Review complete (commit 2ab467a) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped fix that prevents a remote-attacker-triggerable crash via decoderawtransaction under -deprecatedrpc=service when the special-tx payload deserializer bails out early and leaves netInfo null. The patch routes all four deprecated service renderers through a CHECK_NONFATAL-guarded helper and adds a targeted functional test for malformed ProRegTx/ProUpServTx payloads. Both reviewers (Claude and Codex) found no in-scope blocking issues; only minor test-brittleness nitpicks were raised.
💬 1 nitpick(s)
| block_count = node.getblockcount() | ||
| for tx_hex in [ | ||
| "03000100000000000000020000", # ProRegTx with only nVersion=0 payload | ||
| "03000200000000000000020000", # ProUpServTx with only nVersion=0 payload |
There was a problem hiding this comment.
💬 Nitpick: Brittle assertion couples test to helper parameter name
The substring "Internal bug detected: obj.netInfo" binds the test to the CHECK_NONFATAL stringification of the condition, which depends on the helper's parameter being named obj in src/evo/core_write.cpp. Renaming the parameter (e.g. to state or tx) would break this assertion for reasons unrelated to the bug being guarded. Matching on "Internal bug detected" alone would still confirm the non-fatal path was taken without binding to internal naming.
source: ['claude']
Fix deprecated service netInfo access
Issue being fixed or feature implemented
Malformed provider-register and provider-service-update payloads can be
decoded by raw transaction RPCs before special transaction validation has run.
When
-deprecatedrpc=serviceis enabled, the deprecatedserviceJSON fielddereferenced
netInfodirectly, which could terminate the process if thedecoded provider payload left
netInfounset.What was done?
serviceJSON rendering through a small helper that appliesCHECK_NONFATALbefore readingnetInfo.provider-service-update payloads, and simplified masternode list entries.
transaction decoding under
-deprecatedrpc=service.How Has This Been Tested?
Tested on macOS 15.6 arm64 with depends-based local build:
git diff --checkpython3 -m py_compile test/functional/rpc_deprecated.pymake -j8 src/dashdtest/functional/rpc_deprecated.py --cachedir=/Users/claw/.openclaw/workspace/cache/dash-functionalThe focused functional test verifies both malformed provider payload types
return a nonfatal RPC error and the node remains responsive.
Breaking Changes
None.
Checklist
(for repository code-owners and collaborators only)