Skip to content

feat!: constrain discarding of side-effects for tx-level operations in AVM#16258

Merged
dbanks12 merged 1 commit intonextfrom
db/tx-discard
Aug 14, 2025
Merged

feat!: constrain discarding of side-effects for tx-level operations in AVM#16258
dbanks12 merged 1 commit intonextfrom
db/tx-discard

Conversation

@dbanks12
Copy link
Contributor

@dbanks12 dbanks12 commented Aug 7, 2025

Link to DD.

Relevant sections from the discard DD

TX trace columns

  • phase: the phase of the current row
  • is_revertible: is this row in a revertible phase (revertible insertions, app logic or teardown)
  • is_padded: boolean representing whether this row is an empty row as a placeholder for a skipped phase.
  • failure/ reverted: this TX row fails in some way (enqueued call fails or insertion fails due to limit reached or nullifier collision.
    • Currently named reverted in the TX trace which is probably fine because there is no distinction between error and revert in TX.
  • discard: this context or one of its ancestors fails. All side effects should be discarded.

TX trace constraints

  1. discard is a boolean
  2. During non-revertible phases (startup, non-revertible insertions, setup, and fee-payment/cleanup phases), failure and discard must remain 0. These phases cannot fail at the top level.
    1. Note: nested calls can fail in the execution trace, but top-level setup calls must not fail. The TX trace only sees top-level failures.
  3. A row that fails must discard. In other words, if failure == 1, then discard == 1 must hold true.
  4. By default, the value of discard is propagated to the next row.
  5. The constraint that propagates discard to the next row is lifted if any one of the following conditions are met:
    1. The next row is the first row after setup (is_revertible == 0 && is_revertible’ == 1).
      1. Note that here we define the last row of setup as the last non-revertible row before a revertible row.
    2. This row is a failure (reverted == 1).
    3. Note: the propagation constraint is not lifted at the end of app logic and teardown. Instead, we rely on the constraint that the non-revertible phases after teardown cannot discard. So, if discard == 1 is propagated past teardown because it was 1 and teardown does not fail, the constraint will fail (item 2 above) that disallows discarding during non-revertible phases. Together, these constraints enforce that if discard is set for teardown, teardown must fail, in which case propagation is lifted and discard can be reset to 0 for the following non-revertible phases.
  6. For an enqueued call row, its context_id is used as the key for a lookup to the execution trace to retrieve failure.
  7. For filler/padding rows representing empty phases, we make the following assumptions:
    1. The normal propagation/other constraints listed above still hold
    2. A padding row cannot fail/revert.
    3. is_revertible is constrained to be correct even for padding rows (so app-logic padding should have is_revertible == 1.
    4. After a failure occurs in revertible insertions or app-logic, there will be no “padding” rows after the failure. The trace will jump straight to teardown on the row directly following the failure.

TX tracegen

For TX tracegen, we need a preprocessing phase similar to execution because in order to populate discard, we need to figure out whether a failure occurs later in the current phase (or phase grouping), or in some later phase.

The preprocessing phase should figure out:

  1. r_insertion_or_app_logic_failure: (boolean) did revertible insertions or app logic fail
  2. teardown_failure: (boolean) did teardown fail

Then we populate discard for each phase as follows:

  1. For revertible insertions and app logic:
    1. discard = r_insertion_or_app_logic_failure || teardown_failure
  2. For teardown:
    1. Note: if revertible insertions or app logic fail, we still proceed to teardown.
    2. discard = teardown_failure
# What phases fail
for r in 0..tx_events.length:
    if r.reverted:
        if r.phase == REVERTIBLES || r.phase == APP_LOGIC:
            r_insertion_or_app_logic_failure = true
        else if r.phase == TEARDOWN:
            teardown_failure = true

for r in 0..tx_events.length:
  e = tx_events[r]
  row = rows[r]

  if e.phase == NONREVERTIBLES || SETUP || FEE_PAYMENT:
    row.discard = 0
  else if e.phase == REVERTIBLES || e.phase == APP_LOGIC:
    row.discard = r_insertion_or_app_logic_failure || teardown_failure
  else if e.phase == TEARDOWN:
    row.discard = teardown_failure

@dbanks12 dbanks12 force-pushed the db/dedup-bc-commit branch 2 times, most recently from 32b01b3 to 7ff82e3 Compare August 8, 2025 18:57
Base automatically changed from db/dedup-bc-commit to next August 8, 2025 20:28
@dbanks12 dbanks12 force-pushed the db/tx-discard branch 3 times, most recently from 4d11b7a to 51cb9bc Compare August 12, 2025 16:32
@dbanks12 dbanks12 marked this pull request as ready for review August 12, 2025 16:33
@dbanks12 dbanks12 requested review from sirasistant and removed request for IlyasRidhuan, Maddiaa0, fcarreiro and jeanmon August 12, 2025 16:33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could live in tx.pil. I opted to throw it in a virtual file as I did with execution discard.

Comment on lines 103 to 106
WrittenPublicDataSlotsTreeCheckInterface& written_public_data_slots_tree,
SideEffectStates side_effect_states)
SideEffectStates side_effect_states,
TransactionPhase phase)
: merkle_db(merkle_db)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to add phase to the context like this. There are probably other ways. Phase is (and already was) needed for execution tracegen of discard, but we were just hardcoding it to APP LOGIC before.

@dbanks12 dbanks12 force-pushed the db/tx-discard branch 2 times, most recently from b7ed13d to f18cfa8 Compare August 12, 2025 18:38
bool res = avm.verify(proof, inputs.publicInputs, vk);
info("verification: ", res ? "success" : "failure");
if (!res) {
throw std::runtime_error("Generated proof is invalid!1!!1");
Copy link
Contributor

Choose a reason for hiding this comment

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

sad

Comment on lines +480 to +499
bool discard = false;
if (is_revertible(tx_phase_event->phase)) {
if (tx_phase_event->phase == TransactionPhase::TEARDOWN) {
discard = teardown_failure;
} else {
// Even if we don't fail until later in teardown, all revertible phases discard.
discard = teardown_failure || r_insertion_or_app_logic_failure;
}
}
if (tx_phase_event->reverted) {
// If we are in a revertible phase, we set the discard column to 1
// If we are in a non-revertible phase, we set the discard column to 0
discard = is_revertible(tx_phase_event->phase);
} else if (r_insertion_or_app_logic_failure && is_revertible(tx_phase_event->phase)) {
// If we have a revertible failure in a previous phase, we set the discard column to 1
discard = true;
} else if (teardown_failure && tx_phase_event->phase == TransactionPhase::TEARDOWN) {
// If we have a teardown failure, we set the discard column to 1
discard = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the mix of two strategies to set discard? or am i missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we can probably compute discard outside of the for, since discard only needs to be computed once per phase I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm i think you're right on both accounts. I'll clean up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleaned it up to only use the first approach, and moved that to the start of the outer "buckets" loop so it only happens once per-phase.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool will recheck

@dbanks12 dbanks12 force-pushed the db/tx-discard branch 3 times, most recently from 3dcc73f to 248fa75 Compare August 13, 2025 18:08
@dbanks12 dbanks12 requested a review from sirasistant August 13, 2025 19:15
@netlify
Copy link

netlify bot commented Aug 14, 2025

Deploy Preview for barretenberg ready!

Name Link
🔨 Latest commit e59c72f
🔍 Latest deploy log https://app.netlify.com/projects/barretenberg/deploys/689e546b17c0d5000846a4a3
😎 Deploy Preview https://deploy-preview-16258--barretenberg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@dbanks12 dbanks12 enabled auto-merge August 14, 2025 21:34
@dbanks12 dbanks12 added this pull request to the merge queue Aug 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2025
@dbanks12 dbanks12 added this pull request to the merge queue Aug 14, 2025
Merged via the queue into next with commit cd9768f Aug 14, 2025
15 checks passed
@dbanks12 dbanks12 deleted the db/tx-discard branch August 14, 2025 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants