diff --git a/src/ripple/app/consensus/RCLValidations.cpp b/src/ripple/app/consensus/RCLValidations.cpp index 75bc2edd49c..9cb4d73b367 100644 --- a/src/ripple/app/consensus/RCLValidations.cpp +++ b/src/ripple/app/consensus/RCLValidations.cpp @@ -171,100 +171,60 @@ RCLValidationsAdaptor::doStaleWrite(ScopedLockType&) } bool -handleNewValidation( - Application& app, +handleNewValidation(Application& app, STValidation::ref val, std::string const& source) { - PublicKey const& signer = val->getSignerPublic(); + PublicKey const& signingKey = val->getSignerPublic(); uint256 const& hash = val->getLedgerHash(); // Ensure validation is marked as trusted if signer currently trusted - boost::optional pubKey = app.validators().getTrustedKey(signer); - if (!val->isTrusted() && pubKey) + boost::optional masterKey = + app.validators().getTrustedKey(signingKey); + if (!val->isTrusted() && masterKey) val->setTrusted(); - RCLValidations& validations = app.getValidations(); - - beast::Journal j = validations.adaptor().journal(); - - // Do not process partial validations. - if (!val->isFull()) - { - const bool current = isCurrent( - validations.parms(), - app.timeKeeper().closeTime(), - val->getSignTime(), - val->getSeenTime()); - - JLOG(j.debug()) << "Val (partial) for " << hash << " from " - << toBase58(TokenType::TOKEN_NODE_PUBLIC, signer) - << " ignored " - << (val->isTrusted() ? "trusted/" : "UNtrusted/") - << (current ? "current" : "stale"); - - // Only forward if current and trusted - return current && val->isTrusted(); - } - - if (!val->isTrusted()) - { - JLOG(j.trace()) << "Node " - << toBase58(TokenType::TOKEN_NODE_PUBLIC, signer) - << " not in UNL st=" - << val->getSignTime().time_since_epoch().count() - << ", hash=" << hash - << ", shash=" << val->getSigningHash() - << " src=" << source; - } // If not currently trusted, see if signer is currently listed - if (!pubKey) - pubKey = app.validators().getListedKey(signer); + if (!masterKey) + masterKey = app.validators().getListedKey(signingKey); bool shouldRelay = false; + RCLValidations& validations = app.getValidations(); + beast::Journal j = validations.adaptor().journal(); - // only add trusted or listed - if (pubKey) + // masterKey is seated only if validator is trusted or listed + if (masterKey) { - ValStatus const res = validations.add(*pubKey, val); - - // This is a duplicate validation - if (res == ValStatus::repeat) - return false; - - // This validation replaced a prior one with the same sequence number - if (res == ValStatus::sameSeq) - { - auto const seq = val->getFieldU32(sfLedgerSequence); - JLOG(j.warn()) << "Trusted node " - << toBase58(TokenType::TOKEN_NODE_PUBLIC, *pubKey) - << " published multiple validations for ledger " - << seq; - } - - JLOG(j.debug()) << "Val for " << hash << " from " - << toBase58(TokenType::TOKEN_NODE_PUBLIC, signer) - << " added " - << (val->isTrusted() ? "trusted/" : "UNtrusted/") - << ((res == ValStatus::current) ? "current" : "stale"); - - // Trusted current validations should be checked and relayed. - // Trusted validations with sameSeq replaced an older validation - // with that sequence number, so should still be checked and relayed. - if (val->isTrusted() && - (res == ValStatus::current || res == ValStatus::sameSeq)) + ValStatus const outcome = validations.add(*masterKey, val); + + auto dmp = [&](beast::Journal::Stream s, std::string const& msg) { + s << "Val for " << hash + << (val->isTrusted() ? " trusted/" : " UNtrusted/") + << (val->isFull() ? " full" : "partial") << " from " + << toBase58(TokenType::TOKEN_NODE_PUBLIC, *masterKey) + << "signing key " + << toBase58(TokenType::TOKEN_NODE_PUBLIC, signingKey) << " " + << msg + << " src=" << source; + }; + + if(j.debug()) + dmp(j.debug(), to_string(outcome)); + + if (val->isTrusted() && outcome == ValStatus::current) { app.getLedgerMaster().checkAccept( hash, val->getFieldU32(sfLedgerSequence)); shouldRelay = true; } + } else { JLOG(j.debug()) << "Val for " << hash << " from " - << toBase58(TokenType::TOKEN_NODE_PUBLIC, signer) - << " not added UNtrusted/"; + << toBase58(TokenType::TOKEN_NODE_PUBLIC, signingKey) + << " not added UNlisted"; } // This currently never forwards untrusted validations, though we may diff --git a/src/ripple/consensus/Validations.h b/src/ripple/consensus/Validations.h index 7a1608152f5..2e27a2eca53 100644 --- a/src/ripple/consensus/Validations.h +++ b/src/ripple/consensus/Validations.h @@ -142,9 +142,7 @@ enum class ValStatus { /// Already had this exact same validation repeat, /// Not current or was older than current from this node - stale, - /// A validation was re-issued for the same sequence number - sameSeq + stale }; inline std::string @@ -158,8 +156,6 @@ to_string(ValStatus m) return "repeat"; case ValStatus::stale: return "stale"; - case ValStatus::sameSeq: - return "sameSeq"; default: return "unknown"; } @@ -403,123 +399,45 @@ class Validations Attempt to add a new validation. - @param key The NodeKey to use for the validation - @param val The validation to store - @return The outcome of the attempt - - @note The provided key may differ from the validation's - key() member since we might be storing by master key and the - validation might be signed by a temporary or rotating key. + @param key The master key associated with this validation + @param val The validationo to store + @return The outcome + @note The provided key may differ from the validations's key() + member if the validator is using ephemeral signing keys. */ ValStatus add(NodeKey const& key, Validation const& val) { - NetClock::time_point t = adaptor_.now(); - if (!isCurrent(parms_, t, val.signTime(), val.seenTime())) + if (!isCurrent(parms_, adaptor_.now(), val.signTime(), val.seenTime())) return ValStatus::stale; - ID const& id = val.ledgerID(); - - // This is only seated if a validation became stale - boost::optional maybeStaleValidation; - - ValStatus result = ValStatus::current; - { ScopedLock lock{mutex_}; - auto const ret = byLedger_[id].emplace(key, val); - // This validation is a repeat if we already have - // one with the same id and signing key. + // one with the same id for this key + auto const ret = byLedger_[val.ledgerID()].emplace(key, val); if (!ret.second && ret.first->second.key() == val.key()) return ValStatus::repeat; - // Attempt to insert auto const ins = current_.emplace(key, val); - if (!ins.second) { - // Had a previous validation from the node, consider updating + // Replace existing only if this one is newer Validation& oldVal = ins.first->second.val; - ID const previousLedgerID = ins.first->second.prevLedgerID; - - Seq const oldSeq{oldVal.seq()}; - Seq const newSeq{val.seq()}; - - // Sequence of 0 indicates a missing sequence number - if ((oldSeq != Seq{0}) && (newSeq != Seq{0}) && - oldSeq == newSeq) + if (val.signTime() > oldVal.signTime()) { - result = ValStatus::sameSeq; - - // If the validation key was revoked, update the - // existing validation in the byLedger_ set - if (val.key() != oldVal.key()) - { - auto const mapIt = byLedger_.find(oldVal.ledgerID()); - if (mapIt != byLedger_.end()) - { - auto& validationMap = mapIt->second; - // If a new validation with the same ID was - // reissued we simply replace. - if(oldVal.ledgerID() == val.ledgerID()) - { - auto replaceRes = validationMap.emplace(key, val); - // If it was already there, replace - if(!replaceRes.second) - replaceRes.first->second = val; - } - else - { - // If the new validation has a different ID, - // we remove the old. - validationMap.erase(key); - // Erase the set if it is now empty - if (validationMap.empty()) - byLedger_.erase(mapIt); - } - } - } - } - - if (val.signTime() > oldVal.signTime() || - val.key() != oldVal.key()) - { - // This is either a newer validation or a new signing key - ID const prevID = [&]() { - // In the normal case, the prevID is the ID of the - // ledger we replace - if (oldVal.ledgerID() != val.ledgerID()) - return oldVal.ledgerID(); - // In the case the key was revoked and a new validation - // for the same ledger ID was sent, the previous ledger - // is still the one the now revoked validation had - return previousLedgerID; - }(); - - // Allow impl to take over oldVal - maybeStaleValidation.emplace(std::move(oldVal)); - // Replace old val in the map and set the previous ledger ID + ID oldID = oldVal.ledgerID(); + adaptor_.onStale(std::move(oldVal)); ins.first->second.val = val; - ins.first->second.prevLedgerID = prevID; + ins.first->second.prevLedgerID = oldID; } else - { - // We already have a newer validation from this source - result = ValStatus::stale; - } + return ValStatus::stale; } } - - // Handle the newly stale validation outside the lock - if (maybeStaleValidation) - { - adaptor_.onStale(std::move(*maybeStaleValidation)); - } - - return result; + return ValStatus::current; } /** Expire old validation sets diff --git a/src/test/consensus/Validations_test.cpp b/src/test/consensus/Validations_test.cpp index b03dc82e526..f5dbf900d88 100644 --- a/src/test/consensus/Validations_test.cpp +++ b/src/test/consensus/Validations_test.cpp @@ -294,7 +294,7 @@ class Validations_test : public beast::unit_test::suite void testAddValidation() { - // Test adding current,stale,repeat,sameSeq validations + // Test adding current,stale,repeat validations using namespace std::chrono_literals; TestHarness harness; @@ -340,39 +340,12 @@ class Validations_test : public beast::unit_test::suite BEAST_EXPECT(harness.vals().getNodesAfter(Ledger::ID{2}) == 0); BEAST_EXPECT( - ValStatus::sameSeq == + ValStatus::stale == harness.add(a.validate(Ledger::Seq{2}, Ledger::ID{20}))); - // Old ID should be gone ... - BEAST_EXPECT(harness.vals().numTrustedForLedger(Ledger::ID{2}) == 0); - BEAST_EXPECT(harness.vals().numTrustedForLedger(Ledger::ID{20}) == 1); - { - // Should be the only trusted for ID{20} - auto trustedVals = - harness.vals().getTrustedForLedger(Ledger::ID{20}); - BEAST_EXPECT(trustedVals.size() == 1); - BEAST_EXPECT(trustedVals[0].key() == a.currKey()); - // ... and should be the only node after ID{2} - BEAST_EXPECT(harness.vals().getNodesAfter(Ledger::ID{2}) == 1); - - } - - // A new key, but re-issue a validation with the same ID and - // Sequence - a.advanceKey(); - - BEAST_EXPECT( - ValStatus::sameSeq == - harness.add(a.validate(Ledger::Seq{2}, Ledger::ID{20}))); - { - // Still the only trusted validation for ID{20} - auto trustedVals = - harness.vals().getTrustedForLedger(Ledger::ID{20}); - BEAST_EXPECT(trustedVals.size() == 1); - BEAST_EXPECT(trustedVals[0].key() == a.currKey()); - // and still follows ID{2} since it was a re-issue - BEAST_EXPECT(harness.vals().getNodesAfter(Ledger::ID{2}) == 1); - } + BEAST_EXPECT(harness.vals().numTrustedForLedger(Ledger::ID{2}) == 1); + // THIS FAILS pending filtering on increasing seq #'s + BEAST_EXPECT(harness.vals().numTrustedForLedger(Ledger::ID{20}) == 0); } {