fix: Skip the AccountTxnID update for Batch wrappers (fixCleanup3_4_0)

[dangell7]

AccountTxnID chaining is unusable inside a Batch. The wrapper stamps the account root's sfAccountTxnID with its own transaction ID, and that ID hashes over sfRawTransactions, so a same-account inner can never carry a matching prior-txn ID. Any value fails preclaim with tefWRONG_PRIOR. Reported as Attackathon finding F121 (issue 993); it fails closed, so this is a functionality gap rather than an exploit.

Gated on fixCleanup3_4_0: a ttBATCH transaction no longer updates sfAccountTxnID. The first inner chains off the last pre-batch transaction, the inners update the field as they apply, and a post-batch transaction chains off the last inner. The wrapper's only effects are its inners plus fee and sequence, so the chain still covers every state-changing action.

Behavior change on activation: a later transaction can no longer chain off the wrapper's own ID. It chains off the last same-account inner instead, and a batch with no same-account inners does not advance the chain.

New account txn id case in Batch_test covers both amendment paths, and a subscriptions case pins the stream behavior: the proposed stream carries only the outer, the validated stream carries the outer and both inners with metadata.

[xrplf-ai-reviewer]

The core fix in Transactor.cpp correctly gates the sfAccountTxnID stamp to skip only when the currently-applying transaction is the ttBATCH wrapper itself and fixBatchAccountTxnID is enabled — inner transactions still go through their own Transactor::apply() call with their own txn type, so they continue to update the field normally. The accompanying test exercises both the fix-enabled path (inner chains off the last pre-batch AccountTxnID, chain ends at the last applied inner, and a post-batch txn can chain off it) and the fix-disabled path (wrapper stamps its own ID, causing tefWRONG_PRIOR and an all-or-nothing revert), with sequence/balance assertions that check out arithmetically. No correctness, security, or consistency issues found in the changed lines.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR introduces a new fix amendment (fixBatchAccountTxnID) to make sfAccountTxnID chaining usable when submitting ttBATCH transactions. When the fix is enabled, the Batch wrapper no longer stamps the account root’s sfAccountTxnID, allowing same-account inner transactions to reference the last pre-batch transaction ID without failing preclaim.

Changes:

• Register the new BatchAccountTxnID fix amendment (default vote: No).
• Update Transactor::apply() to skip sfAccountTxnID stamping for ttBATCH when fixBatchAccountTxnID is enabled.
• Add a new Batch_test unit test case covering both amendment-enabled and amendment-disabled behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/test/app/Batch_test.cpp	Adds coverage for sfAccountTxnID behavior inside Batch under both amendment states.
src/libxrpl/tx/Transactor.cpp	Conditionally skips updating sfAccountTxnID for ttBATCH when the fix amendment is enabled.
include/xrpl/protocol/detail/features.macro	Registers the new BatchAccountTxnID fix amendment.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[dangell7]

Done. resolved: 9f89fce012

[xrplf-ai-reviewer]

The core logic change (skip stamping sfAccountTxnID on the ttBATCH outer transaction when the guarding amendment is enabled) is correctly implemented — the boolean condition is right in all branches, and the new test exercises both the with-fix and without-fix paths with reasonable assertions. The one thing worth a second look before merge is that the code and test gate on fixCleanup3_4_0, not fixBatchAccountTxnID as named in the MR title/description — worth confirming that's intentional (e.g. a shared release-bundle amendment) rather than a leftover flag from another change.

[xrplf-ai-reviewer]

The core fix in Transactor.cpp correctly gates the sfAccountTxnID update: it skips stamping the account root's sfAccountTxnID only when the transaction is a ttBATCH wrapper and fixCleanup3_4_0 is enabled, letting inner transactions (which are not ttBATCH) continue to chain normally. This matches the described intent (wrapper's ID hashes over sfRawTransactions so no inner can match it) and is properly amendment-gated. Test coverage in Batch_test.cpp exercises both the fixed and pre-fix paths. No correctness, security, or infrastructure issues found in the changed lines.

[xrplf-ai-reviewer]

Focused, well-tested fix: gates the account-root sfAccountTxnID stamp on the ttBATCH wrapper behind fixCleanup3_4_0, restoring usable AccountTxnID chaining for inner transactions. The core logic change is a single condition and looks correct (skip update only when txn type is ttBATCH and the amendment is enabled); new Batch_test coverage exercises both the fixed and pre-fix paths plus a subscriptions regression test. No clear bugs found in the changed lines; flagging one edge case worth confirming with the author.

[dangell7]

@xrplf-ai-reviewer your last review says you're flagging one edge case worth confirming with the author, but there's no comment with it. What's the edge case?

[xrplf-ai-reviewer]

Chain invariant gap when all Batch inners fail — see inline.

[kennyzlei]

The fix and its gating condition look correct — the skip applies only to the ttBATCH wrapper with fixCleanup3_4_0 enabled, inners still stamp, and preclaim still validates. Two test-coverage suggestions inline, covering the two corner behaviors the fix creates: a batch whose inners all belong to another account, and an outer Batch that itself carries sfAccountTxnID. Both were verified against the transactor/preclaim code paths but not compiled or run.

Separately, worth an explicit decision: this gates new consensus-affecting behavior on fixCleanup3_4_0, which already shipped as Supported::Yes in the 3.4.0-b1/b2 betas — nodes on those builds would stamp the wrapper after activation while nodes with this commit would not. Presumably fine if no persistent network's validators are voting it from b1/b2 binaries, but flagging in case it should be its own amendment.

---

Generated by Claude Code

[kennyzlei]

The new test covers same-account inners, but not a batch whose inners all belong to another account — which is exactly where the wrapper's stamp/skip becomes observable on its own. With the fix, nothing writes the outer account's sfAccountTxnID (the wrapper skips and no inner is the outer account's), so it must keep its pre-batch value; without the fix the wrapper stamps its own ID even though the outer account authored no inner. In the current test the same-account inners re-stamp the field as they apply, so a future change that stamps the wrapper "when no same-account inner applied" (e.g. on an all-foreign batch) would pass the suite unnoticed.

Suggested change

	}
	}
	// A wrapper whose inners all belong to another account: with the fix
	// nothing stamps the outer account's sfAccountTxnID, so it keeps its
	// pre-batch value; without the fix the wrapper stamps its own ID even
	// though the outer account authored no inner.
	for (bool const withFix : {true, false})
	{
	auto const amend = withFix ? features : features - fixCleanup3_4_0;
	Env env{*this, amend};
	auto const alice = Account("alice");
	auto const bob = Account("bob");
	env.fund(XRP(10000), alice, bob);
	env.close();
	env(fset(alice, asfAccountTxnID));
	env.close();
	env(noop(alice));
	env.close();
	uint256 const priorID = env.tx()->getTransactionID();
	auto const preAlice = env.balance(alice);
	auto const aliceSeq = env.seq(alice);
	auto const bobSeq = env.seq(bob);
	auto const batchFee = batch::calcBatchFee(env, 1, 2);
	auto const [txIDs, batchID] = submitBatch(
	env,
	tesSUCCESS,
	batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing),
	batch::Inner(pay(bob, alice, XRP(1)), bobSeq),
	batch::Inner(pay(bob, alice, XRP(2)), bobSeq + 1),
	batch::Sig(bob));
	env.close();
	// Both inners are bob's, so they apply under either amendment
	// state: alice's prior-txn chain constrains only her own
	// transactions.
	std::vector<TestLedgerData> const testCases = {
	{.index = 0,
	.txType = "Batch",
	.result = "tesSUCCESS",
	.txHash = batchID,
	.batchID = std::nullopt},
	{.index = 1,
	.txType = "Payment",
	.result = "tesSUCCESS",
	.txHash = txIDs[0],
	.batchID = batchID},
	{.index = 2,
	.txType = "Payment",
	.result = "tesSUCCESS",
	.txHash = txIDs[1],
	.batchID = batchID},
	};
	validateClosedLedger(env, testCases);
	BEAST_EXPECT(env.seq(alice) == aliceSeq + 1);
	BEAST_EXPECT(env.seq(bob) == bobSeq + 2);
	BEAST_EXPECT(env.balance(alice) == preAlice + XRP(3) - batchFee);
	auto const sle = env.le(keylet::account(alice));
	BEAST_EXPECT(sle && sle->isFieldPresent(sfAccountTxnID));
	if (withFix)
	{
	// Nothing stamped the field: it keeps its pre-batch value.
	BEAST_EXPECT(sle->getFieldH256(sfAccountTxnID) == priorID);
	// A post-batch transaction chains off the pre-batch
	// transaction, not the batch.
	auto jv = pay(alice, bob, XRP(1));
	jv[sfAccountTxnID.jsonName] = strHex(priorID);
	env(jv);
	env.close();
	}
	else
	{
	BEAST_EXPECT(strHex(sle->getFieldH256(sfAccountTxnID)) == batchID);
	}
	}

Caveat: drafted and reviewed against the transactor/preclaim code paths, but not compiled or run (no dependency access in my environment) — please run the Batch suite over it.

---

Generated by Claude Code

[dangell7]

Done. resolved: 7092269f72

[kennyzlei]

The other corner this fix creates: an outer Batch that itself carries sfAccountTxnID. It's a legal common field on ttBATCH, and checkPriorTxAndLastLedger still validates it in preclaim for the wrapper (untouched by the fix) — so a stale value must fail with tefWRONG_PRIOR under both amendment states, and a matching value must admit the batch without the wrapper then stamping over the field. Nothing in the suite exercises this today; the existing tefWRONG_PRIOR expectation fires on an inner Payment during batch application, so it would not catch a refactor that accidentally extends the ttBATCH skip into the preclaim check.

Suggested change

	}
	// The wrapper's own sfAccountTxnID is still validated in preclaim
	// under both amendment states; with the fix a matching value admits
	// the batch without the wrapper then stamping over the field.
	for (bool const withFix : {true, false})
	{
	auto const amend = withFix ? features : features - fixCleanup3_4_0;
	Env env{*this, amend};
	auto const alice = Account("alice");
	auto const bob = Account("bob");
	env.fund(XRP(10000), alice, bob);
	env.close();
	env(fset(alice, asfAccountTxnID));
	env.close();
	// The fset only creates the (empty) field, so its own ID is a
	// guaranteed-stale prior-txn value.
	uint256 const staleID = env.tx()->getTransactionID();
	env(noop(alice));
	env.close();
	uint256 const priorID = env.tx()->getTransactionID();
	// A stale value fails preclaim, fix or no fix.
	{
	auto const aliceSeq = env.seq(alice);
	auto const bobSeq = env.seq(bob);
	auto const batchFee = batch::calcBatchFee(env, 1, 2);
	auto outer = batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing);
	outer[sfAccountTxnID.jsonName] = strHex(staleID);
	submitBatch(
	env,
	tefWRONG_PRIOR,
	outer,
	batch::Inner(pay(bob, alice, XRP(1)), bobSeq),
	batch::Inner(pay(bob, alice, XRP(2)), bobSeq + 1),
	batch::Sig(bob));
	env.close();
	BEAST_EXPECT(env.seq(alice) == aliceSeq);
	BEAST_EXPECT(env.seq(bob) == bobSeq);
	}
	// The current value passes preclaim.
	{
	auto const aliceSeq = env.seq(alice);
	auto const bobSeq = env.seq(bob);
	auto const batchFee = batch::calcBatchFee(env, 1, 2);
	auto outer = batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing);
	outer[sfAccountTxnID.jsonName] = strHex(priorID);
	auto const [txIDs, batchID] = submitBatch(
	env,
	tesSUCCESS,
	outer,
	batch::Inner(pay(bob, alice, XRP(1)), bobSeq),
	batch::Inner(pay(bob, alice, XRP(2)), bobSeq + 1),
	batch::Sig(bob));
	env.close();
	std::vector<TestLedgerData> const testCases = {
	{.index = 0,
	.txType = "Batch",
	.result = "tesSUCCESS",
	.txHash = batchID,
	.batchID = std::nullopt},
	{.index = 1,
	.txType = "Payment",
	.result = "tesSUCCESS",
	.txHash = txIDs[0],
	.batchID = batchID},
	{.index = 2,
	.txType = "Payment",
	.result = "tesSUCCESS",
	.txHash = txIDs[1],
	.batchID = batchID},
	};
	validateClosedLedger(env, testCases);
	BEAST_EXPECT(env.seq(alice) == aliceSeq + 1);
	BEAST_EXPECT(env.seq(bob) == bobSeq + 2);
	auto const sle = env.le(keylet::account(alice));
	BEAST_EXPECT(sle && sle->isFieldPresent(sfAccountTxnID));
	if (withFix)
	{
	// Validated in preclaim, but not stamped.
	BEAST_EXPECT(sle->getFieldH256(sfAccountTxnID) == priorID);
	}
	else
	{
	BEAST_EXPECT(strHex(sle->getFieldH256(sfAccountTxnID)) == batchID);
	}
	}
	}
	}

Same caveat as the sibling suggestion: reviewed against the code paths but not compiled or run in my environment.

---

Generated by Claude Code

[dangell7]

Done. resolved: 7092269f72

[xrplf-ai-reviewer]

The core fix correctly gates the sfAccountTxnID stamp using De Morgan's equivalent of "skip only when the transaction is the ttBATCH wrapper AND fixCleanup3_4_0 is enabled"; since inner transactions are applied through their own Transactor::apply() calls with their real (non-ttBATCH) type, they continue to update the field as before, and non-batch transactions are unaffected. The accompanying Batch_test.cpp coverage exercises same-account inners, other-account-only inners, all-inners-failing, and wrapper-preclaim-validation cases under both amendment states, which lines up with the behavior described in the MR. I did not find correctness, security, or resource-management issues in the added lines.

[shawnxie999]

LGTM

[mvadari]

As @dangell7 and I discussed separately, this will break the chain if a Batch tx is submitted by an account that doesn't have any inner txs, and then wants to use AccountTxnID after the Batch tx.