-
Notifications
You must be signed in to change notification settings - Fork 388
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
Standardize 'txpool' namespace #353
base: main
Are you sure you want to change the base?
Conversation
src/schemas/txpool.yaml
Outdated
value: | ||
title: value | ||
$ref: '#/components/schemas/FilterOperators' | ||
nonce: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a use for being able to filter by nonce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not be used that much, but I can imagine if you have multiple txs pending (or stuck), it could be helpful to be able to filter. But this might be more with the idea that while we're at it we might as well include these
src/schemas/txpool.yaml
Outdated
title: statistics | ||
type: object | ||
properties: | ||
maxSize: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this useful? I lean towards omitting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant to show the configured maximum amount of txs on the pool.
E.g. txpool.globalslots value
or tx-pool-max-size
Which would give some indication of how full / utilized the node's pool is compared to pending/queued txs.
No strong opinion here tho.
title: Result | ||
schema: | ||
$ref: '#/components/schemas/TxPoolTransactions' | ||
- name: txpool_statistics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: txpool_statistics | |
- name: txpool_stats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think introducing txpool_stats
next to the existing txpool_status
might cause confusion and potential typos/errors.
txpool_statistics
would avoid that
But again, no strong opinion
@@ -0,0 +1,2 @@ | |||
>> {"jsonrpc":"2.0","id":31,"method":"txpool_transactions"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think what we should here for the tests is have two eth_sendRawTransaction
s before txpool_transactions
so it actually seeds the pool with the txs. This way we can still use these tests in hive to verify the response of each client. You can do this by defining the interaction as a test generator here: https://github.com/lightclient/rpctestgen/blob/main/testgen/generators.go
params: | ||
- name: Filter | ||
schema: | ||
title: txpool filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having now different kind of txs, probably a type
filter could be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean 4844/blobs transactions? if so, also see other comment that from my understanding 4844/blobs will have its own transaction pool and will not be included for this proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that the filter could accept a type
field where you can specify any of the transaction types: Frontier, EIP2930, EIP1559, EIP4844
type: object | ||
properties: | ||
pending: | ||
title: pending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not yet present elsewhere, I think we should define which criteria make a tx pending and queue, to avoid interpretations that could results in different implementations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this is specified somewhere already?
From my understanding pending means it's ready to be included in the next block
Queued is for the ones which are planned for future processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking about the distinction of the two sets, so what makes a tx ready to be included or not.
At first it seems easier to find the reasons why a tx in pool is not ready for inclusion, and the main reasons are:
- a previous tx is missing for the sender (nonce gap)
- not enough funds to pay for the gas
at the moment I only see these two, but I could miss something.
title: pending | ||
type: array | ||
items: | ||
$ref: '#/components/schemas/GenericTransaction' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to start a discussion, since specially with EIP-4844, we could have quite big network representation (that includes blob content) of a tx, so a way to select what to return and/or to limit the amount of data returned should be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, 4844/blobs will have its own transaction pool and do not use the generic tx pool. So not sure if this should be part of this proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting the previous answer, where I think blob txs should be included, setting some kind of limits make sense
This PR is a proposal to standardize the
txpool
namespace. All clients provide some access totxpool
information, but they currently all have their own implementations. See more details here.Standardizing the
txpool
namespace would be beneficial for interoperability, and hopefully allows for more reliable integrations that enable for example crLists.txpool_content
that most clients have, but with (optional) filter params.txpool_status
that most clients have.