Skip to content

fix(toolkit): emit successful txs from batch-single-tx on partial failure #1731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chrispalaskas wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(toolkit): emit successful txs from batch-single-tx on partial failure #1731

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions util/toolkit/src/tx_generator/builder/builders/common/batch_single_tx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,21 @@ impl<C: BuilderContext<DefaultDB>> BuildTxs for BatchSingleTxBuilder<C> {

progress.finish(format!("batch-single-tx: {} succeeded, {} failed", succeeded, failed));

// Emit every tx we managed to build, even on partial failure. Discarding the
// successful txs because a few transfers failed wastes the whole batch and starves
// downstream load appliers. Only treat a batch with zero successes as an error.
if failed > 0 {

@LGLO LGLO Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make sure that process returns a valid JSON for downstream instead of playing with error codes here.
As pointed by another comment: 0 of 0 is success, 0 of 1 if failure, but in both cases downstream doesn't have much to work on. It seems that return code alone isn't sufficient here.

Also, is 1 okay out of 250 a success? It hardly seems so to me.

It seems simpler to me if downstream makes decision what is enough for it.

return Err(BatchSingleTxError::PartialFailure { succeeded, failed });
if succeeded == 0 {
return Err(BatchSingleTxError::AllFailed { failed });
}
Comment on lines +256 to +258

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve state when accepting partial batches

When failed > 0 but at least one transfer succeeds, this now continues with the mutated shared builder context and emits the remaining txs. For failures that occur after tx_info.prove() starts (for example shielded transfers where OfferInfo::build_inputs has already spent wallet state before proof/validation returns ProvingFailed), a failed, un-emitted transfer can leave local coins/DUST marked spent or create local change; later successes built from the same wallet may then depend on state that will not exist on-chain once only the successful txs are submitted. Please isolate or roll back per-transfer context mutations before treating partial failures as success.

Useful? React with 👍 / 👎.

@HarleysCodes HarleysCodes Jun 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit on the warn-message: structured fields above are succeeded = succeeded, failed = failed but the format placeholders consume them as failed, succeeded + failed, succeeded — i.e. the format string reads {failed} of {total} transfers failed; emitting {succeeded} successful txs. Either swap the format placeholders to match the structured-field ordering, or reorder the args. Cosmetic, but the dual layout is easy to misread.

tracing::warn!(
succeeded = succeeded,
failed = failed,
"Partial batch: {} of {} transfers failed; emitting {} successful txs",
failed,

@HarleysCodes HarleysCodes Jun 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new code path worth a test: failed > 0 && succeeded > 0 must return Ok with the partial batch, not Err. The perf-repo load applier in the PR description relies on this exact behavior. Even a small test that constructs a BatchSingleTxBuilder with 5 transfers where 2 deliberately fail and verifies the function returns Ok(SerializedTxBatches) with 3 entries would lock the contract and document the new semantics for future maintainers. Without it, a future refactor could revert to Err on partial failure and break the load-test pipeline silently.

succeeded + failed,
succeeded
);
}

Ok(SerializedTxBatches { batches: vec![txs] })
Expand All @@ -259,6 +272,6 @@ impl<C: BuilderContext<DefaultDB>> BuildTxs for BatchSingleTxBuilder<C> {

#[derive(Debug, thiserror::Error)]
pub enum BatchSingleTxError {
#[error("{failed} of {} transfers failed", succeeded + failed)]
PartialFailure { succeeded: usize, failed: usize },
#[error("all {failed} transfers failed")]
AllFailed { failed: usize },
}
Loading