fix(agglayer): enforce NoteType::Public for B2AGG bridge-out notes#2988
fix(agglayer): enforce NoteType::Public for B2AGG bridge-out notes#2988partylikeits1983 wants to merge 5 commits into
Conversation
4ce0b10 to
9016ace
Compare
The B2AGG note type lives in NoteMetadata, not in the recipient commitment, so a recipient-identical note marked NoteType::Private was accepted on-chain. Its leaf would be folded into the Local Exit Tree while aggkit could never recover the pre-image, permanently desyncing the LET mirror and bricking the bridge-out path. Enforce the public note type in the bridge branch of B2AGG.masm before the note storage is trusted. The public/private check is factored into a reusable miden::standards::note::metadata::is_note_public procedure that returns whether the active note is public. Add a parameterized test covering the recipient-identical private note alongside the existing invalid-destination case. Closes #2984 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9016ace to
2569f44
Compare
…blic comments Add the missing CHANGELOG.md entry for the B2AGG public-note enforcement so the changelog CI check passes, and tidy the is_note_public doc/inline comments (drop a redundant parenthetical, fix the 'mask off' wording, and split the note-type extraction into one op per line to match the repo's masm style). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline.
| #! | ||
| #! Panics if: | ||
| #! - no note is currently active. | ||
| pub proc is_note_public |
There was a problem hiding this comment.
It feels like this procedure should be in the protocol rather than in standards. Probably the right place for it is active_note module - i.e., active_note::is_public.
| # The bridged note must be public (see the note documentation above). | ||
| exec.metadata::is_note_public assert.err=ERR_B2AGG_NOTE_MUST_BE_PUBLIC |
There was a problem hiding this comment.
Would it make sense to put this check into the bridge::bridge_out::bridge_out procedure? With the note allowlist, it doesn't matter too much - but it feels like the type of the concern that the bridge code should check.
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me, though my agglayer-related knowledge is small.
| exec.active_note::get_metadata | ||
| # => [NOTE_ATTACHMENT, METADATA_HEADER] | ||
|
|
||
| # drop the attachment word, keeping the metadata header | ||
| dropw | ||
| # => [sender_id_suffix_and_note_type, sender_id_prefix, tag, attachment_kind_scheme] | ||
|
|
||
| # move the merged suffix/note_type felt to the bottom and drop the other three header felts | ||
| movdn.3 drop drop drop | ||
| # => [sender_id_suffix_and_note_type] | ||
|
|
||
| # take the low 32 bits of the felt | ||
| u32split swap drop | ||
| # => [suffix_and_note_type_lo] | ||
|
|
||
| # mask to the low byte, which holds the note type | ||
| u32and.0xff | ||
| # => [note_type] | ||
|
|
||
| eq.NOTE_TYPE_PUBLIC | ||
| # => [is_public] |
There was a problem hiding this comment.
I'd replace the extraction logic with a call to miden::protocol::note::metadata_into_note_type, unless this doesn't exist on this branch.
But agreed that this procedure should be moved to active_note.
| # Note type encoding for a public note (see the protocol note type definition). | ||
| const NOTE_TYPE_PUBLIC = 1 |
There was a problem hiding this comment.
We have this constant in miden::protocol::note (again unless this branch doesn't have it yet). More generally, if such protocol-level data is missing, it would be good to add it in the appropriate place to avoid duplication.
| async fn test_active_note_is_note_public( | ||
| #[case] note_type: NoteType, | ||
| #[case] expected_is_public: Felt, | ||
| ) -> anyhow::Result<()> { |
There was a problem hiding this comment.
The NoteType encoding in Rust and MASM is equivalent, so we shouldn't need expected_is_public. We can compute it as note_type.as_u8().
If we add active_note::is_public, might be good to add is_private as well for convenience/completeness and extend this test to cover that procedure, too.
…ote::is_public/is_private Address review feedback on #2988: - Move the public-note check out of the B2AGG note script and into bridge::bridge_out::bridge_out, where it belongs (per Bobbin). - Move the is_public logic into the protocol active_note module as active_note::is_public, backed by a new reusable note::extract_note_type_from_metadata primitive (per Bobbin & Philipp). - Add active_note::is_private for completeness (per Philipp), and drop the redundant expected_is_public test column by deriving the expected flags from NoteType. - Remove the standalone standards/note/metadata.masm module. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mmagician
left a comment
There was a problem hiding this comment.
LGTM modulo the comment verbosity
| #! The note being bridged out must be public: AggLayer's off-chain indexer (aggkit) mirrors the | ||
| #! Local Exit Tree by observing notes in the public block space, so the appended leaf's pre-image | ||
| #! must be publicly observable. The note type lives in the metadata header rather than the recipient | ||
| #! commitment, so it is checked explicitly before the note's data is trusted. |
There was a problem hiding this comment.
too verbose, we should describe behavior not impl details
| // The Rust and MASM `NoteType` encodings are identical, so derive the expected flags from the | ||
| // note type rather than passing them as separate test cases. |
There was a problem hiding this comment.
Looks like Claude commenting on what it's doing
Co-authored-by: Marti <marti@miden.team>
Address review nits on #2988: reduce the bridge_out public-note doc comment to a single behavior-level line (dropping indexer mechanism and metadata-header implementation detail), and remove the change-narrating comment in test_active_note_is_public_and_is_private. Co-Authored-By: Claude (Opus) <noreply@anthropic.com>
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one question inline - but I suspect this is because this is going into the agglayer branch. We should make sure to update the constants when we merge this back into next.
| # Note type encoding, mirroring the Rust `NoteType` definition in `note/note_type.rs` | ||
| # (`PUBLIC = 0b01`, `PRIVATE = 0b10`). | ||
| const NOTE_TYPE_PUBLIC = 1 | ||
| const NOTE_TYPE_PRIVATE = 2 |
There was a problem hiding this comment.
Are we setting these this way because this is going into the agglayer branch? On next, private is 0 and public is 1.
Also, could we not import these from note shared utils module?
Closes #2984.
Problem
B2AGG (bridge-out) notes must be
NoteType::Publicso AggLayer's off-chain indexer (aggkit) can recover the leaf pre-image and mirror the on-chain Local Exit Tree (LET). The note type lives inNoteMetadata, not in the recipient commitment, so an attacker can assemble a note with an identical recipient, attachment, and asset butNoteType::Private. To consensus this is indistinguishable from a compliant B2AGG note: the bridge picks it up,bridge_outruns end-to-end, and the LET frontier is updated. But off-chain, only theNoteIdis published for a private note, so aggkit can never reconstruct the just-appended leaf. From that leaf on, aggkit's LET mirror permanently diverges from the bridge account's storage, bricking the bridge-out exit path for everyone.Fix
Enforce the public note type in the bridge (non-reclaim) branch of
B2AGG.masm, before the note storage is trusted.The public/private check is factored into a reusable procedure
miden::standards::note::metadata::is_note_public, which reads the active note's metadata and returns whether it is public. The note type is the low byte of the metadata header'ssender_id_suffix_and_note_typefelt.Note on the implementation: the fix suggested in the issue uses
miden::protocol::note::metadata_into_note_typeand a re-exportedNOTE_TYPE_PUBLIC, but those were added onnext(PR #2738) and do not exist on theagglayerbranch, so the helper above is implemented against agglayer's existing primitives (active_note::get_metadata).Follow-ups
is_note_publicprocedure (miden::standards::note::metadata). It is useful well beyond the bridge and should be cherry-picked back to thenextbranch, where it can sit alongside / build onmetadata_into_note_type(feat: addmetadata_into_note_typeprocedure to note.masm #2738).