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.

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
base: develop
from

Conversation

Projects
None yet
5 participants
@bachase
Contributor

bachase commented Nov 2, 2017

#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 the nodesUsing count to break such ties.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 2, 2017

Codecov Report

Merging #2257 into develop will increase coverage by <.01%.
The diff coverage is 30.55%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ
src/ripple/app/consensus/RCLConsensus.h 80% <ø> (-2.15%) ⬇️
src/ripple/app/misc/NetworkOPs.cpp 61.74% <0%> (ø) ⬆️
src/ripple/overlay/impl/PeerImp.cpp 0% <0%> (ø) ⬆️
src/ripple/app/consensus/RCLConsensus.cpp 65.77% <91.66%> (+1.38%) ⬆️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp 78.92% <0%> (-1.03%) ⬇️
src/ripple/server/impl/BaseWSPeer.h 70.51% <0%> (-0.65%) ⬇️
src/ripple/beast/core/WaitableEvent.cpp 89.79% <0%> (+2.04%) ⬆️
src/ripple/core/Stoppable.h 100% <0%> (+6.66%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafe18c...4eff1ec. Read the comment docs.

codecov-io commented Nov 2, 2017

Codecov Report

Merging #2257 into develop will increase coverage by <.01%.
The diff coverage is 30.55%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ
src/ripple/app/consensus/RCLConsensus.h 80% <ø> (-2.15%) ⬇️
src/ripple/app/misc/NetworkOPs.cpp 61.74% <0%> (ø) ⬆️
src/ripple/overlay/impl/PeerImp.cpp 0% <0%> (ø) ⬆️
src/ripple/app/consensus/RCLConsensus.cpp 65.77% <91.66%> (+1.38%) ⬆️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp 78.92% <0%> (-1.03%) ⬇️
src/ripple/server/impl/BaseWSPeer.h 70.51% <0%> (-0.65%) ⬇️
src/ripple/beast/core/WaitableEvent.cpp 89.79% <0%> (+2.04%) ⬆️
src/ripple/core/Stoppable.h 100% <0%> (+6.66%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafe18c...4eff1ec. Read the comment docs.

@wilsonianb

LGTM 👍

@nbougalis

Fine as is. I have left a comment with a suggested improvement and cleanup. You can incorporate it at your discretion.

bestVC = it.second;
closedLedger = it.first;
bestCounts = currCounts;
closedLedger = currLedger;
switchLedgers = true;
}
}

This comment has been minimized.

@nbougalis

nbougalis Nov 3, 2017

Member

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.

@nbougalis

nbougalis Nov 3, 2017

Member

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.

This comment has been minimized.

@bachase

bachase Nov 6, 2017

Contributor

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.

@bachase

bachase Nov 6, 2017

Contributor

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.

This comment has been minimized.

@nbougalis

nbougalis Nov 6, 2017

Member

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

@nbougalis

nbougalis Nov 6, 2017

Member

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

@bachase

This comment has been minimized.

Show comment
Hide comment
@bachase

bachase Nov 7, 2017

Contributor

Not exactly related to these changes, but I added 5496ac4 to improve some consensus logging spots.

Contributor

bachase commented Nov 7, 2017

Not exactly related to these changes, but I added 5496ac4 to improve some consensus logging spots.

@JoelKatz

:neckbeard:👍

@bachase bachase added the Passed label Nov 7, 2017

@bachase

This comment has been minimized.

Show comment
Hide comment
@bachase

bachase Nov 8, 2017

Contributor

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

Contributor

bachase commented Nov 8, 2017

@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 bachase removed the Passed label Nov 8, 2017

@wilsonianb

👍

@nbougalis

This comment has been minimized.

Show comment
Hide comment
@nbougalis

nbougalis Nov 11, 2017

Member

👍

Member

nbougalis commented Nov 11, 2017

👍

@nbougalis nbougalis added the Passed label Nov 11, 2017

@bachase

This comment has been minimized.

Show comment
Hide comment
@bachase

bachase Nov 30, 2017

Contributor

Merged as 0a48916

Contributor

bachase commented Nov 30, 2017

Merged as 0a48916

@bachase bachase closed this Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment