Optimize preimage reading in provers#4337
Open
pmikolajczyk41 wants to merge 15 commits intomasterfrom
Open
Conversation
Contributor
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4337 +/- ##
==========================================
- Coverage 33.98% 32.92% -1.07%
==========================================
Files 488 488
Lines 57783 57783
==========================================
- Hits 19640 19025 -615
- Misses 34672 35430 +758
+ Partials 3471 3328 -143 |
bragaigor
previously approved these changes
Feb 9, 2026
tsahee
requested changes
Feb 11, 2026
Contributor
tsahee
left a comment
tsahee
left a comment
There was a problem hiding this comment.
I still didn't do a deep look at the host-io implementation, which I will need to do.
The general design is great!
Just one comment on the go side in the meantime.
We'll only push this after arbos-60.
| preimage := make([]byte, INITIAL_PREIMAGE_ALLOCATION) | ||
|
|
||
| // 1. Read the preimage prefix (up to INITIAL_PREIMAGE_ALLOCATION bytes) | ||
| preimageLen := readPreimage(uint32(ty), hashUnsafe, unsafe.Pointer(&preimage[0]), 0, INITIAL_PREIMAGE_ALLOCATION) |
Contributor
There was a problem hiding this comment.
this should be a for loop that can handle any preimage size.
Blobs are 128Kib, and certificate-DA could be even larger then that.
Contributor
There was a problem hiding this comment.
You can have variation of ResolveTypedPreimage that accepts size per read to use
In /Users/tsahi/src/nitro/cmd/replay/main.go:
- GetBlobs will always read an image of the same size (1 full blob)
- getBlockHeaderByHash reads one block header, much smaller
- (db PreimageDb) Get (in db.go) reads from the triedb and I think that for that the 512 constant works well
Member
Author
There was a problem hiding this comment.
before I handle the variation with size hint, I wanted to clarify this:
this should be a for loop that can handle any preimage size
I claim that the current code will handle any preimage size with at most 2 calls to readPreimage, so no need for any loop.
tsahee
added
the
after-next-version
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
3 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.
Changes
wavmio.readPreimage(described in detail below).wavmio.resolveTypedPreimagefunction. However, both provers still support this API in order to handle oldreplay.wasmbinaries. (The same way as we did withwavmio.resolvePreImage).Previous mechanism for reading preimages
The old
wavmiomodule linked toreplay.wasmprovided a methodwavmio__resolveTypedPreimage, which reads (up to) 32 bytes of a queried preimage at a given offset. Given thatreplay.wasmcares only about reading preimages in full, this was suboptimal, as most of the preimages are between 80 and 500 bytes (with some exceptions). For every preimage longer than 32 bytes we had to loop multiple times over it, each iteration calling the external module function, that was fetching full preimage.In particular, in the SP1 execution environment, this brought a significant performance penalty.
Proposed mechanism
Ideally, we'd add a new function
wavmio__readPreimage( preimageType uint32, hash unsafe.Pointer, output unsafe.Pointer )(so without theoffset uint32argument when compared to the oldwavmio__resolveTypedPreimage). The problem is that the caller has to allocate enough memory for the output (the image). That's why we do as follows.The function has the following signature:
tyis the preimage type (as before)hashis the pointer to the 32 bytes of preimage hash (as before)outputis the pointer to the buffer for either the preimage prefix of suffix (depending on the following arguments)preimageOffsetis the offset in the preimage that we want to start reading from; this will always be either 0 or 512 (a constant)allocatedOutputSpaceis the free allocated space at theoutputpointerThe function always returns the length of the preimage (not the number of bytes read!).
The caller allocates an initial memory buffer of 512 bytes (enough for vast majority of preimages). Then it does the first call to
readPreimage:output,preimageOffsetandallocatedOutputSpacearguments) in order to read the preimage suffix.Thus we always have at most 2 calls to the
wavmiomodule.closes NIT-4372
Co-authored-by: @wakabat