Merge branch 'develop' into ripple/cosign

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

src/libxrpl/ledger/View.cpp

@@ -10,6 +10,7 @@
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/entries/AmendmentsEntry.h>
+#include <xrpl/ledger/entries/DepositPreauthEntry.h>
 #include <xrpl/ledger/entries/LedgerHashesEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
@@ -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())
             {

src/libxrpl/ledger/helpers/CredentialHelpers.cpp

@@ -7,6 +7,7 @@
 #include <xrpl/beast/utility/Journal.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/entries/DepositPreauthEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
@@ -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

src/libxrpl/tx/transactors/account/AccountDelete.cpp

@@ -9,6 +9,7 @@
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/View.h>
+#include <xrpl/ledger/entries/DepositPreauthEntry.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/ledger/helpers/NFTokenHelpers.h>
@@ -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;
         }
     }

src/libxrpl/tx/transactors/bridge/XChainBridge.cpp

@@ -10,6 +10,7 @@
 #include <xrpl/ledger/PaymentSandbox.h>
 #include <xrpl/ledger/RawView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/entries/DepositPreauthEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
 #include <xrpl/protocol/AccountID.h>
@@ -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;
         }

src/libxrpl/tx/transactors/payment/DepositPreauth.cpp

@@ -4,6 +4,7 @@
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/core/ServiceRegistry.h>
+#include <xrpl/ledger/entries/DepositPreauthEntry.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/DirectoryHelpers.h>
@@ -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<SLE>(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<SLE>(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;
 }

src/tests/libxrpl/shamap/SHAMapAddNode.cpp

@@ -0,0 +1,76 @@
+#include <xrpl/shamap/SHAMapAddNode.h>
+
+#include <gtest/gtest.h>
+
+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

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