diff --git a/src/test/rpc/Simulate_test.cpp b/src/test/rpc/Simulate_test.cpp index a4360ccc8b..5b3c0d2372 100644 --- a/src/test/rpc/Simulate_test.cpp +++ b/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] = - strHex(becky.pk().slice()); + tx[sfSigners][0u][sfSigner][jss::SigningPubKey] = ""; 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 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(); diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index c1eed3a9f3..eafa939506 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -502,15 +502,17 @@ InboundLedger::trigger(std::shared_ptr 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 - << " as=" << mHaveState; + ss << " header=" << mHaveHeader << " tx=" << mHaveTransactions + << " as=" << mHaveState; + stream << ss.str(); } if (!mHaveHeader) diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 5a7da4bc3d..b27400767b 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/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,9 +369,9 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) if (balance < feePaid) { - JLOG(ctx.j.trace()) << "Insufficient balance:" - << " balance=" << to_string(balance) - << " paid=" << to_string(feePaid); + JLOG(ctx.j.trace()) + << "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) || - !sigObject.getFieldVL(sfSigningPubKey).empty() || + if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.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)) || - (!(ctx.flags & tapDRY_RUN) && - sigObject.getFieldVL(sfSigningPubKey).empty())) + if (ctx.tx.isFieldPresent(sfSigners)) { 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 "