Merge remote-tracking branch 'XRPLF/develop' into ximinez/fix-batchinnersigs

* XRPLF/develop:
  Implement Lending Protocol (unsupported) (5270)

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/libxrpl/ledger/ApplyView.cpp

@@ -256,7 +256,6 @@ ApplyView::dirRemove(
     uint256 const& key,
     bool keepRoot)
 {
-    keepRoot = false;
     auto node = peek(keylet::page(directory, page));
 
     if (!node)

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();
         }
@@ -4353,6 +4356,7 @@ public:
         using namespace test::jtx;
         auto const sa = testable_amendments();
         testWithFeats(sa);
+        testWithFeats(sa - fixBatchInnerSigs);
     }
 };
 

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/InvariantCheck.cpp

@@ -3553,42 +3553,4 @@ ValidVault::finalize(
     return true;
 }
 
-//------------------------------------------------------------------------------
-
-void
-NoEmptyDirectory::visitEntry(
-    bool isDelete,
-    std::shared_ptr<SLE const> const& before,
-    std::shared_ptr<SLE const> const& after)
-{
-    if (isDelete)
-        return;
-    if (before && before->getType() != ltDIR_NODE)
-        return;
-    if (after && after->getType() != ltDIR_NODE)
-        return;
-    if (!after->isFieldPresent(sfOwner))
-        // Not an account dir
-        return;
-
-    bad_ = after->at(sfIndexes).empty();
-}
-
-bool
-NoEmptyDirectory::finalize(
-    STTx const& tx,
-    TER const result,
-    XRPAmount const,
-    ReadView const& view,
-    beast::Journal const& j)
-{
-    if (bad_)
-    {
-        JLOG(j.fatal()) << "Invariant failed: empty owner directory.";
-        return false;
-    }
-
-    return true;
-}
-
 }  // namespace ripple

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

@@ -901,30 +901,6 @@ public:
         beast::Journal const&);
 };
 
-/**
- * @brief Invariants: An account's directory should never be empty
- *
- */
-class NoEmptyDirectory
-{
-    bool bad_ = false;
-
-public:
-    void
-    visitEntry(
-        bool,
-        std::shared_ptr<SLE const> const&,
-        std::shared_ptr<SLE const> const&);
-
-    bool
-    finalize(
-        STTx const&,
-        TER const,
-        XRPAmount const,
-        ReadView const&,
-        beast::Journal const&);
-};
-
 // additional invariant checks can be declared above and then added to this
 // tuple
 using InvariantChecks = std::tuple<
@@ -951,8 +927,7 @@ using InvariantChecks = std::tuple<
     ValidPseudoAccounts,
     ValidLoanBroker,
     ValidLoan,
-    ValidVault,
-    NoEmptyDirectory>;
+    ValidVault>;
 
 /**
  * @brief get a tuple of all invariant checks

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

@@ -671,7 +671,10 @@ Transactor::checkSign(
 
     auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey);
     // Ignore signature check on batch inner transactions
-    if (parentBatchId && 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() ||

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