avoid waiting for all EVM nodes to sync the latest nonce#1757
Conversation
…nd evm transactions to only the nodes that synced the latest nonce
|
swaps work fine with PLG20 using just the load balanced RPC endpoint https://polygon-rpc.com on both maker and taker: |
| ) -> Box<dyn Future<Item = (U256, Vec<Web3Instance>), Error = String> + Send> { | ||
| let fut = async move { | ||
| let mut errors: u32 = 0; | ||
| loop { |
There was a problem hiding this comment.
You can change this with a repeatable future since we started to work on this function
There was a problem hiding this comment.
Sure, it can be done. Will have to change the function to async to do that.
There was a problem hiding this comment.
Unfortunately, repeatable future can't be used in the above case because of this
https://github.com/KomodoPlatform/atomicDEX-API/blob/ee370707c68e42596ee084da97eaa43384a8f42c/mm2src/common/custom_futures/repeatable.rs#L3-L21 since errors counter should be used instead of counter https://github.com/KomodoPlatform/atomicDEX-API/blob/c5047d40e66a1c287f20a50e9d58e711ab3f3740/mm2src/coins/eth.rs#L5022
There was a problem hiding this comment.
I didn't know about this, then it can stay as it is
| } else { | ||
| warn!("Max nonce {} != {} min nonce", max, min); | ||
| } | ||
| let max = nonces.iter().map(|(n, _)| *n).max().unwrap(); |
There was a problem hiding this comment.
You can use expect here instead of unwrap, writing that nonces is checked above and will not be empty.
| /// Nodes might need some time to sync and there can be other coins that use same nodes in different order. | ||
| /// We need to be sure that nonce is updated on all of them before and after transaction is sent. | ||
| /// Requests the nonce from all available nodes and returns the highest nonce available with the list of nodes that returned the highest nonce. | ||
| /// Transactions will be sent using the nodes that returned the highest nonce. |
There was a problem hiding this comment.
This means we will trust the result from a single node now. Do you think it can cause any problems?
There was a problem hiding this comment.
We already sent the transaction to a single node before this PR, we now make sure that the node that we send the transaction to is up to date, so I don't think we will have more problems than we already did. The idea was that swap operations can continue to work even if only one node is up to date and all other nodes are down or outdated, this eliminates the case that happened recently where a taker couldn't spend a swap transaction because one node was not working right.
Alternatively, I can add a 60s delay for the nodes to reach consensus/same state or nonce, if they didn't, we use only the nodes that are up to date as it's done here and log an error for the nodes that are not up to date or take time to sync, what do you think @caglaryucekaya?
In the end I don't think this is a final solution, there is no best solution for the nonce problem in EVM transactions, some wallets (e.g. metamask) save the nonce value and increase it internally, but this not ideal if a transaction is sent from another wallet by the user (if the user uses the same seed in multiple wallets). Other solutions is to wait for confirmation of a transaction before sending another, I believe some EVM DEXs do that to avoid such problems.
The best solution to avoid trusting a single node is to send the transaction to all nodes but this has a higher cost than sending it to only one node.
There was a problem hiding this comment.
Alternatively, I can add a 60s delay for the nodes to reach consensus/same state or nonce, if they didn't, we use only the nodes that are up to date as it's done here and log an error for the nodes that are not up to date or take time to sync, what do you think @caglaryucekaya?
If we're going to accept the largest nonce in the end, it wouldn't bring anything to wait for the others for 60s. I was thinking of a case where the largest nonce result returned is incorrect, but we can ignore that since it wouldn't happen.
…ad of unwrap when returning the max nonce
5 participants
fixes #1724 (comment), #1724 (comment)
Note: This fix might not work for load balanced nodes since we don't have a way to know which node behind the load balancer returned the latest nonce and the transaction might be sent to another node.