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

Fix underpriced transaction #2943

Merged

Conversation

hackaugusto
Copy link
Contributor

Using the txpool api to get the used nonces because getTransactionCount does not include the transactions from the pool.

The eth_getTransactoinCount RPC method does not include the nonce for
transactions in the mempool. Because of this Raiden was inadvertently
reuse the same nonce, leading to errors with transaction overwrites.
return max(pending.keys()) + 1

# The first valid nonce is 0, therefore the count is already the next
# avaiable nonce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/avaiable/available

"""Returns the next available nonce for `address`."""

# The nonces of the mempool transactions are considered used, and it's
# assumed these transactions are different from the ones currenlty pending
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/currenlty/currently

@hackaugusto
Copy link
Contributor Author

hackaugusto commented Oct 31, 2018

This has to be tested with parity (alternative nextNone)


# The first valid nonce is 0, therefore the count is already the next
# available nonce
return web3.eth.getTransactionCount(to_checksum_address(address), 'latest')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address is already being checksummed when calling this function so calling it here on the address again is redundant.

@rakanalh
Copy link
Contributor

rakanalh commented Nov 1, 2018

I am thinking about the side effects of this solution.... so if we have a close channel transaction that's already in the pool and we send another one for the same channel (upon restart) with a new nonce, this means that the latter will fail as the channel is already closed (which will raise a Recoverable error). While a RecoverableError might not be a direct issue as it is expected, would you think this might have any unexpected side effects?

@LefterisJP
Copy link
Contributor

@hackaugusto I checked and it seems the txpool is a geth only construct. But parity has parity_nextNonce. Unfortunately it's not offered by web3.py but we can simply make a json rpc call to parity. I added a commit that does that. Needs to actually be tested with parity before merging.

Copy link
Contributor

@rakanalh rakanalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hackaugusto
Copy link
Contributor Author

While a RecoverableError might not be a direct issue as it is expected, would you think this might have any unexpected side effects?

Well, it's possible, but the alternative would to match the pending transaction to the transaction in the pool, and that is way more work (I'm not saying we should not do it, but this fix allows fast restarts now)

@palango
Copy link
Contributor

palango commented Nov 5, 2018

This broke infura: See #2951 and messages on gitter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants