avoid waiting for all EVM nodes to sync the latest nonce #1757
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,7 @@ use ethereum_types::{Address, H160, H256, U256}; | |
| use ethkey::{public_to_address, KeyPair, Public, Signature}; | ||
| use ethkey::{sign, verify_address}; | ||
| use futures::compat::Future01CompatExt; | ||
| use futures::future::{join_all, select_ok, Either, FutureExt, TryFutureExt}; | ||
| use futures01::Future; | ||
| use http::StatusCode; | ||
| use mm2_core::mm_ctx::{MmArc, MmWeak}; | ||
|
|
@@ -775,7 +775,7 @@ async fn withdraw_impl(coin: EthCoin, req: WithdrawRequest) -> WithdrawResult { | |
| let (tx_hash, tx_hex) = match coin.priv_key_policy { | ||
| EthPrivKeyPolicy::KeyPair(ref key_pair) => { | ||
| let _nonce_lock = coin.nonce_lock.lock().await; | ||
| let (nonce, _) = get_addr_nonce(coin.my_address, coin.web3_instances.clone()) | ||
| .compat() | ||
| .timeout_secs(30.) | ||
| .await? | ||
|
|
@@ -924,7 +924,7 @@ pub async fn withdraw_erc1155(ctx: MmArc, req: WithdrawErc1155) -> WithdrawNftRe | |
| let (gas, gas_price) = | ||
| get_eth_gas_details(ð_coin, req.fee, eth_value, data.clone().into(), call_addr, false).await?; | ||
| let _nonce_lock = eth_coin.nonce_lock.lock().await; | ||
| let (nonce, _) = get_addr_nonce(eth_coin.my_address, eth_coin.web3_instances.clone()) | ||
| .compat() | ||
| .timeout_secs(30.) | ||
| .await? | ||
|
|
@@ -992,7 +992,7 @@ pub async fn withdraw_erc721(ctx: MmArc, req: WithdrawErc721) -> WithdrawNftResu | |
| let (gas, gas_price) = | ||
| get_eth_gas_details(ð_coin, req.fee, eth_value, data.clone().into(), call_addr, false).await?; | ||
| let _nonce_lock = eth_coin.nonce_lock.lock().await; | ||
| let (nonce, _) = get_addr_nonce(eth_coin.my_address, eth_coin.web3_instances.clone()) | ||
| .compat() | ||
| .timeout_secs(30.) | ||
| .await? | ||
|
|
@@ -2005,7 +2005,7 @@ async fn sign_and_send_transaction_with_keypair( | |
| } | ||
| let _nonce_lock = coin.nonce_lock.lock().await; | ||
| status.status(tags!(), "get_addr_nonce…"); | ||
| let (nonce, web3_instances_with_latest_nonce) = try_tx_s!( | ||
| get_addr_nonce(coin.my_address, coin.web3_instances.clone()) | ||
| .compat() | ||
| .await | ||
|
|
@@ -2026,14 +2026,10 @@ async fn sign_and_send_transaction_with_keypair( | |
| let bytes = Bytes(rlp::encode(&signed).to_vec()); | ||
| status.status(tags!(), "send_raw_transaction…"); | ||
|
|
||
| let futures = web3_instances_with_latest_nonce | ||
| .into_iter() | ||
| .map(|web3_instance| web3_instance.web3.eth().send_raw_transaction(bytes.clone())); | ||
| try_tx_s!(select_ok(futures).await.map_err(|e| ERRL!("{}", e)), signed); | ||
|
|
||
| status.status(tags!(), "get_addr_nonce…"); | ||
| coin.wait_for_addr_nonce_increase(coin.my_address, nonce).await; | ||
|
|
@@ -4012,7 +4008,7 @@ impl EthCoin { | |
| Box::new(fut.boxed().compat()) | ||
| } | ||
|
|
||
| /// Checks every second till at least one ETH node recognizes that nonce is increased. | ||
| /// Parity has reliable "nextNonce" method that always returns correct nonce for address. | ||
| /// But we can't expect that all nodes will always be Parity. | ||
| /// Some of ETH forks use Geth only so they don't have Parity nodes at all. | ||
|
|
@@ -4025,8 +4021,8 @@ impl EthCoin { | |
| async fn wait_for_addr_nonce_increase(&self, addr: Address, prev_nonce: U256) { | ||
| repeatable!(async { | ||
| match get_addr_nonce(addr, self.web3_instances.clone()).compat().await { | ||
| Ok((new_nonce, _)) if new_nonce > prev_nonce => Ready(()), | ||
| Ok((_nonce, _)) => Retry(()), | ||
| Err(e) => { | ||
| error!("Error getting {} {} nonce: {}", self.ticker(), self.my_address, e); | ||
| Retry(()) | ||
|
|
@@ -5015,31 +5011,37 @@ fn checksum_address(addr: &str) -> String { | |
| /// The input must be 0x prefixed hex string | ||
| fn is_valid_checksum_addr(addr: &str) -> bool { addr == checksum_address(addr) } | ||
|
|
||
| /// 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. | ||
| #[cfg_attr(test, mockable)] | ||
| fn get_addr_nonce( | ||
| addr: Address, | ||
| web3s: Vec<Web3Instance>, | ||
| ) -> 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 There was a problem hiding this comment. I didn't know about this, then it can stay as it is |
||
| let (futures, web3s): (Vec<_>, Vec<_>) = web3s | ||
| .iter() | ||
| .map(|web3| { | ||
| if web3.is_parity { | ||
| let parity: ParityNonce<_> = web3.web3.api(); | ||
| (Either::Left(parity.parity_next_nonce(addr)), web3.clone()) | ||
| } else { | ||
| ( | ||
| Either::Right(web3.web3.eth().transaction_count(addr, Some(BlockNumber::Pending))), | ||
| web3.clone(), | ||
| ) | ||
| } | ||
| }) | ||
| .unzip(); | ||
|
|
||
| let nonces: Vec<_> = join_all(futures) | ||
| .await | ||
| .into_iter() | ||
| .zip(web3s.into_iter()) | ||
| .filter_map(|(nonce_res, web3)| match nonce_res { | ||
| Ok(n) => Some((n, web3)), | ||
| Err(e) => { | ||
| error!("Error getting nonce for addr {:?}: {}", addr, e); | ||
| None | ||
|
|
@@ -5053,13 +5055,18 @@ fn get_addr_nonce(addr: Address, web3s: Vec<Web3Instance>) -> Box<dyn Future<Ite | |
| return ERR!("Couldn't get nonce after 5 errored attempts, aborting"); | ||
| } | ||
| } else { | ||
| let max = nonces | ||
| .iter() | ||
| .map(|(n, _)| *n) | ||
| .max() | ||
| .expect("nonces should not be empty!"); | ||
| break Ok(( | ||
| max, | ||
| nonces | ||
| .into_iter() | ||
| .filter_map(|(n, web3)| if n == max { Some(web3) } else { None }) | ||
| .collect(), | ||
| )); | ||
| } | ||
| Timer::sleep(1.).await | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.