fix: remove VM_ERROR auto-disagree in consensus voting#1513
Conversation
VM_ERROR was unconditionally triggering DISAGREE votes before reaching the deterministic comparison logic. Since GenVM reports Python exceptions (ValueError, etc.) as VM_ERROR with "exit_code 1", this caused fully deterministic error contracts to go UNDETERMINED — validators disagreed despite having identical results. All non-timeout VM_ERROR types (exit_code, OOM, wasm_trap, invalid_contract) are deterministic given the same inputs. Timeout is already handled separately. Removing the shortcut lets VM_ERROR results flow through to the normal comparison of execution_result, contract_state, and pending_transactions.
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR refactors VM error handling in the voting logic by removing the dedicated VM_ERROR-specific DISAGREE path. VM errors now flow through deterministic violation checks, resulting in DETERMINISTIC_VIOLATION when validator and leader states differ, or AGREE when states align. Tests are updated to reflect these revised voting semantics. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/node/base.py (1)
717-729:⚠️ Potential issue | 🔴 CriticalCritical:
Vote.DETERMINISTIC_VIOLATIONvotes are silently ignored by the consensus algorithm.The
determine_consensus_from_votes()function inbackend/consensus/utils.py(lines 5-35) only countsAGREE,DISAGREE,TIMEOUT, andIDLEvotes:agree_count = votes_list.count(Vote.AGREE.value) disagree_count = votes_list.count(Vote.DISAGREE.value) timeout_count = votes_list.count(Vote.TIMEOUT.value) idle_count = votes_list.count(Vote.IDLE.value)When validators emit
Vote.DETERMINISTIC_VIOLATION(atbackend/node/base.py:724for state mismatches), these votes are completely dropped from consensus calculation. This causes:
- All-DETERMINISTIC_VIOLATION scenarios (e.g., leader succeeds but all validators detect state divergence) → incorrectly return
NO_MAJORITYinstead of strong disagreement- Mixed scenarios (some validators AGREE, others DETERMINISTIC_VIOLATION) → validators detecting violations have no voice in consensus
The vote reaches the consensus function (
backend/consensus/base.py:3102-3103) but is not counted. Updatedetermine_consensus_from_votes()to treatDETERMINISTIC_VIOLATIONas effective disagree, or reconsider whetherDISAGREEshould be used instead of the new vote type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/node/base.py` around lines 717 - 729, determine_consensus_from_votes currently ignores Vote.DETERMINISTIC_VIOLATION, so validators that set Vote.DETERMINISTIC_VIOLATION (set in backend/node/base.py when leader_receipt != receipt) are dropped; update determine_consensus_from_votes (in backend/consensus/utils.py) to treat Vote.DETERMINISTIC_VIOLATION as a disagreeing vote by counting Vote.DETERMINISTIC_VIOLATION.value toward disagree_count (or explicitly increment a disagree_count when encountering Vote.DETERMINISTIC_VIOLATION), and add/update a short comment and unit tests to assert that DETERMINISTIC_VIOLATION produces a disagree/majority-rejection outcome.tests/unit/test_set_vote.py (1)
221-270:⚠️ Potential issue | 🔴 CriticalCritical:
DETERMINISTIC_VIOLATIONvotes are silently dropped in consensus calculation.The unit tests correctly verify that
_set_vote()generatesDETERMINISTIC_VIOLATIONvotes when there are mismatches. However, there is a critical bug:determine_consensus_from_votes()inbackend/consensus/utils.pyonly countsAGREE,DISAGREE,TIMEOUT, andIDLEvotes.DETERMINISTIC_VIOLATIONvotes are not counted at all and are silently dropped.This means validators can emit
DETERMINISTIC_VIOLATIONvotes, but they have no effect on the consensus outcome. Either:
- Add
DETERMINISTIC_VIOLATIONcounting logic todetermine_consensus_from_votes()(treating it asDISAGREEor as a separate majority category), or- Change
_set_vote()to emitDISAGREEinstead ofDETERMINISTIC_VIOLATION, or- Document that
DETERMINISTIC_VIOLATIONis for logging only and should not affect consensusAdd a test in
test_consensus_voting.pythat verifies the chosen behavior when validators emitDETERMINISTIC_VIOLATIONvotes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_set_vote.py` around lines 221 - 270, The consensus function determine_consensus_from_votes() ignores Vote.DETERMINISTIC_VIOLATION so those votes are dropped; fix by updating determine_consensus_from_votes() to include Vote.DETERMINISTIC_VIOLATION in the vote counting (treat it as a disagree-equivalent for majority calculations), ensure any branching/majority logic uses the new count, and add a unit test in test_consensus_voting.py that constructs a vote set including Vote.DETERMINISTIC_VIOLATION and asserts the expected consensus outcome; refer to determine_consensus_from_votes(), Vote.DETERMINISTIC_VIOLATION, and _set_vote() to locate the relevant code and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/node/base.py`:
- Around line 717-729: determine_consensus_from_votes currently ignores
Vote.DETERMINISTIC_VIOLATION, so validators that set
Vote.DETERMINISTIC_VIOLATION (set in backend/node/base.py when leader_receipt !=
receipt) are dropped; update determine_consensus_from_votes (in
backend/consensus/utils.py) to treat Vote.DETERMINISTIC_VIOLATION as a
disagreeing vote by counting Vote.DETERMINISTIC_VIOLATION.value toward
disagree_count (or explicitly increment a disagree_count when encountering
Vote.DETERMINISTIC_VIOLATION), and add/update a short comment and unit tests to
assert that DETERMINISTIC_VIOLATION produces a disagree/majority-rejection
outcome.
In `@tests/unit/test_set_vote.py`:
- Around line 221-270: The consensus function determine_consensus_from_votes()
ignores Vote.DETERMINISTIC_VIOLATION so those votes are dropped; fix by updating
determine_consensus_from_votes() to include Vote.DETERMINISTIC_VIOLATION in the
vote counting (treat it as a disagree-equivalent for majority calculations),
ensure any branching/majority logic uses the new count, and add a unit test in
test_consensus_voting.py that constructs a vote set including
Vote.DETERMINISTIC_VIOLATION and asserts the expected consensus outcome; refer
to determine_consensus_from_votes(), Vote.DETERMINISTIC_VIOLATION, and
_set_vote() to locate the relevant code and tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2909c90-ec55-4df4-984f-1f0b48ed8411
📒 Files selected for processing (2)
backend/node/base.pytests/unit/test_set_vote.py
node_factory_supplier reads len(created_nodes) to determine votes while multiple validators are created concurrently, causing the vote threshold to be missed intermittently. Adding a lock serializes node creation so the count is always accurate when get_vote() is called.
The test_leader_appeal final assertion timed out at 60s when run under pytest-xdist (-n auto). The consensus loop runs slower in xdist subprocess workers, so the appeal rounds + finality window exceed 60s. Increase the final FINALIZED wait timeout to 180s and revert the unnecessary threading lock (node creation is synchronous within the single-threaded async event loop, so no race condition exists). Root cause: pytest 8→9 upgrade in 925f041 introduced the incompatibility.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/node/base.py (1)
720-732:⚠️ Potential issue | 🔴 CriticalCRITICAL:
DETERMINISTIC_VIOLATIONvotes are silently ignored in consensus determination.The current implementation of
determine_consensus_from_votes()(backend/consensus/utils.py:20-23) only counts AGREE, DISAGREE, TIMEOUT, and IDLE votes. When a validator emitsVote.DETERMINISTIC_VIOLATION, this vote is not tallied or processed—it simply vanishes from the consensus calculation. This means validators correctly detecting execution mismatches are silently dropped from the consensus outcome, undermining the entire consensus mechanism.The vote must be explicitly counted in the consensus logic, either as a distinct outcome or mapped to one of the existing categories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/node/base.py` around lines 720 - 732, determine_consensus_from_votes currently ignores Vote.DETERMINISTIC_VIOLATION so validators flagging execution/state mismatches are dropped; update determine_consensus_from_votes to explicitly handle Vote.DETERMINISTIC_VIOLATION (either add a separate counter for it or map it to an existing category like DISAGREE), increment that counter when seen, and then include that counter in the consensus decision logic so DETERMINISTIC_VIOLATION votes affect the outcome; reference the determine_consensus_from_votes function and the Vote.DETERMINISTIC_VIOLATION enum value when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/node/base.py`:
- Around line 720-732: determine_consensus_from_votes currently ignores
Vote.DETERMINISTIC_VIOLATION so validators flagging execution/state mismatches
are dropped; update determine_consensus_from_votes to explicitly handle
Vote.DETERMINISTIC_VIOLATION (either add a separate counter for it or map it to
an existing category like DISAGREE), increment that counter when seen, and then
include that counter in the consensus decision logic so DETERMINISTIC_VIOLATION
votes affect the outcome; reference the determine_consensus_from_votes function
and the Vote.DETERMINISTIC_VIOLATION enum value when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32fa2232-4e68-4d29-a8ed-43e2e8fbbbaa
📒 Files selected for processing (2)
backend/node/base.pytests/unit/consensus/test_base.py
|
🎉 This PR is included in version 0.110.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Summary
ValueError) were going UNDETERMINED because validators auto-voted DISAGREEVM_ERRORwithexit_code 1, and the old_set_vote()logic short-circuited to DISAGREE for anyVM_ERRORbefore reaching the comparison logicexit_code,OOM,wasm_trap,invalid_contract) are deterministic given the same inputs — timeout is already handled separatelyexecution_result,contract_state, andpending_transactionsReproduction
ValueErrorVM_ERROR("exit_code 1")Test plan
Summary by CodeRabbit
Release Notes