Feat: min slot bid svm #291
Conversation
| @@ -190,11 +191,15 @@ impl Simulator { | |||
| for chunk_result in chunk_results { | |||
| let chunk_result = chunk_result?; | |||
| result.extend(chunk_result.value); | |||
There was a problem hiding this comment.
as a note this could lead to accounts drawn from different slots, there may be some issues with that with regard to mistaken simulation results? it's good to use the min slot below, but there may still be issues resulting from the different results having different context slots, e.g. dex router tx simulates successfully when accounts are all synced, but may fail simulation when they are drawn at different context slots
There was a problem hiding this comment.
agreed, this can become problematic with AMM accounts. I added a comment
| .config | ||
| .chain_config | ||
| .simulator | ||
| .simulate_transaction(&bid.chain_data.transaction) |
There was a problem hiding this comment.
i think the retry should happen within the fetching? before the simulator actually runs? otherwise it's a weird interface where we're asking for a minimum slot to simulate against and then ignoring that as long as it passes simulation.
There was a problem hiding this comment.
we can also add some checks to make sure searchers aren't submitting slots that are more than some reasonable number off of where the rpc currently stands
There was a problem hiding this comment.
I consider this some internal behaviour which is subject to change. Interface only adds the option for a searcher to guarantee successful simulation from the specified slot. How we verify this internally is not part of the interface.
I classify that as a DoS attack and don't think it's a priority
There was a problem hiding this comment.
in that case, maybe it's worth a comment baking that into the interface on the SDK side, or in the BidChainDataCreateSvm object in auction-server/src/auction/entities/bid.rs
There was a problem hiding this comment.
there is a comment already there. do you think we should explain more?
| @@ -578,25 +578,43 @@ impl Service<Svm> { | |||
| } | |||
|
|
|||
| pub async fn simulate_bid(&self, bid: &entities::BidCreate<Svm>) -> Result<(), RestError> { | |||
There was a problem hiding this comment.
I think calling this function recursively will make the code more readable (instead of having loop)
There was a problem hiding this comment.
I also considered that but that was also a bit weird. Refactored a bit to make it more readable
| @@ -613,6 +627,8 @@ impl Service<Svm> { | |||
| reason: msgs.join("\n"), | |||
| }) | |||
| } | |||
| // Not important to check if bid slot is less than simulation slot if simulation is successful | |||
There was a problem hiding this comment.
this comment is a bit confusing, i'm not sure what you're trying to clarify
There was a problem hiding this comment.
trying to clarify why we are not checking the bid slot against result.context.slot in all cases
| @@ -168,6 +168,8 @@ impl Simulator { | |||
| Ok(result.value) | |||
| } | |||
|
|
|||
| /// Fetches multiple accounts from the RPC in chunks | |||
| /// There is no guarantee that all the accounts will be fetched with the same slot | |||
There was a problem hiding this comment.
i think this is still weird, but i don't have a great solution for it atm. maybe highlight this as a TODO to think about downstream simulation issues
There was a problem hiding this comment.
if the max rpc account limit is 100, should be fine though? max of 64 accounts per transaction, where did you get the max limit of 100 from?
There was a problem hiding this comment.
Based on the docs:
https://solana.com/docs/rpc/http/getmultipleaccounts
We try to fetch all the accounts needed for multiple transactions, so we might need chunking.
| .config | ||
| .chain_config | ||
| .simulator | ||
| .simulate_transaction(&bid.chain_data.transaction) |
There was a problem hiding this comment.
in that case, maybe it's worth a comment baking that into the interface on the SDK side, or in the BidChainDataCreateSvm object in auction-server/src/auction/entities/bid.rs
add optional parameter for svm bids to sepcify the minimum slot where the bid is applicable.
If simulation fails for any reason and the simulation context has a lower slot compared to the bid, we will retry hoping that the RPC will catch up soon.