feat: enforce maximum serialized size for output notes#2205
feat: enforce maximum serialized size for output notes#2205bobbinth merged 36 commits into0xMiden:nextfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a maximum serialized size limit of 32 KiB for individual output notes to prevent excessively large notes from being created. The limit is enforced during OutputNotes construction and applies to all output note variants (Full, Partial, and Header).
- Added
NOTE_MAX_SIZEconstant (32 KiB) and integrated it into validation logic - Implemented
get_size_hintmethods throughout the note hierarchy to enable size calculation - Added validation in
OutputNotes::newto reject oversized notes with a descriptive error
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/miden-protocol/src/constants.rs |
Defines NOTE_MAX_SIZE constant (32 KiB) |
crates/miden-protocol/src/errors/mod.rs |
Adds OutputNoteSizeLimitExceeded error variant with note ID and size details |
crates/miden-protocol/src/transaction/outputs.rs |
Implements size validation in OutputNotes::new, adds get_size_hint for OutputNote and OutputNotes, and includes comprehensive tests |
crates/miden-protocol/src/note/mod.rs |
Implements get_size_hint for Note based on metadata and details sizes |
crates/miden-protocol/src/note/partial.rs |
Implements get_size_hint for PartialNote using metadata, recipient digest, and assets sizes |
crates/miden-protocol/src/note/header.rs |
Implements get_size_hint for NoteHeader summing note ID and metadata sizes |
crates/miden-protocol/src/note/metadata.rs |
Implements get_size_hint for NoteMetadata returning Word::SERIALIZED_SIZE |
crates/miden-protocol/src/note/details.rs |
Implements get_size_hint for NoteDetails summing assets and recipient sizes |
crates/miden-protocol/src/note/assets.rs |
Implements get_size_hint for NoteAssets accounting for count prefix and individual asset sizes |
crates/miden-protocol/src/note/inputs.rs |
Implements get_size_hint for NoteInputs accounting for length prefix and individual input sizes |
crates/miden-protocol/src/note/recipient.rs |
Implements get_size_hint for NoteRecipient summing script, inputs, and serial number sizes |
crates/miden-protocol/src/note/script.rs |
Implements get_size_hint for NoteScript summing MAST and entrypoint sizes |
crates/miden-protocol/src/note/note_id.rs |
Implements get_size_hint for NoteId returning Word::SERIALIZED_SIZE |
crates/miden-protocol/src/note/nullifier.rs |
Implements get_size_hint for Nullifier returning Word::SERIALIZED_SIZE |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Forostovec - thank you for working on this! Seems like some tests are not passing. Could you make the CI green? |
Made changes |
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Great work! Looks good!
I left a few small comments. Once these are addressed, I think this is good to merge.
| let mock_note = Note::mock_noop(Word::empty()); | ||
| let output_note = OutputNote::Full(mock_note); | ||
|
|
||
| let bytes = output_note.to_bytes(); | ||
|
|
||
| assert_eq!(bytes.len(), output_note.get_size_hint()); |
There was a problem hiding this comment.
I think we should construct a more complex note to make sure everything is correctly covered. The note returned by mock_noop is a bit too simple, I think. E.g. I would add at least two assets and at least two note inputs.
| fn get_size_hint(&self) -> usize { | ||
| let u16_size = 0u16.get_size_hint(); | ||
| let notes_size: usize = self.notes.iter().map(|note| note.get_size_hint()).sum(); | ||
|
|
||
| u16_size + notes_size | ||
| } |
There was a problem hiding this comment.
I think we don't technically need this method for the purpose of this issue, right? If so, I'd remove it. Especially since it isn't covered by a test.
There was a problem hiding this comment.
I think we don't technically need this method for the purpose of this issue, right? If so, I'd remove it. Especially since it isn't covered by a test.
Did it!
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline.
One thing that I'm still trying to think through is at what level do we want to enforce this limit. The approach taken in this PR is to enforce this at OutputNotes level - but there are other options:
- Enforce it at
Notelevel. This would imply that we'd need to makeNote::new()fallible - which is annoying. And I'm also not 100% sure this is the right approach. - Enforce it at
OutputNotelevel. This would probably be ideal because this would also imply that the limit is automatically imposed forOutputNoteBatchas well, but sinceOutputNoteis an enum, I'm not sure how to do this.
Also, I wonder if we should impose a separate limit on NoteScript. And maybe an alternative approach could be to impose limits on individual note components - though, this has its own downsides.
| // Size of the serialized inputs length prefix. | ||
| let u16_size = 0u16.get_size_hint(); | ||
|
|
||
| let values_size: usize = self.values.iter().map(|value| value.get_size_hint()).sum(); |
There was a problem hiding this comment.
Why do we need to iterate over inputs to compute the size? Each input is just one Felt and so we should be able to just multiple the number of inputs by felt size, right?
|
Good points, thank you!
Having a limit on the note script may allow omitting any checks at the output note level, since then all note components are size limited, e.g:
If we now impose a note script limit of, say, 16 KiB, then the maximum possible note size is 48 KiB. This is higher than the 32 KiB we want to impose as an overall limit here, but this could be okay because fees will already disincentivize constructing needlessly large notes. Not for this PR, but one separate question is whether we will be able to impose a note script limit in the tx kernel (and similarly for account code). I believe we can't really get the script size from within the VM, so it seems to me we can't enforce this at that level, short of hashing the entire script of a note in the VM. So, if we simply add validation to The alternatives are:
Out of these, the second one is annoying but probably cleanest to enforce the limit only in one place. Overall, I would prefer the |
I agree with this. Though, I wonder if we should start with 32KB - mostly because the way we serialize MAST code now is not super efficient (and I'm actually not sure if we strip out all the decorator info). There are a few additional considerations here:
Also cc @greenhat and @huitseeker as some of this is relevant to the topics you are working on. |
If I serialize the
FWIW, we don't use |
Oh wow - that's pretty large. For comparison:
Not directly related to this issue, but would be good to understand where the 70KB size is coming from:
@greenhat - could you create an issue in the compiler repo to investigate these? For this issue, I think we should definitely set the |
Essentially we'd be doing link-time dead code elimination, but starting by explicitly marking certain procedures as dead (e.g. the note script constructor). I'm not sure
It definitely does, such as the We may also need to play with optimization levels and such to see if that makes any meaningful difference. Now that we have some practical example programs, we should do some statistical analysis of the MASM to determine where the bulk of the instruction count comes from. My suspicion is that it is primarily stack management operations, but if we see a lot of common sequences, we could look into factoring those out into intrinsic procedures to collapse those sequences. I'm sure there is fat to be trimmed, but we are also inherently fighting a bit of an uphill battle here, because we are effectively emulating Wasm on Miden, so there will always be some impedance mismatch that results in more code than would otherwise be necessary. Luckily, there aren't too many places where that mismatch is particularly noticeable, but its still something we have to take into account. |
|
I think there could be two points in time where enforcing the size limit could make sense:
As @bobbinth mentioned before, we also want to make sure that these limits continue to be enforced at the batch and block level:
So, maybe we should enforce the output note size limits separately to not make testing harder at the cost of enforcing the limit in a few places. This would be somewhere in:
I'm hoping there's a better solution still, but I don't see it. |
|
The more I think about this, the more it seems like this and #1812 are related and should be tackled together. Specifically:
This makes me think that: we want to enforce note size limits for The only annoying thing here is that Another way to do this is to rethink the
Both of these are adjusted via the |
… output note size hint test assets
Hi , sorry for the delay. Todo, simplification of noteinputs size hint was done, and fixed failling ci because of duplicate fungible asset |
I like this approach as it is more type safe, achieves the note size enforcement and the removal of decorators. One small challenge is that we'll want to enforce the constraints from // Previously OutputNote
pub enum UnprovenOutputNote {
Full(Note),
Partial(PartialNote),
Header(NoteHeader),
}
// New type that is created during proving from `UnprovenOutputNote::shrink` (which may be worth
// renaming at this point)
pub enum ProvenOutputNote {
Public(PublicOutputNote),
Header(NoteHeader),
}
// Thin wrapper around `Note` that enforces that the contained Note is public, does not contain
// decorators and has a maximum size.
pub struct PublicOutputNote(Note);
// Used in ExecutedTransaction
pub type UnprovenOutputNotes = OutputNotes<UnprovenOutputNote>;
// Used in ProvenTransaction
pub type ProvenOutputNotes = OutputNotes<ProvenOutputNote>;
// Structurally unchanged; generic N parameter added.
pub struct OutputNotes<N> {
notes: Vec<N>,
commitment: Word,
}
// Unless we introduce an OutputNote trait, which seems unnecessary, we'll need to abstract
// over Unproven and Proven output notes like this:
impl<N> OutputNotes<N>
where
for<'a> NoteHeader: From<&'a N>,
for<'a> NoteId: From<&'a N>,
{ ... }I'm not sure about the Unproven/Proven naming. An alternative may be Structurally this should work pretty nicely. During output note "shrinking", any public note needs to be converted into a This gives us the chance to either:
Since we have the ability to enforce a max note size explicitly, this seems slightly better, but not a strong opinion. |
|
I really like how this is shaping up. @Forostovec would you be able to take a stab at this new approach? A few comments:
Yeah, I don't love any of these (maybe the "raw" approach is a bit better than "proven"/"unproven" approach) - but also don't have a better suggestion yet.
I'd go with the first option - i.e., enforce the limit at
I wonder if we could use the same approach for |
Yeah, I'm gonna get into this tomorrow morning as soon as possible (now it's 01:26 on my time sorry) |
…ock-chain genesis notes
I hope I didn't miss anything - refactored OutputNote into RawOutputNote / ProvenOutputNote with PublicOutputNote enforcing NOTE_MAX_SIZE at shrink-time, updated tx/batch/block types, and in miden-testing MockChain keep full genesis notes so tests can still create InputNotes. |
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
Hi, sorry for delay, all is done |
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
Left a few minor nits, but otherwise I think this is ready. Let's wait for one more review before merging.
The handling of private notes in the mock chain is a bit of a mess, but that's not the fault of this PR. On the contrary, this PR basically revealed it since we now have proper enforcement that private notes cannot be public beyond the proven tx stage. Let's discuss that in #2307.
| pub fn shrink(&self) -> Self { | ||
| /// # Errors | ||
| /// Returns an error if a public note exceeds the maximum allowed size ([`NOTE_MAX_SIZE`]). | ||
| pub fn to_proven_output_note(&self) -> Result<ProvenOutputNote, PublicOutputNoteError> { |
There was a problem hiding this comment.
After this PR, we can do a follow-up PR that changes this to self so we don't need to clone here. In build_proven_transaction, we should be able to destructure let TransactionOutputs { output_notes, .. } to avoid a clone.
| // Creating a PublicOutputNote should fail with size limit error | ||
| let result = PublicOutputNote::new(oversized_note); | ||
|
|
||
| assert_matches!( | ||
| result, | ||
| Err(PublicOutputNoteError::NoteSizeLimitExceeded { note_id: _, note_size }) | ||
| if note_size > NOTE_MAX_SIZE as usize | ||
| ); |
There was a problem hiding this comment.
Can we combine the test shrink_enforces_size_limit_on_public_notes into this one? The setup looks very similar and I think we can just check that OutputNote::Full(oversized_note).to_proven_output_note() returns an error here.
There was a problem hiding this comment.
Can we combine the test
shrink_enforces_size_limit_on_public_notesinto this one? The setup looks very similar and I think we can just check thatOutputNote::Full(oversized_note).to_proven_output_note()returns an error here.
Sure, did it
CHANGELOG.md
Outdated
| - [BREAKING] Removed OLD_MAP_ROOT from being returned when calling [`native_account::set_map_item`](crates/miden-lib/asm/miden/native_account.masm) ([#2194](https://github.com/0xMiden/miden-base/pull/2194)). | ||
| - [BREAKING] Refactored account component templates into `StorageSchema` ([#2193](https://github.com/0xMiden/miden-base/pull/2193)). | ||
| - [BREAKING] Refactored account component templates into `AccountStorageSchema` ([#2193](https://github.com/0xMiden/miden-base/pull/2193)). | ||
| - Introduce NOTE_MAX_SIZE (32 KiB) and enforce it on individual output notes ([#2205](https://github.com/0xMiden/miden-base/pull/2205)) |
There was a problem hiding this comment.
Since starting this PR we've released version 0.13. Could you move this entry to 0.14 now? Thanks!
There was a problem hiding this comment.
Since starting this PR we've released version 0.13. Could you move this entry to 0.14 now? Thanks!
Ready
| pub enum ProvenOutputNote { | ||
| /// A public note with full details, size-validated. | ||
| Public(PublicOutputNote), | ||
| /// A note header (for private notes or notes without full details). | ||
| Header(NoteHeader), | ||
| } |
There was a problem hiding this comment.
For a separate PR, I think the Header variant can be renamed to Private, since this should only ever be constructed from private notes. To disallow construction of a ProvenOutputNote::Header with a NoteHeader whose metadata is public, maybe this should be a PrivateNoteHeader newtype that enforces this, so:
pub enum ProvenOutputNote {
/// A public note with full details, size-validated.
Public(PublicOutputNote),
/// A note header (for private notes or notes without full details).
Private(PrivateNoteHeader),
}Next to this, I think OutputNote::Partial and OutputNote::Header should also only ever be constructed with metadata that is private, so I think we should consider enforcing private metadata in PartialNote and also use PrivateNoteHeader in OutputNote::Header. Or in other words, only OutputNote::Full can contain both public and private notes. cc @bobbinth in case I'm saying something wrong.
There was a problem hiding this comment.
Agreed - this structure would enforce all relevant invariants for both public and private output notes.
Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
| Public(PublicOutputNote), | ||
| /// A note header (for private notes or notes without full details). | ||
| Header(NoteHeader), | ||
| } |
There was a problem hiding this comment.
Generally, this PR looks great - the only thing I'm not sure about is the naming. Specifically, ProvenOutputNote sounds a bit off since the note itself is not proven. What it is is a more "final form" of the output note. I don't have great suggestions here, FinalizedOutputNote is an option, but I'm not sure it is any better.
Another option is to rename the current OutputNote into something like RawOutputNote and then ProvenOutputNote could be renamed into just OutputNote.
There was a problem hiding this comment.
Another option is to rename the current
OutputNoteinto something likeRawOutputNoteand thenProvenOutputNotecould be renamed into justOutputNote.
I think that's probably the best option I can think of.
I would suggest merging this PR and do the rename in a small follow-up PR to make reviewing that easier. We could also rename ProvenOutputNote::Header to Private in that PR, and I can open an issue for the remaining things mentioned above.
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! As mentioned in the comments, I think we should rename some types/variants - but I agree with @PhilippGackstatter that we can do this in a follow up PR. So, I'll merge this PR as is.
@PhilippGackstatter - could you create an issue with the remaining follow-up items?
| /// Output notes produced during transaction execution (before proving). | ||
| /// | ||
| /// Contains [`OutputNote`] instances which represent notes as they exist immediately after | ||
| /// transaction execution. | ||
| pub type OutputNotes = RawOutputNotes<OutputNote>; | ||
|
|
||
| /// Output notes in a proven transaction. | ||
| /// | ||
| /// Contains [`ProvenOutputNote`] instances which have been processed for inclusion in | ||
| /// proven transactions, with size limits enforced on public notes. | ||
| pub type ProvenOutputNotes = RawOutputNotes<ProvenOutputNote>; |
There was a problem hiding this comment.
In a follow-up PR, this would switch as:
OutputNotes->RawOutputNotes.ProvenOutputNotes->OutputNotes.
This means that we'll need to come up with a new name for the current RawOutputNotes type, though I don't have great suggestions for this yet.
|
@Forostovec Would you be interested in doing a small follow-up PR that renames |
Introduced
NOTE_MAX_SIZE(32 KiB) and enforce it on individual output notes. The limit is applied based on theSerializable::get_size_hintimplementation for notes and their components.NOTE_MAX_SIZEconstant for serialized note sizeget_size_hintforNote,PartialNote, and related note typesget_size_hintforOutputNoteandOutputNotesNOTE_MAX_SIZEinOutputNotes::newusingTransactionOutputError::OutputNoteSizeLimitExceeded { note_id, note_size }OutputNote::get_size_hintmatches serialized lengthOutputNotes::newreturnsOutputNoteSizeLimitExceededfixes #2195