fix amendment to add PreviousTxnID/PreviousTxnLgrSequence (#4751)

This amendment, `fixPreviousTxnID`, adds `PreviousTxnID` and
`PreviousTxnLgrSequence` as fields to all ledger objects that did
not already have them included (`DirectoryNode`, `Amendments`,
`FeeSettings`, `NegativeUNL`, and `AMM`). This makes it much easier
to go through the history of these ledger objects.

src/ripple/ledger/impl/ApplyStateTable.cpp

@@ -189,8 +189,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 +225,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 +612,8 @@ ApplyStateTable::threadTx(
         JLOG(j.warn()) << "Threading to non-existent account: " << toBase58(to);
         return;
     }
+    // threadItem only applied to AccountRoot
+    assert(sle->isThreadedType(base.rules()));
     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

@@ -25,6 +25,7 @@
 
 namespace ripple {
 
+class Rules;
 class Invariants_test;
 
 class STLedgerEntry final : public STObject, public CountedObject<STLedgerEntry>
@@ -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

@@ -17,14 +17,19 @@
 */
 //==============================================================================
 
+#include <ripple/protocol/STLedgerEntry.h>
+
 #include <ripple/basics/Log.h>
 #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/Rules.h>
 #include <ripple/protocol/jss.h>
 #include <boost/format.hpp>
+#include <algorithm>
+#include <array>
 #include <limits>
 
 namespace ripple {
@@ -124,9 +129,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 !excludePrevTxnID && getFieldIndex(sfPreviousTxnID) != -1;
 }
 
 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,104 @@ 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 = [this](Env& env) {
+            Json::Value params;
+            params[jss::type] = jss::directory;
+            params[jss::ledger_index] = "validated";
+            auto const result =
+                env.rpc("json", "ledger_data", to_string(params))[jss::result];
+            BEAST_EXPECT(!result.isMember(jss::marker));
+            return 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);
+            auto const& jstate = jrr[jss::state];
+            BEAST_EXPECTS(checkArraySize(jstate, 2), jrr.toStyledString());
+            for (auto const& directory : jstate)
+            {
+                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)));
+        auto const txID = to_string(env.tx()->getTransactionID());
+        auto const ledgerSeq = env.current()->info().seq;
+        env.close();
+        // Make sure the fields only exist if the object is touched
+        env(noop(gw));
+        env.close();
+
+        {
+            auto const jrr = ledger_data(env);
+            auto const& jstate = jrr[jss::state];
+            BEAST_EXPECTS(checkArraySize(jstate, 3), jrr.toStyledString());
+            for (auto const& directory : jstate)
+            {
+                BEAST_EXPECT(
+                    directory["LedgerEntryType"] ==
+                    jss::DirectoryNode);  // sanity check
+                if (directory[jss::Owner] == gw.human())
+                {
+                    // gw's directory did not get touched, so it
+                    // should not have those fields populated
+                    BEAST_EXPECT(!directory.isMember("PreviousTxnID"));
+                    BEAST_EXPECT(!directory.isMember("PreviousTxnLgrSeq"));
+                }
+                else
+                {
+                    // All of the other directories, including the order
+                    // book, did get touched, so they should have those
+                    // fields
+                    BEAST_EXPECT(
+                        directory.isMember("PreviousTxnID") &&
+                        directory["PreviousTxnID"].asString() == txID);
+                    BEAST_EXPECT(
+                        directory.isMember("PreviousTxnLgrSeq") &&
+                        directory["PreviousTxnLgrSeq"].asUInt() == ledgerSeq);
+                }
+            }
+        }
+    }
+
     void
     run() override
     {
         testDirectoryOrdering();
         testDirIsEmpty();
         testRipd1353();
         testEmptyChain();
+        testPreviousTxnID();
     }
 };