prevent stuck transactions by async locking nonce sequencing (+ estimate gas) #55

emizzle · 2023-09-27T07:50:11Z

During populateTransaction, a new nonce is obtained using signer.getNonce. After this, the transaction's gas is estimated, often times resulting in a failure because the simulated transaction failed (due to a contract revert, nonce too high/low, gas error). The next transaction created will have a nonce that is one too high, resulting in that transaction getting stuck until the previous transaction is executed. To solve this, the previous transaction can be cancelled by sending a 0-value transaction to the sender of the transaction with the same nonce as the previous transaction. This will ensure that any queued transactions with higher nonces will be able to be executed.

Also update the estimateGas error message, and add the revert exception as the parent.

…t estimateGas failure

AuHau

This also LGTM, but again I would live approve to @markspanbroek

markspanbroek

I'd like to avoid sending transactions that were not specifically asked for by the user of the library. To solve the problem of stuck transactions, I think we should either:

ensure that the situation never occurs
signal when this occurs, and let the user of the library decide what to do with it (e.g. cancel the transaction)

If we go for option (2), then we could introduce a specific error that is only raised when a transaction will get stuck, and expose a means of handling it (e.g. the cancelTransaction() proc from this PR)

But thinking about this a bit more, we might actually be able to pull off option (1). A transaction can get stuck if there are multiple concurrent invocations of populateTransaction, whereby the nonce is increased a couple of times, but one of the invocations fails on estimateGas. If we ensure that we handle a single populateTransaction at a time, and never do this concurrently, then I think we will not run into this problem anymore. Either an AsyncLock or an AsyncQueue can probably be used to ensure that increasing the nonce and estimating gas either both succeed, or both fail.

In pseudocode:

try:
  await lock.acquire()
  populated.nonce = await signer.getNonce()
  populated.gasLimit = await signer.estimateGas()
except EstimateGasError:
  signer.decreaseNonce()
  raise
finally:
  await lock.release()

ethers/contract.nim

ethers/signer.nim

ethers/testing.nim

emizzle · 2023-10-18T10:51:13Z

I'm not sure if that will solve the issue because the problem occurs when sendTransaction is called concurrently, which will increase the nonce for each transaction, and queue any transactions where there is a nonce gap. Adding an async lock would prevent concurrency in populating the transaction (and nonce), but it would not wait for the transaction to be sent, nor do I think that's something we want.

I think we can come up with a version of #2, where something like a StuckTransactionError is raised in sendTransaction which would allow the user to call cancelTransaction. Alternatively or in addition to, we could add a parameter to sendTransaction that enables cancelling potentially stuck transactions instead automatically.

emizzle · 2023-10-18T11:00:48Z

Actually thinking about this further, async locking populateTransaction might actually work, because of the decreaseNonce (which I hadn't really thought through the first time I looked).

- remove auto-cancellation of failed transaction (failed during estimate gas) to prevent stuck txs - replace it with an async lock during nonce sequencing + gas estimation - simplified cancelTransaction (still exported) such that the new transaction is populated using populateTransaction, so that all gas and fees are reset - moved reverting contract function into its own testing helpers module, and refactored any tests to use it - updated the test helper reverts to check EstimateGasErrors

emizzle · 2023-10-20T07:10:31Z

Ready for another review 👍

markspanbroek

Thanks @emizzle, this is great!
I have a few small remarks, but otherwise good to go!

markspanbroek · 2023-10-23T09:05:09Z

ethers/contract.nim

+import pkg/stew/byteutils
 import ./basics
 import ./provider
 import ./signer
 import ./events
 import ./fields
+import ./exceptions


Are these imports still needed? I see no changes anymore in this file?

markspanbroek · 2023-10-23T09:05:52Z

ethers/exceptions.nim

@@ -0,0 +1,7 @@
+import ./basics
+
+func msgStack*(error: ref EthersError): string =


Doesn't seem to be used anymore?

markspanbroek · 2023-10-23T09:06:12Z

ethers/providers/jsonrpc.nim

@@ -3,6 +3,7 @@ import std/tables
 import std/uri
 import pkg/json_rpc/rpcclient
 import pkg/json_rpc/errors
+import pkg/stew/byteutils


Unused import?

markspanbroek · 2023-10-23T09:07:54Z

ethers/signer.nim

+import pkg/chronicles

 export basics
+export chronicles
+
+logScope:
+  topics = "ethers signer"


There's no logging anymore in this PR, so this can probably also be removed?

markspanbroek · 2023-10-23T09:10:33Z

ethers/signer.nim

+  ## nonce is decremented. Disallows concurrent calls by use of AsyncLock.
+  ## Immediately returns if tx already has a nonce or gasLimit.
+
+  if not (tx.nonce.isNone and tx.gasLimit.isNone):


Although equivalent, this reads a little better, I think:

Suggested change

if not (tx.nonce.isNone and tx.gasLimit.isNone):

if tx.nonce.isSome or tx.gasLimit.isSome:

markspanbroek · 2023-10-23T09:17:43Z

ethers/signer.nim

+  var populated = await signer.ensureNonceSequence(transaction)

-  if transaction.sender.isNone:
+  if populated.sender.isNone:
    populated.sender = some(await signer.getAddress())
-  if transaction.nonce.isNone:
-    populated.nonce = some(await signer.getNonce())
-  if transaction.chainId.isNone:
+  if populated.chainId.isNone:
    populated.chainId = some(await signer.getChainId())
-  if transaction.gasPrice.isNone and (transaction.maxFee.isNone or transaction.maxPriorityFee.isNone):
+  if populated.gasPrice.isNone and (populated.maxFee.isNone or populated.maxPriorityFee.isNone):
    populated.gasPrice = some(await signer.getGasPrice())
-  if transaction.gasLimit.isNone:
-    populated.gasLimit = some(await signer.estimateGas(populated))
+  if populated.nonce.isNone:
+    populated.nonce = some(await signer.getNonce())
+  if populated.gasLimit.isNone:
+          populated.gasLimit = some(await signer.estimateGas(populated))


It feels a bit weird that we're estimating gas before setting all properties, such as gasLimit and sender.
I would suggest to either do the call to ensureNonceSequence last, or to combine populateTransaction and ensureNonceSequence into a single method.

Fair point. I've merged ensureNonceSequence into populateTransaction.

markspanbroek · 2023-10-25T07:58:47Z

ethers/signer.nim

+    try:
+      populated.nonce = some(await signer.getNonce())
+      populated.gasLimit = some(await signer.estimateGas(populated))
+    except ProviderError, EstimateGasError:
+      let e = getCurrentException()
+      signer.decreaseNonce()
+      raise e
+    finally:
+      signer.populateLock.release()


The try:finally should be around all the statements following lock.acquire(). If one of the getAddress() or getChainId() or ... fails, then the lock is stuck forever.

Fixed in #58

dryajov · 2023-10-26T16:45:57Z

ethers/signer.nim

@@ -73,17 +95,55 @@ method populateTransaction*(signer: Signer,
  if chainId =? transaction.chainId and chainId != await signer.getChainId():
    raiseSignerError("chain id mismatch")

+  if signer.populateLock.isNil:


You want to initialize this in the constructor.

What happened to this @emizzle ?

emizzle added 4 commits October 18, 2023 11:12

cancel transaction after estimateGas failure

f7984ef

ensure nonce increment is done last

c22b7d4

Update revertReason to work with SignerError

d06edb3

Also update the estimateGas error message, and add the revert exception as the parent.

Only cancels transactions if nonce has been incremented since the las…

73963e6

…t estimateGas failure

emizzle force-pushed the feat/cancel-tx-on-estimateGas-failure branch from 6488fd3 to 73963e6 Compare October 18, 2023 00:14

emizzle mentioned this pull request Oct 18, 2023

On transaction failure, fetch revert reason with replayed transaction #57

Merged

AuHau reviewed Oct 18, 2023

View reviewed changes

markspanbroek reviewed Oct 18, 2023

View reviewed changes

ethers/contract.nim Outdated Show resolved Hide resolved

ethers/contract.nim Outdated Show resolved Hide resolved

ethers/signer.nim Outdated Show resolved Hide resolved

ethers/testing.nim Outdated Show resolved Hide resolved

emizzle added 3 commits October 20, 2023 17:14

clean up from rebase

5d83ca4

temp fix until revert reason fetching comes in

9cc3441

emizzle changed the title ~~cancel transaction after estimateGas failure~~ prevent stuck transactions by async locking nonce sequencing (+ estimate gas) Oct 20, 2023

clean up

ff4dc5d

clean up

e5290da

markspanbroek approved these changes Oct 23, 2023

View reviewed changes

cleanup -- combine ensureNonceSequence into populateTransaction

14ead6d

emizzle merged commit 7eac841 into main Oct 24, 2023
4 checks passed

emizzle deleted the feat/cancel-tx-on-estimateGas-failure branch October 24, 2023 23:42

emizzle mentioned this pull request Oct 25, 2023

React on Transaction Failures codex-storage/nim-codex#572

Closed

markspanbroek reviewed Oct 25, 2023

View reviewed changes

dryajov reviewed Oct 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent stuck transactions by async locking nonce sequencing (+ estimate gas) #55

prevent stuck transactions by async locking nonce sequencing (+ estimate gas) #55

emizzle commented Sep 27, 2023

AuHau left a comment

markspanbroek left a comment

emizzle commented Oct 18, 2023 •

edited

emizzle commented Oct 18, 2023

emizzle commented Oct 20, 2023

markspanbroek left a comment

markspanbroek Oct 23, 2023

markspanbroek Oct 23, 2023

markspanbroek Oct 23, 2023

markspanbroek Oct 23, 2023

markspanbroek Oct 23, 2023

markspanbroek Oct 23, 2023

emizzle Oct 24, 2023

markspanbroek Oct 25, 2023

emizzle Oct 26, 2023 •

edited

dryajov Oct 26, 2023

dryajov Nov 4, 2023

		@@ -0,0 +1,7 @@
		import ./basics

		func msgStack*(error: ref EthersError): string =

	if not (tx.nonce.isNone and tx.gasLimit.isNone):
	if tx.nonce.isSome or tx.gasLimit.isSome:

prevent stuck transactions by async locking nonce sequencing (+ estimate gas) #55

prevent stuck transactions by async locking nonce sequencing (+ estimate gas) #55

Conversation

emizzle commented Sep 27, 2023

AuHau left a comment

Choose a reason for hiding this comment

markspanbroek left a comment

Choose a reason for hiding this comment

emizzle commented Oct 18, 2023 • edited

emizzle commented Oct 18, 2023

emizzle commented Oct 20, 2023

markspanbroek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emizzle Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emizzle commented Oct 18, 2023 •

edited

emizzle Oct 26, 2023 •

edited