-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Multichain Scripts: Deploy each sequence of transactions sequentially instead of in parallel. #6271
Multichain Scripts: Deploy each sequence of transactions sequentially instead of in parallel. #6271
Conversation
lgtm |
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.
nice! looks good, pending nit & ci
crates/forge/bin/cmd/script/multi.rs
Outdated
return sequence.verify_contracts(config, verify.clone()).await | ||
} | ||
Ok(()) | ||
let mut results: Vec<Result<(), Report>> = Vec::new(); // Declare a Vec of Result<(), Report> |
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.
no need for this comment, as the vec is already typed
|
||
let errors = | ||
join_all(futs).await.into_iter().filter(|res| res.is_err()).collect::<Vec<_>>(); | ||
let errors = results.into_iter().filter(|res| res.is_err()).collect::<Vec<_>>(); |
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.
Right, so I see what's going on:
- Before, we drove all futures to completion depending on availability (ideally in order), but this wasn't guaranteed—what's guaranteed is that they're stored in the order they were provided.
- Now, we're waiting for each batch to complete and storing its result, avoiding the race condition
This might mean that complex multichain deployments might take longer though, but i'm willing to take the hit for less failures
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, but the potential temporal dependencies between the batches would result in script failures.
I could still see people running into issues if their scripts use blockchain state that changes too quickly even after one block passing: i.e. getting the price of ETH in a ETH/USDC uni pool, and using that as an input to another script.
But for the most part I feel like people just wanna deploy their contracts & maybe do some configurations on them. Not so much use specific chain-state.
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.
and yeah, complex multichain deployments would take longer. I feel like it's a worthwhile tradeoff for the amount of flexibility that's enabled by it.
so, what the solution |
Motivation
As explained here foundry first runs a simulation, collects all the transactions then batch runs all the transactions. This causes issues in scenarios where the script has a flow like this:
In this case, steps 1 and 3 will generate two sequences of transactions both pertaining to Chain A. From the simulation that was previously run, the nonce of the first transaction in Sequence 3 will be one more than the nonce of the last transaction in Sequence 1. However, since foundry attempts to run both Sequences 1 & 3 in parallel, the nonce returned from the RPC will mismatch the first transaction in Sequence 3, and it'll result in the following error:
I can see that multiple users have also run into this issue: #4701 #6184
Solution
We can still run the transactions in each DeploySequence in parallel and enjoy the performances of batch transactions! However, in between sequence changes we have to wait until the last sequence's transactions are all submitted.
Potential Optimization
We could chunk the neighbouring sequences and execute those in parallel. Consider Chains A, B, C and a deploy script that results in these sequences:
A, B, A, B, B, C, A, B, B, A, A, C, C, C
We could chunk the neighbouring sequences like so:
A, B, A, (B, B), C, A, (B, B), (A, A), (C, C, C)
Although this may already be the case, I'm not sure how these sequences get generated.