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

Refactor getTrustedForLedger() #4424

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Refactor getTrustedForLedger() #4424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/ripple/app/consensus/RCLConsensus.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ RCLConsensus::Adaptor::onClose(
// pseudo-transactions
auto validations = app_.validators().negativeUNLFilter(
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
app_.getValidations().getTrustedForLedger(
prevLedger->info().parentHash));
prevLedger->info().parentHash, prevLedger->seq() - 1));
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
if (validations.size() >= app_.validators().quorum())
{
feeVote_->doVoting(prevLedger, validations, initialSet);
Expand Down
10 changes: 6 additions & 4 deletions src/ripple/app/ledger/impl/LedgerMaster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,8 @@ LedgerMaster::setValidLedger(std::shared_ptr<Ledger const> const& l)
if (!standalone_)
{
auto validations = app_.validators().negativeUNLFilter(
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
app_.getValidations().getTrustedForLedger(l->info().hash));
app_.getValidations().getTrustedForLedger(
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
l->info().hash, l->info().seq));
times.reserve(validations.size());
for (auto const& val : validations)
times.push_back(val->getSignTime());
Expand Down Expand Up @@ -987,7 +988,7 @@ LedgerMaster::checkAccept(uint256 const& hash, std::uint32_t seq)
return;

auto validations = app_.validators().negativeUNLFilter(
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
app_.getValidations().getTrustedForLedger(hash));
app_.getValidations().getTrustedForLedger(hash, seq));
valCount = validations.size();
if (valCount >= app_.validators().quorum())
{
Expand Down Expand Up @@ -1053,7 +1054,8 @@ LedgerMaster::checkAccept(std::shared_ptr<Ledger const> const& ledger)

auto const minVal = getNeededValidations();
auto validations = app_.validators().negativeUNLFilter(
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
app_.getValidations().getTrustedForLedger(ledger->info().hash));
app_.getValidations().getTrustedForLedger(
ledger->info().hash, ledger->info().seq));
auto const tvc = validations.size();
if (tvc < minVal) // nothing we can do
{
Expand Down Expand Up @@ -1128,7 +1130,7 @@ LedgerMaster::checkAccept(std::shared_ptr<Ledger const> const& ledger)
{
// Have not printed the warning before, check if need to print.
auto const vals = app_.getValidations().getTrustedForLedger(
ledger->info().parentHash);
ledger->info().parentHash, ledger->info().seq - 1);
std::size_t higherVersionCount = 0;
std::size_t rippledCount = 0;
for (auto const& v : vals)
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/app/misc/NegativeUNLVote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ NegativeUNLVote::buildScoreTable(
for (int i = 0; i < FLAG_LEDGER_INTERVAL; ++i)
{
for (auto const& v : validations.getTrustedForLedger(
ledgerAncestors[numAncestors - 1 - i]))
ledgerAncestors[numAncestors - 1 - i], seq - 2 - i))
levinwinter marked this conversation as resolved.
Show resolved Hide resolved
{
if (scoreTable.count(v->getNodeID()))
++scoreTable[v->getNodeID()];
Expand Down
5 changes: 3 additions & 2 deletions src/ripple/consensus/Validations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,10 +1049,11 @@ class Validations
/** Get trusted full validations for a specific ledger
@param ledgerID The identifier of ledger of interest
@param seq The sequence number of ledger of interest
@return Trusted validations associated with ledger
*/
std::vector<WrappedValidationType>
getTrustedForLedger(ID const& ledgerID)
getTrustedForLedger(ID const& ledgerID, Seq const& seq)
intelliot marked this conversation as resolved.
Show resolved Hide resolved
{
std::vector<WrappedValidationType> res;
std::lock_guard lock{mutex_};
Expand All @@ -1061,7 +1062,7 @@ class Validations
ledgerID,
[&](std::size_t numValidations) { res.reserve(numValidations); },
[&](NodeID const&, Validation const& v) {
if (v.trusted() && v.full())
if (v.trusted() && v.full() && v.seq() == seq)
intelliot marked this conversation as resolved.
Show resolved Hide resolved
res.emplace_back(v.unwrap());
});

Expand Down
24 changes: 16 additions & 8 deletions src/test/consensus/Validations_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,8 @@ class Validations_test : public beast::unit_test::suite
c.setLoadFee(12);
e.setLoadFee(12);

hash_map<Ledger::ID, std::vector<Validation>> trustedValidations;
hash_map<std::pair<Ledger::ID, Ledger::Seq>, std::vector<Validation>>
trustedValidations;

//----------------------------------------------------------------------
// checkers
Expand All @@ -624,14 +625,15 @@ class Validations_test : public beast::unit_test::suite
auto compare = [&]() {
for (auto& it : trustedValidations)
{
auto const& id = it.first;
auto const& id = it.first.first;
auto const& seq = it.first.second;
auto const& expectedValidations = it.second;

BEAST_EXPECT(
harness.vals().numTrustedForLedger(id) ==
expectedValidations.size());
BEAST_EXPECT(
sorted(harness.vals().getTrustedForLedger(id)) ==
sorted(harness.vals().getTrustedForLedger(id, seq)) ==
sorted(expectedValidations));

std::uint32_t baseFee = 0;
Expand All @@ -653,21 +655,22 @@ class Validations_test : public beast::unit_test::suite
Ledger ledgerAC = h["ac"];

// Add a dummy ID to cover unknown ledger identifiers
trustedValidations[Ledger::ID{100}] = {};
trustedValidations[{Ledger::ID{100}, Ledger::Seq{100}}] = {};

// first round a,b,c agree
for (auto const& node : {a, b, c})
{
auto const val = node.validate(ledgerA);
BEAST_EXPECT(ValStatus::current == harness.add(val));
if (val.trusted())
trustedValidations[val.ledgerID()].emplace_back(val);
trustedValidations[{val.ledgerID(), val.seq()}].emplace_back(
val);
}
// d disagrees
{
auto const val = d.validate(ledgerB);
BEAST_EXPECT(ValStatus::current == harness.add(val));
trustedValidations[val.ledgerID()].emplace_back(val);
trustedValidations[{val.ledgerID(), val.seq()}].emplace_back(val);
}
// e only issues partials
{
Expand All @@ -681,7 +684,8 @@ class Validations_test : public beast::unit_test::suite
auto const val = node.validate(ledgerAC);
BEAST_EXPECT(ValStatus::current == harness.add(val));
if (val.trusted())
trustedValidations[val.ledgerID()].emplace_back(val);
trustedValidations[{val.ledgerID(), val.seq()}].emplace_back(
val);
}
// d now thinks ledger 1, but cannot re-issue a previously used seq
// and attempting it should generate a conflict.
Expand Down Expand Up @@ -1035,6 +1039,9 @@ class Validations_test : public beast::unit_test::suite
std::vector<Validation> const& trustedVals) {
Ledger::ID testID = trustedVals.empty() ? this->genesisLedger.id()
: trustedVals[0].ledgerID();
Ledger::Seq testSeq = trustedVals.empty()
? this->genesisLedger.seq()
: trustedVals[0].seq();
BEAST_EXPECT(vals.currentTrusted() == trustedVals);
BEAST_EXPECT(vals.getCurrentNodeIDs() == listed);
BEAST_EXPECT(
Expand All @@ -1046,7 +1053,8 @@ class Validations_test : public beast::unit_test::suite
else
BEAST_EXPECT(
vals.getPreferred(this->genesisLedger)->second == testID);
BEAST_EXPECT(vals.getTrustedForLedger(testID) == trustedVals);
BEAST_EXPECT(
vals.getTrustedForLedger(testID, testSeq) == trustedVals);
BEAST_EXPECT(
vals.numTrustedForLedger(testID) == trustedVals.size());
};
Expand Down