fix: dropped tagging indices no longer cause a pxe crash on sync#23044
Merged
nventuro merged 14 commits intoMay 12, 2026
Conversation
Collaborator
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
nchamo
reviewed
May 9, 2026
benesjan
approved these changes
May 11, 2026
| @@ -0,0 +1,122 @@ | |||
| import { type Logger, createLogger } from '@aztec/foundation/log'; | |||
Contributor
There was a problem hiding this comment.
It makes sense to move this file, persist_sender_tagging_index_ranges.ts, reconcile_tagging_index_ranges.test.ts and reconcile_tagging_index_ranges.ts to yarn-project/pxe/src/tagging/sender_sync as this is sender sync related. Directly in the tagging dir I kept only functionality relevant to both sender and recipient sync.
Contributor
Author
There was a problem hiding this comment.
This is not part of the sync though, it's part of sending a transaction in which tags were used. I think the more complete answer might be that we're missing some intermediate layer e.g. a TaggingService, and I'd rather not hide that here. But it's a good callout at this being smelly.
…yte code size blow up (#23062) Resolves [F-637](https://linear.app/aztec-labs/issue/F-637/aztec-nr-macros-contain-self-construction-to-a-function-to-prevent) Stacks on #23061 - New `generate_public_self_creator` emits a per-contract `__aztec_nr_internals__create_public_self<let N: u32>()` helper - `generate_public_external` now emits a single call to it instead of inlining the preamble. This can be seen in the snapshots. - Helper is emitted from `process_functions` and gated on `public_functions.len() > 0` ~~Improvements tested locally:~~ I need to test further. Either way this is cleaner macro code.
…g-range-in-sendertaggingstore-on-repeated
nventuro
deleted the
nico/f-630-bug-pxe-conflicting-range-in-sendertaggingstore-on-repeated
branch
AztecBot
pushed a commit
that referenced
this pull request
May 12, 2026
) Fixes #22949. We keep track of the range of tagging indices used by a tx as `(lowestIdx, highestIdx)` - we need this so that future transactions start from `highest + 1` even if a previous tx has not yet been mined, avoiding tag duplication. The lowest index is required in case of transaction reverts: we walk all indices from low to high and test which ones survived onchain, and that way determine the actual highest index for a tx. On sync, we validate that our records for a tx match the indices used in said tx - this helps detect bugs (like the bug being fixed here). This check started failing because our `(low, high)` record did not consider the possibility of logs being squashed - if either `low` or `high` got squashed then the range would change. Note that we don't care about indices between low and high being squashed , gaps are ok - we only really care about the edges. This PR fixes this by inspecting tx effects to adjust the range prior to storing the indices in the database. It hints at the need for a tagging service layer, but that was a bigger change that I didn't want to introduce here. I added tests for both new functions created, plus an e2e regression test that checks we've fixed the original issue. I validated that the test fails without this fix. --------- Co-authored-by: Nicolas Chamo <nicolas@chamo.com.ar> Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
Collaborator
|
✅ Successfully backported to backport-to-v4-next-staging #23142. |
This was referenced May 12, 2026
benesjan
added a commit
that referenced
this pull request
May 12, 2026
…ertion regex (#23193) ## Summary Compat-e2e regressed on `v4.3.0-nightly.20260512-1` (https://github.com/AztecProtocol/aztec-packages/actions/runs/25731562906). After review with Jan and David, the two failing tests need different treatments — the PXE error enrichment itself is *not* broken; this is purely test-vs-old-artifact text drift. ## What this PR does - **`e2e_event_logs.test.ts`**: drop from the compat-e2e matrix. The new `tagging cache reconciliation against kernel squashing` test added in PR #23044 calls `TestLog::deliver_squashed_and_surviving_notes`, a method that does not exist in legacy 4.2.x artifacts. There is no reasonable backwards-compat coverage here without backporting the contract method itself. - **`e2e_avm_simulator.test.ts`**: keep it in the compat-e2e matrix and loosen the assertion-message regex to accept either the older or the newer assertion-span form: - old (pre-#22911 nargo): `'not_true == true'` - new (post-#22911 nargo): `'assert(not_true == true, "This assertion should fail!")'` The regex becomes `/Assertion failed: This assertion should fail!.*not_true == true/`, which matches both shapes and still rejects unrelated assertions. The test continues to validate the PXE's public-error enrichment pipeline (assertion decode + opcode→source resolution) against legacy artifacts — which is the actually-valuable coverage David flagged in the team-fairies thread. ## Why not exclude `avm_simulator` too? David pointed out that this case is the only public-error-enrichment test we have, and the enrichment code path is genuinely worth keeping under compat watch. Verified manually that enrichment runs correctly against legacy 4.2.x artifacts (paths, line/col, function names, assertion message all resolve); the only thing that differed was the locationText span granularity that nargo bakes into the artifact at compile time. Loosening the regex preserves the coverage without breaking on the legacy span shape. ## Test plan - `node -e '/Assertion failed: This assertion should fail!.*not_true == true/.test(…)'` matches both old and new forms, rejects unrelated assertions. - `bash -c 'shopt -s extglob; ls src/e2e_!(block_building|prover_*|kernelless_simulation|event_logs).test.ts'` from `yarn-project/end-to-end` resolves to 49 files (was 48 when `avm_simulator` was also excluded), `event_logs` correctly absent. - Compat-e2e is `continue-on-error` for nightlies — next nightly should be green on `ci-compat-e2e` once this lands. A forward-port of the regex loosening to `next` is also worthwhile (same file, identical strict regex there today), but mainline `next` doesn't run compat-e2e so it isn't blocking; can ship as a follow-up.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #22949.
We keep track of the range of tagging indices used by a tx as
(lowestIdx, highestIdx)- we need this so that future transactions start fromhighest + 1even if a previous tx has not yet been mined, avoiding tag duplication. The lowest index is required in case of transaction reverts: we walk all indices from low to high and test which ones survived onchain, and that way determine the actual highest index for a tx.On sync, we validate that our records for a tx match the indices used in said tx - this helps detect bugs (like the bug being fixed here). This check started failing because our
(low, high)record did not consider the possibility of logs being squashed - if eitherloworhighgot squashed then the range would change. Note that we don't care about indices between low and high being squashed , gaps are ok - we only really care about the edges.This PR fixes this by inspecting tx effects to adjust the range prior to storing the indices in the database. It hints at the need for a tagging service layer, but that was a bigger change that I didn't want to introduce here. I added tests for both new functions created, plus an e2e regression test that checks we've fixed the original issue. I validated that the test fails without this fix.