ethers-io / ethers.js

Complete Ethereum library and wallet implementation in JavaScript.
https://ethers.org/
MIT License
7.95k stars 1.85k forks source link

How do I manage nonces? #363

Closed andrewgordstewart closed 4 years ago

andrewgordstewart commented 5 years ago

I've come across this "incorrect nonce" error while trying to write a kind of assertRevert function to assert that a particular revert occurred.

https://github.com/magmo/test-contract/commit/392f4b3d09af0445bd21091574630893246ea5ca

The assertRevert as written isn't quite robust yet, but it seems to have highlighted a bug in ethers, where raw transactions are created with an incorrect nonce:

 FAIL  test/test-contract.test.ts
  TestContract
    ✕ reverts for no reason (809ms)

  ● TestContract › reverts for no reason

    the tx doesn't have the correct nonce. account has nonce of: 95 tx has nonce of: 94

      at getResult (node_modules/ethers/providers/json-rpc-provider.js:41:21)
      at Object.<anonymous>.exports.XMLHttpRequest.request.onreadystatechange (node_modules/ethers/utils/web.js:108:30)
      at Object.<anonymous>.exports.XMLHttpRequest.dispatchEvent (node_modules/xmlhttprequest/lib/XMLHttpRequest.js:591:25)
      at setState (node_modules/xmlhttprequest/lib/XMLHttpRequest.js:610:14)
      at IncomingMessage.<anonymous> (node_modules/xmlhttprequest/lib/XMLHttpRequest.js:447:13)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        2.148s, estimated 3s
Ran all test suites.

Watch Usage: Press w to show more.

Removing the gasLimit override causes the test to pass: https://github.com/magmo/test-contract/commit/2d31c71972bdf3c99971729349f86de67453d9d8

Reproduction instructions are in the readme.

andrewgordstewart commented 5 years ago

(You can also make it pass by adding a delay before the second deposit transaction: https://github.com/magmo/test-contract/commit/661e4932adf18c225f68e2e3f7b94519f9d38d6b)

ricmoo commented 5 years ago

Is this using the JsonRpcSigner from a JsonRpcProvider?

Are you making many requests to your node in a very short time? Keep in mind that a TCP connection is not instantaneous, so if you make 2 TCP connects at the same time, the order may not be retained.

There is a change coming in a few minutes (just waiting for the test cases to complete) which may address this for JsonRpcSigners by not calling populateTransaction, so it will be up to the node to assign nonces.

ricmoo commented 5 years ago

Just published; please try 4.0.16 to see if this addresses your issue.

andrewgordstewart commented 5 years ago

It still seems to fail on 4.0.16: https://github.com/magmo/test-contract/commit/f5291c340bfc8fe849fbd18e57888a625106ca90

What actually seems to happen is that eth_getTransactionCount is called twice in sequence, followed by eth_sendRawTransaction twice in sequence. Both of these raw transactions have the same nonce.

https://gist.github.com/andrewgordstewart/5186dd8731e3f218746592bb6efc53be (the nonce 0x30d is returned twice. note that 0x30d == 781)

I'm sure it has to do with the behaviour of assertRevert as written. I'm not sure if there's a also bug in ethers that needs to be addressed.

ricmoo commented 5 years ago

That is part of Ethereum though, so not really an issue that ethers can address directly in the way it is being used.

The getTransactionCount returns the number of transactions that have been mined, so two quick back-to-back transactions, will get the same nonce. If you are not using a system which manages nonces for you (such as a JsonRpcSigner), you will need to manually manage that. I am working on a cookbook entry to demonstrate a Signer that will manage the nonce for you.

What Provider are you suing and how are you creating the Signer?

andrewgordstewart commented 5 years ago

Is this using the JsonRpcSigner from a JsonRpcProvider?

It's a new ethers.Wallet that has a JsonRpcProvider as its provider: https://github.com/magmo/test-contract/commit/392f4b3d09af0445bd21091574630893246ea5ca#diff-91c28e6e211fc99720ebcf9e72e952a5R21

ricmoo commented 5 years ago

Yeah, a Wallet cannot manage nonces. The new NonceManager Signer will be able to do that, as well as handle re-broadcasting.

If you instead use:

let signer = provider.getSigner();

you will find this does not happen (probably) because the node will manage the nonce for you.

andrewgordstewart commented 5 years ago

That makes sense. However, the signer's account wouldn't have any eth in that case. How would I set up the provider so that its signer corresponds to a given private key?

ricmoo commented 5 years ago

The NonceManager will have a Signer it wraps, so it can work with a private key, or a Ledger Nano, or Firefly Hardware Wallet, or anything else. :)

andrewgordstewart commented 5 years ago

Ok, great. I assume by this, you mean that the NonceManager will come in a future (hopefully soon 😄) release?

The NonceManager will have a Signer it wraps

For the moment, I'll deal with this issue by adding a small delay between transactions as needed :pig:

andrewgordstewart commented 5 years ago

However, the signer's account wouldn't have any eth in that case.

It turns out that the way we set up our ganache server, the account await provider.getSigner().getAccount() is unlocked, and has eth.

So, I switched to using this technique in our code testContract = await ContractFactory.fromSolidity(TestContractArtifact, provider.signer()).deploy();

However, I now have seen a flickering test failure with the same error.

Here's an example: Test runs #178, #180 and #182 are all running off of commit https://github.com/magmo/force-move-games/commit/ad4468d761356c5556899e4077c6aa788c71617a, and #182 experiences

 FAIL  src/test/state.test.ts (5.107s)
  ● State › identifies the mover based on the turnNum

    the tx doesn't have the correct nonce. account has nonce of: 10 tx has nonce of: 9

(The commit introducing the provider.getSigner() technique is the previous commit: https://github.com/magmo/force-move-games/commit/01ab56812cd3bd7cbb8090f210aa5c7c6cdc2854

andrewgordstewart commented 5 years ago

@ricmoo do you have any insight into the flickering test above? The provider.signer, which is supposed to manage nonces, still randomly causes a the tx doesn't have the correct nonce. account has nonce of: 10 tx has nonce of: 9 error.

liangguangzhi commented 5 years ago

`it("reverts for no reason", async () => { await testContract.deposit(DEPOSIT_AMOUNT, { value: DEPOSIT_AMOUNT }); assertRevert(testContract.revertForNoReason(SIG.message, SIG.v, SIG.r, SIG.s, { gasLimit: 30000 }), "No reason"); await testContract.deposit(DEPOSIT_AMOUNT, { value: DEPOSIT_AMOUNT, nonce: await web3.eth.getTransactionCount(DEPOSIT_AMOUNT) }); }); }); `

add the nonce parameter will work

ricmoo commented 5 years ago

If you use await contract.foobar(), the result is a transaction, which has been sent to the network, but has not yet been mined. If you wish to wait for it to be mined, you can then additionally call await tx.wait(), which will wait for it to be mined (in fact, it returns the receipt); if the transaction is reverted this will throw, so you can wrap it in a try catch if you’d like.

I think that may be the problem you are concerned with? If not, let me know more about what you are expecting. :)

liangguangzhi commented 5 years ago

If you use await contract.foobar(), the result is a transaction, which has been sent to the network, but has not yet been mined. If you wish to wait for it to be mined, you can then additionally call await tx.wait(), which will wait for it to be mined (in fact, it returns the receipt); if the transaction is reverted this will throw, so you can wrap it in a try catch if you’d like.

I think that may be the problem you are concerned with? If not, let me know more about what you are expecting. :)

hi,thx for your reply^_^

my problem maybe related to this https://github.com/MetaMask/web3-provider-engine/issues/300,i think it maybe the reason of this topic。

it cause when test the code with ganache and use truffle-hdwallet-provider lib, truffle-hdwallet-provider rely web3-provider-engine lib. when a transaction revert,every transaction after the revert fail with error "Error: the tx doesn't have the correct nonce. account has nonce of: (n) tx has nonce of: (n-1)"

i don't kwon how to fixed the problem with comprehensible way, but it will work add parameter "nonce: await web3.eth.getTransactionCount(account[0])" on transaction, i think this way will keep away from same bug with related lib

related problem i found https://ethereum.stackexchange.com/questions/70549/a-single-exception-in-truffle-test-causes-every-other-test-cases-to-fail/73819#73819

kermankohli commented 5 years ago

+1. Facing this issue as well and not sure what's going on

kermankohli commented 5 years ago

Caught the issue here. Turns out I was using Jest without the --runInBand flag which caused the tests to run in parallel instead of sequentially. If anyone else comes across this I hope this saves you lost time lol.

phiresky commented 4 years ago

Is it possible to prevent wallet.sendTransaction from setting nonce now? I guess that should make geth/parity choose the nonce correctly? Because from what I can tell right now there's not really a way to send multiple transactions one after another via wallet, and wallet always sets nonce:

https://github.com/ethers-io/ethers.js/blob/b1c6575a1b8cc666a9173eceedb7a367329819c7/src.ts/wallet.ts#L97

phiresky commented 4 years ago

Ok I guess what I'm saying doesn't make sense since wallet signs the transaction to it has to know the nonce before sending it to the node.

ricmoo commented 4 years ago

If you are using v5, there is a NonceManager, which is a Signer that accepts any other Signer and will track the nonce for you, if that helps. :)

ricmoo commented 4 years ago

Is this resolved? I think the NonceManager should take care of what is needed by most people? I'm going to close this now, but feel free to re-open.

Thanks! :)

EvilJordan commented 4 years ago

Apologies for re-opening this, but I am having a hell-of-a-time getting the NonceManager to work with my signed Wallet.

Existing code:

wallet = new ethers.Wallet(privateKey, provider);
let managedSigner = new NonceManager(wallet);
let contractWithSigner =  contract.connect(managedSigner);
let tx = await contractWithSigner.contractFunction(params);

How do I adapt the NonceManager to this, as I'm seeing the issues of sending transactions in rapid succession returning the error: the tx doesn't have the correct nonce. account has nonce of: N tx has nonce of: N-1

EvilJordan commented 4 years ago

After a lot of reading and experimentation, I have some insight.

My issue is caused because I'm signing a bunch of transactions on the code-side, all at once. They don't hit the pool fast enough for the NonceManager to even know they exist, and by the time the first one ends up in "pending", the rest have no hope of catching up. Exactly what you've mentioned before about pending becoming "overloaded."

Where I think this could be solved is by having a NonceHolder completely outside of the getTransactionCount() return value... perhaps setting it at the start of operations, then incrementing on its own, in the global space, and pushing that value in to an override for the transaction operating.

Maybe that's naive thinking, but it's where I've ended up at the moment.

ricmoo commented 4 years ago

@EvilJordan the NonceManager will increment the nonce synchronously, so for sending it doesn’t matter how fast you call it, the nonce will be incremented.

But for signing-only, you should call NonceManager.incrementTransactionCount() between each signed transaction to move the nonce forward.

gitpusha commented 4 years ago

Hi @ricmoo . I have a question which matches this issue's title. So I will post it here. However, it is pretty much asking the opposite of what the discussion has been about.

In my new design I want to make sure that the default nonce that my transactions use will always reflect the actual nonce on my account.

Therefore, if I previously sent a transaction, and that transaction still has not been mined, and I now send a new transaction, I automatically want the new transaction to use the same nonce as the unmined previous transaction, so that only one of them makes it through.

Is this the default behavior that ethers uses under the hood, or do I need to somehow implement custom nonce management for this?

How does ethers.js get the default nonce value under the hood anyway? Say I use an Infura Provider connected to my Wallet. How do you fill the default nonce value there?

Say my script does this:

// pseudocode
setInterval(every60seconds)
--- interval 1
await sendTx1();
--- interval 2 sends another TX but Tx1 from previous interval has not been mined yet 
await sendTx2();

What default nonce would be used for sendTx1 and sendTx2 ? Does ethers just get the value to use from the connected Infura Provider and rely on that?

I want to determine whether, if Tx1 was never mined for whatever reason, ethers.js built-in default nonce selection would be so that my Intervals could keep sending transactions, basically cancelling the TX sent during the previous Interval, if it was not mined within 60 seconds (since the Intervals are every 60 seconds).

CryptoKiddies commented 2 years ago

At some point, everyone is responsible for managing their pending transactions. Some of what I've read in this discussion should be handled by bespoke code, e.g., local storage in the browser or a DB to track script based txns. Even @ricmoo 's suggestion of using NonceManager.incrementTransactionCount() is a courtesy implementation for manual nonce shifting, which should happen in a routine's for loops. Txns should be managed by polling and not awaits, which take an arbitrarily long time, block scripts, and can timeout and fail.

beshup commented 2 years ago

What if I'm signing transactions with the same private key (ie. for the same account), from different instances of NonceManager? eg. Different services. Do I have to use a singleton instance in my app?

atropos0902 commented 2 years ago

I had the same issue. I used NonceManager to manage the nonce automatically. Seems like it's working fine. https://www.npmjs.com/package/@ethersproject/experimental