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

@@ -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

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)

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
-
-  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
+template withLock*(signer: Signer, body: untyped) =
+  if signer.populateLock.isNil:
+    signer.populateLock = newAsyncLock()
 
-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)

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)

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

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