Skip to content

Commit

Permalink
[FOLD] Simplify Validations::add:
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bachase
bachase committed Dec 15, 2017
1 parent 2698fde commit 38603d4
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 201 deletions.
102 changes: 31 additions & 71 deletions src/ripple/app/consensus/RCLValidations.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<PublicKey> pubKey = app.validators().getTrustedKey(signer);
if (!val->isTrusted() && pubKey)
boost::optional<PublicKey> 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
Expand Down
114 changes: 16 additions & 98 deletions src/ripple/consensus/Validations.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -158,8 +156,6 @@ to_string(ValStatus m)
return "repeat";
case ValStatus::stale:
return "stale";
case ValStatus::sameSeq:
return "sameSeq";
default:
return "unknown";
}
Expand Down Expand Up @@ -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<Validation> 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
Expand Down
37 changes: 5 additions & 32 deletions src/test/consensus/Validations_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

{
Expand Down

0 comments on commit 38603d4

Please sign in to comment.