From 38603d495597a16e86be1f7afda9e862b0b8aaea Mon Sep 17 00:00:00 2001 From: Brad Chase Date: Fri, 15 Dec 2017 11:37:18 -0500 Subject: [PATCH] [FOLD] Simplify Validations::add: In the case when a validator switched signing keys, the old code tried to detect a case where the new validator re-issued the same sequence validation with the new key, but for a different ledger. This is not ideal, since there is no gaurantee when other validators see the two different validations or the manifest updates. The next changeset requiring increasing full validation sequence numbers will makes this change more relevant, so updating tests will be deferred until then. --- src/ripple/app/consensus/RCLValidations.cpp | 102 ++++++------------ src/ripple/consensus/Validations.h | 114 +++----------------- src/test/consensus/Validations_test.cpp | 37 +------ 3 files changed, 52 insertions(+), 201 deletions(-) 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); } {