Use ledger hash to break ties (RIPD-1468) #2169
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2169 +/- ##
===========================================
+ Coverage 69.96% 69.98% +0.02%
===========================================
Files 689 689
Lines 50734 50704 -30
===========================================
- Hits 35495 35487 -8
+ Misses 15239 15217 -22
Continue to review full report at Codecov.
|
| if ((it.second.count > netLgrCount) || | ||
| ((it.second.count== netLgrCount) && (it.first == ledgerID))) | ||
| if ((it.second > netLgrCount) || | ||
| ((it.second== netLgrCount) && (it.first == ledgerID))) |
There was a problem hiding this comment.
it.second== netLgrCount --> it.second == netLgrCount
| { | ||
| // Should the the only trusted for ID{20} |
| //! of the ledger it replaces | ||
| struct ValidationAndPrevID | ||
| { | ||
| ValidationAndPrevID(Validation const& v) : val{v}, prevLedgerID{0} |
There was a problem hiding this comment.
This may be out of scope for this PR too, but currentTrustedDistribution and getNodesAfter can still return incorrect info if a validator sends a validation for the first time (in ~5 minutes) and prevLedgerID is zero.
#2084 (comment)
There was a problem hiding this comment.
I agree that is still worth addressing, but would prefer to do as an independent change.
src/ripple/app/misc/NetworkOPs.cpp
Outdated
| @@ -1348,10 +1351,9 @@ bool NetworkOPsImp::checkLastClosedLedger ( | |||
| try | |||
| { | |||
| auto& vc = ledgers[peerLedger]; | |||
| auto const nodeId = calcNodeID(peer->getNodePublic ()); | |||
There was a problem hiding this comment.
Does removing this line mean we no longer need the try/catch?
There was a problem hiding this comment.
Yes, I believe you are correct. Although, I'm not clear what was throwing before. The comment below suggests a peer may have disconnected, but peer is a shared_ptr here.
src/ripple/app/misc/NetworkOPs.cpp
Outdated
| @@ -1223,7 +1223,9 @@ class ValidationCount | |||
| { | |||
| public: | |||
| int trustedValidations, nodesUsing; | |||
| NodeID highNodeUsing, highValidation; | |||
| // Ledger hashes to break ties | |||
| uint256 highUsingHash, highTrustedValHash; | |||
There was a problem hiding this comment.
Since this class is only used in checkLastClosedLedger in a hash_map<uint256, ValidationCount>, I'd consider removing both highUsingHash and highTrustedValHash (we already have the ledger hash as the key). Then you could handle tiebreaking outside of the comparison operator.
| @@ -1378,17 +1349,20 @@ bool NetworkOPsImp::checkLastClosedLedger ( | |||
| // Temporary logging to make sure tiebreaking isn't broken | |||
There was a problem hiding this comment.
I don't think this temporary logging is still useful, especially with the debug log above
|
Rebased on 0.80.0-b3 |
When two ledgers have the same number of validations, the code will now use the ledger hash itself to break the tie rather than the highest node ID supporting each validation.
bachase
force-pushed
the
validation-hash
branch
from
ae731c3 to
f8774f8
Compare
|
Rebased on 0.80.0-b4 |
👍
|
Merged as 60dd194 |
When two ledgers have the same number of validations, the code will now use the ledger hash itself to break the tie rather than the highest node ID supporting each validation. This simplifies the interface for the generic
Validationscode.Additionally, the second changeset makes storing the previous ledger ID/hash a detail of the
Validationsclass, rather than tacking it onSTValidationas an unserialized member. This simplifies the use of genericValidationsin the consensus simulation framework.