Failsafes to prevent a consensus round from taking too long

[ximinez]

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 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.

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.

[codecov]

Codecov Report

Attention: Patch coverage is 88.99083% with 12 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (2406b28) to head (88cf631).

Files with missing lines	Patch %	Lines
src/xrpld/consensus/Consensus.h	81.1%	7 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp	66.7%	3 Missing ⚠️
src/xrpld/consensus/Consensus.cpp	92.3%	1 Missing ⚠️
src/xrpld/consensus/DisputedTx.h	96.8%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5277   +/-   ##
=======================================
  Coverage     78.1%   78.1%           
=======================================
  Files          790     790           
  Lines        67908   67984   +76     
  Branches      8227    8231    +4     
=======================================
+ Hits         53028   53106   +78     
+ Misses       14880   14878    -2     

Files with missing lines	Coverage Δ
src/xrpld/consensus/ConsensusParms.h	100.0% <100.0%> (ø)
src/xrpld/consensus/ConsensusTypes.h	79.5% <ø> (ø)
src/xrpld/consensus/Consensus.cpp	75.2% <92.3%> (+1.3%)	⬆️
src/xrpld/consensus/DisputedTx.h	99.0% <96.8%> (+3.0%)	⬆️
src/xrpld/app/misc/NetworkOPs.cpp	69.1% <66.7%> (-<0.1%)	⬇️
src/xrpld/consensus/Consensus.h	88.3% <81.1%> (+1.5%)	⬆️

... and 4 files with indirect coverage changes

🚀 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.

[Bronek]

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.

[vlntb]

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.

[ximinez]

@Bronek @vlntb Other than testDisjointNetwork, or any changes in the logic / rules, I think this is as done as it's going to get. Could you please re-review?

CC: @JoelKatz

[Bronek]

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

[Bronek]

Suggested change

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

[Bronek]

Suggested change

	std::size_t const consensusTime;
	int const consensusTime;

[Bronek]

Make this const please

[Bronek]

I know this is also old behaviour, but would it be useful to show here tx_.id(), like we do below when our vote is changed ?

[Bronek]

I would prefer if stalled was logged at a higher severity level, perhaps j.warn()

[Bronek]

Is there any value in BEAST_EXPECT inside the commented out section ?

[Bronek]

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).

[Bronek]

This is interesting. We only get stalled when every disputed transaction is unequivocally either under 20% or over 80% consensus (repectively "nay" or "yay") so there is no way to manipulate transactions which do not cut the threshold into the ledger. Would be good to put explanation to such effect in a comment, though.