fixPreviousTxnID: add sfPreviousTxnID/sfPreviousTxnLgrSequence to all ledger objects

src/ripple/ledger/impl/ApplyStateTable.cpp

@@ -21,8 +21,10 @@
 #include <ripple/json/to_string.h>
 #include <ripple/ledger/detail/ApplyStateTable.h>
 #include <ripple/protocol/Feature.h>
+#include <ripple/protocol/LedgerFormats.h>
 #include <ripple/protocol/st.h>
 #include <cassert>
+#include <set>
 
 namespace ripple {
 namespace detail {
@@ -189,8 +191,9 @@ ApplyStateTable::apply(
             {
                 assert(curNode && origNode);
 
-                if (curNode->isThreadedType())  // thread transaction to node
-                                                // item modified
+                if (curNode->isThreadedType(
+                        to.rules()))  // thread transaction to node
+                                      // item modified
                     threadItem(meta, curNode);
 
                 STObject prevs(sfPreviousFields);
@@ -224,7 +227,8 @@ ApplyStateTable::apply(
                 assert(curNode && !origNode);
                 threadOwners(to, meta, curNode, newMod, j);
 
-                if (curNode->isThreadedType())  // always thread to self
+                if (curNode->isThreadedType(
+                        to.rules()))  // always thread to self
                     threadItem(meta, curNode);
 
                 STObject news(sfNewFields);
@@ -610,6 +614,7 @@ ApplyStateTable::threadTx(
         JLOG(j.warn()) << "Threading to non-existent account: " << toBase58(to);
         return;
     }
+    // threadItem only applied to AccountRoot
     threadItem(meta, sle);
 }
 

src/ripple/protocol/Feature.h

@@ -74,7 +74,7 @@ namespace detail {
 // Feature.cpp. Because it's only used to reserve storage, and determine how
 // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
 // the actual number of amendments. A LogicError on startup will verify this.
-static constexpr std::size_t numFeatures = 71;
+static constexpr std::size_t numFeatures = 72;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -358,6 +358,7 @@ extern uint256 const fixAMMOverflowOffer;
 extern uint256 const featurePriceOracle;
 extern uint256 const fixEmptyDID;
 extern uint256 const fixXChainRewardRounding;
+extern uint256 const fixPreviousTxnID;
 
 }  // namespace ripple
 

src/ripple/protocol/STLedgerEntry.h

@@ -21,6 +21,7 @@
 #define RIPPLE_PROTOCOL_STLEDGERENTRY_H_INCLUDED
 
 #include <ripple/protocol/Indexes.h>
+#include <ripple/protocol/Rules.h>
 #include <ripple/protocol/STObject.h>
 
 namespace ripple {
@@ -67,7 +68,7 @@ class STLedgerEntry final : public STObject, public CountedObject<STLedgerEntry>
 
     // is this a ledger entry that can be threaded
     bool
-    isThreadedType() const;
+    isThreadedType(Rules const& rules) const;
 
     bool
     thread(

src/ripple/protocol/impl/Feature.cpp

@@ -465,6 +465,7 @@ REGISTER_FIX    (fixAMMOverflowOffer,           Supported::yes, VoteBehavior::De
 REGISTER_FEATURE(PriceOracle,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixEmptyDID,                   Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FIX    (fixXChainRewardRounding,       Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixPreviousTxnID,              Supported::yes, VoteBehavior::DefaultNo);
 
 // The following amendments are obsolete, but must remain supported
 // because they could potentially get enabled.

src/ripple/protocol/impl/LedgerFormats.cpp

@@ -74,6 +74,8 @@ LedgerFormats::LedgerFormats()
             {sfIndexNext,            soeOPTIONAL},
             {sfIndexPrevious,        soeOPTIONAL},
             {sfNFTokenID,            soeOPTIONAL},
+            {sfPreviousTxnID,        soeOPTIONAL},
+            {sfPreviousTxnLgrSeq,    soeOPTIONAL},
         },
         commonFields);
 
@@ -142,21 +144,25 @@ LedgerFormats::LedgerFormats()
         {
             {sfAmendments,           soeOPTIONAL},  // Enabled
             {sfMajorities,           soeOPTIONAL},
+            {sfPreviousTxnID,        soeOPTIONAL},
+            {sfPreviousTxnLgrSeq,    soeOPTIONAL},
         },
         commonFields);
 
     add(jss::FeeSettings,
         ltFEE_SETTINGS,
         {
             // Old version uses raw numbers
-            {sfBaseFee,              soeOPTIONAL},
-            {sfReferenceFeeUnits,    soeOPTIONAL},
-            {sfReserveBase,          soeOPTIONAL},
-            {sfReserveIncrement,     soeOPTIONAL},
+            {sfBaseFee,                soeOPTIONAL},
+            {sfReferenceFeeUnits,      soeOPTIONAL},
+            {sfReserveBase,            soeOPTIONAL},
+            {sfReserveIncrement,       soeOPTIONAL},
             // New version uses Amounts
             {sfBaseFeeDrops,           soeOPTIONAL},
             {sfReserveBaseDrops,       soeOPTIONAL},
             {sfReserveIncrementDrops,  soeOPTIONAL},
+            {sfPreviousTxnID,          soeOPTIONAL},
+            {sfPreviousTxnLgrSeq,      soeOPTIONAL},
         },
         commonFields);
 
@@ -240,6 +246,8 @@ LedgerFormats::LedgerFormats()
             {sfDisabledValidators,   soeOPTIONAL},
             {sfValidatorToDisable,   soeOPTIONAL},
             {sfValidatorToReEnable,  soeOPTIONAL},
+            {sfPreviousTxnID,        soeOPTIONAL},
+            {sfPreviousTxnLgrSeq,    soeOPTIONAL},
         },
         commonFields);
 
@@ -272,14 +280,16 @@ LedgerFormats::LedgerFormats()
     add(jss::AMM,
         ltAMM,
         {
-            {sfAccount, soeREQUIRED},
-            {sfTradingFee, soeDEFAULT},
-            {sfVoteSlots, soeOPTIONAL},
-            {sfAuctionSlot, soeOPTIONAL},
-            {sfLPTokenBalance, soeREQUIRED},
-            {sfAsset, soeREQUIRED},
-            {sfAsset2, soeREQUIRED},
-            {sfOwnerNode, soeREQUIRED},
+            {sfAccount,              soeREQUIRED},
+            {sfTradingFee,           soeDEFAULT},
+            {sfVoteSlots,            soeOPTIONAL},
+            {sfAuctionSlot,          soeOPTIONAL},
+            {sfLPTokenBalance,       soeREQUIRED},
+            {sfAsset,                soeREQUIRED},
+            {sfAsset2,               soeREQUIRED},
+            {sfOwnerNode,            soeREQUIRED},
+            {sfPreviousTxnID,        soeOPTIONAL},
+            {sfPreviousTxnLgrSeq,    soeOPTIONAL},
         },
         commonFields);
 

src/ripple/protocol/impl/STLedgerEntry.cpp

@@ -21,10 +21,13 @@
 #include <ripple/basics/contract.h>
 #include <ripple/basics/safe_cast.h>
 #include <ripple/json/to_string.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/Indexes.h>
 #include <ripple/protocol/STLedgerEntry.h>
 #include <ripple/protocol/jss.h>
 #include <boost/format.hpp>
+#include <algorithm>
+#include <array>
 #include <limits>
 
 namespace ripple {
@@ -124,9 +127,18 @@ STLedgerEntry::getJson(JsonOptions options) const
 }
 
 bool
-STLedgerEntry::isThreadedType() const
+STLedgerEntry::isThreadedType(Rules const& rules) const
 {
-    return getFieldIndex(sfPreviousTxnID) != -1;
+    static constexpr std::array<LedgerEntryType, 5> newPreviousTxnIDTypes = {
+        ltDIR_NODE, ltAMENDMENTS, ltFEE_SETTINGS, ltNEGATIVE_UNL, ltAMM};
+    // Exclude PrevTxnID/PrevTxnLgrSeq if the fixPreviousTxnID amendment is not
+    // enabled and the ledger object type is in the above set
+    bool const excludePrevTxnID = !rules.enabled(fixPreviousTxnID) &&
+        std::count(
+            newPreviousTxnIDTypes.cbegin(),
+            newPreviousTxnIDTypes.cend(),
+            type_);
+    return getFieldIndex(sfPreviousTxnID) != -1 && !excludePrevTxnID;
 }
 
 bool

src/ripple/protocol/jss.h

@@ -524,6 +524,7 @@ JSS(node_writes_duration_us);    // out: GetCounts
 JSS(node_write_retries);         // out: GetCounts
 JSS(node_writes_delayed);        // out::GetCounts
 JSS(nth);                        // out: RPC server_definitions
+JSS(nunl);                       // in: AccountObjects
 JSS(obligations);                // out: GatewayBalances
 JSS(offer);                      // in: LedgerEntry
 JSS(offers);                     // out: NetworkOPs, AccountOffers, Subscribe

src/ripple/rpc/impl/RPCHelpers.cpp

@@ -934,30 +934,31 @@ chooseLedgerEntryType(Json::Value const& params)
     std::pair<RPC::Status, LedgerEntryType> result{RPC::Status::OK, ltANY};
     if (params.isMember(jss::type))
     {
-        static constexpr std::array<std::pair<char const*, LedgerEntryType>, 21>
+        static constexpr std::array<std::pair<char const*, LedgerEntryType>, 22>
             types{
                 {{jss::account, ltACCOUNT_ROOT},
                  {jss::amendments, ltAMENDMENTS},
+                 {jss::amm, ltAMM},
+                 {jss::bridge, ltBRIDGE},
                  {jss::check, ltCHECK},
                  {jss::deposit_preauth, ltDEPOSIT_PREAUTH},
+                 {jss::did, ltDID},
                  {jss::directory, ltDIR_NODE},
                  {jss::escrow, ltESCROW},
                  {jss::fee, ltFEE_SETTINGS},
                  {jss::hashes, ltLEDGER_HASHES},
+                 {jss::nunl, ltNEGATIVE_UNL},
+                 {jss::oracle, ltORACLE},
+                 {jss::nft_offer, ltNFTOKEN_OFFER},
+                 {jss::nft_page, ltNFTOKEN_PAGE},
                  {jss::offer, ltOFFER},
                  {jss::payment_channel, ltPAYCHAN},
                  {jss::signer_list, ltSIGNER_LIST},
                  {jss::state, ltRIPPLE_STATE},
                  {jss::ticket, ltTICKET},
-                 {jss::nft_offer, ltNFTOKEN_OFFER},
-                 {jss::nft_page, ltNFTOKEN_PAGE},
-                 {jss::amm, ltAMM},
-                 {jss::bridge, ltBRIDGE},
                  {jss::xchain_owned_claim_id, ltXCHAIN_OWNED_CLAIM_ID},
                  {jss::xchain_owned_create_account_claim_id,
-                  ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID},
-                 {jss::did, ltDID},
-                 {jss::oracle, ltORACLE}}};
+                  ltXCHAIN_OWNED_CREATE_ACCOUNT_CLAIM_ID}}};
 
         auto const& p = params[jss::type];
         if (!p.isString())

src/test/app/AMMExtended_test.cpp

@@ -3271,7 +3271,7 @@ struct AMMExtended_test : public jtx::AMMTest
             if (!BEAST_EXPECT(checkArraySize(affected, 4u)))
                 return;
             auto ff =
-                affected[2u][sfModifiedNode.fieldName][sfFinalFields.fieldName];
+                affected[1u][sfModifiedNode.fieldName][sfFinalFields.fieldName];
             BEAST_EXPECT(
                 ff[sfHighLimit.fieldName] ==
                 bob["USD"](100).value().getJson(JsonOptions::none));

src/test/app/TxQ_test.cpp

@@ -2806,6 +2806,12 @@ class TxQPosNegFlows_test : public beast::unit_test::suite
     {
         // This test focuses on which gaps in queued transactions are
         // allowed to be filled even when the account's queue is full.
+
+        // NOTE: This test is fragile and dependent on ordering of
+        // transactions, which is affected by the closed/validated
+        // ledger hash. This test may need to be edited if changes
+        // are made that impact the ledger hash.
+        // TODO: future-proof this test.
         using namespace jtx;
         testcase("full queue gap handling");
 
@@ -2946,9 +2952,9 @@ class TxQPosNegFlows_test : public beast::unit_test::suite
         // may not reduce to 8.
         env.close();
         checkMetrics(__LINE__, env, 9, 50, 6, 5, 256);
-        BEAST_EXPECT(env.seq(alice) == aliceSeq + 13);
+        BEAST_EXPECT(env.seq(alice) == aliceSeq + 15);
 
-        // Close ledger 7.  That should remove 7 more of alice's transactions.
+        // Close ledger 7.  That should remove 4 more of alice's transactions.
         env.close();
         checkMetrics(__LINE__, env, 2, 60, 7, 6, 256);
         BEAST_EXPECT(env.seq(alice) == aliceSeq + 19);

src/test/ledger/Directory_test.cpp

@@ -396,13 +396,78 @@ struct Directory_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testPreviousTxnID()
+    {
+        testcase("fixPreviousTxnID");
+        using namespace jtx;
+
+        auto const gw = Account{"gateway"};
+        auto const alice = Account{"alice"};
+        auto const USD = gw["USD"];
+
+        auto ledger_data = [](Env& env) {
+            Json::Value params;
+            params[jss::type] = jss::directory;
+            params[jss::ledger_index] = "validated";
+            return env.rpc(
+                "json", "ledger_data", to_string(params))[jss::result];
+        };
+
+        {
+            // fixPreviousTxnID is disabled.
+            Env env(*this, supported_amendments() - fixPreviousTxnID);
+            env.fund(XRP(10000), alice, gw);
+            env.close();
+            env.trust(USD(1000), alice);
+            env(pay(gw, alice, USD(1000)));
+            env.close();
+
+            {
+                auto const jrr = ledger_data(env);
+                BEAST_EXPECTS(
+                    checkArraySize(jrr[jss::state], 2), jrr.toStyledString());
+                auto const directory = jrr[jss::state][0u];
+                BEAST_EXPECT(
+                    directory["LedgerEntryType"] ==
+                    jss::DirectoryNode);  // sanity check
+                // The PreviousTxnID and PreviousTxnLgrSeq fields should not be
+                // on the DirectoryNode object when the amendment is disabled
+                BEAST_EXPECT(!directory.isMember("PreviousTxnID"));
+                BEAST_EXPECT(!directory.isMember("PreviousTxnLgrSeq"));
+            }
+
+            // Now enable the amendment so the directory node is updated.
+            env.enableFeature(fixPreviousTxnID);
+            env.close();
+
+            // Make sure the `PreviousTxnID` and `PreviousTxnLgrSeq` fields now
+            // exist
+            env(offer(alice, XRP(1), USD(1)));
+            env.close();
+
+            {
+                auto const jrr = ledger_data(env);
+                BEAST_EXPECTS(
+                    checkArraySize(jrr[jss::state], 3), jrr.toStyledString());
+                auto const directory = jrr[jss::state][0u];
+                BEAST_EXPECT(
+                    directory["LedgerEntryType"] ==
+                    jss::DirectoryNode);  // sanity check
+                BEAST_EXPECT(directory.isMember("PreviousTxnID"));
+                BEAST_EXPECT(directory.isMember("PreviousTxnLgrSeq"));
+            }
+        }
+    }
+
     void
     run() override
     {
         testDirectoryOrdering();
         testDirIsEmpty();
         testRipd1353();
         testEmptyChain();
+        testPreviousTxnID();
     }
 };
 

src/test/rpc/TransactionEntry_test.cpp

@@ -328,7 +328,7 @@ class TransactionEntry_test : public beast::unit_test::suite
             "TransactionType" : "Payment",
             "TxnSignature" : "3044022033D9EBF7F02950AF2F6B13C07AEE641C8FEBDD540A338FCB9027A965A4AED35B02206E4E227DCC226A3456C0FEF953449D21645A24EB63CA0BB7C5B62470147FD1D1",
 })",
-            "39AA166131D56622EFD96CB4B2BD58003ACD37091C90977FF6B81419DB451775",
+            "3A6E375BFDFF029A571AFBB3BC46C4F52963FAF043B406D0E59A7194C1A8F98E",
             "2000-01-01T00:00:20Z");
 
         check_tx(
@@ -350,7 +350,7 @@ class TransactionEntry_test : public beast::unit_test::suite
             "TransactionType" : "Payment",
             "TxnSignature" : "30450221008A722B7F16EDB2348886E88ED4EC682AE9973CC1EE0FF37C93BB2CEC821D3EDF022059E464472031BA5E0D88A93E944B6A8B8DB3E1D5E5D1399A805F615789DB0BED",
 })",
-            "39AA166131D56622EFD96CB4B2BD58003ACD37091C90977FF6B81419DB451775",
+            "3A6E375BFDFF029A571AFBB3BC46C4F52963FAF043B406D0E59A7194C1A8F98E",
             "2000-01-01T00:00:20Z");
 
         env(offer(A2, XRP(100), A2["USD"](1)));
@@ -379,7 +379,7 @@ class TransactionEntry_test : public beast::unit_test::suite
             "TransactionType" : "OfferCreate",
             "TxnSignature" : "304502210093FC93ACB77B4E3DE3315441BD010096734859080C1797AB735EB47EBD541BD102205020BB1A7C3B4141279EE4C287C13671E2450EA78914EFD0C6DB2A18344CD4F2",
 })",
-            "0589B876DF5AFE335781E8FC12C2EC62A80151DF13BBAFE9EB2DA62E798ED434",
+            "73D6C8E66E0DC22F3E6F7D39BF795A6831BEB412823A986C7CC19470C93557C0",
             "2000-01-01T00:00:30Z");
     }