Merge branch 'develop' into ximinez/fix-batchinnersigs

- Introduces amendment `fixBatchInnerSigs`

include/xrpl/protocol/detail/features.macro

@@ -14,6 +14,7 @@
 // Add new amendments to the top of this list.
 // Keep it sorted in reverse chronological order.
 
+XRPL_FIX    (BatchInnerSigs,             Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FEATURE(PermissionDelegationV1_1,   Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (DirectoryLimit,             Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (IncludeKeyletFields,        Supported::yes, VoteBehavior::DefaultNo)

src/test/app/Batch_test.cpp

@@ -2295,9 +2295,12 @@ class Batch_test : public beast::unit_test::suite
             Serializer s;
             parsed.object->add(s);
             auto const jrr = env.rpc("submit", strHex(s.slice()))[jss::result];
-            BEAST_EXPECT(
-                jrr[jss::status] == "success" &&
-                jrr[jss::engine_result] == "temINVALID_FLAG");
+            BEAST_EXPECTS(
+                jrr[jss::status] == "error" &&
+                    jrr[jss::error] == "invalidTransaction" &&
+                    jrr[jss::error_exception] ==
+                        "fails local checks: Empty SigningPubKey.",
+                to_string(jrr));
 
             env.close();
         }
@@ -4151,6 +4154,7 @@ public:
         using namespace test::jtx;
         auto const sa = testable_amendments();
         testWithFeats(sa);
+        testWithFeats(sa - fixBatchInnerSigs);
     }
 };
 

src/test/jtx/batch.h

@@ -49,8 +49,7 @@ public:
     inner(
         Json::Value const& txn,
         std::uint32_t const& sequence,
-        std::optional<std::uint32_t> const& ticket = std::nullopt,
-        std::optional<std::uint32_t> const& fee = std::nullopt)
+        std::optional<std::uint32_t> const& ticket = std::nullopt)
         : txn_(txn), seq_(sequence), ticket_(ticket)
     {
         txn_[jss::SigningPubKey] = "";

src/test/rpc/LedgerEntry_test.cpp

@@ -5,8 +5,6 @@
 #include <test/jtx/multisign.h>
 #include <test/jtx/xchain_bridge.h>
 
-#include <xrpld/app/tx/apply.h>
-
 #include <xrpl/beast/unit_test.h>
 #include <xrpl/json/json_value.h>
 #include <xrpl/protocol/AccountID.h>
@@ -2010,370 +2008,6 @@ class LedgerEntry_test : public beast::unit_test::suite
         }
     }
 
-    /// Test the ledger entry types that don't take parameters
-    void
-    testLedgerEntryFixed()
-    {
-        using namespace test::jtx;
-
-        Account const alice{"alice"};
-        Account const bob{"bob"};
-
-        Env env{*this, envconfig([](auto cfg) {
-                    cfg->START_UP = Config::FRESH;
-                    return cfg;
-                })};
-
-        env.close();
-
-        /** Verifies that the RPC result has the expected data
-         *
-         * @param good: Indicates that the request should have succeeded
-         *   and returned a ledger object of `expectedType` type.
-         * @param jv: The RPC result Json value
-         * @param expectedType: The type that the ledger object should
-         *   have if "good".
-         * @param expectedError: Optional. The expected error if not
-         *   good. Defaults to "entryNotFound".
-         */
-        auto checkResult =
-            [&](bool good,
-                Json::Value const& jv,
-                Json::StaticString const& expectedType,
-                std::optional<std::string> const& expectedError = {}) {
-                if (good)
-                {
-                    BEAST_EXPECTS(
-                        jv.isObject() && jv.isMember(jss::result) &&
-                            !jv[jss::result].isMember(jss::error) &&
-                            jv[jss::result].isMember(jss::node) &&
-                            jv[jss::result][jss::node].isMember(
-                                sfLedgerEntryType.jsonName) &&
-                            jv[jss::result][jss::node]
-                              [sfLedgerEntryType.jsonName] == expectedType,
-                        to_string(jv));
-                }
-                else
-                {
-                    BEAST_EXPECTS(
-                        jv.isObject() && jv.isMember(jss::result) &&
-                            jv[jss::result].isMember(jss::error) &&
-                            !jv[jss::result].isMember(jss::node) &&
-                            jv[jss::result][jss::error] ==
-                                expectedError.value_or("entryNotFound"),
-                        to_string(jv));
-                }
-            };
-
-        /** Runs a series of tests for a given fixed-position ledger
-         * entry.
-         *
-         * @param field: The Json request field to use.
-         * @param expectedType: The type that the ledger object should
-         *   have if "good".
-         * @param expectedKey: The keylet of the fixed object.
-         * @param good: Indicates whether the object is expected to
-         *   exist.
-         */
-        auto test = [&](Json::StaticString const& field,
-                        Json::StaticString const& expectedType,
-                        Keylet const& expectedKey,
-                        bool good) {
-            testcase << "ledger_entry " << expectedType.c_str()
-                     << (good ? "" : " not") << " found";
-
-            auto const hexKey = strHex(expectedKey.key);
-
-            // Test bad values
-            // "field":null
-            Json::Value params;
-            params[jss::ledger_index] = jss::validated;
-            params[field] = Json::nullValue;
-            auto jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, expectedType, "malformedRequest");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "field":"string"
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[field] = "arbitrary string";
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, expectedType, "malformedRequest");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "field":false
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[field] = false;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, expectedType, "invalidParams");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            {
-                // "field":[incorrect index hash]
-                auto const badKey = strHex(expectedKey.key + uint256{1});
-                params.clear();
-                params[jss::ledger_index] = jss::validated;
-                params[field] = badKey;
-                jv = env.rpc("json", "ledger_entry", to_string(params));
-                checkResult(false, jv, expectedType, "entryNotFound");
-                BEAST_EXPECTS(
-                    jv[jss::result][jss::index] == badKey, to_string(jv));
-            }
-
-            // "index":"field" using API 2
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::index] = field;
-            params[jss::api_version] = 2;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, expectedType, "malformedRequest");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // Test good values
-            // Use the "field":true notation
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[field] = true;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            // Index will always be returned for valid parameters.
-            std::string const pdIdx = jv[jss::result][jss::index].asString();
-            BEAST_EXPECTS(hexKey == pdIdx, to_string(jv));
-            checkResult(good, jv, expectedType);
-
-            // "field":"[index hash]"
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[field] = hexKey;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(good, jv, expectedType);
-            BEAST_EXPECT(jv[jss::result][jss::index].asString() == hexKey);
-
-            // Use the "index":"field" notation with API 3
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::index] = field;
-            params[jss::api_version] = 3;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            // Index is correct either way
-            BEAST_EXPECT(jv[jss::result][jss::index].asString() == hexKey);
-            checkResult(good, jv, expectedType);
-
-            // Use the "index":"[index hash]" notation
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::index] = pdIdx;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            // Index is correct either way
-            BEAST_EXPECT(jv[jss::result][jss::index].asString() == hexKey);
-            checkResult(good, jv, expectedType);
-        };
-
-        test(jss::amendments, jss::Amendments, keylet::amendments(), true);
-        test(jss::fee, jss::FeeSettings, keylet::fees(), true);
-        // There won't be an nunl
-        test(jss::nunl, jss::NegativeUNL, keylet::negativeUNL(), false);
-        // Can only get the short skip list this way
-        test(jss::hashes, jss::LedgerHashes, keylet::skip(), true);
-    }
-
-    void
-    testLedgerEntryHashes()
-    {
-        using namespace test::jtx;
-
-        Account const alice{"alice"};
-        Account const bob{"bob"};
-
-        Env env{*this, envconfig([](auto cfg) {
-                    cfg->START_UP = Config::FRESH;
-                    return cfg;
-                })};
-
-        env.close();
-
-        /** Verifies that the RPC result has the expected data
-         *
-         * @param good: Indicates that the request should have succeeded
-         *   and returned a ledger object of `expectedType` type.
-         * @param jv: The RPC result Json value
-         * @param expectedCount: The number of Hashes expected in the
-         *   object if "good".
-         * @param expectedError: Optional. The expected error if not
-         *   good. Defaults to "entryNotFound".
-         */
-        auto checkResult =
-            [&](bool good,
-                Json::Value const& jv,
-                int expectedCount,
-                std::optional<std::string> const& expectedError = {}) {
-                if (good)
-                {
-                    BEAST_EXPECTS(
-                        jv.isObject() && jv.isMember(jss::result) &&
-                            !jv[jss::result].isMember(jss::error) &&
-                            jv[jss::result].isMember(jss::node) &&
-                            jv[jss::result][jss::node].isMember(
-                                sfLedgerEntryType.jsonName) &&
-                            jv[jss::result][jss::node]
-                              [sfLedgerEntryType.jsonName] == jss::LedgerHashes,
-                        to_string(jv));
-                    BEAST_EXPECTS(
-                        jv[jss::result].isMember(jss::node) &&
-                            jv[jss::result][jss::node].isMember("Hashes") &&
-                            jv[jss::result][jss::node]["Hashes"].size() ==
-                                expectedCount,
-                        to_string(jv[jss::result][jss::node]["Hashes"].size()));
-                }
-                else
-                {
-                    BEAST_EXPECTS(
-                        jv.isObject() && jv.isMember(jss::result) &&
-                            jv[jss::result].isMember(jss::error) &&
-                            !jv[jss::result].isMember(jss::node) &&
-                            jv[jss::result][jss::error] ==
-                                expectedError.value_or("entryNotFound"),
-                        to_string(jv));
-                }
-            };
-
-        /** Runs a series of tests for a given ledger index.
-         *
-         * @param ledger: The ledger index value of the "hashes" request
-         *   parameter. May not necessarily be a number.
-         * @param expectedKey: The expected keylet of the object.
-         * @param good: Indicates whether the object is expected to
-         *   exist.
-         * @param expectedCount: The number of Hashes expected in the
-         *   object if "good".
-         */
-        auto test = [&](Json::Value ledger,
-                        Keylet const& expectedKey,
-                        bool good,
-                        int expectedCount = 0) {
-            testcase << "ledger_entry LedgerHashes: seq: "
-                     << env.current()->info().seq
-                     << " \"hashes\":" << to_string(ledger)
-                     << (good ? "" : " not") << " found";
-
-            auto const hexKey = strHex(expectedKey.key);
-
-            // Test bad values
-            // "hashes":null
-            Json::Value params;
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = Json::nullValue;
-            auto jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, 0, "malformedRequest");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "hashes":"non-uint string"
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = "arbitrary string";
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, 0, "malformedRequest");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "hashes":"uint string" is invalid, too
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = "10";
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, 0, "malformedRequest");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "hashes":false
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = false;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, 0, "invalidParams");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "hashes":-1
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = -1;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(false, jv, 0, "internal");
-            BEAST_EXPECT(!jv[jss::result].isMember(jss::index));
-
-            // "hashes":[incorrect index hash]
-            {
-                auto const badKey = strHex(expectedKey.key + uint256{1});
-                params.clear();
-                params[jss::ledger_index] = jss::validated;
-                params[jss::hashes] = badKey;
-                jv = env.rpc("json", "ledger_entry", to_string(params));
-                checkResult(false, jv, 0, "entryNotFound");
-                BEAST_EXPECT(jv[jss::result][jss::index] == badKey);
-            }
-
-            // Test good values
-            // Use the "hashes":ledger notation
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = ledger;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(good, jv, expectedCount);
-            // Index will always be returned for valid parameters.
-            std::string const pdIdx = jv[jss::result][jss::index].asString();
-            BEAST_EXPECTS(hexKey == pdIdx, strHex(pdIdx));
-
-            // "hashes":"[index hash]"
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::hashes] = hexKey;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(good, jv, expectedCount);
-            // Index is correct either way
-            BEAST_EXPECTS(
-                hexKey == jv[jss::result][jss::index].asString(),
-                strHex(jv[jss::result][jss::index].asString()));
-
-            // Use the "index":"[index hash]" notation
-            params.clear();
-            params[jss::ledger_index] = jss::validated;
-            params[jss::index] = hexKey;
-            jv = env.rpc("json", "ledger_entry", to_string(params));
-            checkResult(good, jv, expectedCount);
-            // Index is correct either way
-            BEAST_EXPECTS(
-                hexKey == jv[jss::result][jss::index].asString(),
-                strHex(jv[jss::result][jss::index].asString()));
-        };
-
-        // short skip list
-        test(true, keylet::skip(), true, 2);
-        // long skip list at index 0
-        test(1, keylet::skip(1), false);
-        // long skip list at index 1
-        test(1 << 17, keylet::skip(1 << 17), false);
-
-        // Close more ledgers, but stop short of the flag ledger
-        for (auto i = env.current()->seq(); i <= 250; ++i)
-            env.close();
-
-        // short skip list
-        test(true, keylet::skip(), true, 249);
-        // long skip list at index 0
-        test(1, keylet::skip(1), false);
-        // long skip list at index 1
-        test(1 << 17, keylet::skip(1 << 17), false);
-
-        // Close a flag ledger so the first "long" skip list is created
-        for (auto i = env.current()->seq(); i <= 260; ++i)
-            env.close();
-
-        // short skip list
-        test(true, keylet::skip(), true, 256);
-        // long skip list at index 0
-        test(1, keylet::skip(1), true, 1);
-        // long skip list at index 1
-        test(1 << 17, keylet::skip(1 << 17), false);
-    }
-
     void
     testLedgerEntryCLI()
     {
@@ -2423,8 +2057,6 @@ public:
         testOracleLedgerEntry();
         testLedgerEntryMPT();
         testLedgerEntryPermissionedDomain();
-        testLedgerEntryFixed();
-        testLedgerEntryHashes();
         testLedgerEntryCLI();
     }
 };

src/xrpld/app/ledger/detail/OpenLedger.cpp

@@ -106,7 +106,10 @@ OpenLedger::accept(
 
         // skip batch txns
         // LCOV_EXCL_START
-        if (tx->isFlag(tfInnerBatchTxn) && rules.enabled(featureBatch))
+        // The flag should only be settable if Batch feature is enabled. If
+        // Batch is not enabled, the flag is always invalid, so don't relay it
+        // regardless.
+        if (tx->isFlag(tfInnerBatchTxn))
         {
             XRPL_ASSERT(
                 txpair.second && txpair.second->isFieldPresent(sfParentBatchID),

src/xrpld/app/misc/NetworkOPs.cpp

@@ -1677,10 +1677,11 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
                     app_.getHashRouter().shouldRelay(e.transaction->getID());
                 if (auto const sttx = *(e.transaction->getSTransaction());
                     toSkip &&
-                    // Skip relaying if it's an inner batch txn and batch
-                    // feature is enabled
-                    !(sttx.isFlag(tfInnerBatchTxn) &&
-                      newOL->rules().enabled(featureBatch)))
+                    // Skip relaying if it's an inner batch txn. The flag should
+                    // only be set if the Batch feature is enabled. If Batch is
+                    // not enabled, the flag is always invalid, so don't relay
+                    // it regardless.
+                    !(sttx.isFlag(tfInnerBatchTxn)))
                 {
                     protocol::TMTransaction tx;
                     Serializer s;
@@ -3040,9 +3041,11 @@ NetworkOPsImp::pubProposedTransaction(
     std::shared_ptr<STTx const> const& transaction,
     TER result)
 {
-    // never publish an inner txn inside a batch txn
-    if (transaction->isFlag(tfInnerBatchTxn) &&
-        ledger->rules().enabled(featureBatch))
+    // never publish an inner txn inside a batch txn. The flag should
+    // only be set if the Batch feature is enabled. If Batch is not
+    // enabled, the flag is always invalid, so don't publish it
+    // regardless.
+    if (transaction->isFlag(tfInnerBatchTxn))
         return;
 
     MultiApiJson jvObj =

src/xrpld/app/tx/detail/Transactor.cpp

@@ -204,12 +204,17 @@ Transactor::preflight2(PreflightContext const& ctx)
         // regardless of success or failure
         return *ret;
 
-    auto const sigValid = checkValidity(
-        ctx.app.getHashRouter(), ctx.tx, ctx.rules, ctx.app.config());
-    if (sigValid.first == Validity::SigBad)
+    // Skip signature check on batch inner transactions
+    if (!ctx.tx.isFlag(tfInnerBatchTxn) || !ctx.rules.enabled(featureBatch))
     {
-        JLOG(ctx.j.debug()) << "preflight2: bad signature. " << sigValid.second;
-        return temINVALID;  // LCOV_EXCL_LINE
+        auto const sigValid = checkValidity(
+            ctx.app.getHashRouter(), ctx.tx, ctx.rules, ctx.app.config());
+        if (sigValid.first == Validity::SigBad)
+        {
+            JLOG(ctx.j.debug())
+                << "preflight2: bad signature. " << sigValid.second;
+            return temINVALID;  // LCOV_EXCL_LINE
+        }
     }
     return tesSUCCESS;
 }
@@ -642,13 +647,17 @@ NotTEC
 Transactor::checkSign(
     ReadView const& view,
     ApplyFlags flags,
+    std::optional<uint256 const> const& parentBatchId,
     AccountID const& idAccount,
     STObject const& sigObject,
     beast::Journal const j)
 {
     auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey);
     // Ignore signature check on batch inner transactions
-    if (sigObject.isFlag(tfInnerBatchTxn) && view.rules().enabled(featureBatch))
+    bool const useCtx = view.rules().enabled(fixBatchInnerSigs);
+    if ((useCtx ? parentBatchId.has_value()
+                : sigObject.isFlag(tfInnerBatchTxn)) &&
+        view.rules().enabled(featureBatch))
     {
         // Defensive Check: These values are also checked in Batch::preflight
         if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() ||
@@ -701,7 +710,8 @@ Transactor::checkSign(PreclaimContext const& ctx)
     auto const idAccount = ctx.tx.isFieldPresent(sfDelegate)
         ? ctx.tx.getAccountID(sfDelegate)
         : ctx.tx.getAccountID(sfAccount);
-    return checkSign(ctx.view, ctx.flags, idAccount, ctx.tx, ctx.j);
+    return checkSign(
+        ctx.view, ctx.flags, ctx.parentBatchId, idAccount, ctx.tx, ctx.j);
 }
 
 NotTEC

src/xrpld/app/tx/detail/Transactor.h

@@ -266,6 +266,7 @@ protected:
     checkSign(
         ReadView const& view,
         ApplyFlags flags,
+        std::optional<uint256 const> const& parentBatchId,
         AccountID const& idAccount,
         STObject const& sigObject,
         beast::Journal const j);

src/xrpld/app/tx/detail/apply.cpp

@@ -41,15 +41,22 @@ checkValidity(
                 Validity::SigBad,
                 "Malformed: Invalid inner batch transaction."};
 
-        std::string reason;
-        if (!passesLocalChecks(tx, reason))
+        // This block should probably have never been included in the
+        // original `Batch` implementation. An inner transaction never
+        // has a valid signature.
+        bool const neverValid = rules.enabled(fixBatchInnerSigs);
+        if (!neverValid)
         {
-            router.setFlags(id, SF_LOCALBAD);
-            return {Validity::SigGoodOnly, reason};
-        }
+            std::string reason;
+            if (!passesLocalChecks(tx, reason))
+            {
+                router.setFlags(id, SF_LOCALBAD);
+                return {Validity::SigGoodOnly, reason};
+            }
 
-        router.setFlags(id, SF_SIGGOOD);
-        return {Validity::Valid, ""};
+            router.setFlags(id, SF_SIGGOOD);
+            return {Validity::Valid, ""};
+        }
     }
 
     if (any(flags & SF_SIGBAD))

src/xrpld/overlay/detail/PeerImp.cpp

@@ -1365,8 +1365,7 @@ PeerImp::handleTransaction(
 
         // Charge strongly for attempting to relay a txn with tfInnerBatchTxn
         // LCOV_EXCL_START
-        if (stx->isFlag(tfInnerBatchTxn) &&
-            getCurrentTransactionRules()->enabled(featureBatch))
+        if (stx->isFlag(tfInnerBatchTxn))
         {
             JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing "
                                        "tfInnerBatchTxn (handleTransaction).";
@@ -2924,8 +2923,7 @@ PeerImp::checkTransaction(
     {
         // charge strongly for relaying batch txns
         // LCOV_EXCL_START
-        if (stx->isFlag(tfInnerBatchTxn) &&
-            getCurrentTransactionRules()->enabled(featureBatch))
+        if (stx->isFlag(tfInnerBatchTxn))
         {
             JLOG(p_journal_.warn()) << "Ignoring Network relayed Tx containing "
                                        "tfInnerBatchTxn (checkSignature).";

src/xrpld/rpc/handlers/LedgerEntry.cpp

@@ -18,32 +18,6 @@
 
 namespace ripple {
 
-using FunctionType = std::function<Expected<uint256, Json::Value>(
-    Json::Value const&,
-    Json::StaticString const,
-    unsigned apiVersion)>;
-
-static Expected<uint256, Json::Value>
-parseFixed(
-    Keylet const& keylet,
-    Json::Value const& params,
-    Json::StaticString const& fieldName,
-    unsigned apiVersion);
-
-// Helper function to return FunctionType for objects that have a fixed
-// location. That is, they don't take parameters to compute the index.
-// e.g. amendments, fees, negative UNL, etc.
-static FunctionType
-fixed(Keylet const& keylet)
-{
-    return [&keylet](
-               Json::Value const& params,
-               Json::StaticString const fieldName,
-               unsigned apiVersion) -> Expected<uint256, Json::Value> {
-        return parseFixed(keylet, params, fieldName, apiVersion);
-    };
-}
-
 static Expected<uint256, Json::Value>
 parseObjectID(
     Json::Value const& params,
@@ -59,33 +33,13 @@ parseObjectID(
 }
 
 static Expected<uint256, Json::Value>
-parseIndex(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseIndex(Json::Value const& params, Json::StaticString const fieldName)
 {
-    if (apiVersion > 2u && params.isString())
-    {
-        std::string const index = params.asString();
-        if (index == jss::amendments.c_str())
-            return keylet::amendments().key;
-        if (index == jss::fee.c_str())
-            return keylet::fees().key;
-        if (index == jss::nunl)
-            return keylet::negativeUNL().key;
-        if (index == jss::hashes)
-            // Note this only finds the "short" skip list. Use "hashes":index to
-            // get the long list.
-            return keylet::skip().key;
-    }
     return parseObjectID(params, fieldName, "hex string");
 }
 
 static Expected<uint256, Json::Value>
-parseAccountRoot(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseAccountRoot(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (auto const account = LedgerEntryHelpers::parse<AccountID>(params))
     {
@@ -96,13 +50,14 @@ parseAccountRoot(
         "malformedAddress", fieldName, "AccountID");
 }
 
-auto const parseAmendments = fixed(keylet::amendments());
+static Expected<uint256, Json::Value>
+parseAmendments(Json::Value const& params, Json::StaticString const fieldName)
+{
+    return parseObjectID(params, fieldName, "hex string");
+}
 
 static Expected<uint256, Json::Value>
-parseAMM(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseAMM(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -129,10 +84,7 @@ parseAMM(
 }
 
 static Expected<uint256, Json::Value>
-parseBridge(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseBridge(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isMember(jss::bridge))
     {
@@ -163,19 +115,13 @@ parseBridge(
 }
 
 static Expected<uint256, Json::Value>
-parseCheck(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseCheck(Json::Value const& params, Json::StaticString const fieldName)
 {
     return parseObjectID(params, fieldName, "hex string");
 }
 
 static Expected<uint256, Json::Value>
-parseCredential(
-    Json::Value const& cred,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseCredential(Json::Value const& cred, Json::StaticString const fieldName)
 {
     if (!cred.isObject())
     {
@@ -206,10 +152,7 @@ parseCredential(
 }
 
 static Expected<uint256, Json::Value>
-parseDelegate(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseDelegate(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -300,10 +243,7 @@ parseAuthorizeCredentials(Json::Value const& jv)
 }
 
 static Expected<uint256, Json::Value>
-parseDepositPreauth(
-    Json::Value const& dp,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseDepositPreauth(Json::Value const& dp, Json::StaticString const fieldName)
 {
     if (!dp.isObject())
     {
@@ -356,10 +296,7 @@ parseDepositPreauth(
 }
 
 static Expected<uint256, Json::Value>
-parseDID(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseDID(Json::Value const& params, Json::StaticString const fieldName)
 {
     auto const account = LedgerEntryHelpers::parse<AccountID>(params);
     if (!account)
@@ -374,8 +311,7 @@ parseDID(
 static Expected<uint256, Json::Value>
 parseDirectoryNode(
     Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+    Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -428,10 +364,7 @@ parseDirectoryNode(
 }
 
 static Expected<uint256, Json::Value>
-parseEscrow(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseEscrow(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -450,53 +383,20 @@ parseEscrow(
     return keylet::escrow(*id, *seq).key;
 }
 
-auto const parseFeeSettings = fixed(keylet::fees());
-
 static Expected<uint256, Json::Value>
-parseFixed(
-    Keylet const& keylet,
-    Json::Value const& params,
-    Json::StaticString const& fieldName,
-    unsigned apiVersion)
+parseFeeSettings(Json::Value const& params, Json::StaticString const fieldName)
 {
-    if (!params.isBool())
-    {
-        return parseObjectID(params, fieldName, "hex string");
-    }
-    if (!params.asBool())
-    {
-        return LedgerEntryHelpers::invalidFieldError(
-            "invalidParams", fieldName, "true");
-    }
-
-    return keylet.key;
+    return parseObjectID(params, fieldName, "hex string");
 }
 
 static Expected<uint256, Json::Value>
-parseLedgerHashes(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseLedgerHashes(Json::Value const& params, Json::StaticString const fieldName)
 {
-    if (params.isUInt() || params.isInt())
-    {
-        // If the index doesn't parse as a UInt, throw
-        auto const index = params.asUInt();
-
-        // Return the "long" skip list for the given ledger index.
-        auto const keylet = keylet::skip(index);
-        return keylet.key;
-    }
-    // Return the key in `params` or the "short" skip list, which contains
-    // hashes since the last flag ledger.
-    return parseFixed(keylet::skip(), params, fieldName, apiVersion);
+    return parseObjectID(params, fieldName, "hex string");
 }
 
 static Expected<uint256, Json::Value>
-parseMPToken(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseMPToken(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -519,8 +419,7 @@ parseMPToken(
 static Expected<uint256, Json::Value>
 parseMPTokenIssuance(
     Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+    Json::StaticString const fieldName)
 {
     auto const mptIssuanceID = LedgerEntryHelpers::parse<uint192>(params);
     if (!mptIssuanceID)
@@ -531,30 +430,25 @@ parseMPTokenIssuance(
 }
 
 static Expected<uint256, Json::Value>
-parseNFTokenOffer(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseNFTokenOffer(Json::Value const& params, Json::StaticString const fieldName)
 {
     return parseObjectID(params, fieldName, "hex string");
 }
 
 static Expected<uint256, Json::Value>
-parseNFTokenPage(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseNFTokenPage(Json::Value const& params, Json::StaticString const fieldName)
 {
     return parseObjectID(params, fieldName, "hex string");
 }
 
-auto const parseNegativeUNL = fixed(keylet::negativeUNL());
+static Expected<uint256, Json::Value>
+parseNegativeUNL(Json::Value const& params, Json::StaticString const fieldName)
+{
+    return parseObjectID(params, fieldName, "hex string");
+}
 
 static Expected<uint256, Json::Value>
-parseOffer(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseOffer(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -575,10 +469,7 @@ parseOffer(
 }
 
 static Expected<uint256, Json::Value>
-parseOracle(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseOracle(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -599,10 +490,7 @@ parseOracle(
 }
 
 static Expected<uint256, Json::Value>
-parsePayChannel(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parsePayChannel(Json::Value const& params, Json::StaticString const fieldName)
 {
     return parseObjectID(params, fieldName, "hex string");
 }
@@ -610,8 +498,7 @@ parsePayChannel(
 static Expected<uint256, Json::Value>
 parsePermissionedDomain(
     Json::Value const& pd,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+    Json::StaticString const fieldName)
 {
     if (pd.isString())
     {
@@ -640,8 +527,7 @@ parsePermissionedDomain(
 static Expected<uint256, Json::Value>
 parseRippleState(
     Json::Value const& jvRippleState,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+    Json::StaticString const fieldName)
 {
     Currency uCurrency;
 
@@ -691,19 +577,13 @@ parseRippleState(
 }
 
 static Expected<uint256, Json::Value>
-parseSignerList(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseSignerList(Json::Value const& params, Json::StaticString const fieldName)
 {
     return parseObjectID(params, fieldName, "hex string");
 }
 
 static Expected<uint256, Json::Value>
-parseTicket(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseTicket(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -724,10 +604,7 @@ parseTicket(
 }
 
 static Expected<uint256, Json::Value>
-parseVault(
-    Json::Value const& params,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+parseVault(Json::Value const& params, Json::StaticString const fieldName)
 {
     if (!params.isObject())
     {
@@ -750,8 +627,7 @@ parseVault(
 static Expected<uint256, Json::Value>
 parseXChainOwnedClaimID(
     Json::Value const& claim_id,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+    Json::StaticString const fieldName)
 {
     if (!claim_id.isObject())
     {
@@ -776,8 +652,7 @@ parseXChainOwnedClaimID(
 static Expected<uint256, Json::Value>
 parseXChainOwnedCreateAccountClaimID(
     Json::Value const& claim_id,
-    Json::StaticString const fieldName,
-    unsigned apiVersion)
+    Json::StaticString const fieldName)
 {
     if (!claim_id.isObject())
     {
@@ -801,6 +676,10 @@ parseXChainOwnedCreateAccountClaimID(
     return keylet.key;
 }
 
+using FunctionType = Expected<uint256, Json::Value> (*)(
+    Json::Value const&,
+    Json::StaticString const);
+
 struct LedgerEntry
 {
     Json::StaticString fieldName;
@@ -833,7 +712,7 @@ doLedgerEntry(RPC::JsonContext& context)
         {jss::ripple_state, parseRippleState, ltRIPPLE_STATE},
     });
 
-    auto const hasMoreThanOneMember = [&]() {
+    auto hasMoreThanOneMember = [&]() {
         int count = 0;
 
         for (auto const& ledgerEntry : ledgerEntryParsers)
@@ -877,8 +756,8 @@ doLedgerEntry(RPC::JsonContext& context)
                 Json::Value const& params = ledgerEntry.fieldName == jss::bridge
                     ? context.params
                     : context.params[ledgerEntry.fieldName];
-                auto const result = ledgerEntry.parseFunction(
-                    params, ledgerEntry.fieldName, context.apiVersion);
+                auto const result =
+                    ledgerEntry.parseFunction(params, ledgerEntry.fieldName);
                 if (!result)
                     return result.error();
 
@@ -909,13 +788,9 @@ doLedgerEntry(RPC::JsonContext& context)
             throw;
     }
 
-    // Return the computed index regardless of whether the node exists.
-    jvResult[jss::index] = to_string(uNodeIndex);
-
     if (uNodeIndex.isZero())
     {
-        RPC::inject_error(rpcENTRY_NOT_FOUND, jvResult);
-        return jvResult;
+        return RPC::make_error(rpcENTRY_NOT_FOUND);
     }
 
     auto const sleNode = lpLedger->read(keylet::unchecked(uNodeIndex));
@@ -927,14 +802,12 @@ doLedgerEntry(RPC::JsonContext& context)
     if (!sleNode)
     {
         // Not found.
-        RPC::inject_error(rpcENTRY_NOT_FOUND, jvResult);
-        return jvResult;
+        return RPC::make_error(rpcENTRY_NOT_FOUND);
     }
 
     if ((expectedType != ltANY) && (expectedType != sleNode->getType()))
     {
-        RPC::inject_error(rpcUNEXPECTED_LEDGER_TYPE, jvResult);
-        return jvResult;
+        return RPC::make_error(rpcUNEXPECTED_LEDGER_TYPE);
     }
 
     if (bNodeBinary)
@@ -944,10 +817,12 @@ doLedgerEntry(RPC::JsonContext& context)
         sleNode->add(s);
 
         jvResult[jss::node_binary] = strHex(s.peekData());
+        jvResult[jss::index] = to_string(uNodeIndex);
     }
     else
     {
         jvResult[jss::node] = sleNode->getJson(JsonOptions::none);
+        jvResult[jss::index] = to_string(uNodeIndex);
     }
 
     return jvResult;