Skip to content

Conversation

@kstroobants
Copy link
Contributor

@kstroobants kstroobants commented Sep 19, 2025

Fixes #DXP-646

What

Updated the transactions_processor.py to handle consensus results more robustly by adding checks for empty votes which means leader timeout.

Why

To fix a bug where transactions with no votes was marked as no_majority instead of leader_timeout.

Testing done

Decisions made

Checks

  • I have tested this code
  • I have reviewed my own PR
  • I have created an issue for this PR
  • I have set a descriptive PR title compliant with conventional commits

Reviewing tips

Focus on the changes.

User facing release notes

Improved handling of consensus results in transaction processing.

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of consensus outcomes when votes are missing or empty, reducing misclassified results.
    • Distinguishes “Timeout” (no votes) from “No Majority” (consensus data lacking votes) more reliably.
    • Normalizes vote values before determining outcomes to avoid case-related mismatches.
    • Ensures consistent result values and readable result names shown in transaction details.

@kstroobants kstroobants self-assigned this Sep 19, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Updates consensus handling in transactions_processor.py: imports ConsensusResult, removes unused import, and changes _process_result to normalize votes, treat empty votes as TIMEOUT, missing votes as NO_MAJORITY, or derive consensus via determine_consensus_from_votes; stores result as enum int and name.

Changes

Cohort / File(s) Summary
Consensus handling in transaction processing
backend/database_handler/transactions_processor.py
Added ConsensusResult import; removed unused os. In _process_result: extract votes from consensus_data["votes"].values(), normalize votes to lowercase strings, set TIMEOUT if votes list is empty, NO_MAJORITY if consensus_data has no votes, otherwise call determine_consensus_from_votes. Persist transaction_data["result"] = int(consensus_result) and transaction_data["result_name"] = consensus_result.value.

Sequence Diagram(s)

sequenceDiagram
  participant TP as TransactionsProcessor
  participant CM as ConsensusModule
  participant DB as Database

  Note over TP: _process_result(consensus_data)

  alt consensus_data contains "votes"
    TP->>TP: votes_values = list(consensus_data["votes"].values())
    TP->>TP: normalized_votes = [str(v).lower() for v in votes_values]
    alt votes_values is empty
      TP->>TP: consensus_result = ConsensusResult.TIMEOUT
    else votes present
      TP->>CM: determine_consensus_from_votes(normalized_votes)
      CM-->>TP: consensus_result
    end
  else consensus_data present without "votes"
    TP->>TP: consensus_result = ConsensusResult.NO_MAJORITY
  end

  TP->>DB: Save result (result = int(consensus_result), result_name = consensus_result.value)
  DB-->>TP: Ack
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

run-tests

Suggested reviewers

  • cristiam86

Poem

I twitch my whiskers, count each vote with care,
If none reply—TIMEOUT hangs in the air.
No "votes" at all? NO_MAJORITY I say,
Else lowercase whispers decide the day.
I hop to save the result and then I play. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: Incorrect error reporting (leader timeout)" is concise, follows conventional commit style, and accurately summarizes the primary change which corrects how leader timeouts are reported in transaction processing. It clearly signals the bugfix intent and is specific enough for reviewers scanning PR history to understand the main purpose.
Description Check ✅ Passed The PR description follows the repository template and includes an explicit "Fixes #DXP-646" reference along with clear "What" and "Why" sections explaining the change to consensus handling, which communicates intent and links the issue. The "Testing done" and "Decisions made" sections are present but lack concrete test steps, results, and rationale, which reduces reviewer confidence in verification. Because the core sections and issue linkage are present and the change intent is clearly described, this check passes while noting the missing test details.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dxp-646-incorrect-error-reporting-leader-timeout

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16d76c6 and baa292a.

📒 Files selected for processing (1)
  • backend/database_handler/transactions_processor.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{backend,examples,tests}/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Python code must be formatted with Black and include type hints

Files:

  • backend/database_handler/transactions_processor.py
🧬 Code graph analysis (1)
backend/database_handler/transactions_processor.py (2)
backend/consensus/types.py (1)
  • ConsensusResult (4-32)
backend/consensus/utils.py (1)
  • determine_consensus_from_votes (5-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Load Tests / load-test
  • GitHub Check: test
  • GitHub Check: backend-unit-tests
  • GitHub Check: load-test

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.

coderabbitai[bot]

This comment was marked as resolved.

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants