-
Notifications
You must be signed in to change notification settings - Fork 463
Add RPC endpoint testing_buildBlockV1 #710
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| * This method MUST NOT be exposed on public-facing RPC APIs. | ||
|
|
||
| * It is strongly recommended that this method, and its `testing_` namespace, be disabled by default. Enabling it should require an explicit command-line flag or configuration setting by the node operator. |
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.
Would this fit better under debug_ namespace?
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.
The idea of creating a new namespace is to prevent enabling this method by mistake, together with other debug ones.
It was specified here: ethpandaops/gas-lighting-tracker#5
This new endpoint is sensitive and should exist under a new, non-exposed namespace (i.e., not debug_ or admin_ which are sometimes exposed by RPC providers).
@spencer-tb has another idea - to make it _debug so it will be different than current debug, but will not introduce quite exotic testing namespace.
I'm open for every option.
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.
Besu actually already has a similar endpoint called engine_preparePayload_debug so would support the _debug suffix 👍
I think debug_ prefix is also fine because there are already debug_ methods that can do quite a lot of damage so it would be a misconfiguration to leave them publicly accessible IMO.
|
|
||
| * The client MUST include all transactions from the `transactions` array on the block's transaction list, in the order they were provided. | ||
|
|
||
| * The client MUST NOT include any transactions from its local transaction pool. The resulting block MUST only contain the transactions specified in the `transactions` array. |
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.
| * The client MUST NOT include any transactions from its local transaction pool. The resulting block MUST only contain the transactions specified in the `transactions` array. | |
| * If the `transactions` array is non-empty, the client MUST NOT include any transactions from its local transaction pool. The resulting block MUST only contain the transactions specified in the `transactions` array. |
It might be a useful (optional?) feature to allow for selectiung txpool transactions in the case where the transaction array is empty.
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 would keep it as simple as possible in V1 to make it as easy as possible to implement for all EL teams. The use case is simple here - to build a block from provided txs. Your proposition is interesting, but:
- It can break some test cases e.g. empty payload tests
- It will add complexity - in current form we can totally ignore mempool and it's transactions
Also here, if majority of people will opt to have such feature, I'm fine with adding 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.
I think my wording keeps the option open without specifying what the client should do in the case of empty transaction array.
In Besu's case, we actually have a similar RPC method we use for internal testing, engine_preparePayload_debug see hyperledger/besu#4427
There are enough similarities that we would probably modify it to fit this spec, but the main use case for ours is to test txpool selection.
Addressing #705