-
Notifications
You must be signed in to change notification settings - Fork 0
feat: adds host simulation to block build #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a2adc44 to
9b8137b
Compare
9b8137b to
40a2b56
Compare
59afcac to
cf31bc9
Compare
0c07c93 to
eb5a2f7
Compare
anna-carroll
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
84b510e to
6166fcb
Compare
cf31bc9 to
7600937
Compare
Merge activity
|
- properly builds a cfg env and block env from prev host header - makes max host gas a configurable property - cleans up the build prep in the sim loop
* fix: move block env creation for host to sim task * rename vars to clearly distinguish between rollup and host (#170) * comment --------- Co-authored-by: Anna Carroll <[email protected]> Co-authored-by: Anna Carroll <[email protected]>
b6d52e8 to
9c61587
Compare
| #[from_env( | ||
| var = "MAX_HOST_GAS_COEFFICIENT", | ||
| desc = "Optional maximum host gas coefficient, as a percentage, to use when building blocks", | ||
| default = 80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make this number a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is a percentage, it should be f64, and explicitly capped to 99.9% (due to eth consensus rules on limit changes
| /// Returns the maximum host gas to use for block building based on the configured max host gas coefficient. | ||
| pub fn max_host_gas(&self, gas_limit: u64) -> u64 { | ||
| // Set max host gas to a percentage of the host block gas limit | ||
| ((gas_limit as u128 * (self.max_host_gas_coefficient.unwrap_or(80) as u128)) / 100u128) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, this should be a constant so there's only 1 place that needs to change if we decide to change the coefficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly, I think that this should not be configurable right now. instead of having this be a config struct prop, we could just make it a constant 80%. we can make it configurable later if we want to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i'm happy with that, I think it's safer that way and makes other builders understand that changing this means you're running modified software
| 1740681556, // pecorino start timestamp as sane default | ||
| 0, 1, | ||
| ), | ||
| max_host_gas_coefficient: Some(80), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| let host_block_opt = res_unwrap_or_continue!( | ||
| self.host_provider.get_block_by_number(host_block_number.into()).await, | ||
| span, | ||
| error!("error fetching previous host block - skipping block submission") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would love someone to check my logic here:
it’s occurring to me that if the Builder is configured to use a Host RPC served by a different Node than is running the Rollup ExEx and serving the Rollup RPC, then they are more likely to be out of sync which would cause reliability issues in the Env task
in testnet this is impossible to configure incorrectly because there is only one Host RPC and it’s the exact same Node as is running the ExEx for the Rollup RPC, but in mainnet it would be easy to configure incorrectly
if i'm right we should document this configuration need for Builders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, possibly add some retry logic for the Host RPC call

feat: adds host simulation to block build
This PR adds host simulation to the block build loop.
0.15.1BuilderHelper wrapper contract submission in favor of the rawZenith contractHostProvider into the Simulator for host transaction simulation