ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-30 02:20:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: refactor checkBatchSign & checkSign

Browse Source
This commit is contained in:
Denis Angell
2026-06-29 18:15:15 -04:00
parent ec66319555
commit 095389672a
8 changed files with 118 additions and 168 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
include/xrpl/core/HashRouter.h
View File

@@ -28,9 +28,7 @@ enum class HashRouterFlags : std::uint16_t {
PRIVATE4 = 0x0800,
// Used in EscrowFinish.cpp
PRIVATE5 = 0x1000,
PRIVATE6 = 0x2000,
// Used in Batch.cpp
PRIVATE7 = 0x4000
PRIVATE6 = 0x2000
};
constexpr HashRouterFlags

3
include/xrpl/protocol/STTx.h
View File

@@ -12,6 +12,7 @@
#include <expected>
#include <functional>
#include <optional>
namespace xrpl {
@@ -162,7 +163,7 @@ private:
move(std::size_t n, void* buf) override;
friend class detail::STVar;
std::vector<uint256> batchTxnIds_;
std::optional<std::vector<uint256>> batchTxnIds_;
};
bool

7
include/xrpl/tx/Transactor.h
View File

@@ -370,7 +370,12 @@ protected:
std::optional<uint256 const> const& parentBatchId,
AccountID const& idAccount,
STObject const& sigObject,
beast::Journal const j);
beast::Journal const j,
// A batch may carry an inner from an account that an earlier inner
// creates, so the signer account need not exist yet; when it does not,
// only its own master key may authorize it. Normal transactions require
// the account to already exist.
bool permitUncreatedAccount = false);
// Base class always returns true
static bool

40
src/libxrpl/protocol/STTx.cpp
View File

@@ -281,6 +281,17 @@ STTx::checkSign(Rules const& rules) const
if (auto const ret = checkSign(rules, counterSig); !ret)
return std::unexpected("Counterparty: " + ret.error());
}
// Verify the batch signer signatures here too, so they are cached with the
// rest of signature checking (checkValidity / SF_SIGGOOD) and stay out of
// the transaction engine. Gated on a batch (batchTxnIds_ seated) that
// actually carries signers; a batch whose inners are all from the outer
// account has no sfBatchSigners and needs no signer crypto.
if (batchTxnIds_ && isFieldPresent(sfBatchSigners))
{
if (auto const ret = checkBatchSign(rules); !ret)
return ret;
}
return {};
}
@@ -581,12 +592,13 @@ STTx::buildBatchTxnIds()
auto const& raw = getFieldArray(sfRawTransactions);
if (raw.size() > kMaxBatchTxCount)
return;
batchTxnIds_.reserve(raw.size());
// Seated for any batch with raw transactions. The count is validated in
// preflight and at the relay boundary, so build every id here; this keeps
// the invariant batchTxnIds_->size() == rawTransactions.size().
auto& ids = batchTxnIds_.emplace();
ids.reserve(raw.size());
for (STObject const& rb : raw)
batchTxnIds_.push_back(rb.getHash(HashPrefix::TransactionId));
ids.push_back(rb.getHash(HashPrefix::TransactionId));
}
std::vector<uint256> const&
@@ -594,11 +606,11 @@ STTx::getBatchTransactionIDs() const
{
XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : batch transaction");
XRPL_ASSERT(
!batchTxnIds_.empty(), "STTx::getBatchTransactionIDs : batch transaction IDs not built");
batchTxnIds_.has_value(), "STTx::getBatchTransactionIDs : batch transaction IDs built");
XRPL_ASSERT(
batchTxnIds_.size() == getFieldArray(sfRawTransactions).size(),
batchTxnIds_->size() == getFieldArray(sfRawTransactions).size(),
"STTx::getBatchTransactionIDs : batch transaction IDs size mismatch");
return batchTxnIds_;
return *batchTxnIds_;
}
//------------------------------------------------------------------------------
@@ -740,10 +752,14 @@ isBatchRawTransactionOkay(STObject const& st, std::string& reason)
if (!st.isFieldPresent(sfRawTransactions))
return true;
// sfRawTransactions only appears on a Batch.
XRPL_ASSERT(
safeCast<TxType>(st.getFieldU16(sfTransactionType)) == ttBATCH,
"xrpl::isBatchRawTransactionOkay : batch transaction");
// sfRawTransactions only appears on a Batch. passesLocalChecks runs on
// unverified user and peer input, so reject (rather than assert) a non-batch
// transaction that carries it.
if (safeCast<TxType>(st.getFieldU16(sfTransactionType)) != ttBATCH)
{
reason = "Only Batch transactions may contain raw transactions.";
return false;
}
if (st.isFieldPresent(sfBatchSigners) &&
st.getFieldArray(sfBatchSigners).size() > kMaxBatchSigners)

31
src/libxrpl/tx/Transactor.cpp
View File

@@ -244,8 +244,11 @@ Transactor::preflight2(PreflightContext const& ctx)
return *ret;
}
// Skip signature check on batch inner transactions. These flag and
// parentBatchId checks duplicate preflight1 as defense in depth.
// Skip the signature check on batch inner transactions. preflight1 already
// enforces both conditions; re-checking them as defense in depth guarantees
// we never return success (and so skip signature validation) for an inner
// transaction unless the amendment is enabled and it really sits inside a
// batch.
if (ctx.tx.isFlag(tfInnerBatchTxn))
{
if (!ctx.rules.enabled(featureBatchV1_1))
@@ -710,16 +713,19 @@ Transactor::checkSign(
std::optional<uint256 const> const& parentBatchId,
AccountID const& idAccount,
STObject const& sigObject,
beast::Journal const j)
beast::Journal const j,
bool permitUncreatedAccount)
{
{
auto const sle = view.read(keylet::account(idAccount));
if (view.rules().enabled(featureLendingProtocol) && isPseudoAccount(sle))
if ((view.rules().enabled(featureLendingProtocol) ||
view.rules().enabled(featureBatchV1_1) || view.rules().enabled(fixCleanup3_3_0)) &&
isPseudoAccount(sle))
{
// Pseudo-accounts can't sign transactions. This check is gated on
// the Lending Protocol amendment because that's the project it was
// added under, and it doesn't justify another amendment
// Pseudo-accounts can't sign transactions. This check is gated on a
// few different amendments so that it takes effect as soon as any of
// them is activated.
return tefBAD_AUTH;
}
}
@@ -764,7 +770,16 @@ Transactor::checkSign(
auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner)));
auto const sleAccount = view.read(keylet::account(idAccount));
if (!sleAccount)
return terNO_ACCOUNT;
{
// An account that does not exist yet can only be authorized by its own
// master key, and only where an un-created signer is permitted (a batch
// whose earlier inner creates the account). Otherwise it cannot sign.
if (!permitUncreatedAccount)
return terNO_ACCOUNT;
if (idAccount != idSigner)
return tefBAD_AUTH;
return tesSUCCESS;
}
return checkSingleSign(view, idSigner, idAccount, sleAccount, j);
}

14
src/libxrpl/tx/apply.cpp
View File

@@ -8,7 +8,6 @@
#include <xrpl/core/ServiceRegistry.h>
#include <xrpl/ledger/ApplyView.h>
#include <xrpl/ledger/OpenView.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Rules.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STObject.h>
@@ -39,13 +38,14 @@ checkValidity(HashRouter& router, STTx const& tx, Rules const& rules)
auto const id = tx.getTransactionID();
auto const flags = router.getFlags(id);
// Ignore signature check on batch inner transactions
if (tx.isFlag(tfInnerBatchTxn) && rules.enabled(featureBatchV1_1))
// Batch inner transactions are never independently valid: they are applied
// within their batch, not through checkValidity. Reaching here means one was
// relayed or submitted on its own, so mark it bad regardless of the
// amendment (like PeerImp and NetworkOPs).
if (tx.isFlag(tfInnerBatchTxn))
{
// Defensive Check: These values are also checked in Batch::preflight
if (tx.isFieldPresent(sfTxnSignature) || !tx.getSigningPubKey().empty() ||
tx.isFieldPresent(sfSigners))
return {Validity::SigBad, "Malformed: Invalid inner batch transaction."};
router.setFlags(id, kSfSigbad);
return {Validity::SigBad, "Batch inner transactions are never considered validly signed."};
}
if (any(flags & kSfSigbad))

83
src/libxrpl/tx/transactors/system/Batch.cpp
View File

@@ -1,19 +1,14 @@
#include <xrpl/tx/transactors/system/Batch.h>
#include <xrpl/basics/Log.h>
#include <xrpl/basics/Slice.h>
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/core/HashRouter.h>
#include <xrpl/core/ServiceRegistry.h>
#include <xrpl/ledger/ApplyView.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/PublicKey.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/STObject.h>
@@ -38,10 +33,6 @@
namespace xrpl {
// Set on a batch's tx id once its signer signatures verify, so
// preflightSigValidated can skip the expensive re-check on later preflights.
constexpr HashRouterFlags kSfBatchSigGood = HashRouterFlags::PRIVATE7;
/**
* @brief Calculates the total base fee for a batch transaction.
*
@@ -171,7 +162,8 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx)
JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in signerFees calculation.";
return XRPAmount{kInitialXrp};
}
if (txnFees + signerFees > maxAmount - batchBase)
XRPAmount const innerFees = txnFees + signerFees;
if (innerFees > maxAmount - batchBase)
{
JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in total fee calculation.";
return XRPAmount{kInitialXrp};
@@ -179,7 +171,7 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx)
// LCOV_EXCL_STOP
// 10 drops per batch signature + sum of inner tx fees + batchBase
return signerFees + txnFees + batchBase;
return innerFees + batchBase;
}
std::uint32_t
@@ -485,7 +477,7 @@ Batch::preflightSigValidated(PreflightContext const& ctx)
requiredSigners[numReqSignersMatched] != signerAccount)
{
JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: "
<< "extra signer provided: " << signerAccount;
<< "missing signer or extra signer provided: " << signerAccount;
return temBAD_SIGNER;
}
++numReqSignersMatched;
@@ -502,25 +494,6 @@ Batch::preflightSigValidated(PreflightContext const& ctx)
return temBAD_SIGNER;
}
// Check the batch signer signatures (only present when there are signers).
if (ctx.tx.isFieldPresent(sfBatchSigners))
{
// Caches only the cryptographic signature check, not whether each
// signer is authorized to sign for its account - that ledger-dependent
// check is Batch::checkBatchSign in preclaim and is never cached.
auto& router = ctx.registry.get().getHashRouter();
if (!any(router.getFlags(parentBatchId) & kSfBatchSigGood))
{
if (auto const sigResult = ctx.tx.checkBatchSign(ctx.rules); !sigResult)
{
JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: "
<< "invalid batch txn signature: " << sigResult.error();
return temBAD_SIGNATURE;
}
router.setFlags(parentBatchId, kSfBatchSigGood);
}
}
return tesSUCCESS;
}
@@ -530,41 +503,21 @@ Batch::checkBatchSign(PreclaimContext const& ctx)
STArray const& signers{ctx.tx.getFieldArray(sfBatchSigners)};
for (auto const& signer : signers)
{
// Reuse the standard signer-authorization rules so the batch and
// non-batch paths cannot drift. permitUncreatedAccount allows an inner
// from an account that an earlier inner creates, which must be
// authorized by its own master key.
auto const idAccount = signer.getAccountID(sfAccount);
Blob const& pkSigner = signer.getFieldVL(sfSigningPubKey);
if (pkSigner.empty())
{
if (auto const ret = checkMultiSign(ctx.view, ctx.flags, idAccount, signer, ctx.j);
!isTesSuccess(ret))
return ret;
}
else
{
if (!publicKeyType(makeSlice(pkSigner)))
return tefBAD_AUTH; // LCOV_EXCL_LINE
auto const idSigner = calcAccountID(PublicKey(makeSlice(pkSigner)));
auto const sleAccount = ctx.view.read(keylet::account(idAccount));
if (sleAccount)
{
if (isPseudoAccount(sleAccount))
return tefBAD_AUTH; // LCOV_EXCL_LINE
if (auto const ret =
checkSingleSign(ctx.view, idSigner, idAccount, sleAccount, ctx.j);
!isTesSuccess(ret))
return ret;
}
else
{
if (idAccount != idSigner)
return tefBAD_AUTH;
// A batch can include transactions from an un-created account ONLY
// when the account master key is the signer
}
}
if (auto const ret = Transactor::checkSign(
ctx.view,
ctx.flags,
ctx.parentBatchId,
idAccount,
signer,
ctx.j,
/*permitUncreatedAccount=*/true);
!isTesSuccess(ret))
return ret;
}
return tesSUCCESS;
}

104
src/test/app/Batch_test.cpp
View File

@@ -232,12 +232,10 @@ class Batch_test : public beast::unit_test::Suite
}
// tfInnerBatchTxn
// If the feature is disabled, the transaction fails with
// temINVALID_FLAG. If the feature is enabled, the transaction fails
// early in checkValidity()
// A standalone transaction carrying this flag is never valid, so it
// is rejected early in checkValidity() regardless of the amendment.
{
auto const txResult = withBatch ? Ter(telENV_RPC_FAILED) : Ter(temINVALID_FLAG);
env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), txResult);
env(pay(alice, bob, XRP(1)), Txflags(tfInnerBatchTxn), Ter(telENV_RPC_FAILED));
env.close();
}
@@ -663,7 +661,7 @@ class Batch_test : public beast::unit_test::Suite
jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] =
strHex(Slice{sig.data(), sig.size()});
env(jt.jv, Ter(temBAD_SIGNATURE));
env(jt.jv, Ter(telENV_RPC_FAILED));
env.close();
}
@@ -2650,37 +2648,20 @@ class Batch_test : public beast::unit_test::Suite
env.fund(XRP(10000), alice, bob);
env.close();
auto submitAndValidate = [&](std::string caseName,
Slice const& slice,
int line,
std::optional<std::string> expectedEnabled = std::nullopt,
std::optional<std::string> expectedDisabled = std::nullopt,
bool expectInvalidFlag = false) {
testcase << testName << caseName
<< (expectInvalidFlag ? " - Expected to reach tx engine!" : "");
// Any transaction carrying tfInnerBatchTxn is rejected in checkValidity()
// before it reaches the tx engine, regardless of its signing fields or
// whether the amendment is enabled.
auto submitAndValidate = [&](std::string caseName, Slice const& slice, int line) {
testcase << testName << caseName;
auto const jrr = env.rpc("submit", strHex(slice))[jss::result];
auto const expected = withBatch
? expectedEnabled.value_or(
"fails local checks: Malformed: Invalid inner batch "
"transaction.")
: expectedDisabled.value_or("fails local checks: Empty SigningPubKey.");
if (expectInvalidFlag)
{
expect(
jrr[jss::status] == "success" && jrr[jss::engine_result] == "temINVALID_FLAG",
pretty(jrr),
__FILE__,
line);
}
else
{
expect(
jrr[jss::status] == "error" && jrr[jss::error] == "invalidTransaction" &&
jrr[jss::error_exception] == expected,
pretty(jrr),
__FILE__,
line);
}
expect(
jrr[jss::status] == "error" && jrr[jss::error] == "invalidTransaction" &&
jrr[jss::error_exception] ==
"fails local checks: Batch inner transactions are never "
"considered validly signed.",
pretty(jrr),
__FILE__,
line);
env.close();
};
@@ -2709,12 +2690,7 @@ class Batch_test : public beast::unit_test::Suite
STParsedJSONObject parsed("test", txn.getTxn());
Serializer s;
parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access)
submitAndValidate(
"SigningPubKey set",
s.slice(),
__LINE__,
std::nullopt,
"fails local checks: Invalid signature.");
submitAndValidate("SigningPubKey set", s.slice(), __LINE__);
}
// Invalid RPC Submission: Signers
@@ -2728,12 +2704,7 @@ class Batch_test : public beast::unit_test::Suite
STParsedJSONObject parsed("test", txn.getTxn());
Serializer s;
parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access)
submitAndValidate(
"Signers set",
s.slice(),
__LINE__,
std::nullopt,
"fails local checks: Invalid Signers array size.");
submitAndValidate("Signers set", s.slice(), __LINE__);
}
{
@@ -2744,8 +2715,7 @@ class Batch_test : public beast::unit_test::Suite
STParsedJSONObject parsed("test", jt.jv);
Serializer s;
parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access)
submitAndValidate(
"Fully signed", s.slice(), __LINE__, std::nullopt, std::nullopt, !withBatch);
submitAndValidate("Fully signed", s.slice(), __LINE__);
}
// Invalid RPC Submission: tfInnerBatchTxn
@@ -2758,12 +2728,7 @@ class Batch_test : public beast::unit_test::Suite
STParsedJSONObject parsed("test", txn.getTxn());
Serializer s;
parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access)
submitAndValidate(
"No signing fields set",
s.slice(),
__LINE__,
"fails local checks: Empty SigningPubKey.",
"fails local checks: Empty SigningPubKey.");
submitAndValidate("No signing fields set", s.slice(), __LINE__);
}
// Invalid RPC Submission: tfInnerBatchTxn pseudo-transaction
@@ -2782,12 +2747,7 @@ class Batch_test : public beast::unit_test::Suite
STParsedJSONObject parsed("test", txn.getTxn());
Serializer s;
parsed.object->add(s); // NOLINT(bugprone-unchecked-optional-access)
submitAndValidate(
"Pseudo-transaction",
s.slice(),
__LINE__,
"fails local checks: Empty SigningPubKey.",
"fails local checks: Empty SigningPubKey.");
submitAndValidate("Pseudo-transaction", s.slice(), __LINE__);
}
}
@@ -5716,7 +5676,7 @@ class Batch_test : public beast::unit_test::Suite
batch::Inner(pay(carol, alice, XRP(50)), carolSeq));
jt2.jv[sfBatchSigners.jsonName] = capturedSigners;
env(jt2.jv, Ter(temBAD_SIGNATURE));
env(jt2.jv, Ter(telENV_RPC_FAILED));
env.close();
BEAST_EXPECT(env.seq(carol) == carolSeq);
@@ -5760,7 +5720,7 @@ class Batch_test : public beast::unit_test::Suite
batch::Inner(pay(carol, alice, XRP(500)), carolSeq));
jt2.jv[sfBatchSigners.jsonName] = capturedSigners;
env(jt2.jv, Ter(temBAD_SIGNATURE));
env(jt2.jv, Ter(telENV_RPC_FAILED));
env.close();
BEAST_EXPECT(env.balance(carol) == preCarol);
@@ -5813,7 +5773,7 @@ class Batch_test : public beast::unit_test::Suite
bobSignerEntry[sfBatchSigner.jsonName][sfSigners.jsonName];
jt2.jv[sfBatchSigners.jsonName][0u] = carolSigner;
env(jt2.jv, Ter(temBAD_SIGNATURE));
env(jt2.jv, Ter(telENV_RPC_FAILED));
env.close();
}
}
@@ -5860,8 +5820,10 @@ class Batch_test : public beast::unit_test::Suite
using namespace test::jtx;
// Mirrors Batch.cpp's file-local kSfBatchSigGood; keep in sync.
constexpr HashRouterFlags kSfBatchSigGood = HashRouterFlags::PRIVATE7;
// Mirrors apply.cpp's file-local kSfSiggood (the standard signature-good
// cache); batch signer sigs are now verified and cached alongside the
// outer signature in checkSign/checkValidity.
constexpr HashRouterFlags kSfSiggood = HashRouterFlags::PRIVATE2;
// Valid batch: alice (outer) + an inner from bob, who co-signs.
auto buildValidBatch = [](Env& env) {
@@ -5885,9 +5847,9 @@ class Batch_test : public beast::unit_test::Suite
auto jt = buildValidBatch(env);
auto const txid = jt.stx->getTransactionID();
BEAST_EXPECT(!any(env.app().getHashRouter().getFlags(txid) & kSfBatchSigGood));
BEAST_EXPECT(!any(env.app().getHashRouter().getFlags(txid) & kSfSiggood));
env(jt, Ter(tesSUCCESS));
BEAST_EXPECT(any(env.app().getHashRouter().getFlags(txid) & kSfBatchSigGood));
BEAST_EXPECT(any(env.app().getHashRouter().getFlags(txid) & kSfSiggood));
env.close();
}
@@ -5901,7 +5863,7 @@ class Batch_test : public beast::unit_test::Suite
auto jt = buildValidBatch(env);
jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] =
"00";
env(jt.jv, Ter(temBAD_SIGNATURE));
env(jt.jv, Ter(telENV_RPC_FAILED));
env.close();
}
{
@@ -5914,7 +5876,7 @@ class Batch_test : public beast::unit_test::Suite
jt.jv[sfBatchSigners.jsonName][0u][sfBatchSigner.jsonName][sfTxnSignature.jsonName] =
"00";
auto const txid = STTx{parse(jt.jv)}.getTransactionID();
env.app().getHashRouter().setFlags(txid, kSfBatchSigGood);
env.app().getHashRouter().setFlags(txid, kSfSiggood);
env(jt.jv, Ter(tesSUCCESS));
env.close();
}