near / mpc

30 stars 6 forks source link

Avoiding/addressing "Exceeded the prepaid gas" issue with cross-contract call loop #513

Closed mikedotexe closed 1 month ago

mikedotexe commented 3 months ago

Description

Offline, Felipe was mentioning how he saw timeout issues and linked to this tx: https://testnet.nearblocks.io/txns/CpVaKgR9kTjn944AKWhmHcHcxERpuwK5D7N1Uf7c4KLf#execution

If you scroll down you can see the number of cross-contract calls to itself, in hopes that it'll find the signature. His tx is interesting, because if you move away from the "Execution" tab (at the top of NEAR Blocks) and into the "Overview" you can see it says "Success" without any annotation.

However, if we take a look at this transaction from Matt Lockyer, he's attaching the maximum gas (300 TGas) and it's more of a smoking gun for this issue. In the screenshot below you can see the "Exceeded the prepaid gas" message below "Success"

Screenshot 2024-03-21 at 3 01 49 PM

https://testnet.nearblocks.io/txns/EKfbEyYvqH6KKBid63uCbY4vvEpuKyTH8tLTJkYFoCnj

Optional: User Story

As a user, I expect that when I submit a payload to be signed, that I won't spend undue gas and that the result will come back to me. Given the current approach, it seems that we're relying on the cross-contract call loop to complete every time, and within enough cycles such that we don't exceed the prepaid gas, which can apparently exceed the maximum gas allowed.


Suggestion to not use the cross-contract call loop as the only avenue, and to have the client query the state of the blockchain for that payload. It's great if the xcc loop works quickly, but this can also be accomplished via a normal view call using JavaScript.

It shows that we cannot rely on something like:

const signatureFromMPC = await(…)

and we'll likely have to use setInterval or something and do a basic query, asking if a signature is ready by providing a payload arg to a view call.

So I think this mpc-recovery contract will want to add a view function that's something like:

pub fn get_signature(&self, payload: [u8; 32]) -> Option<(String, String)> {
  self.pending_requests.get(&payload)?
}

and we can add a mutating function that'll remove the kv, though I'm slightly concerned that anyone can remove anyone else's signature this way, since it seems that we're not mapping the creator (predecessor) to their payloads.

pub fn clear_payload_and_signature(&mut self, payload: [u8; 32]) -> Option<(String, String)> {
  self.pending_requests.remove(&payload)?
}
volovyks commented 3 months ago

@mikedotexe We no longer return Out Of Gas error. Now it is a more readable Signature was not provided in time. Please, try again. Also, we added a limit to the number of requests waiting for response on a contract level, currently 8. If users hit this limit, they will get Too many pending requests. Please, try again later. error.

The current design is centered on sync calls. Each payload that got a signature or Signature was not provided in time error is deleted from the map. In the async approach, it is not clear how much time it needs to stay there and who should call the clear function.

The current waiting time is ~30 seconds and it is doubtful that we should ask the user to wait longer than that. We can change the design later if we have constant traffic that MPC can handle (not the case ATM).

I hope with the updated version of the protocol, we will get rid of the recursive approach.

mikedotexe commented 2 months ago

@mikedotexe We no longer return Out Of Gas error.

Thanks, Serhii. I believe the error might be a bit different, like shown in this transaction from today:

Screenshot 2024-04-08 at 2 54 54 PM

https://testnet.nearblocks.io/txns/GU4UyktQ7XLnSEgRuNVKDLDWU69ABLKtexiQ8FwpZi4i

volovyks commented 2 months ago

@mikedotexe Probably you passed less gas than the contract expected. I have just merged the gas check. The update should be released this week.

volovyks commented 1 month ago

@mikedotexe are you still getting this error? Can we close this issue?

mikedotexe commented 1 month ago

@mikedotexe are you still getting this error? Can we close this issue?

Looks like the v2 contract is still getting this.

Screenshot 2024-05-16 at 9 02 41 AM

I'm not sure which contract people should use between:

https://testnet.nearblocks.io/address/multichain-testnet-2.testnet multichain-testnet-2.testnet https://testnet.nearblocks.io/address/v2.multichain-mpc-dev.testnet v2.multichain-mpc-dev.testnet https://testnet.nearblocks.io/address/v5.multichain-mpc-dev.testnet v5.multichain-mpc-dev.testnet

https://testnet.nearblocks.io/txns/J7BovCcMwuB5haxD7JWfuxAZLGCs6RSzWnptomzyRjvn

volovyks commented 1 month ago

Clarification on our environments:

v1, v2, etc. is a version of the contract. We will change it each time we can not upgrade the contract. Ideally never. For development, you should usev2.multichain-mpc.testnet.

Please, reopen the issue if you see this error again.

mikedotexe commented 1 month ago

Thanks, it looks like the v2 contract I saw mentioned in Telegram is different than what I'm reading here.

So v2.multichain-mpc.testnet instead of v2.multichain-mpc-dev.testnet it seems.

I'm trying to get this updated in documentation, which has been pointing to the wrong one for a while. I'll update my 1-liner PR to docs to point to v2.multichain-mpc.testnet, thanks.

Screenshot 2024-05-17 at 7 01 28 AM