feat: Batch (XLS-56) V1_1#6446
Conversation
- add early `sfBatchSigners` size check - fix log nomenclature
There was a problem hiding this comment.
Pull request overview
This PR updates the Batch transaction implementation to a new amendment name/version (“BatchV1_1”), adjusts batch signature construction/verification, and refreshes the related validation paths and unit tests.
Changes:
- Renames/gates Batch functionality under
featureBatchV1_1(replacing priorfeatureBatch/ removingfixBatchInnerSigsusage). - Updates batch signature message construction to include
finishMultiSigningDatafor batch signers and strengthens signer validation to ensure all signers are checked. - Refactors batch signer authorization checks from
TransactorintoBatch, and updates/extends tests accordingly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/overlay/detail/PeerImp.cpp | Updates overlay comments to reference featureBatchV1_1. |
| src/xrpld/app/misc/NetworkOPs.cpp | Switches network submission/processing checks to featureBatchV1_1. |
| src/test/rpc/Feature_test.cpp | Updates feature name expectation for the Batch amendment. |
| src/test/jtx/impl/batch.cpp | Adjusts batch signer signature construction (finishMultiSigningData). |
| src/test/app/Batch_test.cpp | Updates tests to use featureBatchV1_1, removes fixBatchInnerSigs branching, and adds a regression test ensuring all signers are validated. |
| src/libxrpl/tx/transactors/Lending/LoanSet.cpp | Switches inner-batch gating to featureBatchV1_1. |
| src/libxrpl/tx/transactors/Batch.cpp | Adds bounds checks, improves signer diagnostics, and introduces Batch::checkBatchSign used from Batch::checkSign. |
| src/libxrpl/tx/apply.cpp | Updates inner-batch handling to key off featureBatchV1_1 and removes legacy inner-sig behavior. |
| src/libxrpl/tx/Transactor.cpp | Switches inner-batch gating to featureBatchV1_1 and removes Transactor::checkBatchSign. |
| src/libxrpl/protocol/STTx.cpp | Tightens batch signature verification flow; updates single-sign batch digest construction and error reporting. |
| include/xrpl/tx/transactors/Batch.h | Declares Batch::checkBatchSign. |
| include/xrpl/tx/Transactor.h | Removes checkBatchSign API and adjusts access to allow Batch to reuse signing helpers. |
| include/xrpl/protocol/detail/transactions.macro | Gates ttBATCH on featureBatchV1_1. |
| include/xrpl/protocol/detail/features.macro | Introduces BatchV1_1 and removes prior Batch-related amendment entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6446 +/- ##
========================================
Coverage 82.0% 82.0%
========================================
Files 1007 1007
Lines 76888 77070 +182
Branches 8971 8973 +2
========================================
+ Hits 63042 63212 +170
- Misses 13837 13849 +12
Partials 9 9
... and 15 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Co-authored-by: Mayukha Vadari <mvadari@ripple.com>
dangell7
force-pushed
the
dangell7/batch-v1
branch
from
07c3143 to
70752f8
Compare
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
One historical security issue called out inline (now fixed in this MR) — signature binding in checkBatchSingleSign previously excluded Account and Sequence, enabling cross-account replay. The fix is present in this diff; comment is for audit traceability.
Review by Claude Sonnet 4.6 · Prompt: V15
| if (tx->getTxnType() == ttBATCH) | ||
| return {telCAN_NOT_QUEUE, false}; |
There was a problem hiding this comment.
there is a PR that does the same if it's a delegate txn. #7640
The check should probably be moved to canBeHeld as well
There was a problem hiding this comment.
Moved the check into canBeHeld, matching #7640's delegate check. Put it above the existing checks so #7640 still merges clean. resolved: 188bde223f
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
Another partial review. This one covers the only changes since my last review.
| // this guard only protects release builds where asserts are no-ops. | ||
| if (getTxnType() != ttBATCH) | ||
| { | ||
| JLOG(debugLog().fatal()) << "not a batch transaction"; |
There was a problem hiding this comment.
You didn't necessarily have to remove the log message, but I don't have an opinion one way or the other whether you should put it back.
There was a problem hiding this comment.
Leaving the log removed, no strong reason either way. Thanks.
| // sfRawTransactions only appears on a Batch. | ||
| XRPL_ASSERT( | ||
| safeCast<TxType>(st.getFieldU16(sfTransactionType)) == ttBATCH, | ||
| "xrpl::isBatchRawTransactionOkay : not a batch transaction"); |
There was a problem hiding this comment.
Nit: Assertion descriptions are supposed to describe the desired state. So this should be ... : batch transaction. (UNREACHABLE is the opposite. That description should describe what is wrong.)
There was a problem hiding this comment.
Fixed, the description now states the desired condition: "batch transaction". Swept the sibling batch asserts too. resolved: d031a22027
| << "invalid batch txn signature: " << sigResult.error(); | ||
| return temBAD_SIGNATURE; | ||
| } | ||
| router.setFlags(parentBatchId, kSfBatchSigGood); |
There was a problem hiding this comment.
You should probably cache failures, too. That way, if the same transaction is resubmitted (or even retried locally), you won't waste the time to check the batch signatures again.
You could do that with a new PRIVATE8, or I think even better would be to reuse HashRouterFlags::BAD. The advantage of BAD is that if the transaction is seen again (before the entry expires), it'll get stopped and dropped well before getting into the transaction engine.
Note: NetworkOPsImp::apply sets BAD if the transaction engine returns a tem code, so ignore this suggestion.
There was a problem hiding this comment.
Agreed, NetworkOPs already sets BAD on a tem result, so keeping it good-only. Thanks.
There was a problem hiding this comment.
This block should go before the inner signature checking (immediately after the signer matching loop). It's way cheaper than checking signatures, and more readable, IMHO.
There was a problem hiding this comment.
Done, moved the required-signer count check before the signature check so it fails fast. resolved: d031a22027
| if (ctx.tx.isFlag(tfInnerBatchTxn)) | ||
| { | ||
| if (!ctx.rules.enabled(featureBatchV1_1)) | ||
| return temINVALID_FLAG; | ||
| if (!ctx.parentBatchId.has_value()) | ||
| return temINVALID_INNER_BATCH; |
There was a problem hiding this comment.
Is there any harm in keeping these checks? They seem like a good defense in depth mechanism.
There was a problem hiding this comment.
Good point, restored them as defense in depth, with a comment noting the preflight1 copy. resolved: d031a22027
| // A Batch is never queued: it can advance the account sequence by more | ||
| // than one, which the TxQ's single-sequence forecast cannot model. It must | ||
| // apply straight to the open ledger or not at all. | ||
| if (tx.getTxnType() == ttBATCH) | ||
| return telCAN_NOT_QUEUE; |
There was a problem hiding this comment.
Correction: The TxQ's sequence model can handle the account sequence advancing by more than one. See TicketCreate. The reason to exclude Batch is that it cannot model one transaction changing the sequence number for more than one account. For a single account consuming more than one sequence, you could customize the TxConsequences generator, but that has to be more definitive, too.
There was a problem hiding this comment.
Thanks for the correction. Updated the comment: a Batch changes sequence numbers for multiple accounts, which the per-account model can't forecast. resolved: d031a22027
| // sfRawTransactions only appears on a Batch. | ||
| XRPL_ASSERT( | ||
| safeCast<TxType>(st.getFieldU16(sfTransactionType)) == ttBATCH, | ||
| "xrpl::isBatchRawTransactionOkay : not a batch transaction"); | ||
|
|
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
Partial review.
I need to pick up at src/test/app/Batch_test.cpp, though I'll probably skip over the tests to get through the first pass.
| void | ||
| STTx::buildBatchTxnIds() | ||
| { | ||
| // Precondition: the template must have been applied first, so the fields | ||
| // (including sfRawTransactions) are canonical before the inner txns are | ||
| // hashed. The constructors call this immediately after applying the | ||
| // template; isFree() being false confirms a template is set. | ||
| XRPL_ASSERT(!isFree(), "STTx::buildBatchTxnIds : template not applied"); | ||
| if (getTxnType() != ttBATCH || !isFieldPresent(sfRawTransactions)) | ||
| return; | ||
|
|
||
| auto const& raw = getFieldArray(sfRawTransactions); | ||
|
|
||
| if (raw.size() > kMaxBatchTxCount) | ||
| return; | ||
|
|
||
| batchTxnIds_.reserve(raw.size()); | ||
| for (STObject const& rb : raw) | ||
| batchTxnIds_.push_back(rb.getHash(HashPrefix::TransactionId)); | ||
| } | ||
|
|
||
| std::vector<uint256> const& | ||
| STTx::getBatchTransactionIDs() const | ||
| { | ||
| XRPL_ASSERT(getTxnType() == ttBATCH, "STTx::getBatchTransactionIDs : not a batch transaction"); | ||
| XRPL_ASSERT( | ||
| !getFieldArray(sfRawTransactions).empty(), | ||
| "STTx::getBatchTransactionIDs : empty raw transactions"); | ||
|
|
||
| // The list of inner ids is built once, then reused on subsequent calls. | ||
| // After the list is built, it must always have the same size as the array | ||
| // `sfRawTransactions`. The assert below verifies that. | ||
| if (batchTxnIds_.empty()) | ||
| { | ||
| for (STObject const& rb : getFieldArray(sfRawTransactions)) | ||
| batchTxnIds_.push_back(rb.getHash(HashPrefix::TransactionId)); | ||
| } | ||
|
|
||
| !batchTxnIds_.empty(), "STTx::getBatchTransactionIDs : batch transaction IDs not built"); | ||
| XRPL_ASSERT( | ||
| batchTxnIds_.size() == getFieldArray(sfRawTransactions).size(), | ||
| "STTx::getBatchTransactionIDs : batch transaction IDs size mismatch"); |
There was a problem hiding this comment.
When a transaction with too many batch signers or no batch signers calls buildBatchTxnIds(), you end up with an empty batchTxnIds_ collection.
If later, that transaction calls getBatchTransactionIDs(), it will fail one of these assertions. Right now, the code is structured so that getBatchTransactionIDs() won't be called until after the signer matching has happened... (Wait, is that why you deferred the checking until Batch::preflight?) ...but that could change down the road, and may not be detected until user input crashes a node running debug. It's fragile.
Here's how to fix it:
- Change the declaration to
std::optional<std::vector<uint256>> batchTxnIds_;. - In
buildBatchTxnIds(), right after you check that the type isttBATCH, callbatchTxnIds_.emplace(). - Change the places that read it to use
->instead of.. - Either change the return type of
getBatchTransactionIDs()to matchbatchTxnIds_, and handle the case where it's unseated, or check whetherbatchTxnIds_is seated before calling it.
a. Change the assertions insidegetBatchTransactionIDs()as appropriate based on what you did in the previous step. - Move the call to
checkBatchSignfromBatch::preflightSigValidatedtoSTTx::checkSign(Rules const&). Gate it onif (batchTxnIds_)or something like that, not ontype == ttBATCH. - Get rid of the now unnecessary
HashRouterFlags::PRIVATE7andkSfBatchSigGood. - Tada! The batch signature checking happens with the rest of the signature checking, is cached with the rest of the signature checking, is out of the transaction engine, and is out of the serialized path processing sets of transactions.
This is a blocker.
| // sfRawTransactions only appears on a Batch. | ||
| XRPL_ASSERT( | ||
| safeCast<TxType>(st.getFieldU16(sfTransactionType)) == ttBATCH, | ||
| "xrpl::isBatchRawTransactionOkay : not a batch transaction"); |
There was a problem hiding this comment.
This function is called by passesLocalChecks, which is called from checkValidity, which is called from potentially unverified user and peer input. Thus, I don't think it's impossible with malicious or malformed input.
Replace this assert to
// sfRawTransactions only appears on a Batch.
if(safeCast<TxType>(st.getFieldU16(sfTransactionType)) != ttBATCH)
return false;
I have considered that applyTemplate will fail for any transaction with sfRawTransactions that is of a type that doesn't specify sfRawTransactions. And only Batch has sfRawTransactions, but I don't think that is sufficiently fool- and future-proof, particularly when checking it this way is so simple.
| JLOG(debugLog().error()) << "BatchTrace: XRPAmount overflow in signerFees calculation."; | ||
| return XRPAmount{kInitialXrp}; | ||
| } | ||
| if (txnFees + signerFees > maxAmount - batchBase) |
There was a problem hiding this comment.
Micro-optimization: Instead of computing txnFees + signerFees twice, define auto const innerFees = txnFees + signerFees. Use it here, and in the final return calculation.
Optional
| requiredSigners.insert(innerAccount); | ||
| // A delegated inner is signed by the delegate, not the account holder, | ||
| // so the delegate is the required signer when present. | ||
| AccountID const authorizer = rb[~sfDelegate].value_or(rb[sfAccount]); |
There was a problem hiding this comment.
| AccountID const authorizer = rb[~sfDelegate].value_or(rb[sfAccount]); | |
| AccountID const authorizer = rb.getFeePayer(); |
We've got the utility functions - we need to use them.
| { | ||
| JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " | ||
| << "extra signer provided: " << signerAccount; | ||
| return temBAD_SIGNER; |
There was a problem hiding this comment.
Nit: I think this might also indicate a missing signer. It might be worth tweaking the log message to say, "missing signer or extra signer provided: " << signerAccount. I don't think it's necessarily worth checking which is which.
| else | ||
| { | ||
| if (!publicKeyType(makeSlice(pkSigner))) | ||
| return tefBAD_AUTH; // LCOV_EXCL_LINE |
There was a problem hiding this comment.
Suggestion: This could be checked in preflightSigValidated.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not 100% certain, but I think this whole loop can be replaced by Transactor::checkSign:
auto const idAccount = signer.getAccountID(sfAccount);
if (auto const ret = checkSign(ctx.view, ctx.flags, ctx.parentBatchId, idAccount, signer, ctx.j;
!isTesSuccess(ret))
return ret;
Reduce code duplication, prevent the different implementations from drifting, know for certain that they're both following the same rules, etc.
Also, change the isPseudoAccount check at the top of Transactor::checkSign to
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
// a few different amendments so that it can take effect as soon as
// any of them are activated.
return tefBAD_AUTH;
}
| return {Validity::Valid, ""}; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Correct me if I'm wrong, but I don't think we should ever be calling checkValidity on an inner transaction now. If so, replace this whole thing with:
if (tx.isFlag(tfInnerBatchTxn))
{
router.setFlags(id, kSfSigbad);
return {Validity::SigBad, "Batch inner transactions are never considered validly signed."};
}
Note that, similar to NetworkOPs and PeerImp, we don't need to gate the check on the amendment. If the tx has this flag, it's never valid.
| // Skip signature check on batch inner transactions. The inner-batch flag | ||
| // and parentBatchId checks are in preflight1. | ||
| if (ctx.tx.isFlag(tfInnerBatchTxn)) | ||
| return tesSUCCESS; |
There was a problem hiding this comment.
I feel like, unlike the places where tfInnerBatchTxn is never valid, so don't check the amendment, this is returning a success result. And even though we shouldn't get here without the amendment being enabled, it would be a good defensive check to just make sure. ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatchV1_1).
| Serializer msg; | ||
| serializeBatch(msg, getFlags(), getBatchTransactionIDs()); | ||
| serializeBatch( | ||
| msg, getAccountID(sfAccount), getSeqValue(), getFlags(), getBatchTransactionIDs()); |
There was a problem hiding this comment.
Confirms the security fix: serializeBatch now binds signatures to the outer Account and Sequence, and finishMultiSigningData includes the batch signer account — preventing cross-account/sequence signature replay.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
High Level Overview of Change
Feature
Batchtransactor andfeatureBatchV1_1amendment (69084a6ff5)359a94b766)jtxbatch helpers (aafe4b6201, plus tests bundled with the fixes below)Security / audit fixes
These came out of the security audit and harden authorization and input-bounding:
7618b726b3) —serializeBatchnow hashes in the outerAccountandSequence, and multi-sign data includes the batch signer account. Prevents a batch signature from being lifted and replayed against a different outer account/sequence.tfInnerBatchTxnunconditionally from the network (6393ac8c47) — inner-batch flag is now refused inNetworkOPsregardless of amendment state, closing a window where the gate depended onfeatureBatchV1_1being enabled.477532edfe) — signers must be sorted byAccountID; duplicate/unsorted arrays returntemBAD_SIGNER. Removes ambiguity and a malleability vector in the signer set.sfRawTransactionsbefore eager txn-id hashing (d90fc44c0b) — skip hashing when the raw array exceedsmaxBatchTxCount, preventing unbounded work on oversized input.c27613e1df) —BuildLedgerno longer re-applies inner transactions on replay.Signersarray (c729a26dd3) —calculateBaseFeecaps nested signers atSTTx::maxMultiSigners.Transactor(871d60f910, fix: BatchV_1 add defensive checks #6719).Context of Change
XLS spec: https://github.com/XRPLF/XRPL-Standards/blob/master/XLS-0056-batch/README.md
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Edit: Fixed spec URL