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.

Sign up for GitHub

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

Properly use ledger hash to break ties #2257

Closed
wants to merge 4 commits into from
Closed

Properly use ledger hash to break ties #2257

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a429c23
Properly use ledger hash to break ties
bachase Nov 2, 2017
e11279f
[FOLD] Bump logging level
bachase Nov 2, 2017
5496ac4
[FOLD] Add additional logging
bachase Nov 7, 2017
4eff1ec
[FOLD] Copy consensus results into accept job
bachase Nov 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 30 additions & 42 deletions src/ripple/app/misc/NetworkOPs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,29 +1275,8 @@ bool NetworkOPsImp::checkLastClosedLedger (

struct ValidationCount
{
int trustedValidations, nodesUsing;

ValidationCount() : trustedValidations(0), nodesUsing(0)
{
}

auto
asTie() const
{
return std::tie(trustedValidations, nodesUsing);
}

bool
operator>(const ValidationCount& v) const
{
return asTie() > v.asTie();
}

bool
operator==(const ValidationCount& v) const
{
return asTie() == v.asTie();
}
std::uint32_t trustedValidations = 0;
std::uint32_t nodesUsing = 0;
};

hash_map<uint256, ValidationCount> ledgers;
Expand Down Expand Up @@ -1327,38 +1306,47 @@ bool NetworkOPsImp::checkLastClosedLedger (
++ledgers[peerLedger].nodesUsing;
}

auto bestVC = ledgers[closedLedger];

// 3) Is there a network ledger we'd like to switch to? If so, do we have
// it?
bool switchLedgers = false;
ValidationCount bestCounts = ledgers[closedLedger];

for (auto const& it: ledgers)
{
JLOG(m_journal.debug()) << "L: " << it.first
<< " t=" << it.second.trustedValidations
<< ", n=" << it.second.nodesUsing;
uint256 const & currLedger = it.first;
ValidationCount const & currCounts = it.second;

// Temporary logging to make sure tiebreaking isn't broken
if (it.second.trustedValidations > 0)
JLOG(m_journal.trace())
<< " TieBreakTV: " << it.first;
else
JLOG(m_journal.debug()) << "L: " << currLedger
<< " t=" << currCounts.trustedValidations
<< ", n=" << currCounts.nodesUsing;

bool const preferCurr = [&]()
{
if (it.second.nodesUsing > 0)
// Prefer ledger with more trustedValidations
if (currCounts.trustedValidations > bestCounts.trustedValidations)
return true;
if (currCounts.trustedValidations < bestCounts.trustedValidations)
return false;
// If neither are trusted, prefer more nodesUsing
if (currCounts.trustedValidations == 0)
{
JLOG(m_journal.trace())
<< " TieBreakNU: " << it.first;
if (currCounts.nodesUsing > bestCounts.nodesUsing)
return true;
if (currCounts.nodesUsing < bestCounts.nodesUsing)
return false;
}
}
// If tied trustedValidations (non-zero) or tied nodesUsing,
// prefer higher ledger hash
return currLedger > closedLedger;

}();

// Switch to a ledger with more support
// or the one with higher hash if they have the same support
if (it.second > bestVC ||
(it.second == bestVC && it.first > closedLedger))
// Switch to current ledger if it is preferred over best so far
if (preferCurr)
{
bestVC = it.second;
closedLedger = it.first;
bestCounts = currCounts;
closedLedger = currLedger;
switchLedgers = true;
}
}
Copy link
Contributor

@nbougalis nbougalis Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you’re right, it (understandably) won’t. That could be a problem when examining logs.

Expand Down
6 changes: 3 additions & 3 deletions src/ripple/overlay/impl/PeerImp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,7 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMStatusChange> const& m)
{
if (!closedLedgerHash_.isZero ())
{
JLOG(p_journal_.trace()) << "Status: Out of sync";
JLOG(p_journal_.debug()) << "Status: Out of sync";
closedLedgerHash_.zero ();
}

Expand All @@ -1318,11 +1318,11 @@ PeerImp::onMessage (std::shared_ptr <protocol::TMStatusChange> const& m)
// a peer has changed ledgers
memcpy (closedLedgerHash_.begin (), m->ledgerhash ().data (), 256 / 8);
addLedger (closedLedgerHash_);
JLOG(p_journal_.trace()) << "LCL is " << closedLedgerHash_;
JLOG(p_journal_.debug()) << "LCL is " << closedLedgerHash_;
}
else
{
JLOG(p_journal_.trace()) << "Status: No ledger";
JLOG(p_journal_.debug()) << "Status: No ledger";
closedLedgerHash_.zero ();
}

Expand Down