From 765379a662e93edf5217b8656c0af4dcf6865fd2 Mon Sep 17 00:00:00 2001 From: Eric <5089238+emizzle@users.noreply.github.com> Date: Fri, 25 Oct 2024 15:08:00 +1100 Subject: [PATCH] fix: nonce too high (#81) * fix nonce issues by locking populate and send transaction Concurrent asynchronous population of transactions cause issues with nonces not being in sync with the transaction count for an account on chain. This was being mitigated by tracking a "last seen" nonce and locking inside of `populateTransaction` so that the nonce could be populated in a concurrent fashion. However, if there was an async cancellation before the transaction was sent, then the nonce would become out of sync. One solution was to decrease the nonce if a cancellation occurred. The other solution, in this commit, is simply to lock the populate and sendTransaction calls together, so that there will not be concurrent nonce discrepancies. This removes the need for "lastSeenNonce" and is overall more simple. * remove lastSeenNonce Internal nonce tracking is no longer needed since populate/sendTransaction is now locked. Even if cancelled midway, the nonce will get a refreshed value from the number of transactions from chain. * chronos v4 exception tracking * Add tests --- ethers/contract.nim | 32 ++++---- ethers/providers/jsonrpc.nim | 2 - ethers/signer.nim | 82 ++++++++----------- ethers/signers/wallet.nim | 2 - .../providers/jsonrpc/testJsonRpcSigner.nim | 21 ----- testmodule/testContracts.nim | 41 ++++++++++ 6 files changed, 91 insertions(+), 89 deletions(-) diff --git a/ethers/contract.nim b/ethers/contract.nim index 41de35c..db6e68e 100644 --- a/ethers/contract.nim +++ b/ethers/contract.nim @@ -90,17 +90,17 @@ proc decodeResponse(T: type, bytes: seq[byte]): T = proc call(provider: Provider, transaction: Transaction, - overrides: TransactionOverrides): Future[seq[byte]] = + overrides: TransactionOverrides): Future[seq[byte]] {.async: (raises: [ProviderError]).} = if overrides of CallOverrides and blockTag =? CallOverrides(overrides).blockTag: - provider.call(transaction, blockTag) + await provider.call(transaction, blockTag) else: - provider.call(transaction) + await provider.call(transaction) proc call(contract: Contract, function: string, parameters: tuple, - overrides = TransactionOverrides()) {.async.} = + overrides = TransactionOverrides()) {.async: (raises: [ProviderError, SignerError]).} = var transaction = createTransaction(contract, function, parameters, overrides) if signer =? contract.signer and transaction.sender.isNone: @@ -112,7 +112,7 @@ proc call(contract: Contract, function: string, parameters: tuple, ReturnType: type, - overrides = TransactionOverrides()): Future[ReturnType] {.async.} = + overrides = TransactionOverrides()): Future[ReturnType] {.async: (raises: [ProviderError, SignerError, ContractError]).} = var transaction = createTransaction(contract, function, parameters, overrides) if signer =? contract.signer and transaction.sender.isNone: @@ -121,16 +121,20 @@ proc call(contract: Contract, let response = await contract.provider.call(transaction, overrides) return decodeResponse(ReturnType, response) -proc send(contract: Contract, - function: string, - parameters: tuple, - overrides = TransactionOverrides()): - Future[?TransactionResponse] {.async.} = +proc send( + contract: Contract, + function: string, + parameters: tuple, + overrides = TransactionOverrides() +): Future[?TransactionResponse] {.async: (raises: [AsyncLockError, CancelledError, CatchableError]).} = + if signer =? contract.signer: - let transaction = createTransaction(contract, function, parameters, overrides) - let populated = await signer.populateTransaction(transaction) - var txResp = await signer.sendTransaction(populated) - return txResp.some + withLock(signer): + let transaction = createTransaction(contract, function, parameters, overrides) + let populated = await signer.populateTransaction(transaction) + trace "sending contract transaction", function, params = $parameters + let txResp = await signer.sendTransaction(populated) + return txResp.some else: await call(contract, function, parameters, overrides) return TransactionResponse.none diff --git a/ethers/providers/jsonrpc.nim b/ethers/providers/jsonrpc.nim index 33442f8..b1e653e 100644 --- a/ethers/providers/jsonrpc.nim +++ b/ethers/providers/jsonrpc.nim @@ -327,8 +327,6 @@ method sendTransaction*( {.async: (raises:[SignerError, ProviderError]).} = convertError: - if nonce =? transaction.nonce: - signer.updateNonce(nonce) let client = await signer.provider.client hash = await client.eth_sendTransaction(transaction) diff --git a/ethers/signer.nim b/ethers/signer.nim index b38b6ed..86e9093 100644 --- a/ethers/signer.nim +++ b/ethers/signer.nim @@ -8,7 +8,6 @@ export basics type Signer* = ref object of RootObj - lastSeenNonce: ?UInt256 populateLock: AsyncLock SignerError* = object of EthersError @@ -81,34 +80,26 @@ method getChainId*( method getNonce( signer: Signer): Future[UInt256] {.base, async: (raises: [SignerError, ProviderError]).} = - var nonce = await signer.getTransactionCount(BlockTag.pending) + return await signer.getTransactionCount(BlockTag.pending) - if lastSeen =? signer.lastSeenNonce and lastSeen >= nonce: - nonce = (lastSeen + 1.u256) - signer.lastSeenNonce = some nonce +template withLock*(signer: Signer, body: untyped) = + if signer.populateLock.isNil: + signer.populateLock = newAsyncLock() - return nonce - -method updateNonce*( - signer: Signer, - nonce: UInt256 -) {.base, gcsafe.} = - - without lastSeen =? signer.lastSeenNonce: - signer.lastSeenNonce = some nonce - return - - if nonce > lastSeen: - signer.lastSeenNonce = some nonce - -method decreaseNonce*(signer: Signer) {.base, gcsafe.} = - if lastSeen =? signer.lastSeenNonce and lastSeen > 0: - signer.lastSeenNonce = some lastSeen - 1 + await signer.populateLock.acquire() + try: + body + finally: + signer.populateLock.release() method populateTransaction*( signer: Signer, transaction: Transaction): Future[Transaction] - {.base, async: (raises: [CancelledError, AsyncLockError, ProviderError, SignerError]).} = + {.base, async: (raises: [CancelledError, ProviderError, SignerError]).} = + ## Populates a transaction with sender, chainId, gasPrice, nonce, and gasLimit. + ## NOTE: to avoid async concurrency issues, this routine should be called with + ## a lock if it is followed by sendTransaction. For reference, see the `send` + ## function in contract.nim. var address: Address convertError: @@ -119,20 +110,14 @@ method populateTransaction*( if chainId =? transaction.chainId and chainId != await signer.getChainId(): raiseSignerError("chain id mismatch") - if signer.populateLock.isNil: - signer.populateLock = newAsyncLock() - - await signer.populateLock.acquire() - var populated = transaction - try: - if transaction.sender.isNone: - populated.sender = some(address) - if transaction.chainId.isNone: - populated.chainId = some(await signer.getChainId()) - if transaction.gasPrice.isNone and (transaction.maxFee.isNone or transaction.maxPriorityFee.isNone): - populated.gasPrice = some(await signer.getGasPrice()) + if transaction.sender.isNone: + populated.sender = some(address) + if transaction.chainId.isNone: + populated.chainId = some(await signer.getChainId()) + if transaction.gasPrice.isNone and (transaction.maxFee.isNone or transaction.maxPriorityFee.isNone): + populated.gasPrice = some(await signer.getGasPrice()) if transaction.nonce.isNone and transaction.gasLimit.isNone: # when both nonce and gasLimit are not populated, we must ensure getNonce is @@ -143,27 +128,23 @@ method populateTransaction*( try: populated.gasLimit = some(await signer.estimateGas(populated, BlockTag.pending)) except EstimateGasError as e: - signer.decreaseNonce() raise e except ProviderError as e: - signer.decreaseNonce() raiseSignerError(e.msg) - else: - if transaction.nonce.isNone: - populated.nonce = some(await signer.getNonce()) - if transaction.gasLimit.isNone: - populated.gasLimit = some(await signer.estimateGas(populated, BlockTag.pending)) - - finally: - signer.populateLock.release() + else: + if transaction.nonce.isNone: + let nonce = await signer.getNonce() + populated.nonce = some nonce + if transaction.gasLimit.isNone: + populated.gasLimit = some(await signer.estimateGas(populated, BlockTag.pending)) return populated method cancelTransaction*( signer: Signer, tx: Transaction -): Future[TransactionResponse] {.base, async: (raises: [SignerError, ProviderError]).} = +): Future[TransactionResponse] {.base, async: (raises: [SignerError, CancelledError, AsyncLockError, ProviderError]).} = # cancels a transaction by sending with a 0-valued transaction to ourselves # with the failed tx's nonce @@ -172,7 +153,8 @@ method cancelTransaction*( without nonce =? tx.nonce: raiseSignerError "transaction must have nonce" - var cancelTx = Transaction(to: sender, value: 0.u256, nonce: some nonce) - convertError: - cancelTx = await signer.populateTransaction(cancelTx) - return await signer.sendTransaction(cancelTx) + withLock(signer): + convertError: + var cancelTx = Transaction(to: sender, value: 0.u256, nonce: some nonce) + cancelTx = await signer.populateTransaction(cancelTx) + return await signer.sendTransaction(cancelTx) diff --git a/ethers/signers/wallet.nim b/ethers/signers/wallet.nim index edc2254..752ae25 100644 --- a/ethers/signers/wallet.nim +++ b/ethers/signers/wallet.nim @@ -86,6 +86,4 @@ method sendTransaction*( {.async: (raises:[SignerError, ProviderError]).} = let signed = await signTransaction(wallet, transaction) - if nonce =? transaction.nonce: - wallet.updateNonce(nonce) return await provider(wallet).sendTransaction(signed) diff --git a/testmodule/providers/jsonrpc/testJsonRpcSigner.nim b/testmodule/providers/jsonrpc/testJsonRpcSigner.nim index e63a9a3..df2e824 100644 --- a/testmodule/providers/jsonrpc/testJsonRpcSigner.nim +++ b/testmodule/providers/jsonrpc/testJsonRpcSigner.nim @@ -84,24 +84,3 @@ suite "JsonRpcSigner": transaction.chainId = 0xdeadbeef.u256.some expect SignerError: discard await signer.populateTransaction(transaction) - - test "concurrent populate calls increment nonce": - let signer = provider.getSigner() - let count = await signer.getTransactionCount(BlockTag.pending) - var transaction1 = Transaction.example - var transaction2 = Transaction.example - var transaction3 = Transaction.example - - let populated = await allFinished( - signer.populateTransaction(transaction1), - signer.populateTransaction(transaction2), - signer.populateTransaction(transaction3) - ) - - transaction1 = await populated[0] - transaction2 = await populated[1] - transaction3 = await populated[2] - - check !transaction1.nonce == count - check !transaction2.nonce == count + 1.u256 - check !transaction3.nonce == count + 2.u256 diff --git a/testmodule/testContracts.nim b/testmodule/testContracts.nim index ed2158a..f7ef78e 100644 --- a/testmodule/testContracts.nim +++ b/testmodule/testContracts.nim @@ -272,3 +272,44 @@ for url in ["ws://" & providerUrl, "http://" & providerUrl]: .confirm(1) check receipt.status == TransactionStatus.Success + + test "can cancel procs that execute transactions": + let signer = provider.getSigner() + let token = TestToken.new(token.address, signer) + let countBefore = await signer.getTransactionCount(BlockTag.pending) + + proc executeTx {.async.} = + discard await token.mint(accounts[0], 100.u256) + + await executeTx().cancelAndWait() + let countAfter = await signer.getTransactionCount(BlockTag.pending) + check countBefore == countAfter + + test "concurrent transactions succeed even if one is cancelled": + let signer = provider.getSigner() + let token = TestToken.new(token.address, signer) + let balanceBefore = await token.myBalance() + + proc executeTx: Future[Confirmable] {.async.} = + return await token.mint(accounts[0], 100.u256) + + proc executeTxWithCancellation: Future[Confirmable] {.async.} = + let fut = token.mint(accounts[0], 100.u256) + fut.cancelSoon() + return await fut + + # emulate concurrent populateTransaction/sendTransaction calls, where the + # first one fails + let futs = await allFinished( + executeTxWithCancellation(), + executeTx(), + executeTx() + ) + let receipt1 = await futs[1].confirm(0) + let receipt2 = await futs[2].confirm(0) + + check receipt1.status == TransactionStatus.Success + check receipt2.status == TransactionStatus.Success + + let balanceAfter = await token.myBalance() + check balanceAfter == balanceBefore + 200.u256 \ No newline at end of file