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/get block transactions count #4340

Merged
merged 14 commits into from
Aug 12, 2022
Merged

Conversation

tanishqjasoria
Copy link
Contributor

Fixes getBlockTransactionCount when blockParameter is "pending". It did not take in account the pending transactions.

Changes:

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

@tanishqjasoria tanishqjasoria force-pushed the fix/get-block-transactions-count branch from 16b5b50 to 4535b15 Compare August 3, 2022 02:15
@tanishqjasoria tanishqjasoria marked this pull request as ready for review August 3, 2022 02:17
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

So this doesn't exactly do what it is stating. It only takes the nonce of the transaction from the pool. Those transactions don't have to be included in the next block. (Unless it should work like that just by giving you what is in the pool instead of what is in potential next block)

The main problem is that Nethermind doesn't have proper support for pending block. It should construct that block and return results based on that. That was never implemented. Also Geth team is keen on deprecating and removing the pending flag altogether.

Taking all that into account I'm more keep on just rejecting this change.

src/Nethermind/Nethermind.TxPool/TxPool.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.TxPool/TxPool.cs Outdated Show resolved Hide resolved
@tanishqjasoria
Copy link
Contributor Author

So this doesn't exactly do what it is stating. It only takes the nonce of the transaction from the pool. Those transactions don't have to be included in the next block. (Unless it should work like that just by giving you what is in the pool instead of what is in potential next block)

The main problem is that Nethermind doesn't have proper support for pending block. It should construct that block and return results based on that. That was never implemented. Also Geth team is keen on deprecating and removing the pending flag altogether.

Taking all that into account I'm more keep on just rejecting this change.

So should we just tell users to manage nonce locally? I still think there should be a way using which users can query if there are pending transactions from a particular address.
I understand the logic behind the "pending" block, but in most of the places getTransactionCount with pending parameter is used to get the value of next nonce that should be used for the next transaction.

@LukaszRozmej
Copy link
Member

How Geth implements it?

@tanishqjasoria
Copy link
Contributor Author

How Geth implements it?

pool.pendingNonces.get(addr) - they do something similar. They have a dict in txPool where they store pending nonces.

@tanishqjasoria
Copy link
Contributor Author

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

  1. Don't use _nonce, that was added only for ManagedNonce transactions explicitly.
  2. If you need you can go through a bucket explicitly in _transactions.
  3. What about potential gaps between nonces?

@tanishqjasoria
Copy link
Contributor Author

The way pending nonce is maintained in geth, they return the next available nonce.
If there is a gap, like there are txns in pool with nonces [2,3,5,6], then geth will return 4 as the next available nonce.

@LukaszRozmej
Copy link
Member

ok but it's maintained in just a dictionary? It is able to maintain that there and ignore gaps correctly?
On our side we can implement it by grabbing a bucket with transactions for address and do a simple scan there (maybe first check last item vs the count too to avoid it for non-gap cases)

@tanishqjasoria
Copy link
Contributor Author

tanishqjasoria commented Aug 10, 2022

They do is somewhat differently. They maintain two levels in the txPool (pending and all). One where all the transactions are present and other where only the transactions that can be added in any future block. Like the transactions with sequential nonce, if there is a gap in nonce, the rest of the txn are not added in the pending pool. And the pendingNonces dict is only maintained for txns in the pending pool.

Since our system is quite different, we can do it by scanning the txPool.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

I pushed a fix and optimizations.

  1. Current nonce from state was ignored. This resulted in incorrect values being returned.
  2. Avoid copying a bucket when doing scan.
  3. Avoid full collection scan if no gap.

@tanishqjasoria This needs more tests and a review.
I would prefer fairly straightforward test through RPC and sophisticated tests on TxPool.

@tanishqjasoria tanishqjasoria force-pushed the fix/get-block-transactions-count branch from bab7793 to bf8268e Compare August 10, 2022 10:03
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Please run a node for a while + spam it with request a bit

@tanishqjasoria tanishqjasoria force-pushed the fix/get-block-transactions-count branch from a8ad8da to 1a0f6a5 Compare August 10, 2022 15:12
@LukaszRozmej LukaszRozmej merged commit 6cb565b into master Aug 12, 2022
@LukaszRozmej LukaszRozmej deleted the fix/get-block-transactions-count branch August 12, 2022 15:22
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.

None yet

4 participants