Expose consensus parameters for simulation (RIPD-1355) #2094
Conversation
| @@ -206,12 +206,12 @@ class ValidationsImp : public Validations | |||
|
|
|||
| auto const now = app_.timeKeeper().closeTime(); | |||
| auto const signTime = val->getSignTime(); | |||
|
|
|||
| auto const p = ConsensusParms{}; | |||
There was a problem hiding this comment.
The changes in this file will be replaced by #2084
There was a problem hiding this comment.
I'm not sure how p will look in the replacement, but here it could be made constexpr instead of const, assuming VS will swallow that. Doing so will ensure these constants are compile-time initialized, and will therefore be a little more efficient at run time.
There was a problem hiding this comment.
It won't be compile time constant there. Instead Validations has its own parameters that it takes in its constructor which will allow simulations to vary those values too.
bachase
requested review from
JoelKatz,
miguelportilla and
HowardHinnant
and removed request for
JoelKatz
src/ripple/consensus/Consensus.h
Outdated
| @param j The journal to log debug output | ||
| */ | ||
| Consensus(clock_type const& clock, beast::Journal j); | ||
| Consensus(clock_type const& clock, ConsensusParms p, beast::Journal j); |
There was a problem hiding this comment.
This will be more efficient if you pass by ConsensusParms const& p.
| // Vary the time it takes to process validations to exercise detecting | ||
| // the wrong LCL at different phases of consensus | ||
| for (auto validationDelay : {0s, LEDGER_MIN_CLOSE}) | ||
| for (auto validationDelay : {milliseconds{0}, parms.ledgerMIN_CLOSE}) |
src/test/csf/Peer.h
Outdated
| @@ -202,7 +202,7 @@ struct Peer : public Consensus<Peer, Traits> | |||
| [ this, ledgerHash, ledger = it->second ]() { | |||
| ledgers.emplace(ledgerHash, ledger); | |||
| }); | |||
| if (missingLedgerDelay == 0ms) | |||
| if (missingLedgerDelay == std::chrono::milliseconds{0}) | |||
There was a problem hiding this comment.
Why not prefer the 0ms syntax?
There was a problem hiding this comment.
No good reason and I can't recall why I switched it. I'll switch back to 0ms.
Codecov Report
@@ Coverage Diff @@
## develop #2094 +/- ##
===========================================
- Coverage 68.98% 68.97% -0.01%
===========================================
Files 684 684
Lines 50394 50400 +6
===========================================
+ Hits 34764 34765 +1
- Misses 15630 15635 +5
Continue to review full report at Codecov.
|
|
Latest update tests good for me. 👍 |
|
@miguelportilla looks to have quite a few review requests on his plate, so thought @mellery451 could be the next |
src/ripple/consensus/Consensus.cpp
Outdated
| @@ -98,7 +102,7 @@ checkConsensusReached(std::size_t agreeing, std::size_t total, bool count_self) | |||
|
|
|||
| int currentPercentage = (agreeing * 100) / total; | |||
There was a problem hiding this comment.
minor: I wonder if currentPercentage should be declared auto here so the types match.
There was a problem hiding this comment.
Good idea. This was actually a torn refactor--parms.minCONSENSUS_PCT is an int, but I'm using std::size_t everywhere else. I'll fix.
| std::size_t agreeing, | ||
| std::size_t total, | ||
| bool count_self, | ||
| std::size_t minConsensusPct) |
There was a problem hiding this comment.
just so I understand - what's the motivation for passing minConsensusPct directly and not the parms object like you do in other places?
There was a problem hiding this comment.
Just to keep things narrow since this function only needs the one parameter.
bachase
added
the
Passed
|
Squashed and rebased on 0.70.0-b6 |
bachase
added
Rebase
Passed
|
Rebased and moved to #2084 since there are merge conflicts. |
In order to use the consensus simulation framework, we need the ability to change consensus parameters.