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

[BESU-176] JRPC response to eth_getTransactionCount is different from other node implementations. #351

Merged

Conversation

AbdelStark
Copy link
Contributor

@AbdelStark AbdelStark commented Jan 31, 2020

PR description

Impact

Someone is using their BESU node as they use the other implementations and following good practices, their transactions will eventually get stuck, while in the other implementations this wouldn´t happen.

How it works

When asking for transaction count with the param ‘pending’, Besu returns the latest pending transaction nonce + 1, other implementations return the next user nonce non used by the pending transactions.
This means that if a transaction with a future nonce reaches a Besu node (Sending many nonce requests, latency issues, dropped transaction, planning future transactions), all the responses to getTransactionCount will return future nonces.
This means that none of the transactions are mined, and after a short while they are rejected by other nodes.
In other node implementations, getTransactionCount would return the missing nonce, enabling the execution of all broadcasted transactions.

Fixed Issue(s)

fixes https://jira.hyperledger.org/browse/BESU-176

Credits

Enrique Alcázar Garzás, the original reporter of the issue.
Many thanks for this valuable work which helped me a lot fixing this bug.

Signed-off-by: Abdelhamid Bakhta abdelhamid.bakhta@consensys.net

@AbdelStark AbdelStark added bug Something isn't working testing labels Jan 31, 2020
@AbdelStark AbdelStark self-assigned this Jan 31, 2020
@AbdelStark AbdelStark force-pushed the fix-nonce-management-local-tx-pool branch 2 times, most recently from d1babb9 to 775c1a9 Compare January 31, 2020 09:33
Added test.

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
@AbdelStark AbdelStark force-pushed the fix-nonce-management-local-tx-pool branch from 775c1a9 to 71c075c Compare January 31, 2020 09:41
…ent-local-tx-pool

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
… implementation, more robust against load testing.

A queue of gaps between used nonces is maintained.
`getNextNonceForSender` has been updated and have the following logic:
- if no infos corresponding to the nonce returns an empty optional
- if gap list is not empty returns the lowest nonce in the gap
- else returns the max nonce + 1

Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
Signed-off-by: Abdelhamid Bakhta <abdelhamid.bakhta@consensys.net>
@AbdelStark AbdelStark marked this pull request as ready for review February 3, 2020 18:01
@AbdelStark AbdelStark merged commit d7d82e4 into hyperledger:master Feb 3, 2020
@kikoncuo
Copy link

Feels kinda bad to find the bug, create tests, make a report on it, and just having someone copy the issue you created word by word and not credit you at all as the original reporter...

https://jira.hyperledger.org/browse/BESU-176

Thanks for the fix thou

@AbdelStark
Copy link
Contributor Author

Feels kinda bad to find the bug, create tests, make a report on it, and just having someone copy the issue you created word by word and not credit you at all as the original reporter...

https://jira.hyperledger.org/browse/BESU-176

Thanks for the fix thou

Sorry, indeed you are right. It was not intentional. I will update the description of the pull request for tracking record. Thanks for this valuable work.

@AbdelStark AbdelStark deleted the fix-nonce-management-local-tx-pool branch February 17, 2020 09:33
@faraggi
Copy link
Contributor

faraggi commented Feb 18, 2020

@kikoncuo Sorry about this slip up on our part. Your contribution was ideal, lots of details and it really let us understand the problem.

Send me your address and I can get some Besu stickers to you. PM me at https://twitter.com/felipefaraggi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants