Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How do I manage nonces? #363

Closed
andrewgordstewart opened this issue Dec 4, 2018 · 28 comments
Closed

How do I manage nonces? #363

andrewgordstewart opened this issue Dec 4, 2018 · 28 comments
Labels
discussion Questions, feedback and general information. duplicate Duplicate of another issue.

Comments

@andrewgordstewart
Copy link

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

magmo/test-contract@392f4b3

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:
magmo/test-contract@2d31c71

Reproduction instructions are in the readme.

@andrewgordstewart
Copy link
Author

(You can also make it pass by adding a delay before the second deposit transaction: magmo/test-contract@661e493)

@ricmoo
Copy link
Member

ricmoo commented Dec 4, 2018

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
Copy link
Member

ricmoo commented Dec 4, 2018

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

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Dec 4, 2018
@andrewgordstewart
Copy link
Author

It still seems to fail on 4.0.16: magmo/test-contract@f5291c3

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
Copy link
Member

ricmoo commented Dec 4, 2018

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?

@ricmoo ricmoo changed the title "the tx doesn't have the correct nonce. account has nonce of: 6 tx has nonce of: 5" How do I manage nonces? Dec 4, 2018
@ricmoo ricmoo added the duplicate Duplicate of another issue. label Dec 4, 2018
@andrewgordstewart
Copy link
Author

Is this using the JsonRpcSigner from a JsonRpcProvider?

It's a new ethers.Wallet that has a JsonRpcProvider as its provider: magmo/test-contract@392f4b3#diff-91c28e6e211fc99720ebcf9e72e952a5R21

@ricmoo
Copy link
Member

ricmoo commented Dec 4, 2018

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
Copy link
Author

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
Copy link
Member

ricmoo commented Dec 5, 2018

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
Copy link
Author

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 🐷

andrewgordstewart added a commit to magmo/turbo-adjudicator that referenced this issue Dec 5, 2018
Until we can use the NonceManager to manage nonces, we ensure some time
separation between our contract calls to ensure that nonces increment.

See ethers-io/ethers.js#363 for discussion.
andrewgordstewart added a commit to magmo/force-move-protocol that referenced this issue Dec 7, 2018
See discussion here: ethers-io/ethers.js#363
`await provider.getSigner().getAddress()` seems to have ether, though
it's not clear why?
andrewgordstewart added a commit to magmo/force-move-protocol that referenced this issue Dec 7, 2018
See discussion here: ethers-io/ethers.js#363
`await provider.getSigner().getAddress()` seems to have ether, though
it's not clear why?
andrewgordstewart added a commit to magmo/force-move-protocol that referenced this issue Dec 7, 2018
See discussion here: ethers-io/ethers.js#363
`await provider.getSigner().getAddress()` seems to have ether, though
it's not clear why?
@andrewgordstewart
Copy link
Author

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 magmo/force-move-protocol@ad4468d, 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: magmo/force-move-protocol@01ab568

andrewgordstewart added a commit to magmo/turbo-adjudicator that referenced this issue Dec 13, 2018
I'm experiencing this issue on random test cases, so it's hard to see
whether the tests are actually passing.
ethers-io/ethers.js#363 (comment)
@andrewgordstewart
Copy link
Author

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

`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
Copy link
Member

ricmoo commented Aug 10, 2019

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
Copy link

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 MetaMask/web3-provider-engine#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
Copy link

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

@kermankohli
Copy link

kermankohli commented Aug 16, 2019

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.

snario pushed a commit to statechannels/statechannels that referenced this issue Sep 23, 2019
See discussion here: ethers-io/ethers.js#363
`await provider.getSigner().getAddress()` seems to have ether, though
it's not clear why?
@phiresky
Copy link

phiresky commented Apr 12, 2020

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:

transaction.nonce = this.getTransactionCount("pending");

@phiresky
Copy link

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
Copy link
Member

ricmoo commented Apr 13, 2020

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
Copy link
Member

ricmoo commented May 7, 2020

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! :)

@ricmoo ricmoo closed this as completed May 7, 2020
@EvilJordan
Copy link

EvilJordan commented Jun 10, 2020

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
Copy link

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
Copy link
Member

ricmoo commented Jun 26, 2020

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

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
Copy link

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
Copy link

beshup commented Dec 28, 2021

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Questions, feedback and general information. duplicate Duplicate of another issue.
Projects
None yet
Development

No branches or pull requests

10 participants