-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: cost estimator #68
Conversation
} | ||
|
||
impl fmt::Display for ExecutionStats { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { |
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.
I thin it would be good to print this out in a comma separated format (like a CSV file), something like this: https://github.com/succinctlabs/rsp/blob/main/bin/host/src/execute.rs
ENV PATH="${GOPATH}/bin:${PATH}" | ||
|
||
# Set up Go environment | ||
WORKDIR /optimism |
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.
I feel like the correct way to do this is to put in the optimism repo & then build the docker file from there and push the image to docker hub?
I think it's semantically a bit incorrect to clone github repos in a dockerfile lol
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 after the server is finalized can do this.
Ok(response.ranges) | ||
} | ||
|
||
async fn start_span_batch_server() -> Result<Child> { |
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.
nit: I feel like what's more "semantically correct" is to execute the docker command first & then this bin, instead of starting the server in bin/cost_estimator.rs
let rollup_config = RollupConfig::from_l2_chain_id(l2_chain_id).unwrap(); | ||
|
||
// Fetch the span batch ranges according to args.start and args.end | ||
// TODO: If the ranges are greater than 20 blocks, we will have to split them in a custom way. |
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.
Is this because our executor is slow for large blocks?
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.
Yes
|
||
let prover = ProverClient::new(); | ||
|
||
// TODO: These should be executed in parallel. |
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 that's a good callout--I feel like you could do a par_iter
and use different threads since each executor is just single-threaded
* write in parallel * write in parallel * wip, wtf * add * feat: cost estimator --------- Co-authored-by: Ubuntu <[email protected]>
Overview
Add a cost estimator tool that outputs the relevant statistics across a range of blocks.
Simplify the pipeline for surfacing the next relevant block.
Simplify program block execution logic, given that only one payload is produced at a time.
Add more resilient logic for timing out the execution of the native host.