Properly use ledger hash to break ties #2257
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2257 +/- ##
===========================================
+ Coverage 70.11% 70.11% +<.01%
===========================================
Files 689 689
Lines 50800 50799 -1
===========================================
+ Hits 35617 35619 +2
+ Misses 15183 15180 -3
Continue to review full report at Codecov.
|
| bestVC = it.second; | ||
| closedLedger = it.first; | ||
| bestCounts = currCounts; | ||
| closedLedger = currLedger; | ||
| switchLedgers = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe that the code from line 1313 through here can be replaced with this:
auto result = std::max_element(ledgers.begin(), ledgers.end(),
[&m_journal](auto const& best, auto const& curr)
{
JLOG(m_journal.debug()) <<
"L: " << curr.first <<
": t=" << curr.second.trustedValidations <<
", n=" << curr.second.nodesUsing;
// Prefer ledger with more trustedValidations
if (curr.second.trustedValidations > best.second.trustedValidations)
return true;
if (curr.second.trustedValidations < best.second.trustedValidations)
return false;
// If neither are trusted, prefer more nodesUsing
if (curr.second.trustedValidations == 0)
{
if (curr.second.nodesUsing > best.second.nodesUsing)
return true;
if (curr.second.nodesUsing < best.second.nodesUsing)
return false;
}
// If tied trustedValidations (non-zero) or tied nodesUsing,
// prefer higher ledger hash
return curr.first > best.first;
});
assert (result != ledgers.end());
Note, that we are guaranteed that ledgers is not empty and will contain an entry for the initial value of closedLedger because of line 1294:
auto& ourVC = ledgers[closedLedger];
Determining whether we need to switch ledgers now becomes trivial: simply compare result->first to closedLedger. The bestCounts is no longer needed.
There was a problem hiding this comment.
I like the suggestion, although I think it won't print the debug output for the first item in ledgers. This area is likely to see a refactor soon so I will take this idea up then.
There was a problem hiding this comment.
Yes, you’re right, it (understandably) won’t. That could be a problem when examining logs.
|
Not exactly related to these changes, but I added 5496ac4 to improve some consensus logging spots. |
👍
bachase
added
the
Passed
|
@nbougalis and @wilsonianb added one last fix to copy the inputs into the offloaded accept call. Please drop a quick 👍 / 👎 when you get a chance. |
bachase
removed
the
Passed
|
👍 |
nbougalis
added
the
Passed
|
Merged as 0a48916 |
#2169 changed the behavior for selecting the best working ledger for the next consensus round. This fix restores that behavior, so that if there are several ledgers with the same number of
trustedValidations > 0, the best is chosen based on the ledger hash, as opposed to the current code which prefers thenodesUsingcount to break such ties.