Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failsafes to prevent a consensus round from taking too long #5277

Merged
merged 37 commits into from
Mar 20, 2025

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Feb 5, 2025

High Level Overview of Change

This PR, if merged, will introduce two fail safes into the consensus logic to prevent a consensus round from remaining in the establish phase indefinitely.

  1. Detects if the consensus process is "stalled". If it is, then we can declare a consensus and end successfully even if we do not have 80% agreement on our proposal. Details below in "Before / After".
  2. If we have been in the establish phase for more than 10x the previous consensus establish phase's time, then consensus is considered "expired", and we will leave the round, which sends a partial validation (indicating that the node is moving on without validating). There are restrictions intended to avoid prematurely exiting, or having an extended exit in extreme situations. Details below in "Before / After".
    1. When enough nodes leave the round, any remaining nodes will see they've fallen behind, and move on, too, generally before hitting the timeout. Any validations or partial validations sent during this time will help the consensus process bring the nodes back together.

Context of Change

At about 9:54pm UTC on 2/4/2025, the network successfully validated ledger 93927173, and started the consensus round for 93927174. That round did not end for over an hour.

The current evidence indicates that two things happened.

  1. Some disputed transactions had just enough "yes" votes that validators voting "yes" saw the approval as just over 95%, while those voting "no" saw the approval as just under 95%. Thus, every node thought that it was doing the right thing, and no nodes changed their vote. While this is annoying, normally consensus will move on because at least 80% of the UNL validators will be in agreement over which transaction set to use, and so consensus moves on with that set. However,
  2. The disputed transactions with the close approval rates were distributed such that there were several clumps of validators voting yes for different transactions than other clumps of validators. This led to a situation where no transaction set had 80% approval.

This led to a livelock situation where every node was waiting for some other node to make a change, while none of the nodes were willing to change.

This decision algorithm has been in place for at least 8 years, and possibly since the first release of rippled. The odds of it happening were thought to be 0, but it turns out they're just very very small.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

This change is fully backward and forward compatible, and does not require an amendment.

Before / After

This is an outline of the changes in this PR.

  1. In NetworkOPsImp::processTrustedProposal, if the proposal is from us, it don't process it. This should be impossible, but this will help confirm if not.
  2. Detects if the consensus process is "stalled". If it is, then we can declare a consensus and end successfully even if we do not have 80% agreement on our proposal. (checkConsensusReached's other restrictions, such as minimum proposers, still apply)
    1. "Stalled" is distinct from "stuck" used as a consensus percentage state. Naming things is hard.
    2. "Stalled" is defined as:
      1. We have a close time consensus
      2. Each disputed transaction is individually stalled:
        1. It has been in the final "stuck" 95% requirement for at least 2 (avMIN_ROUNDS) "inner rounds" of phaseEstablish,
        2. and either all of the other trusted proposers or this validator, if proposing, have had the same vote(s) vote for at least 4 (avSTALLED_ROUNDS) "inner rounds",
        3. and at least 80% of the validators (including this one, if appropriate) agree about the vote (whether yes or no).
  3. If we have been in the establish phase for more than 10x the previous consensus round time, then consensus is considered "expired", and we will leave the round, which sends a partial validation. There are two restrictions intended to avoid prematurely exiting, or having an extended exit in extreme situations.
    1. The 10x time is clamped to be within a range of 15s (ledgerMAX_CONSENSUS) to 120s (ledgerABANDON_CONSENSUS).
    2. If consensus has not had an opportunity to walk through all percentage avalanche states (defined as not going through 8 "inner rounds" of phaseEstablish), then ConsensusState::Expired is treated as ConsensusState::No.
  4. The close time avalanching defined in ConsensusParms.h has been rewritten as more of an explicit state machine. It's basically a map of states to their time cutoff, percentage cutoff, and next state, and a getNeededWeight function that will evaluate whether to move to the next state.
    1. This function is used for both disputed transactions and close time consensus.
    2. In addition to the the "previous round time percentage" limits, disputed transactions will also be required to spend at least 2 (avMIN_ROUNDS) "inner rounds" in each state. Close time consensus does not have this restriction (but it could).
    3. This map is more easily modifiable than the previous individual variables, so if we decide to change parameters, it only requires changing the map.
  5. Adds tests of the functionality that detects the "stalled" state.
  6. Finally, it adds a simulation unit test that attempts to recreate the scenario that got us here in the first place. However, I'm not very familiar with the simulator, and we're still not 100% sure how we got into this state in the first place, so it's not very good.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 82.72727% with 19 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (75a2019) to head (76ef214).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/consensus/Consensus.h 58.3% 15 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp 66.7% 3 Missing ⚠️
src/xrpld/consensus/DisputedTx.h 97.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5277   +/-   ##
=======================================
  Coverage     78.1%   78.1%           
=======================================
  Files          790     790           
  Lines        67911   67988   +77     
  Branches      8234    8252   +18     
=======================================
+ Hits         53025   53093   +68     
- Misses       14886   14895    +9     
Files with missing lines Coverage Δ
src/xrpld/consensus/Consensus.cpp 76.6% <100.0%> (+2.7%) ⬆️
src/xrpld/consensus/ConsensusParms.h 100.0% <100.0%> (ø)
src/xrpld/consensus/ConsensusTypes.h 79.5% <ø> (ø)
src/xrpld/consensus/DisputedTx.h 99.0% <97.0%> (+3.0%) ⬆️
src/xrpld/app/misc/NetworkOPs.cpp 69.1% <66.7%> (-<0.1%) ⬇️
src/xrpld/consensus/Consensus.h 85.0% <58.3%> (-1.9%) ⬇️

... and 3 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ximinez ximinez changed the title Drop out of consensus if the round takes too long Failsafes to prevent a consensus round from taking too long Feb 5, 2025
@ximinez ximinez requested review from Bronek, JoelKatz and vlntb February 5, 2025 19:01
@ximinez ximinez marked this pull request as ready for review February 5, 2025 23:16
@ximinez ximinez requested a review from Bronek February 6, 2025 00:02
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Would be cool to have a unit test for the last part of DisputedTx.h ; not sure how realistic that request is. Approved in any case.

@ximinez ximinez force-pushed the ximinez/consensus branch 4 times, most recently from 6e513d9 to 26ab221 Compare February 11, 2025 01:15
@bthomee bthomee added this to the 2.4.0 (Q1 2025) milestone Feb 11, 2025
@ximinez ximinez force-pushed the ximinez/consensus branch 2 times, most recently from 8dcbf91 to a6d3cea Compare February 12, 2025 04:12
- Stable state means that neither we, nor any of our peers has changed
  a vote on a disputed transaction in a while. This is undesirable if an
  80% consensus has not otherwise been reached. It will cause
  a validation to be sent, which will help get other (trusting)
  validators back on track using preferred ledger logic.
Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

The current version fails to build on MacOS because Mac's version of libstdc++ is dropping the assignment operator for std::map pairs. There are two viable fixes:

  1. Add an assignment operator to ConsensusParms:
    We could add a custom assignment operator for ConsensusParms that, instead of assigning the avalancheCutoffs map directly (which triggers the error), manually copies its contents into the target map.

  2. Make avalancheCutoffs const and remove the assignment:
    By declaring the map as const, you avoid any assignment after its construction. I would prefer this option, but it means that we must update the unit tests that does

peer->consensusParms = parms;

so that it no longer tries to perform such an assignment.

* upstream/develop:
  chore: Rename missing-commits job, and combine nix job files (5268)
* upstream/develop:
  fix: Replace charge() by fee_.update() in OnMessage functions (5269)
  docs: ensure build_type and CMAKE_BUILD_TYPE match (5274)
  chore: Fix small typos in protocol files (5279)
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Two suggestions for now; I admire the unit test coverage of this but am not yet finished.

// See if enough time has passed to move on to the next.
XRPL_ASSERT(
nextCutoff.consensusTime >= currentCutoff.consensusTime,
"ripple::DisputedTx::updateVote next state valid");
Copy link
Collaborator

@Bronek Bronek Mar 3, 2025

Choose a reason for hiding this comment

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

Suggested change
"ripple::DisputedTx::updateVote next state valid");
"ripple::getNeededWeight : next state valid");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Probably a cut/paste error.

@Bronek Bronek self-requested a review March 4, 2025 15:36
@bthomee bthomee requested a review from vlntb March 4, 2025 21:20
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

This is good change. I like the unis tests coverage (but an open question re. commented out section), I like how the new Expired state only kicks in when we are not stalled on all transactions (nice decoupling on both states).

ximinez added 6 commits March 11, 2025 11:33
* upstream/develop:
  Set version to 2.4.0
  Set version to 2.4.0-rc4
  chore: Update XRPL Foundation Validator List URL (5326)
* upstream/develop:
  refactor: Remove unused and add missing includes (5293)
- Document "stalled" in checkConsensusReached. Also return early.
- Log stalled consensus at higher level than regular.
- const correctness (stalled in haveConsensus)
- Change type of AvalancheCutoff::consensusTime.
- Fix XRPL_ASSERT label
- Log tx ID on unchanged dispute vote
// See if enough time has passed to move on to the next.
XRPL_ASSERT(
nextCutoff.consensusTime >= currentCutoff.consensusTime,
"ripple::getNeededWeight next state valid");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put colon : between scope name and description

Suggested change
"ripple::getNeededWeight next state valid");
"ripple::getNeededWeight : next state valid");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please put colon : between scope name and description

Fixed

@vlntb
Copy link
Collaborator

vlntb commented Mar 13, 2025

This led to a deadlock-like situation where every node was waiting for some other node to make a change, while none of the nodes were willing to change.

Would the better name for the state be a livelock rather than a deadlock?
From wiki : "A livelock is similar to a deadlock, except that the states of the processes involved in the livelock constantly change with regard to one another, none progressing."

Copy link
Collaborator

@vlntb vlntb left a comment

Choose a reason for hiding this comment

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

LGTM. One minor suggestion for PR description:

This led to a deadlock-like situation where every node was waiting for some other node to make a change, while none of the nodes were willing to change.

Would the better name for the state be a livelock rather than a deadlock?
From wiki : "A livelock is similar to a deadlock, except that the states of the processes involved in the livelock constantly change with regard to one another, none progressing."

@ximinez ximinez added Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. and removed Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. labels Mar 17, 2025
@ximinez ximinez added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 17, 2025
@ximinez
Copy link
Collaborator Author

ximinez commented Mar 17, 2025

Proposed commit message:

Prevent consensus from getting stuck in the establish phase (#5277)

- Detects if the consensus process is "stalled". If it is, then we can declare a 
  consensus and end successfully even if we do not have 80% agreement on
  our proposal.
  - "Stalled" is defined as:
    - We have a close time consensus
    - Each disputed transaction is individually stalled:
      - It has been in the final "stuck" 95% requirement for at least 2
        (avMIN_ROUNDS) "inner rounds" of phaseEstablish,
      - and either all of the other trusted proposers or this validator, if proposing,
        have had the same vote(s) for at least 4 (avSTALLED_ROUNDS) "inner
        rounds", and at least 80% of the validators (including this one, if
        appropriate) agree about the vote (whether yes or no).
- If we have been in the establish phase for more than 10x the previous
  consensus establish phase's time, then consensus is considered "expired",
  and we will leave the round, which sends a partial validation (indicating
  that the node is moving on without validating). Two restrictions avoid
  prematurely exiting, or having an extended exit in extreme situations.
  - The 10x time is clamped to be within a range of 15s
    (ledgerMAX_CONSENSUS) to 120s (ledgerABANDON_CONSENSUS).
  - If consensus has not had an opportunity to walk through all avalanche
    states (defined as not going through 8 "inner rounds" of phaseEstablish),
    then ConsensusState::Expired is treated as ConsensusState::No.
- When enough nodes leave the round, any remaining nodes will see they've
  fallen behind, and move on, too, generally before hitting the timeout. Any
  validations or partial validations sent during this time will help the
  consensus process bring the nodes back together.

@ximinez ximinez merged commit d22a505 into develop Mar 20, 2025
24 checks passed
@ximinez ximinez deleted the ximinez/consensus branch March 20, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants