From 69686304be86293c1079a8807e5c79071b4dbe2a Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Tue, 5 May 2020 23:40:29 -0700 Subject: [PATCH 01/10] Improve handling of sfLedgerSequence field: The sfLedgerSequence field is designated as optional in the object template but it is effectively required and validations which do not include it were, correctly, rejected. This commit migrates the check outside of the peer code and into the constructor used for validations being deserialized for the network. Furthermore, the code will generate an error if a validation that is generated by a server does not include the field. --- src/ripple/app/consensus/RCLValidations.cpp | 7 ------- src/ripple/overlay/impl/PeerImp.cpp | 3 ++- src/ripple/protocol/STValidation.h | 12 ++++++++++++ 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/ripple/app/consensus/RCLValidations.cpp b/src/ripple/app/consensus/RCLValidations.cpp index c44f98bd484..6311677c214 100644 --- a/src/ripple/app/consensus/RCLValidations.cpp +++ b/src/ripple/app/consensus/RCLValidations.cpp @@ -182,13 +182,6 @@ handleNewValidation( << " [" << val->getSerializer().slice() << "]"; }; - if (!val->isFieldPresent(sfLedgerSequence)) - { - if (j.error()) - dmp(j.error(), "missing ledger sequence field"); - return false; - } - // masterKey is seated only if validator is trusted or listed if (masterKey) { diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 845f64e4492..02ef2347d2d 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -2173,7 +2173,8 @@ PeerImp::onMessage(std::shared_ptr const& m) } catch (std::exception const& e) { - JLOG(p_journal_.warn()) << "Validation: Exception, " << e.what(); + JLOG(p_journal_.warn()) + << "Exception processing validation: " << e.what(); fee_ = Resource::feeInvalidRequest; } } diff --git a/src/ripple/protocol/STValidation.h b/src/ripple/protocol/STValidation.h index 8410860007a..8e1db26bdd4 100644 --- a/src/ripple/protocol/STValidation.h +++ b/src/ripple/protocol/STValidation.h @@ -88,6 +88,13 @@ class STValidation final : public STObject, public CountedObject Throw("Invalid signature in validation"); } + if (!isFieldPresent(sfLedgerSequence)) + { + JLOG(debugLog().error()) << "Signature with no ledger sequence: " + << getJson(JsonOptions::none); + Throw("Missing ledger sequence in validation"); + } + nodeID_ = lookupNodeID(PublicKey(makeSlice(spk))); assert(nodeID_.isNonZero()); } @@ -122,6 +129,11 @@ class STValidation final : public STObject, public CountedObject // Perform additional initialization f(*this); + // This is a logic error because we should never assemble a + // validation without a ledger sequence number. + if (!isFieldPresent(sfLedgerSequence)) + LogicError("Missing ledger sequence in validation"); + // Finally, sign the validation and mark it as trusted: setFlag(vfFullyCanonicalSig); setFieldVL(sfSignature, signDigest(pk, sk, getSigningHash())); From c251be4c9f5d32e4eb277c862d5661df3421326f Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Tue, 5 May 2020 23:40:55 -0700 Subject: [PATCH 02/10] Tune relaying of untrusted proposals & validations: In deciding whether to relay a proposal or validation, a server would consider whether it was issued by a validator on that server's UNL. While both trusted proposals and validations were always relayed, the code prioritized relaying of untrusted proposals over untrusted validations. While not technically incorrect, validations are generally more "valuable" because they are required during the consensus process, whereas proposals are not, strictly, required. The commit introduces two new configuration options, allowing server operators to fine-tune the relaying behavior: The `[relay_proposals]` option controls the relaying behavior for proposals received by this server. It has two settings: "trusted" and "all" and the default is "trusted". The `[relay_validations]` options controls the relaying behavior for validations received by this server. It has two settings: "trusted" and "all" and the default is "all". This change does not require an amendment as it does not affect transaction processing. --- cfg/rippled-example.cfg | 17 ++++++++++ src/ripple/app/consensus/RCLConsensus.cpp | 15 +++++---- src/ripple/app/consensus/RCLValidations.cpp | 36 +++++++-------------- src/ripple/app/consensus/RCLValidations.h | 6 ++-- src/ripple/app/misc/NetworkOPs.cpp | 22 ++++--------- src/ripple/app/misc/NetworkOPs.h | 6 ++-- src/ripple/core/Config.h | 2 ++ src/ripple/core/ConfigSections.h | 2 ++ src/ripple/core/impl/Config.cpp | 24 ++++++++++++++ src/ripple/overlay/impl/PeerImp.cpp | 24 ++++---------- 10 files changed, 85 insertions(+), 69 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 3c856b40fd8..3b56f351b3a 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -593,6 +593,23 @@ # # # +# +# [relay_proposals] +# +# Controls the relaying behavior for proposals received by this server that +# are issued by validators that are not on the server's UNL. +# +# Legal values are: "trusted" and "all". The default is "trusted". +# +# +# [relay_validations] +# +# Controls the relaying behavior for proposals received by this server that +# are issued by validators that are not on the server's UNL. +# +# Legal values are: "trusted" and "all". The default is "all". +# +# # [node_size] # # Tunes the servers based on the expected load and available memory. Legal diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 90e9fd72968..5b56af5586e 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -828,12 +828,15 @@ RCLConsensus::Adaptor::validate( // suppress it if we receive it app_.getHashRouter().addSuppression( sha512Half(makeSlice(v->getSerialized()))); - handleNewValidation(app_, v, "local"); - Blob validation = v->getSerialized(); - protocol::TMValidation val; - val.set_validation(&validation[0], validation.size()); - // Send signed validation to all of our directly connected peers - app_.overlay().send(val); + + if (handleNewValidation(app_, v, "local", true)) + { + Blob validation = v->getSerialized(); + protocol::TMValidation val; + val.set_validation(&validation[0], validation.size()); + // Send signed validation to all of our directly connected peers + app_.overlay().send(val); + } } void diff --git a/src/ripple/app/consensus/RCLValidations.cpp b/src/ripple/app/consensus/RCLValidations.cpp index 6311677c214..becec535a22 100644 --- a/src/ripple/app/consensus/RCLValidations.cpp +++ b/src/ripple/app/consensus/RCLValidations.cpp @@ -152,7 +152,8 @@ bool handleNewValidation( Application& app, std::shared_ptr const& val, - std::string const& source) + std::string const& source, + bool relayUntrusted) { PublicKey const& signingKey = val->getSignerPublic(); uint256 const& hash = val->getLedgerHash(); @@ -166,7 +167,6 @@ handleNewValidation( if (!masterKey) masterKey = app.validators().getListedKey(signingKey); - bool shouldRelay = false; RCLValidations& validations = app.getValidations(); beast::Journal const j = validations.adaptor().journal(); @@ -186,13 +186,17 @@ handleNewValidation( if (masterKey) { ValStatus const outcome = validations.add(calcNodeID(*masterKey), val); + auto const seq = val->getFieldU32(sfLedgerSequence); if (j.debug()) dmp(j.debug(), to_string(outcome)); + // One might think that we would not wish to relay validations that + // fail these checks. Somewhat counterintively, we actually want to + // do it for validations that we receive and deem to be suspicious, + // so that our peers will also observe them and realize they're bad. if (outcome == ValStatus::conflicting && j.warn()) { - auto const seq = val->getFieldU32(sfLedgerSequence); dmp(j.warn(), "conflicting validations issued for " + to_string(seq) + " (likely from a Byzantine validator)"); @@ -200,25 +204,13 @@ handleNewValidation( if (outcome == ValStatus::multiple && j.warn()) { - auto const seq = val->getFieldU32(sfLedgerSequence); dmp(j.warn(), "multiple validations issued for " + to_string(seq) + " (multiple validators operating with the same key?)"); } - if (outcome == ValStatus::badSeq && j.warn()) - { - auto const seq = val->getFieldU32(sfLedgerSequence); - dmp(j.debug(), - "already validated sequence at or past " + std::to_string(seq)); - } - if (val->isTrusted() && outcome == ValStatus::current) - { - app.getLedgerMaster().checkAccept( - hash, val->getFieldU32(sfLedgerSequence)); - shouldRelay = true; - } + app.getLedgerMaster().checkAccept(hash, seq); } else { @@ -227,15 +219,9 @@ handleNewValidation( << " not added UNlisted"; } - // This currently never forwards untrusted validations, though we may - // reconsider in the future. From @JoelKatz: - // The idea was that we would have a certain number of validation slots with - // priority going to validators we trusted. Remaining slots might be - // allocated to validators that were listed by publishers we trusted but - // that we didn't choose to trust. The shorter term plan was just to forward - // untrusted validations if peers wanted them or if we had the - // ability/bandwidth to. None of that was implemented. - return shouldRelay; + // We will always forward trusted validations; if configured, we will + // also relay all untrusted validations. + return relayUntrusted || val->isTrusted(); } } // namespace ripple diff --git a/src/ripple/app/consensus/RCLValidations.h b/src/ripple/app/consensus/RCLValidations.h index 3f0245d6793..7597c76c7e2 100644 --- a/src/ripple/app/consensus/RCLValidations.h +++ b/src/ripple/app/consensus/RCLValidations.h @@ -243,14 +243,16 @@ using RCLValidations = Validations; @param app Application object containing validations and ledgerMaster @param val The validation to add @param source Name associated with validation used in logging - + @param relayUntrusted whether untrusted validations should be relayed @return Whether the validation should be relayed */ +[[nodiscard]] bool handleNewValidation( Application& app, std::shared_ptr const& val, - std::string const& source); + std::string const& source, + bool relayUntrusted = false); } // namespace ripple diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index c4b7ad31c8f..1870b3517b6 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -356,10 +356,8 @@ class NetworkOPsImp final : public NetworkOPs Json::Value& jvResult) override; // Ledger proposal/close functions. - void - processTrustedProposal( - RCLCxPeerPos proposal, - std::shared_ptr set) override; + bool + processTrustedProposal(RCLCxPeerPos proposal) override; bool recvValidation( @@ -1780,17 +1778,10 @@ NetworkOPsImp::getConsensusLCL() return mConsensus.prevLedgerID(); } -void -NetworkOPsImp::processTrustedProposal( - RCLCxPeerPos peerPos, - std::shared_ptr set) +bool +NetworkOPsImp::processTrustedProposal(RCLCxPeerPos peerPos) { - if (mConsensus.peerProposal(app_.timeKeeper().closeTime(), peerPos)) - { - app_.overlay().relay(*set, peerPos.suppressionID()); - } - else - JLOG(m_journal.info()) << "Not relaying trusted proposal"; + return mConsensus.peerProposal(app_.timeKeeper().closeTime(), peerPos); } void @@ -2482,7 +2473,8 @@ NetworkOPsImp::recvValidation( JLOG(m_journal.debug()) << "recvValidation " << val->getLedgerHash() << " from " << source; pubValidation(val); - return handleNewValidation(app_, val, source); + return handleNewValidation( + app_, val, source, app_.config().RELAY_UNTRUSTED_VALIDATIONS); } Json::Value diff --git a/src/ripple/app/misc/NetworkOPs.h b/src/ripple/app/misc/NetworkOPs.h index fa0499526eb..34da5092b1e 100644 --- a/src/ripple/app/misc/NetworkOPs.h +++ b/src/ripple/app/misc/NetworkOPs.h @@ -169,10 +169,8 @@ class NetworkOPs : public InfoSub::Source //-------------------------------------------------------------------------- // ledger proposal/close functions - virtual void - processTrustedProposal( - RCLCxPeerPos peerPos, - std::shared_ptr set) = 0; + virtual bool + processTrustedProposal(RCLCxPeerPos peerPos) = 0; virtual bool recvValidation( diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index 0b7525efac7..7943906fdae 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -136,6 +136,8 @@ class Config : public BasicConfig std::size_t NETWORK_QUORUM = 1; // Peer networking parameters + bool RELAY_UNTRUSTED_VALIDATIONS = true; + bool RELAY_UNTRUSTED_PROPOSALS = false; // True to ask peers not to relay current IP. bool PEER_PRIVATE = false; diff --git a/src/ripple/core/ConfigSections.h b/src/ripple/core/ConfigSections.h index cec6619f3b0..c9b61c2cb2b 100644 --- a/src/ripple/core/ConfigSections.h +++ b/src/ripple/core/ConfigSections.h @@ -69,6 +69,8 @@ struct ConfigSection #define SECTION_PATH_SEARCH_MAX "path_search_max" #define SECTION_PEER_PRIVATE "peer_private" #define SECTION_PEERS_MAX "peers_max" +#define SECTION_RELAY_PROPOSALS "relay_proposals" +#define SECTION_RELAY_VALIDATIONS "relay_validations" #define SECTION_RPC_STARTUP "rpc_startup" #define SECTION_SIGNING_SUPPORT "signing_support" #define SECTION_SNTP "sntp_servers" diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 3fc805b21e6..f12ba7dbcee 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -397,6 +397,30 @@ Config::loadFromString(std::string const& fileContents) if (getSingleSection(secConfig, SECTION_SSL_VERIFY, strTemp, j_)) SSL_VERIFY = beast::lexicalCastThrow(strTemp); + if (getSingleSection(secConfig, SECTION_RELAY_VALIDATIONS, strTemp, j_)) + { + if (boost::iequals(strTemp, "all")) + RELAY_UNTRUSTED_VALIDATIONS = true; + else if (boost::iequals(strTemp, "trusted")) + RELAY_UNTRUSTED_VALIDATIONS = false; + else + Throw( + "Invalid value specified in [" SECTION_RELAY_VALIDATIONS + "] section"); + } + + if (getSingleSection(secConfig, SECTION_RELAY_PROPOSALS, strTemp, j_)) + { + if (boost::iequals(strTemp, "all")) + RELAY_UNTRUSTED_PROPOSALS = true; + else if (boost::iequals(strTemp, "trusted")) + RELAY_UNTRUSTED_PROPOSALS = false; + else + Throw( + "Invalid value specified in [" SECTION_RELAY_PROPOSALS + "] section"); + } + if (exists(SECTION_VALIDATION_SEED) && exists(SECTION_VALIDATOR_TOKEN)) Throw("Cannot have both [" SECTION_VALIDATION_SEED "] " diff --git a/src/ripple/overlay/impl/PeerImp.cpp b/src/ripple/overlay/impl/PeerImp.cpp index 02ef2347d2d..cd335df26da 100644 --- a/src/ripple/overlay/impl/PeerImp.cpp +++ b/src/ripple/overlay/impl/PeerImp.cpp @@ -2461,25 +2461,15 @@ PeerImp::checkPropose( return; } + bool relay; + if (isTrusted) - { - app_.getOPs().processTrustedProposal(peerPos, packet); - } + relay = app_.getOPs().processTrustedProposal(peerPos); else - { - if (cluster() || - (app_.getOPs().getConsensusLCL() == - peerPos.proposal().prevLedger())) - { - // relay untrusted proposal - JLOG(p_journal_.trace()) << "relaying UNTRUSTED proposal"; - overlay_.relay(*packet, peerPos.suppressionID()); - } - else - { - JLOG(p_journal_.debug()) << "Not relaying UNTRUSTED proposal"; - } - } + relay = app_.config().RELAY_UNTRUSTED_PROPOSALS || cluster(); + + if (relay) + app_.overlay().relay(*packet, peerPos.suppressionID()); } void From b428eff141341aa59ed4fc5119542994212a7b4e Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Wed, 6 May 2020 09:25:56 -0700 Subject: [PATCH 03/10] [FOLD] Address review comments --- src/ripple/app/consensus/RCLConsensus.cpp | 20 +++++++------ src/ripple/app/consensus/RCLValidations.cpp | 9 ++---- src/ripple/app/consensus/RCLValidations.h | 8 ++---- src/ripple/app/misc/NetworkOPs.cpp | 9 ++++-- src/ripple/overlay/Overlay.h | 4 +-- src/ripple/overlay/impl/OverlayImpl.cpp | 32 +++++++++------------ src/ripple/overlay/impl/OverlayImpl.h | 4 +-- src/test/app/AmendmentTable_test.cpp | 1 + src/test/app/RCLValidations_test.cpp | 4 ++- 9 files changed, 43 insertions(+), 48 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index 5b56af5586e..f05497a5332 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -217,7 +217,7 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) app_.getHashRouter().addSuppression(suppression); - app_.overlay().send(prop); + app_.overlay().broadcast(prop); } void @@ -829,14 +829,16 @@ RCLConsensus::Adaptor::validate( app_.getHashRouter().addSuppression( sha512Half(makeSlice(v->getSerialized()))); - if (handleNewValidation(app_, v, "local", true)) - { - Blob validation = v->getSerialized(); - protocol::TMValidation val; - val.set_validation(&validation[0], validation.size()); - // Send signed validation to all of our directly connected peers - app_.overlay().send(val); - } + handleNewValidation(app_, v, "local"); + + // Broadcast to all our peers: + Blob validation = v->getSerialized(); + protocol::TMValidation val; + val.set_validation(&validation[0], validation.size()); + app_.overlay().broadcast(val); + + // Publish to all our subscribers: + app_.getOPs().pubValidation(v); } void diff --git a/src/ripple/app/consensus/RCLValidations.cpp b/src/ripple/app/consensus/RCLValidations.cpp index becec535a22..24952d83537 100644 --- a/src/ripple/app/consensus/RCLValidations.cpp +++ b/src/ripple/app/consensus/RCLValidations.cpp @@ -148,12 +148,11 @@ RCLValidationsAdaptor::acquire(LedgerHash const& hash) return RCLValidatedLedger(std::move(ledger), j_); } -bool +void handleNewValidation( Application& app, std::shared_ptr const& val, - std::string const& source, - bool relayUntrusted) + std::string const& source) { PublicKey const& signingKey = val->getSignerPublic(); uint256 const& hash = val->getLedgerHash(); @@ -218,10 +217,6 @@ handleNewValidation( << toBase58(TokenType::NodePublic, signingKey) << " not added UNlisted"; } - - // We will always forward trusted validations; if configured, we will - // also relay all untrusted validations. - return relayUntrusted || val->isTrusted(); } } // namespace ripple diff --git a/src/ripple/app/consensus/RCLValidations.h b/src/ripple/app/consensus/RCLValidations.h index 7597c76c7e2..9324e0c834b 100644 --- a/src/ripple/app/consensus/RCLValidations.h +++ b/src/ripple/app/consensus/RCLValidations.h @@ -243,16 +243,12 @@ using RCLValidations = Validations; @param app Application object containing validations and ledgerMaster @param val The validation to add @param source Name associated with validation used in logging - @param relayUntrusted whether untrusted validations should be relayed - @return Whether the validation should be relayed */ -[[nodiscard]] -bool +void handleNewValidation( Application& app, std::shared_ptr const& val, - std::string const& source, - bool relayUntrusted = false); + std::string const& source); } // namespace ripple diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 1870b3517b6..28afc7dfb92 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2472,9 +2472,14 @@ NetworkOPsImp::recvValidation( { JLOG(m_journal.debug()) << "recvValidation " << val->getLedgerHash() << " from " << source; + + handleNewValidation(app_, val, source); + pubValidation(val); - return handleNewValidation( - app_, val, source, app_.config().RELAY_UNTRUSTED_VALIDATIONS); + + // We will always relay trusted validations; if configured, we will + // also relay all untrusted validations. + return app_.config().RELAY_UNTRUSTED_VALIDATIONS || val->isTrusted(); } Json::Value diff --git a/src/ripple/overlay/Overlay.h b/src/ripple/overlay/Overlay.h index 3c9bd1f4a50..d5f0c4bae2b 100644 --- a/src/ripple/overlay/Overlay.h +++ b/src/ripple/overlay/Overlay.h @@ -141,11 +141,11 @@ class Overlay : public Stoppable, public beast::PropertyStream::Source /** Broadcast a proposal. */ virtual void - send(protocol::TMProposeSet& m) = 0; + broadcast(protocol::TMProposeSet& m) = 0; /** Broadcast a validation. */ virtual void - send(protocol::TMValidation& m) = 0; + broadcast(protocol::TMValidation& m) = 0; /** Relay a proposal. */ virtual void diff --git a/src/ripple/overlay/impl/OverlayImpl.cpp b/src/ripple/overlay/impl/OverlayImpl.cpp index 883b0a456f5..0efd2426d03 100644 --- a/src/ripple/overlay/impl/OverlayImpl.cpp +++ b/src/ripple/overlay/impl/OverlayImpl.cpp @@ -1134,26 +1134,11 @@ OverlayImpl::findPeerByPublicKey(PublicKey const& pubKey) } void -OverlayImpl::send(protocol::TMProposeSet& m) +OverlayImpl::broadcast(protocol::TMProposeSet& m) { auto const sm = std::make_shared(m, protocol::mtPROPOSE_LEDGER); for_each([&](std::shared_ptr&& p) { p->send(sm); }); } -void -OverlayImpl::send(protocol::TMValidation& m) -{ - auto const sm = std::make_shared(m, protocol::mtVALIDATION); - for_each([&](std::shared_ptr&& p) { p->send(sm); }); - - SerialIter sit(m.validation().data(), m.validation().size()); - auto val = std::make_shared( - std::ref(sit), - [this](PublicKey const& pk) { - return calcNodeID(app_.validatorManifests().getMasterKey(pk)); - }, - false); - app_.getOPs().pubValidation(val); -} void OverlayImpl::relay(protocol::TMProposeSet& m, uint256 const& uid) @@ -1163,18 +1148,27 @@ OverlayImpl::relay(protocol::TMProposeSet& m, uint256 const& uid) auto const sm = std::make_shared(m, protocol::mtPROPOSE_LEDGER); for_each([&](std::shared_ptr&& p) { - if (toSkip->find(p->id()) == toSkip->end()) - p->send(sm); + if (toSkip->find(p->id()) == toSkip->end()) + p->send(sm); }); } } +void +OverlayImpl::broadcast(protocol::TMValidation& m) +{ + + auto const sm = std::make_shared(m, protocol::mtVALIDATION); + for_each([sm](std::shared_ptr&& p) { p->send(sm); }); +} + void OverlayImpl::relay(protocol::TMValidation& m, uint256 const& uid) { if (auto const toSkip = app_.getHashRouter().shouldRelay(uid)) { - auto const sm = std::make_shared(m, protocol::mtVALIDATION); + auto const sm = + std::make_shared(m, protocol::mtVALIDATION); for_each([&](std::shared_ptr&& p) { if (toSkip->find(p->id()) == toSkip->end()) p->send(sm); diff --git a/src/ripple/overlay/impl/OverlayImpl.h b/src/ripple/overlay/impl/OverlayImpl.h index 02b87679ed5..b36004d51fb 100644 --- a/src/ripple/overlay/impl/OverlayImpl.h +++ b/src/ripple/overlay/impl/OverlayImpl.h @@ -201,10 +201,10 @@ class OverlayImpl : public Overlay findPeerByPublicKey(PublicKey const& pubKey) override; void - send(protocol::TMProposeSet& m) override; + broadcast(protocol::TMProposeSet& m) override; void - send(protocol::TMValidation& m) override; + broadcast(protocol::TMValidation& m) override; void relay(protocol::TMProposeSet& m, uint256 const& uid) override; diff --git a/src/test/app/AmendmentTable_test.cpp b/src/test/app/AmendmentTable_test.cpp index f0e9f401a1c..01e08244c1c 100644 --- a/src/test/app/AmendmentTable_test.cpp +++ b/src/test/app/AmendmentTable_test.cpp @@ -400,6 +400,7 @@ class AmendmentTable_test final : public beast::unit_test::suite if (!field.empty()) v.setFieldV256( sfAmendments, STVector256(sfAmendments, field)); + v.setFieldU32(sfLedgerSequence, 6180339); }); validations.emplace_back(v); diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index a84b44c3010..8f2d9645056 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -39,7 +39,9 @@ class RCLValidations_test : public beast::unit_test::suite keys.first, keys.second, calcNodeID(keys.first), - [&](STValidation& v) {}); + [&](STValidation& v) { + v.setFieldU32(sfLedgerSequence, 123456); + }); BEAST_EXPECT(v->isTrusted()); v->setUntrusted(); From f9c581973a1924d8f3c7509a323c448d8f04f7cb Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Wed, 6 May 2020 09:30:19 -0700 Subject: [PATCH 04/10] [FOLD] Fix typo --- src/ripple/app/consensus/RCLValidations.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ripple/app/consensus/RCLValidations.cpp b/src/ripple/app/consensus/RCLValidations.cpp index 24952d83537..0546a86d6b4 100644 --- a/src/ripple/app/consensus/RCLValidations.cpp +++ b/src/ripple/app/consensus/RCLValidations.cpp @@ -191,9 +191,9 @@ handleNewValidation( dmp(j.debug(), to_string(outcome)); // One might think that we would not wish to relay validations that - // fail these checks. Somewhat counterintively, we actually want to - // do it for validations that we receive and deem to be suspicious, - // so that our peers will also observe them and realize they're bad. + // fail these checks. Somewhat counterintuitively, we actually want + // to do it for validations that we receive but deem suspicious, so + // that our peers will also observe them and realize they're bad. if (outcome == ValStatus::conflicting && j.warn()) { dmp(j.warn(), From b18349b57d51db4296cc551324276c18c1fccde1 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Wed, 6 May 2020 09:35:39 -0700 Subject: [PATCH 05/10] [FOLD] Log message matches exception --- src/ripple/protocol/STValidation.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ripple/protocol/STValidation.h b/src/ripple/protocol/STValidation.h index 8e1db26bdd4..4cfde044084 100644 --- a/src/ripple/protocol/STValidation.h +++ b/src/ripple/protocol/STValidation.h @@ -90,9 +90,9 @@ class STValidation final : public STObject, public CountedObject if (!isFieldPresent(sfLedgerSequence)) { - JLOG(debugLog().error()) << "Signature with no ledger sequence: " + JLOG(debugLog().error()) << "No ledger sequence in validation: " << getJson(JsonOptions::none); - Throw("Missing ledger sequence in validation"); + Throw("No ledger sequence in validation"); } nodeID_ = lookupNodeID(PublicKey(makeSlice(spk))); From 396dfd78a864230ec453ceb35cc9633aa26879ba Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Wed, 6 May 2020 10:06:06 -0700 Subject: [PATCH 06/10] [FOLD] clang-format --- src/ripple/overlay/impl/OverlayImpl.cpp | 8 +++----- src/test/app/RCLValidations_test.cpp | 4 +--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/ripple/overlay/impl/OverlayImpl.cpp b/src/ripple/overlay/impl/OverlayImpl.cpp index 0efd2426d03..9531382060a 100644 --- a/src/ripple/overlay/impl/OverlayImpl.cpp +++ b/src/ripple/overlay/impl/OverlayImpl.cpp @@ -1148,8 +1148,8 @@ OverlayImpl::relay(protocol::TMProposeSet& m, uint256 const& uid) auto const sm = std::make_shared(m, protocol::mtPROPOSE_LEDGER); for_each([&](std::shared_ptr&& p) { - if (toSkip->find(p->id()) == toSkip->end()) - p->send(sm); + if (toSkip->find(p->id()) == toSkip->end()) + p->send(sm); }); } } @@ -1157,7 +1157,6 @@ OverlayImpl::relay(protocol::TMProposeSet& m, uint256 const& uid) void OverlayImpl::broadcast(protocol::TMValidation& m) { - auto const sm = std::make_shared(m, protocol::mtVALIDATION); for_each([sm](std::shared_ptr&& p) { p->send(sm); }); } @@ -1167,8 +1166,7 @@ OverlayImpl::relay(protocol::TMValidation& m, uint256 const& uid) { if (auto const toSkip = app_.getHashRouter().shouldRelay(uid)) { - auto const sm = - std::make_shared(m, protocol::mtVALIDATION); + auto const sm = std::make_shared(m, protocol::mtVALIDATION); for_each([&](std::shared_ptr&& p) { if (toSkip->find(p->id()) == toSkip->end()) p->send(sm); diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index 8f2d9645056..5fe4f1aff19 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -39,9 +39,7 @@ class RCLValidations_test : public beast::unit_test::suite keys.first, keys.second, calcNodeID(keys.first), - [&](STValidation& v) { - v.setFieldU32(sfLedgerSequence, 123456); - }); + [&](STValidation& v) { v.setFieldU32(sfLedgerSequence, 123456); }); BEAST_EXPECT(v->isTrusted()); v->setUntrusted(); From 8e1606ea8ce63ded8ee459c2f1cc223fb91981e6 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Wed, 6 May 2020 19:18:11 -0700 Subject: [PATCH 07/10] [FOLD] Fix typo, mark LedgerSequence as required --- cfg/rippled-example.cfg | 2 +- src/ripple/protocol/STValidation.h | 7 -- src/ripple/protocol/impl/STValidation.cpp | 2 +- src/test/app/RCLValidations_test.cpp | 86 +++++++++++++++++++++++ 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 3b56f351b3a..c4917d65044 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -604,7 +604,7 @@ # # [relay_validations] # -# Controls the relaying behavior for proposals received by this server that +# Controls the relaying behavior for validations received by this server that # are issued by validators that are not on the server's UNL. # # Legal values are: "trusted" and "all". The default is "all". diff --git a/src/ripple/protocol/STValidation.h b/src/ripple/protocol/STValidation.h index 4cfde044084..996b56887d2 100644 --- a/src/ripple/protocol/STValidation.h +++ b/src/ripple/protocol/STValidation.h @@ -88,13 +88,6 @@ class STValidation final : public STObject, public CountedObject Throw("Invalid signature in validation"); } - if (!isFieldPresent(sfLedgerSequence)) - { - JLOG(debugLog().error()) << "No ledger sequence in validation: " - << getJson(JsonOptions::none); - Throw("No ledger sequence in validation"); - } - nodeID_ = lookupNodeID(PublicKey(makeSlice(spk))); assert(nodeID_.isNonZero()); } diff --git a/src/ripple/protocol/impl/STValidation.cpp b/src/ripple/protocol/impl/STValidation.cpp index 458b601bf7f..779a031a7ef 100644 --- a/src/ripple/protocol/impl/STValidation.cpp +++ b/src/ripple/protocol/impl/STValidation.cpp @@ -34,7 +34,7 @@ STValidation::validationFormat() static SOTemplate const format{ {sfFlags, soeREQUIRED}, {sfLedgerHash, soeREQUIRED}, - {sfLedgerSequence, soeOPTIONAL}, + {sfLedgerSequence, soeREQUIRED}, {sfCloseTime, soeOPTIONAL}, {sfLoadFee, soeOPTIONAL}, {sfAmendments, soeOPTIONAL}, diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index 5fe4f1aff19..ea93a07ca14 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -20,6 +20,8 @@ #include #include #include +#include +#include #include #include #include @@ -310,6 +312,89 @@ class RCLValidations_test : public beast::unit_test::suite BEAST_EXPECT(trie.branchSupport(ledg_259) == 4); } + void + testLedgerSequence() + { + testcase("Validations with and without the LedgerSequence field"); + + auto const nodeID = + from_hex_text("38ECC15DBD999DE4CE70A6DC69A4166AB18031A7"); + + try + { + std::string const withLedgerSequence = + "228000000126034B9FFF2926460DC55185937F7F41DD7977F21B9DF95FCB61" + "9E5132ABB0D7ADEA0F7CE8A9347871A34250179D85BDE824F57FFE0AC8F89B" + "55FCB89277272A1D83D08ADEC98096A88EF723137321029D19FB0940E5C0D8" + "5873FA711999944A687D129DA5C33E928C2751FC1B31EB3276463044022022" + "6229CF66A678EE021F62CA229BA006B41939845004D3FAF8347C6FFBB7C613" + "02200BE9CD3629FD67C6C672BD433A2769FCDB36B1ECA2292919C58A86224E" + "2BF5970313C13F00C1FC4A53E60AB02C864641002B3172F38677E29C26C540" + "6685179B37E1EDAC157D2D480E006395B76F948E3E07A45A05FE10230D88A7" + "993C71F97AE4B1F2D11F4AFA8FA1BC8827AD4C0F682C03A8B671DCDF6B5C4D" + "E36D44243A684103EF8825BA44241B3BD880770BFA4DA21C71805768318553" + "68CBEC6A3154FDE4A7676E3012E8230864E95A58C60FD61430D7E1B4D33531" + "95F2981DC12B0C7C0950FFAC30CD365592B8EE40489BA01AE2F7555CAC9C98" + "3145871DC82A42A31CF5BAE7D986E83A7D2ECE3AD5FA87AB2195AE015C9504" + "69ABF0B72EAACED318F74886AE9089308AF3B8B10B7192C4E613E1D2E4D9BA" + "64B2EE2D5232402AE82A6A7220D953"; + + if (auto ret = strUnHex(withLedgerSequence); ret) + { + SerialIter sit(makeSlice(*ret)); + + auto val = std::make_shared( + std::ref(sit), + [nodeID](PublicKey const& pk) { return nodeID; }, + false); + + BEAST_EXPECT(val); + BEAST_EXPECT(calcNodeID(val->getSignerPublic()) == nodeID); + BEAST_EXPECT(val->isFieldPresent(sfLedgerSequence)); + } + } + catch (std::exception const& ex) + { + fail(std::string("Unexpected exception thrown: ") + ex.what()); + } + + try + { + std::string const withoutLedgerSequence = + "22800000012926460DC55185937F7F41DD7977F21B9DF95FCB619E5132ABB0" + "D7ADEA0F7CE8A9347871A34250179D85BDE824F57FFE0AC8F89B55FCB89277" + "272A1D83D08ADEC98096A88EF723137321029D19FB0940E5C0D85873FA7119" + "99944A687D129DA5C33E928C2751FC1B31EB3276473045022100BE2EA49CF2" + "FFB7FE7A03F6860B8C35FEA04A064C7023FE28EC97E5A32E85DEC4022003B8" + "5D1D497F504B34F089D5BDB91BD888690C3D3A242A0FEF1DD52875FBA02E03" + "13C13F00C1FC4A53E60AB02C864641002B3172F38677E29C26C5406685179B" + "37E1EDAC157D2D480E006395B76F948E3E07A45A05FE10230D88A7993C71F9" + "7AE4B1F2D11F4AFA8FA1BC8827AD4C0F682C03A8B671DCDF6B5C4DE36D4424" + "3A684103EF8825BA44241B3BD880770BFA4DA21C7180576831855368CBEC6A" + "3154FDE4A7676E3012E8230864E95A58C60FD61430D7E1B4D3353195F2981D" + "C12B0C7C0950FFAC30CD365592B8EE40489BA01AE2F7555CAC9C983145871D" + "C82A42A31CF5BAE7D986E83A7D2ECE3AD5FA87AB2195AE015C950469ABF0B7" + "2EAACED318F74886AE9089308AF3B8B10B7192C4E613E1D2E4D9BA64B2EE2D" + "5232402AE82A6A7220D953"; + + if (auto ret = strUnHex(withoutLedgerSequence); ret) + { + SerialIter sit(makeSlice(*ret)); + + auto val = std::make_shared( + std::ref(sit), + [nodeID](PublicKey const& pk) { return nodeID; }, + false); + + fail("Expected exception not thrown from validation"); + } + } + catch (std::exception const& ex) + { + pass(); + } + } + public: void run() override @@ -317,6 +402,7 @@ class RCLValidations_test : public beast::unit_test::suite testChangeTrusted(); testRCLValidatedLedger(); testLedgerTrieRCLValidatedLedger(); + testLedgerSequence(); } }; From 64300dbcf0df449ff2b8aa8f7556c37bf050adcf Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Thu, 7 May 2020 14:00:41 -0700 Subject: [PATCH 08/10] [FOLD] Improve unit tests and be stricted about required fields --- src/ripple/protocol/STValidation.h | 14 +- src/ripple/protocol/impl/STValidation.cpp | 2 +- src/test/app/RCLValidations_test.cpp | 84 ------ src/test/protocol/STValidation_test.cpp | 311 +++++++++++++++++++--- 4 files changed, 284 insertions(+), 127 deletions(-) diff --git a/src/ripple/protocol/STValidation.h b/src/ripple/protocol/STValidation.h index 996b56887d2..be1537ad04b 100644 --- a/src/ripple/protocol/STValidation.h +++ b/src/ripple/protocol/STValidation.h @@ -122,15 +122,19 @@ class STValidation final : public STObject, public CountedObject // Perform additional initialization f(*this); - // This is a logic error because we should never assemble a - // validation without a ledger sequence number. - if (!isFieldPresent(sfLedgerSequence)) - LogicError("Missing ledger sequence in validation"); - // Finally, sign the validation and mark it as trusted: setFlag(vfFullyCanonicalSig); setFieldVL(sfSignature, signDigest(pk, sk, getSigningHash())); setTrusted(); + + // Check to ensure that all required fields are present. + for (auto const& e : validationFormat()) + { + if (e.style() == soeREQUIRED && !isFieldPresent(e.sField())) + LogicError( + "Required field '" + e.sField().getName() + + "' missing from validation."); + } } STBase* diff --git a/src/ripple/protocol/impl/STValidation.cpp b/src/ripple/protocol/impl/STValidation.cpp index 779a031a7ef..9f13c2be99a 100644 --- a/src/ripple/protocol/impl/STValidation.cpp +++ b/src/ripple/protocol/impl/STValidation.cpp @@ -43,7 +43,7 @@ STValidation::validationFormat() {sfReserveIncrement, soeOPTIONAL}, {sfSigningTime, soeREQUIRED}, {sfSigningPubKey, soeREQUIRED}, - {sfSignature, soeOPTIONAL}, + {sfSignature, soeREQUIRED}, {sfConsensusHash, soeOPTIONAL}, {sfCookie, soeDEFAULT}, {sfValidatedHash, soeOPTIONAL}, diff --git a/src/test/app/RCLValidations_test.cpp b/src/test/app/RCLValidations_test.cpp index ea93a07ca14..eca66a26a88 100644 --- a/src/test/app/RCLValidations_test.cpp +++ b/src/test/app/RCLValidations_test.cpp @@ -312,89 +312,6 @@ class RCLValidations_test : public beast::unit_test::suite BEAST_EXPECT(trie.branchSupport(ledg_259) == 4); } - void - testLedgerSequence() - { - testcase("Validations with and without the LedgerSequence field"); - - auto const nodeID = - from_hex_text("38ECC15DBD999DE4CE70A6DC69A4166AB18031A7"); - - try - { - std::string const withLedgerSequence = - "228000000126034B9FFF2926460DC55185937F7F41DD7977F21B9DF95FCB61" - "9E5132ABB0D7ADEA0F7CE8A9347871A34250179D85BDE824F57FFE0AC8F89B" - "55FCB89277272A1D83D08ADEC98096A88EF723137321029D19FB0940E5C0D8" - "5873FA711999944A687D129DA5C33E928C2751FC1B31EB3276463044022022" - "6229CF66A678EE021F62CA229BA006B41939845004D3FAF8347C6FFBB7C613" - "02200BE9CD3629FD67C6C672BD433A2769FCDB36B1ECA2292919C58A86224E" - "2BF5970313C13F00C1FC4A53E60AB02C864641002B3172F38677E29C26C540" - "6685179B37E1EDAC157D2D480E006395B76F948E3E07A45A05FE10230D88A7" - "993C71F97AE4B1F2D11F4AFA8FA1BC8827AD4C0F682C03A8B671DCDF6B5C4D" - "E36D44243A684103EF8825BA44241B3BD880770BFA4DA21C71805768318553" - "68CBEC6A3154FDE4A7676E3012E8230864E95A58C60FD61430D7E1B4D33531" - "95F2981DC12B0C7C0950FFAC30CD365592B8EE40489BA01AE2F7555CAC9C98" - "3145871DC82A42A31CF5BAE7D986E83A7D2ECE3AD5FA87AB2195AE015C9504" - "69ABF0B72EAACED318F74886AE9089308AF3B8B10B7192C4E613E1D2E4D9BA" - "64B2EE2D5232402AE82A6A7220D953"; - - if (auto ret = strUnHex(withLedgerSequence); ret) - { - SerialIter sit(makeSlice(*ret)); - - auto val = std::make_shared( - std::ref(sit), - [nodeID](PublicKey const& pk) { return nodeID; }, - false); - - BEAST_EXPECT(val); - BEAST_EXPECT(calcNodeID(val->getSignerPublic()) == nodeID); - BEAST_EXPECT(val->isFieldPresent(sfLedgerSequence)); - } - } - catch (std::exception const& ex) - { - fail(std::string("Unexpected exception thrown: ") + ex.what()); - } - - try - { - std::string const withoutLedgerSequence = - "22800000012926460DC55185937F7F41DD7977F21B9DF95FCB619E5132ABB0" - "D7ADEA0F7CE8A9347871A34250179D85BDE824F57FFE0AC8F89B55FCB89277" - "272A1D83D08ADEC98096A88EF723137321029D19FB0940E5C0D85873FA7119" - "99944A687D129DA5C33E928C2751FC1B31EB3276473045022100BE2EA49CF2" - "FFB7FE7A03F6860B8C35FEA04A064C7023FE28EC97E5A32E85DEC4022003B8" - "5D1D497F504B34F089D5BDB91BD888690C3D3A242A0FEF1DD52875FBA02E03" - "13C13F00C1FC4A53E60AB02C864641002B3172F38677E29C26C5406685179B" - "37E1EDAC157D2D480E006395B76F948E3E07A45A05FE10230D88A7993C71F9" - "7AE4B1F2D11F4AFA8FA1BC8827AD4C0F682C03A8B671DCDF6B5C4DE36D4424" - "3A684103EF8825BA44241B3BD880770BFA4DA21C7180576831855368CBEC6A" - "3154FDE4A7676E3012E8230864E95A58C60FD61430D7E1B4D3353195F2981D" - "C12B0C7C0950FFAC30CD365592B8EE40489BA01AE2F7555CAC9C983145871D" - "C82A42A31CF5BAE7D986E83A7D2ECE3AD5FA87AB2195AE015C950469ABF0B7" - "2EAACED318F74886AE9089308AF3B8B10B7192C4E613E1D2E4D9BA64B2EE2D" - "5232402AE82A6A7220D953"; - - if (auto ret = strUnHex(withoutLedgerSequence); ret) - { - SerialIter sit(makeSlice(*ret)); - - auto val = std::make_shared( - std::ref(sit), - [nodeID](PublicKey const& pk) { return nodeID; }, - false); - - fail("Expected exception not thrown from validation"); - } - } - catch (std::exception const& ex) - { - pass(); - } - } - public: void run() override @@ -402,7 +319,6 @@ class RCLValidations_test : public beast::unit_test::suite testChangeTrusted(); testRCLValidatedLedger(); testLedgerTrieRCLValidatedLedger(); - testLedgerSequence(); } }; diff --git a/src/test/protocol/STValidation_test.cpp b/src/test/protocol/STValidation_test.cpp index 716936cc309..e816408750d 100644 --- a/src/test/protocol/STValidation_test.cpp +++ b/src/test/protocol/STValidation_test.cpp @@ -18,7 +18,9 @@ //============================================================================== #include +#include #include +#include #include #include #include @@ -33,79 +35,314 @@ namespace ripple { class STValidation_test : public beast::unit_test::suite { + // No public key: + static constexpr std::uint8_t payload1[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x29, 0x26, + 0x47, 0x31, 0x1A, 0x3A, 0x4E, 0x69, 0x6B, 0x2B, 0x54, 0x69, 0x66, 0x66, + 0x51, 0x53, 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, 0x16, 0xF8, 0x3E, + 0xEA, 0x5C, 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, 0x8C, 0xDE, 0xB8, + 0x79, 0x39, 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, 0x50, 0x17, 0x21, + 0x0B, 0xAB, 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, 0x1E, 0x07, 0xB4, + 0xFB, 0xC5, 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, 0x35, 0x28, 0xEB, + 0xD9, 0x06, 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x76, 0x47, 0x30, 0x45, 0x02, + 0x21, 0x00, 0xAF, 0x1D, 0x17, 0xA2, 0x12, 0x7B, 0xA4, 0x6B, 0x40, 0xBD, + 0x58, 0x76, 0x39, 0x3F, 0xF4, 0x49, 0x6B, 0x25, 0xA1, 0xAD, 0xB7, 0x36, + 0xFB, 0x64, 0x4C, 0x05, 0x21, 0x0C, 0x43, 0x02, 0xE5, 0xEE, 0x02, 0x20, + 0x26, 0x01, 0x7C, 0x5F, 0x69, 0xDA, 0xD1, 0xC3, 0x28, 0xED, 0x80, 0x05, + 0x36, 0x86, 0x8B, 0x1B, 0x22, 0xE4, 0x8E, 0x09, 0x11, 0x52, 0x28, 0x5A, + 0x48, 0x8F, 0x98, 0x7A, 0x5A, 0x10, 0x74, 0xCC}; + + // Short public key: + static constexpr std::uint8_t payload2[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x29, 0x26, + 0x47, 0x31, 0x1A, 0x51, 0x53, 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, + 0x16, 0xF8, 0x3E, 0xEA, 0x5C, 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, + 0x8C, 0xDE, 0xB8, 0x79, 0x39, 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, + 0x50, 0x17, 0x21, 0x0B, 0xAB, 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, + 0x1E, 0x07, 0xB4, 0xFB, 0xC5, 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, + 0x35, 0x28, 0xEB, 0xD9, 0x06, 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x20, + 0x02, 0x9D, 0x19, 0xFB, 0x09, 0x40, 0xE5, 0xC0, 0xD8, 0x58, 0x73, 0xFA, + 0x71, 0x19, 0x99, 0x94, 0x4A, 0x68, 0x7D, 0x12, 0x9D, 0xA5, 0xC3, 0x3E, + 0x92, 0x8C, 0x27, 0x51, 0xFC, 0x1B, 0x31, 0xEB, 0x76, 0x46, 0x30, 0x44, + 0x02, 0x20, 0x34, 0x89, 0xA3, 0xBF, 0xA9, 0x97, 0x13, 0xBC, 0x87, 0x61, + 0xC5, 0x2B, 0x7F, 0xAA, 0xE9, 0x31, 0x4C, 0xCD, 0x6F, 0x57, 0x68, 0x70, + 0xC8, 0xDC, 0x58, 0x76, 0x91, 0x2F, 0x70, 0x2F, 0xD0, 0x78, 0x02, 0x20, + 0x7E, 0x57, 0x9D, 0xCA, 0x11, 0xF1, 0x3B, 0xA0, 0x39, 0x38, 0x37, 0x40, + 0xC5, 0xC8, 0xFE, 0xC1, 0xFC, 0xE9, 0xE7, 0x84, 0x6C, 0x2D, 0x47, 0x6E, + 0xD7, 0xFF, 0x83, 0x9D, 0xEF, 0x7D, 0xF7, 0x6A}; + + // Long public key: + static constexpr std::uint8_t payload3[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x29, 0x26, + 0x47, 0x31, 0x1A, 0x51, 0x53, 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, + 0x16, 0xF8, 0x3E, 0xEA, 0x5C, 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, + 0x8C, 0xDE, 0xB8, 0x79, 0x39, 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, + 0x50, 0x17, 0x21, 0x0B, 0xAB, 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, + 0x1E, 0x07, 0xB4, 0xFB, 0xC5, 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, + 0x35, 0x28, 0xEB, 0xD9, 0x06, 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x22, + 0x02, 0x9D, 0x19, 0xFB, 0x09, 0x40, 0xE5, 0xC0, 0xD8, 0x58, 0x73, 0xFA, + 0x71, 0x19, 0x99, 0x94, 0x4A, 0x68, 0x7D, 0x12, 0x9D, 0xA5, 0xC3, 0x3E, + 0x92, 0x8C, 0x27, 0x51, 0xFC, 0x1B, 0x31, 0xEB, 0x32, 0x78, 0x76, 0x46, + 0x30, 0x44, 0x02, 0x20, 0x3C, 0xAB, 0xEE, 0x36, 0xD8, 0xF3, 0x74, 0x5F, + 0x50, 0x28, 0x66, 0x17, 0x57, 0x26, 0x6A, 0xBD, 0x9A, 0x19, 0x08, 0xAA, + 0x65, 0x94, 0x0B, 0xDF, 0x24, 0x20, 0x44, 0x99, 0x05, 0x8C, 0xB7, 0x3D, + 0x02, 0x20, 0x79, 0x66, 0xE6, 0xCC, 0xA2, 0x5E, 0x15, 0xFE, 0x18, 0x4B, + 0xB2, 0xA8, 0x01, 0x3A, 0xD6, 0x63, 0x54, 0x08, 0x1B, 0xDA, 0xD0, 0x04, + 0xEF, 0x4C, 0x73, 0xB3, 0xFF, 0xFE, 0xA9, 0x8E, 0x92, 0xE8}; + + // Ed25519 public key: + static constexpr std::uint8_t payload4[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x29, 0x26, + 0x47, 0x31, 0x1A, 0x51, 0x53, 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, + 0x16, 0xF8, 0x3E, 0xEA, 0x5C, 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, + 0x8C, 0xDE, 0xB8, 0x79, 0x39, 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, + 0x50, 0x17, 0x21, 0x0B, 0xAB, 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, + 0x1E, 0x07, 0xB4, 0xFB, 0xC5, 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, + 0x35, 0x28, 0xEB, 0xD9, 0x06, 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x21, + 0xED, 0x04, 0x8B, 0x9A, 0x31, 0x5E, 0xC7, 0x33, 0xC0, 0x15, 0x3B, 0x67, + 0x04, 0x73, 0x7A, 0x91, 0x3D, 0xEF, 0x57, 0x1D, 0xAD, 0xEC, 0x57, 0xE5, + 0x91, 0x5D, 0x55, 0xD9, 0x32, 0x9D, 0x45, 0x12, 0x85, 0x76, 0x40, 0x52, + 0x07, 0xF9, 0x0D, 0x18, 0x2B, 0xB7, 0xAF, 0x5D, 0x43, 0xF8, 0xF9, 0xC5, + 0xAD, 0xF9, 0xBA, 0x33, 0x23, 0xC0, 0x2F, 0x95, 0xFF, 0x36, 0x94, 0xD8, + 0x99, 0x99, 0xE0, 0x66, 0xF8, 0xB6, 0x27, 0x22, 0xFD, 0x29, 0x39, 0x30, + 0x39, 0xAB, 0x93, 0xDB, 0x9D, 0x2C, 0xE5, 0xF0, 0x4C, 0xB7, 0x30, 0xFD, + 0xC7, 0xD3, 0x21, 0xC9, 0x4E, 0x0D, 0x8A, 0x1B, 0xB2, 0x89, 0x97, 0x10, + 0x7E, 0x84, 0x09}; + + // No ledger sequence: + static constexpr std::uint8_t payload5[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x29, 0x26, 0x47, 0x31, 0x1A, 0x51, 0x53, + 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, 0x16, 0xF8, 0x3E, 0xEA, 0x5C, + 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, 0x8C, 0xDE, 0xB8, 0x79, 0x39, + 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, 0x50, 0x17, 0x21, 0x0B, 0xAB, + 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, 0x1E, 0x07, 0xB4, 0xFB, 0xC5, + 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, 0x35, 0x28, 0xEB, 0xD9, 0x06, + 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x21, 0x02, 0x9D, 0x19, 0xFB, 0x09, + 0x40, 0xE5, 0xC0, 0xD8, 0x58, 0x73, 0xFA, 0x71, 0x19, 0x99, 0x94, 0x4A, + 0x68, 0x7D, 0x12, 0x9D, 0xA5, 0xC3, 0x3E, 0x92, 0x8C, 0x27, 0x51, 0xFC, + 0x1B, 0x31, 0xEB, 0x32, 0x76, 0x47, 0x30, 0x45, 0x02, 0x21, 0x00, 0x83, + 0xB3, 0x1B, 0xE9, 0x03, 0x8F, 0x4A, 0x92, 0x8B, 0x9B, 0x51, 0xEF, 0x79, + 0xED, 0xA1, 0x4A, 0x58, 0x9B, 0x20, 0xCF, 0x89, 0xC4, 0x75, 0x99, 0x5F, + 0x6D, 0x79, 0x51, 0x79, 0x07, 0xF9, 0x93, 0x02, 0x20, 0x39, 0xA6, 0x0C, + 0x77, 0x68, 0x84, 0x50, 0xDB, 0xDA, 0x64, 0x32, 0x74, 0xEC, 0x63, 0x48, + 0x48, 0x96, 0xB5, 0x94, 0x57, 0x55, 0x8D, 0x7D, 0xD8, 0x25, 0x78, 0xD1, + 0xEA, 0x5F, 0xD9, 0xC7, 0xAA}; + + // No sign time: + static constexpr std::uint8_t payload6[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x51, 0x53, + 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, 0x16, 0xF8, 0x3E, 0xEA, 0x5C, + 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, 0x8C, 0xDE, 0xB8, 0x79, 0x39, + 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, 0x50, 0x17, 0x21, 0x0B, 0xAB, + 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, 0x1E, 0x07, 0xB4, 0xFB, 0xC5, + 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, 0x35, 0x28, 0xEB, 0xD9, 0x06, + 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x21, 0x02, 0x9D, 0x19, 0xFB, 0x09, + 0x40, 0xE5, 0xC0, 0xD8, 0x58, 0x73, 0xFA, 0x71, 0x19, 0x99, 0x94, 0x4A, + 0x68, 0x7D, 0x12, 0x9D, 0xA5, 0xC3, 0x3E, 0x92, 0x8C, 0x27, 0x51, 0xFC, + 0x1B, 0x31, 0xEB, 0x32, 0x76, 0x47, 0x30, 0x45, 0x02, 0x21, 0x00, 0xDD, + 0xB0, 0x59, 0x9A, 0x02, 0x3E, 0xF2, 0x44, 0xCE, 0x1D, 0xA8, 0x99, 0x06, + 0xF3, 0x8A, 0x4B, 0xEB, 0x95, 0x42, 0x63, 0x6A, 0x6C, 0x04, 0x30, 0x7F, + 0x62, 0x78, 0x3A, 0x89, 0xB0, 0x3F, 0x22, 0x02, 0x20, 0x4E, 0x6A, 0x55, + 0x63, 0x8A, 0x19, 0xED, 0xFE, 0x70, 0x34, 0xD1, 0x30, 0xED, 0x7C, 0xAF, + 0xB2, 0x78, 0xBB, 0x15, 0x6C, 0x42, 0x3E, 0x19, 0x5D, 0xEA, 0xC5, 0x5E, + 0x23, 0xE2, 0x14, 0x80, 0x54}; + + // No signature field: + static constexpr std::uint8_t payload7[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x29, 0x26, + 0x47, 0x31, 0x1A, 0x51, 0x53, 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, + 0x16, 0xF8, 0x3E, 0xEA, 0x5C, 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, + 0x8C, 0xDE, 0xB8, 0x79, 0x39, 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, + 0x50, 0x17, 0x21, 0x0B, 0xAB, 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, + 0x1E, 0x07, 0xB4, 0xFB, 0xC5, 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, + 0x35, 0x28, 0xEB, 0xD9, 0x06, 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x21, + 0x02, 0x9D, 0x19, 0xFB, 0x09, 0x40, 0xE5, 0xC0, 0xD8, 0x58, 0x73, 0xFA, + 0x71, 0x19, 0x99, 0x94, 0x4A, 0x68, 0x7D, 0x12, 0x9D, 0xA5, 0xC3, 0x3E, + 0x92, 0x8C, 0x27, 0x51, 0xFC, 0x1B, 0x31, 0xEB, 0x32}; + + // Good: + static constexpr std::uint8_t payload8[] = { + 0x22, 0x80, 0x00, 0x00, 0x01, 0x26, 0x03, 0x4B, 0xEA, 0x97, 0x29, 0x26, + 0x47, 0x31, 0x1A, 0x51, 0x53, 0x1F, 0x1A, 0x4E, 0xBB, 0x43, 0x19, 0x69, + 0x16, 0xF8, 0x3E, 0xEA, 0x5C, 0x77, 0x94, 0x08, 0x19, 0x0B, 0x4B, 0x40, + 0x8C, 0xDE, 0xB8, 0x79, 0x39, 0xF3, 0x9D, 0x66, 0x7B, 0x12, 0xCA, 0x97, + 0x50, 0x17, 0x21, 0x0B, 0xAB, 0xBC, 0x8C, 0xB7, 0xFB, 0x45, 0x49, 0xED, + 0x1E, 0x07, 0xB4, 0xFB, 0xC5, 0xF2, 0xFB, 0x67, 0x2D, 0x18, 0xA6, 0x43, + 0x35, 0x28, 0xEB, 0xD9, 0x06, 0x3E, 0xB3, 0x8B, 0xC2, 0xE0, 0x73, 0x21, + 0x02, 0x9D, 0x19, 0xFB, 0x09, 0x40, 0xE5, 0xC0, 0xD8, 0x58, 0x73, 0xFA, + 0x71, 0x19, 0x99, 0x94, 0x4A, 0x68, 0x7D, 0x12, 0x9D, 0xA5, 0xC3, 0x3E, + 0x92, 0x8C, 0x27, 0x51, 0xFC, 0x1B, 0x31, 0xEB, 0x32, 0x76, 0x47, 0x30, + 0x45, 0x02, 0x21, 0x00, 0xDD, 0x29, 0xDC, 0xAC, 0x82, 0x5E, 0xF9, 0xE2, + 0x2D, 0x26, 0x03, 0x95, 0xC2, 0x11, 0x3A, 0x2A, 0x83, 0xEE, 0xA0, 0x2B, + 0x9F, 0x2A, 0x51, 0xBD, 0x6B, 0xF7, 0x83, 0xCE, 0x4A, 0x7C, 0x52, 0x29, + 0x02, 0x20, 0x52, 0x45, 0xB9, 0x07, 0x57, 0xEF, 0xB2, 0x6C, 0x69, 0xC5, + 0x47, 0xCA, 0xE2, 0x76, 0x00, 0xFC, 0x35, 0x46, 0x5D, 0x19, 0x64, 0xCE, + 0xCA, 0x88, 0xA1, 0x2A, 0x20, 0xCF, 0x3C, 0xF9, 0xCE, 0xCF}; + public: void testDeserialization() { testcase("Deserialization"); - constexpr std::uint8_t payload1[] = { - // specifies an Ed25519 public key. - 0x22, 0x00, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00, 0x00, - 0x51, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x2a, 0x73, 0x21, 0xed, 0x78, 0x00, 0xe6, 0x73, - 0x00, 0x72, 0x00, 0x3c, 0x00, 0x00, 0x00, 0x88, 0x00, 0xe6, - 0x73, 0x38, 0x00, 0x00, 0x8a, 0x00, 0x88, 0x4e, 0x31, 0x30, - 0x5f, 0x5f, 0x63, 0x78, 0x78, 0x61, 0x62, 0x69}; - - constexpr std::uint8_t payload2[] = { - 0x22, 0x00, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00, 0x00, - 0x51, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x2a, 0x73, 0x21, 0xed, 0xff, 0x03, 0x1c, 0xbe, - 0x65, 0x22, 0x61, 0x9c, 0x5e, 0x13, 0x12, 0x00, 0x3b, 0x43, - 0x00, 0x00, 0x00, 0xf7, 0x3f, 0x3f, 0x3f, 0x3f, 0x3f, 0x3f, - 0x3f, 0x3f, 0x13, 0x13, 0x13, 0x3a, 0x27, 0xff}; - - constexpr std::uint8_t payload3[] = { - // Has no public key at all - 0x22, 0x00, 0x00, 0x00, 0x00, 0x29, 0x00, 0x00, 0x00, 0x00, 0x51, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x2a}; + try + { + SerialIter sit{payload8}; + + auto val = std::make_shared( + sit, [](PublicKey const& pk) { return calcNodeID(pk); }, true); + + BEAST_EXPECT(val); + BEAST_EXPECT(val->isFieldPresent(sfLedgerSequence)); + BEAST_EXPECT(val->isFieldPresent(sfSigningPubKey)); + BEAST_EXPECT(val->isFieldPresent(sfSigningTime)); + BEAST_EXPECT(val->isFieldPresent(sfFlags)); + BEAST_EXPECT(val->isFieldPresent(sfLedgerHash)); + BEAST_EXPECT(val->isFieldPresent(sfSignature)); + } + catch (std::exception const& ex) + { + fail(std::string("Unexpected exception thrown: ") + ex.what()); + } + + testcase("Deserialization: Public Key Tests"); try { SerialIter sit{payload1}; - auto stx = std::make_shared( + auto val = std::make_shared( sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); fail("An exception should have been thrown"); } - catch (std::exception const& e) + catch (std::exception const& ex) { BEAST_EXPECT( - strcmp(e.what(), "Invalid public key in validation") == 0); + strcmp( + ex.what(), + "Field 'SigningPubKey' is required but missing.") == 0); } try { SerialIter sit{payload2}; - auto stx = std::make_shared( + auto val = std::make_shared( sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); fail("An exception should have been thrown"); } - catch (std::exception const& e) + catch (std::exception const& ex) { BEAST_EXPECT( - strcmp(e.what(), "Invalid public key in validation") == 0); + strcmp(ex.what(), "Invalid public key in validation") == 0); } try { SerialIter sit{payload3}; - auto stx = std::make_shared( + auto val = std::make_shared( sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); fail("An exception should have been thrown"); } - catch (std::exception const& e) + catch (std::exception const& ex) + { + BEAST_EXPECT( + strcmp(ex.what(), "Invalid public key in validation") == 0); + } + + try + { + SerialIter sit{payload4}; + auto val = std::make_shared( + sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + fail("An exception should have been thrown"); + } + catch (std::exception const& ex) + { + BEAST_EXPECT( + strcmp(ex.what(), "Invalid public key in validation") == 0); + } + + testcase("Deserialization: Missing Fields"); + + try + { + SerialIter sit{payload5}; + auto val = std::make_shared( + sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + fail("Expected exception not thrown from validation"); + } + catch (std::exception const& ex) { BEAST_EXPECT( strcmp( - e.what(), - "Field 'SigningPubKey' is required but missing.") == 0); + ex.what(), + "Field 'LedgerSequence' is required but missing.") == 0); + } + + try + { + SerialIter sit{payload6}; + auto val = std::make_shared( + sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + fail("Expected exception not thrown from validation"); + } + catch (std::exception const& ex) + { + BEAST_EXPECT( + strcmp( + ex.what(), + "Field 'SigningTime' is required but missing.") == 0); + } + + try + { + SerialIter sit{payload7}; + + auto val = std::make_shared( + sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + + fail("Expected exception not thrown from validation"); + } + catch (std::exception const& ex) + { + BEAST_EXPECT( + strcmp( + ex.what(), "Field 'Signature' is required but missing.") == + 0); + } + + testcase("Deserialization: Corrupted Data / Fuzzing"); + + // Mutate a known-good validation and expect it to fail: + std::vector v; + for (auto c : payload8) + v.push_back(c); + + beast::xor_shift_engine g(148979842); + + for (std::size_t i = 0; i != v.size(); ++i) + { + auto v2 = v; + + while (v2[i] == v[i]) + v2[i] = rand_int(g); + + try + { + SerialIter sit{makeSlice(v2)}; + + auto val = std::make_shared( + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + true); + + fail( + "Mutated validation signature checked out: offset=" + + std::to_string(i)); + } + catch (std::exception const& ex) + { + pass(); + } } } From f6c4a58a47f7bcc1b3c3bd9e1417399664216846 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Thu, 7 May 2020 21:56:53 -0700 Subject: [PATCH 09/10] [FOLD] Make MSVC happy --- src/test/protocol/STValidation_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/protocol/STValidation_test.cpp b/src/test/protocol/STValidation_test.cpp index e816408750d..539494354dc 100644 --- a/src/test/protocol/STValidation_test.cpp +++ b/src/test/protocol/STValidation_test.cpp @@ -339,7 +339,7 @@ class STValidation_test : public beast::unit_test::suite "Mutated validation signature checked out: offset=" + std::to_string(i)); } - catch (std::exception const& ex) + catch (std::exception const&) { pass(); } From b62b3158961597674556b418c56d84057177891c Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 8 May 2020 01:31:09 -0700 Subject: [PATCH 10/10] [FOLD] TIL: std::uniform_int_distribution causes UB if used with std::uint8_t. Thanks MSVC! --- src/ripple/basics/random.h | 27 +++++++++++++++++++++++++ src/test/protocol/STValidation_test.cpp | 2 +- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/ripple/basics/random.h b/src/ripple/basics/random.h index e149503a005..7ea8972eab0 100644 --- a/src/ripple/basics/random.h +++ b/src/ripple/basics/random.h @@ -162,6 +162,33 @@ rand_int() } /** @} */ +/** Return a random byte */ +/** @{ */ +template +std::enable_if_t< + (std::is_same::value || + std::is_same::value) && + detail::is_engine::value, + Byte> +rand_byte(Engine& engine) +{ + return static_cast(rand_int( + engine, + std::numeric_limits::min(), + std::numeric_limits::max())); +} + +template +std::enable_if_t< + (std::is_same::value || + std::is_same::value), + Byte> +rand_byte() +{ + return rand_byte(default_prng()); +} +/** @} */ + /** Return a random boolean value */ /** @{ */ template diff --git a/src/test/protocol/STValidation_test.cpp b/src/test/protocol/STValidation_test.cpp index 539494354dc..3435db22ca7 100644 --- a/src/test/protocol/STValidation_test.cpp +++ b/src/test/protocol/STValidation_test.cpp @@ -324,7 +324,7 @@ class STValidation_test : public beast::unit_test::suite auto v2 = v; while (v2[i] == v[i]) - v2[i] = rand_int(g); + v2[i] = rand_byte(g); try {