Fix unguarded worker_index read and harden POS commit-reveal - #216
Conversation
-master_node_voting: the signature-failure log read quorum.workers[vote.state_change.worker_index] for all vote types, but worker_index only exists on obligation votes -- for a checkpoint vote it aliases attacker-controlled bytes and indexes an empty vector. Guard it by vote type. Also re-enable the two success logs at MTRACE (level 3) instead of leaving them commented out. -pos: only accept a random value reveal from a quorum position that actually committed a hash, so the commit-reveal binding is enforced locally rather than relying on the stage timeout. -pos: drop the dead std::sample in signature selection and fix the comment -- the loop always used the first N.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes update vote-signature logging, require committed hashes for random values, select the lowest participating validator positions for final signatures, and prevent invalid wallet output indexing in an error message. ChangesVoting and PoS validation
Wallet output error safety
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The PR makes localized validation, logging, and cleanup changes; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 too large.) ✨ 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 |
When the requested amount is missing from the light wallet server's response, the search loop in light_wallet_get_outs exits with amount_key == amount_outs.size(). The error message then indexes amount_outs at that position, reading one past the end of the vector. Report `amount` instead: it is the value being searched for, and on the success path it equals amount_outs[amount_key].amount by definition.
2 participants
master_node_voting: the signature-failure log read quorum.workers[vote.state_change.worker_index] for all vote types, but worker_index only exists on obligation votes -- for a checkpoint vote it aliases attacker-controlled bytes and indexes an empty vector. Guard it by vote type. Also re-enable the two success logs at MTRACE (level 3) instead of leaving them commented out.
pos: only accept a random value reveal from a quorum position that actually committed a hash, so the commit-reveal binding is enforced locally rather than relying on the stage timeout.
pos: drop the dead std::sample in signature selection and fix the comment -- the loop always used the first N.