Skip to content

refactor: remove eq_outputs from validator receipts#1529

Open
cristiam86 wants to merge 3 commits intomainfrom
dxp-36-genvm-remove-eq_outputs-from-validators-receipt
Open

refactor: remove eq_outputs from validator receipts#1529
cristiam86 wants to merge 3 commits intomainfrom
dxp-36-genvm-remove-eq_outputs-from-validators-receipt

Conversation

@cristiam86
Copy link
Member

@cristiam86 cristiam86 commented Mar 12, 2026

What

Made Receipt.eq_outputs optional since validators never generate it. Changes:

  • Made eq_outputs optional (dict[int, str] | None = None) in Receipt
  • Only populate eq_outputs for leader receipts; validators get None
  • Omit eq_outputs from JSON serialization for validator receipts
  • Removed eq_outputs={} from all validator receipt constructors in tests

Why

Validators always produced empty eq_outputs dicts with no actual data. Making it optional reduces unnecessary storage and clearer semantics in the data model.

Testing done

  • Backend unit tests: 579 passed
  • DB/SQLAlchemy tests: 60 passed
  • Frontend unit tests: 112 passed (1 pre-existing unrelated failure)

Decisions made

  • Leader receipts still have eq_outputs (always set to dict, even if empty)
  • Validator receipts have eq_outputs = None (omitted from JSON)
  • Handle None when decoding leader receipt for validator execution

Fixes DXP-36

Summary by CodeRabbit

  • Bug Fixes

    • Improved error and timeout handling so validator-generated result objects omit non-essential output data for non-leader roles and treat missing leader outputs as absent results, reducing spurious downstream errors.
  • Tests

    • Updated test fixtures and helpers to reflect the conditional presence of leader-only output data and the adjusted handling of missing results.

Validators never generate eq_outputs, so they always produced empty dicts.
This change:

- Makes Receipt.eq_outputs optional (dict[int, str] | None = None)
- Only populates eq_outputs for leader receipts
- Omits eq_outputs from serialization for validator receipts
- Removes eq_outputs={} from all validator receipt constructors
- Handles None eq_outputs when decoding leader receipt for validator execution

All 579 backend unit tests, 60 DB tests, and 112 frontend tests pass.

Fixes DXP-36
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28c18a03-2d0b-41e9-aaaa-7ce5df9585ab

📥 Commits

Reviewing files that changed from the base of the PR and between 13b744c and 5ef5938.

📒 Files selected for processing (1)
  • backend/node/types.py

📝 Walkthrough

Walkthrough

The PR makes eq_outputs optional on Receipt (type now allows None) and changes code paths to include eq_outputs only for LEADER executions. Helper builders and tests were updated to stop supplying eq_outputs for non-leader receipts.

Changes

Cohort / File(s) Summary
Receipt Type Definition
backend/node/types.py
Changed Receipt.eq_outputs to `dict[int, str]
Runtime/Leader Logic
backend/node/base.py
Treat absence/falsy eq_outputs as no leader result; when constructing Receipt, include encoded eq_outputs only for ExecutionMode.LEADER, otherwise set eq_outputs=None.
Consensus Error/Timeout Builders
backend/consensus/base.py
Removed eq_outputs from timeout and internal-error Receipt constructions (no longer supplied).
Tests and Helpers
tests/unit/consensus/test_helpers.py, tests/unit/consensus/test_validator_exec_timeout.py, tests/unit/test_leader_llm_recovery.py, tests/unit/test_set_vote.py
Updated test fixtures and helpers to omit eq_outputs for non-leader cases and to supply {} for leader success receipts where appropriate; adjusted argument ordering in a couple of fixtures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • MuncleUscles

Poem

🐇 I hopped through code with eager paws,
A field set optional without a pause,
Leaders keep the output in their cheer,
Others rest with None — calm and clear,
Hooray for tidy receipts and fewer flaws! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: making eq_outputs optional and removing it from validator receipts, which is the primary focus of the changeset.
Description check ✅ Passed The description covers all key sections (What, Why, Testing done, Decisions made) with sufficient detail and follows the repository template structure effectively.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dxp-36-genvm-remove-eq_outputs-from-validators-receipt
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/node/types.py`:
- Around line 257-261: The deserialization uses a truthiness check that treats
an empty dict as falsy, causing eq_outputs to become None; update the condition
around raw_eq in the eq_outputs assignment (the comprehension that currently
uses "if (raw_eq := input.get('eq_outputs'))") to explicitly check for None
(e.g., "if (raw_eq := input.get('eq_outputs')) is not None") so empty dicts
deserialize back to {} while still mapping keys via the existing comprehension
that converts keys to int.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7e051cb8-5e10-42a3-9cbd-0a70eaa2bb92

📥 Commits

Reviewing files that changed from the base of the PR and between 6dad06d and 13b744c.

📒 Files selected for processing (7)
  • backend/consensus/base.py
  • backend/node/base.py
  • backend/node/types.py
  • tests/unit/consensus/test_helpers.py
  • tests/unit/consensus/test_validator_exec_timeout.py
  • tests/unit/test_leader_llm_recovery.py
  • tests/unit/test_set_vote.py
💤 Files with no reviewable changes (2)
  • backend/consensus/base.py
  • tests/unit/consensus/test_validator_exec_timeout.py

Empty dict {} is falsy in Python, so the truthiness check would
incorrectly convert a leader's empty eq_outputs to None on
deserialization. Use `is not None` to preserve the round-trip.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant