ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-12-06 17:27:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Improve multi-sign usage of simulate (#5479)

Browse Source 
This change allows users to submit simulate requests from a multi-sign account without needing to specify the accounts that are doing the multi-signing, and fixes an error with simulate that allowed double-"signed" (both single-sign and multi-sign public keys are provided) transactions.
This commit is contained in:
Mayukha Vadari
2025-06-10 14:47:27 +08:00
committed by GitHub
parent d494bf45b2
commit 35a40a8e62
2 changed files with 112 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

92
src/test/rpc/Simulate_test.cpp
View File

@@ -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<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();

38
src/xrpld/app/tx/detail/Transactor.cpp
View File

@@ -184,6 +184,12 @@ preflight2(PreflightContext const& ctx)
return temINVALID; // LCOV_EXCL_LINE
}
}
if (!ctx.tx.getSigningPubKey().empty())
{
// trying to single-sign _and_ multi-sign a transaction
return temINVALID;
}
return tesSUCCESS;
}
@@ -297,9 +303,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())
{
@@ -571,13 +577,13 @@ Transactor::apply()
NotTEC
Transactor::checkSign(PreclaimContext const& ctx)
{
auto const pkSigner = ctx.tx.getSigningPubKey();
// Ignore signature check on batch inner transactions
if (ctx.tx.isFlag(tfInnerBatchTxn) &&
ctx.view.rules().enabled(featureBatch))
{
// Defensive Check: These values are also checked in Batch::preflight
if (ctx.tx.isFieldPresent(sfTxnSignature) ||
!ctx.tx.getSigningPubKey().empty() ||
if (ctx.tx.isFieldPresent(sfTxnSignature) || !pkSigner.empty() ||
ctx.tx.isFieldPresent(sfSigners))
{
return temINVALID_FLAG; // LCOV_EXCL_LINE
@@ -585,25 +591,30 @@ Transactor::checkSign(PreclaimContext const& ctx)
return tesSUCCESS;
}
if ((ctx.flags & tapDRY_RUN) && pkSigner.empty() &&
!ctx.tx.isFieldPresent(sfSigners))
{
// simulate: skip signature validation when neither SigningPubKey nor
// Signers are provided
return tesSUCCESS;
}
auto const idAccount = ctx.tx[~sfDelegate].value_or(ctx.tx[sfAccount]);
// If the pk is empty and not simulate or simulate and signers,
// then we must be multi-signing.
if ((ctx.flags & tapDRY_RUN && ctx.tx.isFieldPresent(sfSigners)) ||
(!(ctx.flags & tapDRY_RUN) && ctx.tx.getSigningPubKey().empty()))
if (ctx.tx.isFieldPresent(sfSigners))
{
STArray const& txSigners(ctx.tx.getFieldArray(sfSigners));
return checkMultiSign(ctx.view, idAccount, txSigners, ctx.flags, ctx.j);
}
// Check Single Sign
auto const pkSigner = ctx.tx.getSigningPubKey();
// 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";
@@ -798,14 +809,15 @@ Transactor::checkMultiSign(
// public key.
auto const spk = txSigner.getFieldVL(sfSigningPubKey);
if (!(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(j.trace())
<< "checkMultiSign: signing public key type is unknown";
return tefBAD_SIGNATURE;
}
// This ternary is only needed to handle `simulate`
XRPL_ASSERT(
(flags & tapDRY_RUN) || !spk.empty(),
"ripple::Transactor::checkMultiSign : non-empty signer or "