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

Improve nonce tracking #1127

Closed
danfinlay opened this issue Feb 16, 2017 · 24 comments
Closed

Improve nonce tracking #1127

danfinlay opened this issue Feb 16, 2017 · 24 comments
Assignees
Labels
area-background Issues relating to the extension background process. area-provider Relating to the provider module. type-bug

Comments

@danfinlay
Copy link
Contributor

danfinlay commented Feb 16, 2017

During the melonport launch, a user transmitted several txs that failed for different reasons at once. The result was that their MetaMask account was put into an incorrect nonce state.

We need to improve our nonce updating logic. Maybe querying the network for an updated nonce before each tx transmission?

@danfinlay danfinlay added this to the Core Maintenance milestone Feb 16, 2017
@kumavis
Copy link
Member

kumavis commented Feb 16, 2017

Maybe querying the network for an updated nonce before each tx transmission?

I wish it were that easy. infura switching to single node will likely open up more options for us here

@danfinlay
Copy link
Contributor Author

Is that just an issue when emitting several txs in series, where the nodes don't have time to synchronize?

In the short term, I guess we have some bug in how we decrement the nonce after tx failures.

@kumavis
Copy link
Member

kumavis commented Feb 16, 2017

yeah need more tests around the nonce tracker
one reason why we have fewer tests than we would like is bc the damn block polling is built in and so makes writing tests a pain. gah. need to attack that beast

@danfinlay
Copy link
Contributor Author

the getTransactionCount method we use to retrieve a nonce accepts a pending parameter, which factors in the number of pending transactions! That's kindof interesting.

I wonder if a single over-nonced tx would then throw off the whole flow, tricking the client into incrementing the nonce to the wrong place in the series...

@tomusdrw
Copy link

For Parity it would be best to use parity_nextNonce, the definition of pending state as implemented in Parity doesn't take the whole queue into consideration (just transactions in the pending block and only if the node is actually sealing blocks).

@danfinlay
Copy link
Contributor Author

Is there a chance this feature will be implemented by other clients with a common name? Our users like being able to point at whatever RPC they are running, @tomusdrw.

@tomusdrw
Copy link

Some discussions on RPC API governance and cross-client tests suites / specs is ongoing here ethereum/EIPs#217 I really hope that it will be the case in a near future.

I've also created an issue in Parity repo to discuss different definitions/exceptions for pending state: openethereum/parity-ethereum#5014

@danfinlay
Copy link
Contributor Author

Glad you're thinking about the same things.

Yeah the RPC API governance will be nice to help coordinate, but we shouldn't use it as a reason to avoid coordinating in the meanwhile. It could take a while to have something good working there.

@danfinlay
Copy link
Contributor Author

If we added nonces to the tx history, it might be easier to visually debug these issues.

@danfinlay danfinlay removed the ready label Apr 17, 2017
@danfinlay
Copy link
Contributor Author

I just helped debug a user on reddit who had another sort of nonce issue:

They had the same account on both metamask and MEW, and after sending a tx on MEW, the next metamask TX failed, because it had the same nonce.

Maybe if we polled for updated nonces more frequently, we could alleviate his issue.

@rickfeiner
Copy link

flyswatter, I am experiencing the same issue that you just described. I get a "Nonce too low" error when trying to submit a transaction from metamask after submitting transactions from the same account in geth.

Here are the reproduction steps:

  1. Import a "loose" account into Metamask via JSON import using geth keys.
  2. Make a transaction with geth using the account you just imported into Metamask
  3. Attempt to make a Metamask transaction using the imported account

Result: "Nonce too low" error.

@danfinlay
Copy link
Contributor Author

Thanks for the report, @rickfeiner, I hope you found that you can restart your browser to fix the issue. If not, at worst, you might need to uninstall & reinstall MetaMask.

@rickfeiner
Copy link

I've found that switching to test-net from main-net (or vise-versa) and then back again, resolves the problem.

@danfinlay
Copy link
Contributor Author

Good tip! Maybe we should just add a reset nonce button until we have this well sorted out.

@2-am-zzz 2-am-zzz added area-background Issues relating to the extension background process. P1-asap labels May 11, 2017
@kumavis
Copy link
Member

kumavis commented May 19, 2017

nonce tracking plan:

  • reduce possibility of unreasonable txs
    • low gasPrice
    • high gasLimit P1
      • disallow txs with gas limit higher than 95% of the block gas limit
      • validation message should indicate current % of block gas limit and/or max gasLimit
      • validation message should indicate that its due to "fluxuating block gas limits"
    • low gasLimit P2
      • i think the lowest possible gasLimit is 21000?
  • track txs
    • incomming external txs P3
      • here by external i mean key reused by other app like 2nd metamask or mew
    • known pending txs
  • handle failure
    • consider pending txs unknown to node as failed P3
    • provide "retry" button P3
  • nonce tracker
    • should calculate nonce based on all pending txs

@danfinlay
Copy link
Contributor Author

Retry button = edit gas settings and resubmit?

@kumavis
Copy link
Member

kumavis commented May 19, 2017

Retry button = edit gas settings and resubmit?

yes and with a fresh nonce. should just drop you on the conf tx page with the same details filled in.

@danfinlay danfinlay added area-provider Relating to the provider module. and removed type-enhancement labels May 21, 2017
@danfinlay
Copy link
Contributor Author

This problem could have been simplified/avoided sooner if we'd shown support for this Parity feature:

openethereum/parity-ethereum#5014

Head down, chime in, and then hopefully we can get the Geth team to do the same.

It's probably a lot slower to wait for that feature than to just fix this without it, though.

@danfinlay
Copy link
Contributor Author

danfinlay commented Jun 27, 2017

Here are some action steps me and Frankie came up with regarding this issue:

step 0:
remove retry limitation on pending txs, retry on block.

step 1:
People have a big ugly queue of failed txs.
Those error’d txs should have the error icon & show failed.

step 2:
calculate nonce based on local pending txs w/o error state.

step 2.5:
Reset nonce switch to cancel pending: #1668

step 3:
Clicking a pending tx goes to tx approval screen,
change “ACCEPT” to “SUBMIT” or “SEND”.

step 4: allow editing nonce via an advanced thing on the tx approval screen.

@arcalinea
Copy link

arcalinea commented Jul 18, 2017

A reset nonce button would be great. I just created this problem for myself on testrpc while developing a dapp that uses metamask, now have a pile of failed txs and would like to just clear the state.

Update: Switching back and forth between networks seems to reset

@kumavis
Copy link
Member

kumavis commented Aug 9, 2017

Nonce tracking has been significantly improved via the new tx-controller and its nonce-tracker
closing this ticket for now

@doowb
Copy link

doowb commented Dec 8, 2017

I ran into this issue using a local ganache instance. I had closed it down and started it up again which resets all of the accounts. It seems like MetaMask doesn't get the new nonce so I'm not able to submit any new transactions with the test accounts that I've already used.

Posting here since I'm not sure if this should be a new issue or not.

@danfinlay
Copy link
Contributor Author

Not a new issue. Two ways around it:

  • Restart ganache with a different network ID
  • Uninstall & reinstall MetaMask.

@doowb
Copy link

doowb commented Dec 8, 2017

Thanks! Reinstalling MetaMask worked for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-background Issues relating to the extension background process. area-provider Relating to the provider module. type-bug
Projects
None yet
Development

No branches or pull requests

7 participants