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/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).";