diff --git a/include/xrpl/shamap/SHAMapAddNode.h b/include/xrpl/shamap/SHAMapAddNode.h index 87222f74d8..6af856a52b 100644 --- a/include/xrpl/shamap/SHAMapAddNode.h +++ b/include/xrpl/shamap/SHAMapAddNode.h @@ -14,35 +14,112 @@ private: public: SHAMapAddNode(); + + /** + * Record one node that was rejected. + */ void incInvalid(); + + /** + * Record one node that produced a good result. + */ void incUseful(); + + /** + * Record one node that was not needed: the map already held it, or the map + * or its acquisition was no longer taking nodes. isGood() counts it on the + * accepted side. + */ void incDuplicate(); - void - reset(); + + /** + * @return How many nodes produced a good result. + */ [[nodiscard]] int getGood() const; + + /** + * @return How many nodes this code rejected, which is not how many the + * batch was given: a batch that stops at its first bad node counts + * one, and a batch that carries on counts each. + */ + [[nodiscard]] int + getBad() const; + + /** + * @return How many nodes were not needed, whether already held or offered + * to a map no longer taking nodes, tallied apart from the good and + * bad counts. + */ + [[nodiscard]] int + getDuplicate() const; + + /** + * Whether nodes that produced a good result or were not needed outnumber + * the ones rejected. + * + * @return Whether the batch was good. + */ [[nodiscard]] bool isGood() const; + + /** + * @return Whether at least one node in the batch was rejected. + */ [[nodiscard]] bool isInvalid() const; + + /** + * @return Whether at least one node in the batch produced a good result. + */ [[nodiscard]] bool isUseful() const; + + /** + * @return A verdict recording one node that was not needed. + */ + static SHAMapAddNode + duplicate(); + + /** + * @return A verdict recording one useful node. + */ + static SHAMapAddNode + useful(); + + /** + * @return A verdict recording one invalid node. + */ + static SHAMapAddNode + invalid(); + + /** + * Clear every count back to zero. + */ + void + reset(); + + /** + * Render the tally as a log line. + * + * @return The tally, e.g. "good:2 bad:1 dupe:1", or "no nodes processed" if + * every count is zero. + */ [[nodiscard]] std::string get() const; + /** + * Add another verdict's counts into this one. + * + * @param n The verdict to add. + * @return This verdict, updated. + */ SHAMapAddNode& operator+=(SHAMapAddNode const& n); - static SHAMapAddNode - duplicate(); - static SHAMapAddNode - useful(); - static SHAMapAddNode - invalid(); - private: SHAMapAddNode(int good, int bad, int duplicate); }; @@ -74,18 +151,30 @@ SHAMapAddNode::incDuplicate() ++duplicate_; } -inline void -SHAMapAddNode::reset() -{ - good_ = bad_ = duplicate_ = 0; -} - inline int SHAMapAddNode::getGood() const { return good_; } +inline int +SHAMapAddNode::getBad() const +{ + return bad_; +} + +inline int +SHAMapAddNode::getDuplicate() const +{ + return duplicate_; +} + +inline bool +SHAMapAddNode::isGood() const +{ + return (good_ + duplicate_) > bad_; +} + inline bool SHAMapAddNode::isInvalid() const { @@ -98,22 +187,6 @@ SHAMapAddNode::isUseful() const return good_ > 0; } -inline SHAMapAddNode& -SHAMapAddNode::operator+=(SHAMapAddNode const& n) -{ - good_ += n.good_; - bad_ += n.bad_; - duplicate_ += n.duplicate_; - - return *this; -} - -inline bool -SHAMapAddNode::isGood() const -{ - return (good_ + duplicate_) > bad_; -} - inline SHAMapAddNode SHAMapAddNode::duplicate() { @@ -132,6 +205,12 @@ SHAMapAddNode::invalid() return SHAMapAddNode(0, 1, 0); } +inline void +SHAMapAddNode::reset() +{ + good_ = bad_ = duplicate_ = 0; +} + inline std::string SHAMapAddNode::get() const { @@ -160,4 +239,14 @@ SHAMapAddNode::get() const return ret; } +inline SHAMapAddNode& +SHAMapAddNode::operator+=(SHAMapAddNode const& n) +{ + good_ += n.good_; + bad_ += n.bad_; + duplicate_ += n.duplicate_; + + return *this; +} + } // namespace xrpl diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index c191cfab7c..707b6f081e 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -451,7 +452,7 @@ canWithdraw( if (toSle->isFlag(lsfDepositAuth)) { - if (!view.exists(keylet::depositPreauth(to, from))) + if (!DepositPreauthEntryR(to, from, view)) { if (credentialIDs.has_value()) { diff --git a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp index 90880446e6..78177edb43 100644 --- a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -266,7 +267,7 @@ authorizedDepositPreauth(ReadView const& view, STVector256 const& credIDs, Accou lifeExtender.push_back(std::move(sleCred)); } - if (!view.exists(keylet::depositPreauth(dst, sorted))) + if (!DepositPreauthEntryR(dst, sorted, view)) return tecNO_PERMISSION; return tesSUCCESS; @@ -386,7 +387,7 @@ checkDepositPreauth( { if (src != dst) { - if (!view.exists(keylet::depositPreauth(dst, src))) + if (!DepositPreauthEntryR(dst, src, view)) { return !tx.isFieldPresent(sfCredentialIDs) ? tecNO_PERMISSION diff --git a/src/libxrpl/tx/transactors/account/AccountDelete.cpp b/src/libxrpl/tx/transactors/account/AccountDelete.cpp index 6739d5363c..01e0122f28 100644 --- a/src/libxrpl/tx/transactors/account/AccountDelete.cpp +++ b/src/libxrpl/tx/transactors/account/AccountDelete.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -245,7 +246,7 @@ AccountDelete::preclaim(PreclaimContext const& ctx) // lsfDepositAuth flag set by default if (sleDst->isFlag(lsfDepositAuth)) { - if (!ctx.view.exists(keylet::depositPreauth(dst, account))) + if (!DepositPreauthEntryR(dst, account, ctx.view)) return tecNO_PERMISSION; } } diff --git a/src/libxrpl/tx/transactors/bridge/XChainBridge.cpp b/src/libxrpl/tx/transactors/bridge/XChainBridge.cpp index ccae913c17..dffce23a22 100644 --- a/src/libxrpl/tx/transactors/bridge/XChainBridge.cpp +++ b/src/libxrpl/tx/transactors/bridge/XChainBridge.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -418,7 +419,7 @@ transferHelper( dst == claimOwner && depositAuthPolicy == DepositAuthPolicy::DstCanBypass; if (!canBypassDepositAuth && sleDst->isFlag(lsfDepositAuth) && - !psb.exists(keylet::depositPreauth(dst, src))) + !DepositPreauthEntryR(dst, src, psb)) { return tecNO_PERMISSION; } diff --git a/src/libxrpl/tx/transactors/payment/DepositPreauth.cpp b/src/libxrpl/tx/transactors/payment/DepositPreauth.cpp index 4d8c04be15..97c1dcb7e7 100644 --- a/src/libxrpl/tx/transactors/payment/DepositPreauth.cpp +++ b/src/libxrpl/tx/transactors/payment/DepositPreauth.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -115,13 +116,13 @@ DepositPreauth::preclaim(PreclaimContext const& ctx) // Verify that the Preauth entry they asked to add is not already // in the ledger. - if (ctx.view.exists(keylet::depositPreauth(account, auth))) + if (DepositPreauthEntryR(account, auth, ctx.view)) return tecDUPLICATE; } else if (ctx.tx.isFieldPresent(sfUnauthorize)) { // Verify that the Preauth entry they asked to remove is in the ledger. - if (!ctx.view.exists(keylet::depositPreauth(account, ctx.tx[sfUnauthorize]))) + if (!DepositPreauthEntryR(account, ctx.tx[sfUnauthorize], ctx.view)) return tecNO_ENTRY; } else if (ctx.tx.isFieldPresent(sfAuthorizeCredentials)) @@ -140,16 +141,16 @@ DepositPreauth::preclaim(PreclaimContext const& ctx) // Verify that the Preauth entry they asked to add is not already // in the ledger. - if (ctx.view.exists(keylet::depositPreauth(account, sorted))) + if (DepositPreauthEntryR(account, sorted, ctx.view)) return tecDUPLICATE; } else if (ctx.tx.isFieldPresent(sfUnauthorizeCredentials)) { // Verify that the Preauth entry is in the ledger. - if (!ctx.view.exists( - keylet::depositPreauth( - account, - credentials::makeSorted(ctx.tx.getFieldArray(sfUnauthorizeCredentials))))) + if (!DepositPreauthEntryR( + account, + credentials::makeSorted(ctx.tx.getFieldArray(sfUnauthorizeCredentials)), + ctx.view)) return tecNO_ENTRY; } return tesSUCCESS; @@ -176,27 +177,27 @@ DepositPreauth::doApply() // Preclaim already verified that the Preauth entry does not yet exist. // Create and populate the Preauth entry. AccountID const auth{ctx_.tx[sfAuthorize]}; - Keylet const preauthKeylet = keylet::depositPreauth(accountID_, auth); - auto const slePreauth = std::make_shared(preauthKeylet); + DepositPreauthEntryW entryPreauth(accountID_, auth, view(), j_); + entryPreauth.newSLE(); - slePreauth->setAccountID(sfAccount, accountID_); - slePreauth->setAccountID(sfAuthorize, auth); - view().insert(slePreauth); + entryPreauth->setAccountID(sfAccount, accountID_); + entryPreauth->setAccountID(sfAuthorize, auth); + entryPreauth.insert(); auto const page = view().dirInsert( - keylet::ownerDir(accountID_), preauthKeylet, describeOwnerDir(accountID_)); + keylet::ownerDir(accountID_), entryPreauth.keylet(), describeOwnerDir(accountID_)); JLOG(j_.trace()) << "Adding DepositPreauth to owner directory " - << to_string(preauthKeylet.key) << ": " << (page ? "success" : "failure"); + << to_string(entryPreauth.key()) << ": " << (page ? "success" : "failure"); if (!page) return tecDIR_FULL; // LCOV_EXCL_LINE - slePreauth->setFieldU64(sfOwnerNode, *page); + entryPreauth->setFieldU64(sfOwnerNode, *page); // If we succeeded, the new entry counts against the creator's reserve. increaseOwnerCount(applyViewContext, sleOwner, 1, j_); - addSponsorToLedgerEntry(applyViewContext, slePreauth); + addSponsorToLedgerEntry(applyViewContext, entryPreauth.mutableRawSle()); } else if (ctx_.tx.isFieldPresent(sfUnauthorize)) { @@ -232,30 +233,30 @@ DepositPreauth::doApply() sortedLE.pushBack(std::move(cred)); } - Keylet const preauthKey = keylet::depositPreauth(accountID_, sortedTX); - auto const slePreauth = std::make_shared(preauthKey); - if (!slePreauth) + DepositPreauthEntryW entryPreauth(accountID_, sortedTX, view(), j_); + entryPreauth.newSLE(); + if (!entryPreauth) return tefINTERNAL; // LCOV_EXCL_LINE - slePreauth->setAccountID(sfAccount, accountID_); - slePreauth->peekFieldArray(sfAuthorizeCredentials) = std::move(sortedLE); + entryPreauth->setAccountID(sfAccount, accountID_); + entryPreauth->peekFieldArray(sfAuthorizeCredentials) = std::move(sortedLE); - view().insert(slePreauth); + entryPreauth.insert(); auto const page = view().dirInsert( - keylet::ownerDir(accountID_), preauthKey, describeOwnerDir(accountID_)); + keylet::ownerDir(accountID_), entryPreauth.keylet(), describeOwnerDir(accountID_)); - JLOG(j_.trace()) << "Adding DepositPreauth to owner directory " << to_string(preauthKey.key) - << ": " << (page ? "success" : "failure"); + JLOG(j_.trace()) << "Adding DepositPreauth to owner directory " + << to_string(entryPreauth.key()) << ": " << (page ? "success" : "failure"); if (!page) return tecDIR_FULL; // LCOV_EXCL_LINE - slePreauth->setFieldU64(sfOwnerNode, *page); + entryPreauth->setFieldU64(sfOwnerNode, *page); // If we succeeded, the new entry counts against the creator's reserve. increaseOwnerCount(applyViewContext, sleOwner, 1, j_); - addSponsorToLedgerEntry(applyViewContext, slePreauth); + addSponsorToLedgerEntry(applyViewContext, entryPreauth.mutableRawSle()); } else if (ctx_.tx.isFieldPresent(sfUnauthorizeCredentials)) { @@ -271,7 +272,7 @@ TER DepositPreauth::removeFromLedger(ApplyView& view, UInt256 const& preauthIndex, beast::Journal j) { // Existence already checked in preclaim and AccountDelete - auto const slePreauth{view.peek(keylet::depositPreauth(preauthIndex))}; + DepositPreauthEntryW slePreauth(preauthIndex, view, j); if (!slePreauth) { JLOG(j.warn()) << "Selected DepositPreauth does not exist."; @@ -293,9 +294,9 @@ DepositPreauth::removeFromLedger(ApplyView& view, UInt256 const& preauthIndex, b if (!sleOwner) return tefINTERNAL; // LCOV_EXCL_LINE - decreaseOwnerCountForObject(view, sleOwner, slePreauth, 1, j); + decreaseOwnerCountForObject(view, sleOwner, slePreauth.mutableRawSle(), 1, j); // Remove DepositPreauth from ledger. - view.erase(slePreauth); + slePreauth.erase(); return tesSUCCESS; } diff --git a/src/tests/libxrpl/shamap/SHAMapAddNode.cpp b/src/tests/libxrpl/shamap/SHAMapAddNode.cpp new file mode 100644 index 0000000000..605b23550d --- /dev/null +++ b/src/tests/libxrpl/shamap/SHAMapAddNode.cpp @@ -0,0 +1,76 @@ +#include + +#include + +namespace xrpl::tests { + +// get() is a log format, so its wording is pinned here, once. Every other site reads the same tally +// by value, through the count and verdict accessors. +TEST(SHAMapAddNode, get_names_every_non_empty_count) +{ + EXPECT_EQ(SHAMapAddNode{}.get(), "no nodes processed"); + EXPECT_EQ(SHAMapAddNode::useful().get(), "good:1"); + EXPECT_EQ(SHAMapAddNode::invalid().get(), "bad:1"); + EXPECT_EQ(SHAMapAddNode::duplicate().get(), "dupe:1"); + + // Several of a kind are counted, and the counts are joined in a fixed order with a single + // space, whichever order they were recorded in. + SHAMapAddNode san; + san.incInvalid(); + san.incUseful(); + san.incUseful(); + san.incDuplicate(); + EXPECT_EQ(san.get(), "good:2 bad:1 dupe:1"); + + san.reset(); + EXPECT_EQ(san.get(), "no nodes processed"); +} + +// The three counts and the verdicts derived from them. +TEST(SHAMapAddNode, counts_and_verdicts_agree) +{ + SHAMapAddNode san; + EXPECT_EQ(san.getGood(), 0); + EXPECT_EQ(san.getBad(), 0); + EXPECT_EQ(san.getDuplicate(), 0); + EXPECT_FALSE(san.isInvalid()); + EXPECT_FALSE(san.isUseful()); + + // Good counts what produced a good result, and useful is that count being non-zero. + san.incUseful(); + EXPECT_EQ(san.getGood(), 1); + EXPECT_TRUE(san.isUseful()); + EXPECT_TRUE(san.isGood()); + + // A duplicate counts toward good. isUseful() here reflects the incUseful() above. + san.incDuplicate(); + EXPECT_EQ(san.getDuplicate(), 1); + EXPECT_FALSE(san.isInvalid()); + EXPECT_TRUE(san.isGood()); + + // Bad is a count, so a batch that carries on past a rejected node reports one per node, which + // distinguishes "stopped on the first" from "rejected several". + san.incInvalid(); + EXPECT_EQ(san.getBad(), 1); + EXPECT_TRUE(san.isInvalid()); + EXPECT_TRUE(san.isGood()) << "one bad node among two accepted ones is still a good batch"; + + san.incInvalid(); + san.incInvalid(); + EXPECT_EQ(san.getGood(), 1); + EXPECT_EQ(san.getBad(), 3); + EXPECT_EQ(san.getDuplicate(), 1); + EXPECT_FALSE(san.isGood()) << "more bad nodes than accepted ones is not a good batch"; + + // Adding one verdict to another sums every count. + SHAMapAddNode total; + total += SHAMapAddNode::useful(); + total += SHAMapAddNode::invalid(); + total += SHAMapAddNode::invalid(); + total += SHAMapAddNode::duplicate(); + EXPECT_EQ(total.getGood(), 1); + EXPECT_EQ(total.getBad(), 2); + EXPECT_EQ(total.getDuplicate(), 1); +} + +} // namespace xrpl::tests diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index b1d67d4fb0..eb5ded81d2 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -180,4 +180,50 @@ TEST_F(SHAMapSyncTest, sync) destination.invariants(); } +// The duplicate verdict also answers for a node that did not need to be added, which is the half +// of incDuplicate()'s contract that "already held" does not cover. addKnownNode() reaches it when +// the map has stopped taking nodes, and returns there before looking at the offer at all. +// +// An A/B on one offer and two maps of the same shape, so the synching state is the only difference +// between the two verdicts. Neither map holds the offered node, so a count that meant "already +// held" would be wrong for it. +TEST_F(SHAMapSyncTest, add_known_node_reports_duplicate_once_a_map_stops_synching) +{ + TestNodeFamily f{j_}; + + // A well-formed inner node with one child. Its contents do not matter: both verdicts below + // are reached without the map reading them. + auto const makeOffer = [] { + Serializer s; + s.addBitString(UInt256{1}); + s.add8(0); + s.add8(kWireTypeCompressedInner); + return SHAMapTreeNode::makeFromWire(makeSlice(s.peekData())); + }; + ASSERT_TRUE(makeOffer()); + + SHAMapNodeID const target{1, UInt256{}}; + + // Synching, so the offer is examined, and refused because the empty root has no branch to + // hook it onto. The point is that the map looked. + SHAMap synching{SHAMapType::FREE, f}; + synching.setSynching(); + + auto const examined = synching.addKnownNode(target, makeOffer(), nullptr); + EXPECT_TRUE(examined.isInvalid()); + EXPECT_EQ(examined.getDuplicate(), 0); + + // The same offer to a map that has stopped taking nodes. + SHAMap stopped{SHAMapType::FREE, f}; + stopped.setSynching(); + stopped.clearSynching(); + ASSERT_FALSE(stopped.isSynching()); + + auto const notNeeded = stopped.addKnownNode(target, makeOffer(), nullptr); + EXPECT_EQ(notNeeded.getGood(), 0); + EXPECT_EQ(notNeeded.getBad(), 0); + EXPECT_EQ(notNeeded.getDuplicate(), 1); + EXPECT_TRUE(notNeeded.isGood()) << "a node that was not needed counts on the accepted side"; +} + } // namespace xrpl::tests