feat: implement Optimism builder DA limits#13757
Conversation
There was a problem hiding this comment.
kind of unfortunate perf wise if we need to serialize every tx, right? should this be memoized somehow?
There was a problem hiding this comment.
agreed - is there a standard way to memoize serialized values? I added a simple memoized property to the struct here, but there might be a better way to do it. d41de90
There was a problem hiding this comment.
we can do this if we change the OpTransactionSigned type and a lazy field for this there, however this wouldn't enough for the building use case because this operates on OpPooledTx mostly, though luckily we can already provide different types for this.
I need to think about this a bit, because I had some ideas how we can improve the builder interface.
and ideally also solve the issue with repeated 2718 encoding as well.
|
Tested end-to-end using Kurtosis and setting block limits manually. I'm planning to add automated tests soon as well, but should be ready to review! |
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
cool, featurewise this is complete, but I'd like to exclude the compressed size caching in this pr because atm this doesn't have any effect anyway
There was a problem hiding this comment.
this makes sense, we def need this here
There was a problem hiding this comment.
I'd like to exclude this from this pr, because due to how we pull in txs from the pool this currently doesn't give us any benefits. I'll tackle this perf improvement shortly, because what we need to change here is op pooled type
There was a problem hiding this comment.
sounds good, I'll update to restrict the uncompressed size of the tx
There was a problem hiding this comment.
I'm not sure if this will be optimized by the compiler, but I think this is fine for now.
we could make this nicer by moving this into a new function of executioninfo
There was a problem hiding this comment.
moved it to execution info and fixed the short circuiting, but lmk if you had something else in mind and I can change it
meyer9
force-pushed
the
meyer9/txpool-da-config
branch
from
4b04de7 to
ab3f07f
Compare
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
lgtm, only one q re da size tracking
| if tx_data_limit.is_some_and(|da_limit| tx.length() as u64 > da_limit) { | ||
| return true; | ||
| } | ||
|
|
||
| if block_data_limit | ||
| .is_some_and(|da_limit| self.cumulative_da_bytes_used + (tx.length() as u64) > da_limit) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
this now uses the length instead of the flz compressed length
should we change this here? or is this fine for this pr?
There was a problem hiding this comment.
ah sorry, I may have misunderstood. I think it's OK to use uncompressed size temporarily until we implement DA cost tracking. I thought the comment above meant to remove the compressed size calculation for now, but I could add it back in without modifying the pooled tx type.
There was a problem hiding this comment.
I wrote an example implementation of this here, but I'm a little concerned about changing revm versions to a Git hash: dbe1610
If this looks OK, I can add it to this PR, but if this isn't urgent, we can just implement after the issue you created.
There was a problem hiding this comment.
meh that's unfortunate that this wasn't included in the latest revm release...
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
3 participants
Addresses #13131
This PR adds:
BestTransactionsAttributesthat will let us request all transactions under a certain DA sizeOpen question:
revmcurrently isn't updated, so I'm referencing the needed crate using a git spec. We could also just clone the function into a reth crate and remove it once we updaterevmif we want to avoid the git crate reference.