Merge branch 'develop' into ximinez/fix-batchinnersigs

- The only significant changes are in testInnerSubmitRPC.
  - Enumerate the test cases.
  - With the fix amendment, a transaction never reaches the transaction
    engine (Transactor and derived classes.)
  - Test submitting a pseudo-transaction. Stopped before reaching the
    transaction engine, but with different errors.
- The tests verify that without the amendment, a transaction with
  tfInnerBatchTxn is immediately rejected. Without the amendment, things
  are safe. The amendment just makes things safer and more future-proof.

include/xrpl/protocol/detail/features.macro

@@ -16,6 +16,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(LendingProtocol,            Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FEATURE(PermissionDelegationV1_1,   Supported::no,  VoteBehavior::DefaultNo)
 XRPL_FIX    (DirectoryLimit,             Supported::yes, VoteBehavior::DefaultNo)

src/test/app/Batch_test.cpp

@@ -148,15 +148,21 @@ class Batch_test : public beast::unit_test::suite
     void
     testEnable(FeatureBitset features)
     {
-        testcase("enabled");
-
         using namespace test::jtx;
         using namespace std::literals;
 
+        bool const withInnerSigFix = features[fixBatchInnerSigs];
+
         for (bool const withBatch : {true, false})
         {
+            testcase << "enabled: Batch "
+                     << (withBatch ? "enabled" : "disabled")
+                     << ", Inner Sig Fix: "
+                     << (withInnerSigFix ? "enabled" : "disabled");
+
             auto const amend = withBatch ? features : features - featureBatch;
-            test::jtx::Env env{*this, envconfig(), amend};
+
+            test::jtx::Env env{*this, amend};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -179,7 +185,7 @@ class Batch_test : public beast::unit_test::suite
 
             // tfInnerBatchTxn
             // If the feature is disabled, the transaction fails with
-            // temINVALID_FLAG If the feature is enabled, the transaction fails
+            // temINVALID_FLAG. If the feature is enabled, the transaction fails
             // early in checkValidity()
             {
                 auto const txResult =
@@ -205,7 +211,7 @@ class Batch_test : public beast::unit_test::suite
         //----------------------------------------------------------------------
         // preflight
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -617,7 +623,7 @@ class Batch_test : public beast::unit_test::suite
         //----------------------------------------------------------------------
         // preclaim
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -858,7 +864,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -949,7 +955,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -1187,7 +1193,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Bad Fee Without Signer
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1209,7 +1215,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Bad Fee With MultiSign
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1236,7 +1242,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Bad Fee With MultiSign + BatchSigners
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1265,7 +1271,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Bad Fee With MultiSign + BatchSigners.Signers
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1297,7 +1303,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Bad Fee With BatchSigners
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1321,7 +1327,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Bad Fee Dynamic Fee Calculation
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1361,7 +1367,7 @@ class Batch_test : public beast::unit_test::suite
 
         // telENV_RPC_FAILED: Batch: txns array exceeds 8 entries.
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1386,7 +1392,7 @@ class Batch_test : public beast::unit_test::suite
 
         // temARRAY_TOO_LARGE: Batch: txns array exceeds 8 entries.
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1419,7 +1425,7 @@ class Batch_test : public beast::unit_test::suite
 
         // telENV_RPC_FAILED: Batch: signers array exceeds 8 entries.
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1438,7 +1444,7 @@ class Batch_test : public beast::unit_test::suite
 
         // temARRAY_TOO_LARGE: Batch: signers array exceeds 8 entries.
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -1472,7 +1478,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -1608,7 +1614,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -1840,7 +1846,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2062,7 +2068,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2248,14 +2254,26 @@ class Batch_test : public beast::unit_test::suite
     }
 
     void
-    testInnerSubmitRPC(FeatureBitset features)
+    doTestInnerSubmitRPC(FeatureBitset features, bool withBatch)
     {
-        testcase("inner submit rpc");
+        bool const withInnerSigFix = features[fixBatchInnerSigs];
+
+        std::string const testName = [&]() {
+            std::stringstream ss;
+            ss << "inner submit rpc: batch "
+               << (withBatch ? "enabled" : "disabled") << ", inner sig fix: "
+               << (withInnerSigFix ? "enabled" : "disabled") << ": ";
+            return ss.str();
+        }();
+
+        auto const amend = withBatch ? features : features - featureBatch;
 
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, amend};
+        if (!BEAST_EXPECT(amend[featureBatch] == withBatch))
+            return;
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2263,75 +2281,170 @@ class Batch_test : public beast::unit_test::suite
         env.fund(XRP(10000), alice, bob);
         env.close();
 
-        auto submitAndValidate = [&](Slice const& slice) {
-            auto const jrr = env.rpc("submit", strHex(slice))[jss::result];
-            BEAST_EXPECT(
-                jrr[jss::status] == "error" &&
-                jrr[jss::error] == "invalidTransaction" &&
-                jrr[jss::error_exception] ==
-                    "fails local checks: Malformed: Invalid inner batch "
-                    "transaction.");
-            env.close();
-        };
+        auto submitAndValidate =
+            [&](std::string caseName,
+                Slice const& slice,
+                int line,
+                std::optional<std::string> expectedEnabled = std::nullopt,
+                std::optional<std::string> expectedDisabled = std::nullopt,
+                bool expectInvalidFlag = false) {
+                testcase << testName << caseName
+                         << (expectInvalidFlag
+                                 ? " - Expected to reach tx engine!"
+                                 : "");
+                auto const jrr = env.rpc("submit", strHex(slice))[jss::result];
+                auto const expected = withBatch
+                    ? expectedEnabled.value_or(
+                          "fails local checks: Malformed: Invalid inner batch "
+                          "transaction.")
+                    : expectedDisabled.value_or(
+                          "fails local checks: Empty SigningPubKey.");
+                if (expectInvalidFlag)
+                {
+                    expect(
+                        jrr[jss::status] == "success" &&
+                            jrr[jss::engine_result] == "temINVALID_FLAG",
+                        pretty(jrr),
+                        __FILE__,
+                        line);
+                }
+                else
+                {
+                    expect(
+                        jrr[jss::status] == "error" &&
+                            jrr[jss::error] == "invalidTransaction" &&
+                            jrr[jss::error_exception] == expected,
+                        pretty(jrr),
+                        __FILE__,
+                        line);
+                }
+                env.close();
+            };
 
         // Invalid RPC Submission: TxnSignature
-        // - has `TxnSignature` field
+        // + has `TxnSignature` field
         // - has no `SigningPubKey` field
         // - has no `Signers` field
-        // - has `tfInnerBatchTxn` flag
+        // + has `tfInnerBatchTxn` flag
         {
             auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice));
             txn[sfTxnSignature] = "DEADBEEF";
             STParsedJSONObject parsed("test", txn.getTxn());
             Serializer s;
             parsed.object->add(s);
-            submitAndValidate(s.slice());
+            submitAndValidate("TxnSignature set", s.slice(), __LINE__);
         }
 
         // Invalid RPC Submission: SigningPubKey
         // - has no `TxnSignature` field
-        // - has `SigningPubKey` field
+        // + has `SigningPubKey` field
         // - has no `Signers` field
-        // - has `tfInnerBatchTxn` flag
+        // + has `tfInnerBatchTxn` flag
         {
             auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice));
             txn[sfSigningPubKey] = strHex(alice.pk());
             STParsedJSONObject parsed("test", txn.getTxn());
             Serializer s;
             parsed.object->add(s);
-            submitAndValidate(s.slice());
+            submitAndValidate(
+                "SigningPubKey set",
+                s.slice(),
+                __LINE__,
+                std::nullopt,
+                "fails local checks: Invalid signature.");
         }
 
         // Invalid RPC Submission: Signers
         // - has no `TxnSignature` field
-        // - has empty `SigningPubKey` field
-        // - has `Signers` field
-        // - has `tfInnerBatchTxn` flag
+        // + has empty `SigningPubKey` field
+        // + has `Signers` field
+        // + has `tfInnerBatchTxn` flag
         {
             auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice));
             txn[sfSigners] = Json::arrayValue;
             STParsedJSONObject parsed("test", txn.getTxn());
             Serializer s;
             parsed.object->add(s);
-            submitAndValidate(s.slice());
+            submitAndValidate(
+                "Signers set",
+                s.slice(),
+                __LINE__,
+                std::nullopt,
+                "fails local checks: Invalid Signers array size.");
+        }
+
+        {
+            // Fully signed inner batch transaction
+            auto const txn =
+                batch::inner(pay(alice, bob, XRP(1)), env.seq(alice));
+            auto const jt = env.jt(txn.getTxn());
+
+            STParsedJSONObject parsed("test", jt.jv);
+            Serializer s;
+            parsed.object->add(s);
+            submitAndValidate(
+                "Fully signed",
+                s.slice(),
+                __LINE__,
+                std::nullopt,
+                std::nullopt,
+                !withBatch);
         }
 
         // Invalid RPC Submission: tfInnerBatchTxn
         // - has no `TxnSignature` field
-        // - has empty `SigningPubKey` field
+        // + has empty `SigningPubKey` field
         // - has no `Signers` field
-        // - has `tfInnerBatchTxn` flag
+        // + has `tfInnerBatchTxn` flag
         {
             auto txn = batch::inner(pay(alice, bob, XRP(1)), env.seq(alice));
             STParsedJSONObject parsed("test", txn.getTxn());
             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");
+            submitAndValidate(
+                "No signing fields set",
+                s.slice(),
+                __LINE__,
+                "fails local checks: Empty SigningPubKey.",
+                "fails local checks: Empty SigningPubKey.",
+                withBatch && !withInnerSigFix);
+        }
 
-            env.close();
+        // Invalid RPC Submission: tfInnerBatchTxn pseudo-transaction
+        // - has no `TxnSignature` field
+        // + has empty `SigningPubKey` field
+        // - has no `Signers` field
+        // + has `tfInnerBatchTxn` flag
+        {
+            STTx amendTx(
+                ttAMENDMENT, [seq = env.closed()->header().seq + 1](auto& obj) {
+                    obj.setAccountID(sfAccount, AccountID());
+                    obj.setFieldH256(sfAmendment, fixBatchInnerSigs);
+                    obj.setFieldU32(sfLedgerSequence, seq);
+                    obj.setFieldU32(sfFlags, tfInnerBatchTxn);
+                });
+            auto txn = batch::inner(
+                amendTx.getJson(JsonOptions::none), env.seq(alice));
+            STParsedJSONObject parsed("test", txn.getTxn());
+            Serializer s;
+            parsed.object->add(s);
+            submitAndValidate(
+                "Pseudo-transaction",
+                s.slice(),
+                __LINE__,
+                withInnerSigFix
+                    ? "fails local checks: Empty SigningPubKey."
+                    : "fails local checks: Cannot submit pseudo transactions.",
+                "fails local checks: Empty SigningPubKey.");
+        }
+    }
+
+    void
+    testInnerSubmitRPC(FeatureBitset features)
+    {
+        for (bool const withBatch : {true, false})
+        {
+            doTestInnerSubmitRPC(features, withBatch);
         }
     }
 
@@ -2343,7 +2456,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2390,7 +2503,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2443,7 +2556,7 @@ class Batch_test : public beast::unit_test::suite
 
         // tfIndependent: account delete success
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -2484,7 +2597,7 @@ class Batch_test : public beast::unit_test::suite
 
         // tfIndependent: account delete fails
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -2529,7 +2642,7 @@ class Batch_test : public beast::unit_test::suite
 
         // tfAllOrNothing: account delete fails
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -2581,7 +2694,6 @@ class Batch_test : public beast::unit_test::suite
 
         test::jtx::Env env{
             *this,
-            envconfig(),
             features | featureSingleAssetVault | featureLendingProtocol |
                 featureMPTokensV1};
 
@@ -2776,7 +2888,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2889,7 +3001,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -2947,7 +3059,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -3009,7 +3121,7 @@ class Batch_test : public beast::unit_test::suite
             using namespace test::jtx;
             using namespace std::literals;
 
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -3058,7 +3170,7 @@ class Batch_test : public beast::unit_test::suite
             using namespace test::jtx;
             using namespace std::literals;
 
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -3106,7 +3218,7 @@ class Batch_test : public beast::unit_test::suite
             using namespace test::jtx;
             using namespace std::literals;
 
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -3169,7 +3281,7 @@ class Batch_test : public beast::unit_test::suite
             // overwritten by the payment in the batch transaction. Because the
             // terPRE_SEQ is outside of the batch this noop transaction will ge
             // reapplied in the following ledger
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob, carol);
             env.close();
 
@@ -3216,7 +3328,7 @@ class Batch_test : public beast::unit_test::suite
             // IMPORTANT: The batch txn is applied first, then the noop txn.
             // Because of this ordering, the noop txn is not applied and is
             // overwritten by the payment in the batch transaction.
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3258,7 +3370,7 @@ class Batch_test : public beast::unit_test::suite
             // IMPORTANT: The batch txn is applied first, then the noop txn.
             // Because of this ordering, the noop txn is not applied and is
             // overwritten by the payment in the batch transaction.
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3295,7 +3407,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Outer Batch terPRE_SEQ
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob, carol);
             env.close();
 
@@ -3353,7 +3465,7 @@ class Batch_test : public beast::unit_test::suite
             // IMPORTANT: The batch txn is applied first, then the noop txn.
             // Because of this ordering, the noop txn is not applied and is
             // overwritten by the payment in the batch transaction.
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3402,7 +3514,7 @@ class Batch_test : public beast::unit_test::suite
             // IMPORTANT: The batch txn is applied first, then the noop txn.
             // Because of this ordering, the noop txn is not applied and is
             // overwritten by the payment in the batch transaction.
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3464,7 +3576,7 @@ class Batch_test : public beast::unit_test::suite
             // batch will run in the close ledger process. The batch will be
             // allied and then retry this transaction in the current ledger.
 
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3511,7 +3623,7 @@ class Batch_test : public beast::unit_test::suite
 
         // Create Object Before Batch Txn
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3558,7 +3670,7 @@ class Batch_test : public beast::unit_test::suite
             // batch will run in the close ledger process. The batch will be
             // applied and then retry this transaction in the current ledger.
 
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             env.fund(XRP(10000), alice, bob);
             env.close();
 
@@ -3605,7 +3717,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
 
         auto const alice = Account("alice");
         auto const bob = Account("bob");
@@ -3644,7 +3756,7 @@ class Batch_test : public beast::unit_test::suite
         using namespace test::jtx;
         using namespace std::literals;
 
-        test::jtx::Env env{*this, envconfig()};
+        test::jtx::Env env{*this, features};
         XRPAmount const baseFee = env.current()->fees().base;
 
         auto const alice = Account("alice");
@@ -3725,6 +3837,7 @@ class Batch_test : public beast::unit_test::suite
                 *this,
                 makeSmallQueueConfig(
                     {{"minimum_txn_in_ledger_standalone", "2"}}),
+                features,
                 nullptr,
                 beast::severities::kError};
 
@@ -3785,6 +3898,7 @@ class Batch_test : public beast::unit_test::suite
                 *this,
                 makeSmallQueueConfig(
                     {{"minimum_txn_in_ledger_standalone", "2"}}),
+                features,
                 nullptr,
                 beast::severities::kError};
 
@@ -3892,7 +4006,7 @@ class Batch_test : public beast::unit_test::suite
 
         // delegated non atomic inner
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -3937,7 +4051,7 @@ class Batch_test : public beast::unit_test::suite
 
         // delegated atomic inner
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -3989,7 +4103,7 @@ class Batch_test : public beast::unit_test::suite
         // this also makes sure tfInnerBatchTxn won't block delegated AccountSet
         // with granular permission
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
 
             auto const alice = Account("alice");
             auto const bob = Account("bob");
@@ -4038,7 +4152,7 @@ class Batch_test : public beast::unit_test::suite
         // this also makes sure tfInnerBatchTxn won't block delegated
         // MPTokenIssuanceSet with granular permission
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             Account alice{"alice"};
             Account bob{"bob"};
             env.fund(XRP(100000), alice, bob);
@@ -4094,7 +4208,7 @@ class Batch_test : public beast::unit_test::suite
         // this also makes sure tfInnerBatchTxn won't block delegated TrustSet
         // with granular permission
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             Account gw{"gw"};
             Account alice{"alice"};
             Account bob{"bob"};
@@ -4134,7 +4248,7 @@ class Batch_test : public beast::unit_test::suite
 
         // inner transaction not authorized by the delegating account.
         {
-            test::jtx::Env env{*this, envconfig()};
+            test::jtx::Env env{*this, features};
             Account gw{"gw"};
             Account alice{"alice"};
             Account bob{"bob"};
@@ -4182,7 +4296,7 @@ class Batch_test : public beast::unit_test::suite
         testcase("Validate RPC response");
 
         using namespace jtx;
-        Env env(*this);
+        Env env(*this, features);
         Account const alice("alice");
         Account const bob("bob");
         env.fund(XRP(10000), alice, bob);
@@ -4259,7 +4373,7 @@ class Batch_test : public beast::unit_test::suite
     testBatchCalculateBaseFee(FeatureBitset features)
     {
         using namespace jtx;
-        Env env(*this);
+        Env env(*this, features);
         Account const alice("alice");
         Account const bob("bob");
         Account const carol("carol");
@@ -4384,6 +4498,7 @@ public:
     {
         using namespace test::jtx;
         auto const sa = testable_amendments();
+        testWithFeats(sa - fixBatchInnerSigs);
         testWithFeats(sa);
     }
 };

src/xrpld/app/misc/NetworkOPs.cpp

@@ -1681,7 +1681,7 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
                     // 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))
+                    !(sttx.isFlag(tfInnerBatchTxn)))
                 {
                     protocol::TMTransaction tx;
                     Serializer s;

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

@@ -204,8 +204,14 @@ Transactor::preflight2(PreflightContext const& ctx)
         // regardless of success or failure
         return *ret;
 
+    // It should be impossible for the InnerBatchTxn flag to be set without
+    // featureBatch being enabled
+    XRPL_ASSERT_PARTS(
+        !ctx.tx.isFlag(tfInnerBatchTxn) || ctx.rules.enabled(featureBatch),
+        "xrpl::Transactor::preflight2",
+        "InnerBatch flag only set if feature enabled");
     // Skip signature check on batch inner transactions
-    if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch))
+    if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatch))
         return tesSUCCESS;
     // Do not add any checks after this point that are relevant for
     // batch inner transactions. They will be skipped.

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))