Merge branch 'develop' into ximinez/lending-XLS-66

src/test/rpc/Simulate_test.cpp

@@ -720,7 +720,11 @@ class Simulate_test : public beast::unit_test::suite
                                       Json::Value const& tx) {
                 auto result = resp[jss::result];
                 checkBasicReturnValidity(
-                    result, tx, env.seq(alice), env.current()->fees().base * 2);
+                    result,
+                    tx,
+                    env.seq(alice),
+                    tx.isMember(jss::Signers) ? env.current()->fees().base * 2
+                                              : env.current()->fees().base);
 
                 BEAST_EXPECT(result[jss::engine_result] == "tesSUCCESS");
                 BEAST_EXPECT(result[jss::engine_result_code] == 0);
@@ -762,6 +766,10 @@ class Simulate_test : public beast::unit_test::suite
             tx[jss::Account] = alice.human();
             tx[jss::TransactionType] = jss::AccountSet;
             tx[sfDomain] = newDomain;
+
+            // test with autofill
+            testTx(env, tx, validateOutput, false);
+
             tx[sfSigners] = Json::arrayValue;
             {
                 Json::Value signer;
@@ -771,7 +779,7 @@ class Simulate_test : public beast::unit_test::suite
                 tx[sfSigners].append(signerOuter);
             }
 
-            // test with autofill
+            // test with just signer accounts
             testTx(env, tx, validateOutput, false);
 
             tx[sfSigningPubKey] = "";
@@ -780,8 +788,7 @@ class Simulate_test : public beast::unit_test::suite
             // transaction requires a non-base fee
             tx[sfFee] =
                 (env.current()->fees().base * 2).jsonClipped().asString();
-            tx[sfSigners][0u][sfSigner][jss::SigningPubKey] =
+            tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = "";
-                strHex(becky.pk().slice());
             tx[sfSigners][0u][sfSigner][jss::TxnSignature] = "";
 
             // test without autofill
@@ -830,11 +837,12 @@ class Simulate_test : public beast::unit_test::suite
             tx[jss::Account] = env.master.human();
             tx[jss::TransactionType] = jss::AccountSet;
             tx[sfDomain] = newDomain;
+            // master key is disabled, so this is invalid
+            tx[jss::SigningPubKey] = strHex(env.master.pk().slice());
 
             // test with autofill
             testTx(env, tx, testSimulation);
 
-            tx[sfSigningPubKey] = "";
             tx[sfTxnSignature] = "";
             tx[sfSequence] = env.seq(env.master);
             tx[sfFee] = env.current()->fees().base.jsonClipped().asString();
@@ -844,6 +852,79 @@ class Simulate_test : public beast::unit_test::suite
         }
     }
 
+    void
+    testInvalidSingleAndMultiSigningTransaction()
+    {
+        testcase(
+            "Transaction with both single-signing SigningPubKey and "
+            "multi-signing Signers");
+
+        using namespace jtx;
+        Env env(*this);
+        static auto const newDomain = "123ABC";
+        Account const alice("alice");
+        Account const becky("becky");
+        Account const carol("carol");
+        env.fund(XRP(10000), alice);
+        env.close();
+
+        // set up valid multisign
+        env(signers(alice, 1, {{becky, 1}, {carol, 1}}));
+        env.close();
+
+        {
+            std::function<void(Json::Value const&, Json::Value const&)> const&
+                testSimulation = [&](Json::Value const& resp,
+                                     Json::Value const& tx) {
+                    auto result = resp[jss::result];
+                    checkBasicReturnValidity(
+                        result,
+                        tx,
+                        env.seq(env.master),
+                        env.current()->fees().base * 2);
+
+                    BEAST_EXPECT(result[jss::engine_result] == "temINVALID");
+                    BEAST_EXPECT(result[jss::engine_result_code] == -277);
+                    BEAST_EXPECT(
+                        result[jss::engine_result_message] ==
+                        "The transaction is ill-formed.");
+
+                    BEAST_EXPECT(
+                        !result.isMember(jss::meta) &&
+                        !result.isMember(jss::meta_blob));
+                };
+
+            Json::Value tx;
+
+            tx[jss::Account] = env.master.human();
+            tx[jss::TransactionType] = jss::AccountSet;
+            tx[sfDomain] = newDomain;
+            // master key is disabled, so this is invalid
+            tx[jss::SigningPubKey] = strHex(env.master.pk().slice());
+            tx[sfSigners] = Json::arrayValue;
+            {
+                Json::Value signer;
+                signer[jss::Account] = becky.human();
+                Json::Value signerOuter;
+                signerOuter[sfSigner] = signer;
+                tx[sfSigners].append(signerOuter);
+            }
+
+            // test with autofill
+            testTx(env, tx, testSimulation, false);
+
+            tx[sfTxnSignature] = "";
+            tx[sfSequence] = env.seq(env.master);
+            tx[sfFee] = env.current()->fees().base.jsonClipped().asString();
+            tx[sfSigners][0u][sfSigner][jss::SigningPubKey] =
+                strHex(becky.pk().slice());
+            tx[sfSigners][0u][sfSigner][jss::TxnSignature] = "";
+
+            // test without autofill
+            testTx(env, tx, testSimulation);
+        }
+    }
+
     void
     testMultisignedBadPubKey()
     {
@@ -1117,6 +1198,7 @@ public:
         testTransactionTecFailure();
         testSuccessfulTransactionMultisigned();
         testTransactionSigningFailure();
+        testInvalidSingleAndMultiSigningTransaction();
         testMultisignedBadPubKey();
         testDeleteExpiredCredentials();
         testSuccessfulTransactionNetworkID();

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

@@ -502,15 +502,17 @@ InboundLedger::trigger(std::shared_ptr<Peer> const& peer, TriggerReason reason)
 
     if (auto stream = journal_.debug())
     {
-        stream << "Trigger acquiring ledger " << hash_;
+        std::stringstream ss;
+        ss << "Trigger acquiring ledger " << hash_;
         if (peer)
-            stream << " from " << peer;
+            ss << " from " << peer;
 
         if (complete_ || failed_)
-            stream << "complete=" << complete_ << " failed=" << failed_;
+            ss << " complete=" << complete_ << " failed=" << failed_;
         else
-            stream << "header=" << mHaveHeader << " tx=" << mHaveTransactions
+            ss << " header=" << mHaveHeader << " tx=" << mHaveTransactions
                << " as=" << mHaveState;
+        stream << ss.str();
     }
 
     if (!mHaveHeader)

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

@@ -206,6 +206,12 @@ preflightCheckSimulateKeys(
                 return temINVALID;  // LCOV_EXCL_LINE
             }
         }
+
+        if (!sigObject.getFieldVL(sfSigningPubKey).empty())
+        {
+            // trying to single-sign _and_ multi-sign a transaction
+            return temINVALID;
+        }
         return tesSUCCESS;
     }
     return {};
@@ -363,8 +369,8 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee)
 
     if (balance < feePaid)
     {
-        JLOG(ctx.j.trace()) << "Insufficient balance:"
+        JLOG(ctx.j.trace())
-                            << " balance=" << to_string(balance)
+            << "Insufficient balance:" << " balance=" << to_string(balance)
             << " paid=" << to_string(feePaid);
 
         if ((balance > beast::zero) && !ctx.view.open())
@@ -651,13 +657,13 @@ Transactor::checkSign(
             return tefBAD_AUTH;
     }
 
+    auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey);
     // Ignore signature check on batch inner transactions
     if (sigObject.isFlag(tfInnerBatchTxn) &&
         ctx.view.rules().enabled(featureBatch))
     {
         // Defensive Check: These values are also checked in Batch::preflight
-        if (sigObject.isFieldPresent(sfTxnSignature) ||
+        if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() ||
-            !sigObject.getFieldVL(sfSigningPubKey).empty() ||
             sigObject.isFieldPresent(sfSigners))
         {
             return temINVALID_FLAG;  // LCOV_EXCL_LINE
@@ -665,23 +671,28 @@ Transactor::checkSign(
         return tesSUCCESS;
     }
 
+    if ((ctx.flags & tapDRY_RUN) && pkSigner.empty() &&
+        !sigObject.isFieldPresent(sfSigners))
+    {
+        // simulate: skip signature validation when neither SigningPubKey nor
+        // Signers are provided
+        return tesSUCCESS;
+    }
+
     // If the pk is empty and not simulate or simulate and signers,
     // then we must be multi-signing.
-    if ((ctx.flags & tapDRY_RUN && sigObject.isFieldPresent(sfSigners)) ||
+    if (ctx.tx.isFieldPresent(sfSigners))
-        (!(ctx.flags & tapDRY_RUN) &&
-         sigObject.getFieldVL(sfSigningPubKey).empty()))
     {
         return checkMultiSign(ctx, idAccount, sigObject);
     }
 
     // Check Single Sign
-    auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey);
     // This ternary is only needed to handle `simulate`
     XRPL_ASSERT(
-        (ctx.flags & tapDRY_RUN) || !pkSigner.empty(),
+        !pkSigner.empty(),
         "ripple::Transactor::checkSingleSign : non-empty signer or simulation");
 
-    if (!(ctx.flags & tapDRY_RUN) && !publicKeyType(makeSlice(pkSigner)))
+    if (!publicKeyType(makeSlice(pkSigner)))
     {
         JLOG(ctx.j.trace())
             << "checkSingleSign: signing public key type is unknown";
@@ -884,14 +895,15 @@ Transactor::checkMultiSign(
         // public key.
         auto const spk = txSigner.getFieldVL(sfSigningPubKey);
 
-        if (!(ctx.flags & tapDRY_RUN) && !publicKeyType(makeSlice(spk)))
+        // spk being non-empty in non-simulate is checked in
+        // STTx::checkMultiSign
+        if (!spk.empty() && !publicKeyType(makeSlice(spk)))
         {
             JLOG(ctx.j.trace())
                 << "checkMultiSign: signing public key type is unknown";
             return tefBAD_SIGNATURE;
         }
 
-        // This ternary is only needed to handle `simulate`
         XRPL_ASSERT(
             (ctx.flags & tapDRY_RUN) || !spk.empty(),
             "ripple::Transactor::checkMultiSign : non-empty signer or "