-
Notifications
You must be signed in to change notification settings - Fork 995
eth_config prototype #8417
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
eth_config prototype #8417
Conversation
siladu
left a comment
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.
Adding this as context for reviewers https://hackmd.io/@shemnon/eth_config
| "result": { | ||
| "current": { | ||
| "activation": 1444660040, | ||
| "blobs": { |
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.
| "blobs": { | |
| "blobSchedule": { |
Think we should align with genesis naming
| "id": 8, | ||
| "result": { | ||
| "current": { | ||
| "activation": 1444660040, |
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.
| "activation": 1444660040, | |
| "activationTime": 1444660040, |
Could this ever include blockNumber activation forks? Maybe for other networks?
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.
Two things, (a) the EIP limits itself to post-merge networks and (b) block numbers for mainnet and tesnets will always be less than the point in 2015 when ethereum launched.
Also, unless we are going to see it on mainnet or the testnets I don't think we should put it in EIPs. Rollups with shorter block times that present an issue for this can post a RRC. So it can be moved to activationTime, but I wouldn't specify anything for blocks.
L2s that adopt it and have block based activatons could update the eth_config generation file and add it.
.../api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthConfig.java
Outdated
Show resolved
Hide resolved
| spec.getBlockHashProcessor() | ||
| .getHistoryContract() | ||
| .ifPresent(a -> contracts.put("HISTORY", a.toHexString())); | ||
| if (!contracts.isEmpty()) { |
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 we need something for the BEACON_ROOTS_ADDRESS too? https://github.com/ethereum/EIPs/blob/2fb744367e5a65ff3afaf30025cba27ec36b1b3f/EIPS/eip-4788.md?plain=1#L34
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 can be added. This would be an example of a true constant being reported, as the beacon root is not configurable in Besu -
Line 40 in 821daf1
| final MutableAccount account = worldUpdater.getAccount(BEACON_ROOTS_ADDRESS); |
Thus it serves two good purposes - (a) a vale for Cancun systemContracts and (b) an example of serving up a compiled constant.
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.
This would be an example of a true constant being reported, as the beacon root is not configurable in Besu
Isn't this already the case with the precompiles and some of the other system contracts like HISTORY_STORAGE_ADDRESS?
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.
HISTORY_STORAGE_ADDRESS is a variable inside the besu implementations, and had multiple values in devnets. When I called BEACON_ROOTS_ADDRESS a constant I was referring to Besu code.
| .orElse(Map.of())); | ||
| spec.getBlockHashProcessor() | ||
| .getHistoryContract() | ||
| .ifPresent(a -> contracts.put("HISTORY", a.toHexString())); |
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 be nice to match the EIP name IMO i.e. HISTORY_STORAGE_ADDRESS (and same for the other contracts)
Think precision is good here as perhaps there will be future contracts that require similar names, e.g. the confusion around "withdrawals" vs "withdrawal requests".
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 have a naming proposal? 2935_HISTORY perhaps?
How about precompiles, should we add the EIP number to the name? What about modexp, which has had multiple tweaks to the gas schedule, would the eip be the most recent one?
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 it should match the name given in the EIP, i.e HISTORY_STORAGE_ADDRESS
I wouldn't object to including the EIP number too, although it is already encoded in the vanity address (though not consistently). However, my comment is less concerned about linking it to an EIP, rather than making it precise enough that a future system contract name is unlikely to collide.
Undecided if we should include EIP number to the precompiles, but if so then maybe the EIP it was introduced in would be more stable?
As an alternative, perhaps more precise naming is also warranted there...how likely is it that a future precompile collides? e.g. a new version of modexp alongside the old?
.../org/hyperledger/besu/evm/precompile/BigIntegerModularExponentiationPrecompiledContract.java
Outdated
Show resolved
Hide resolved
* implement NativeRequirements for named networks Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
A prototype for eth_cofig, sharing operational client configuration data via RPC. Signed-off-by: Danno Ferrin <danno@numisight.com>
Signed-off-by: Danno Ferrin <danno@numisight.com>
Signed-off-by: Danno Ferrin <danno@numisight.com>
Signed-off-by: Danno Ferrin <danno@numisight.com>
|
This pr is stale because it has been open for 30 days with no activity. |
|
This pr was closed because it has been inactive for 14 days since being marked as stale. |
|
Reopening as think we should get this in |
|
@shemnon Would you be happy for this to go in as is? We can always update this when the spec is updated. |
|
I'd feel better if at least one other client had implemented it. But sure, go ahead. |
|
This pr is stale because it has been open for 30 days with no activity. |
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
.../api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthConfig.java
Outdated
Show resolved
Hide resolved
.../api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/EthConfig.java
Outdated
Show resolved
Hide resolved
siladu
left a comment
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.
Wonder if we should add fork names into their as well, for convenience.
| } | ||
|
|
||
| @Override | ||
| public Optional<ScheduledProtocolSpec> getNextProtocolSpec(final long currentTime) { |
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.
Think we should have a test for this, but can be follow on PR.
* Feature/required besu native (hyperledger#8418) * implement NativeRequirements for named networks Signed-off-by: garyschulte <garyschulte@gmail.com> * Fix gradle verification (hyperledger#8435) Signed-off-by: Simon Dudley <simon.dudley@consensys.net> * eth_config prototype A prototype for eth_cofig, sharing operational client configuration data via RPC. Signed-off-by: Danno Ferrin <danno@numisight.com> * review comments Signed-off-by: Danno Ferrin <danno@numisight.com> * update precompile names Signed-off-by: Danno Ferrin <danno@numisight.com> * Refactor how next fork is calculated Signed-off-by: Danno Ferrin <danno@numisight.com> * copyright Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> * formatting Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> * changelog Signed-off-by: Simon Dudley <simon.dudley@consensys.net> --------- Signed-off-by: garyschulte <garyschulte@gmail.com> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Danno Ferrin <danno@numisight.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: garyschulte <garyschulte@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
* Feature/required besu native (hyperledger#8418) * implement NativeRequirements for named networks Signed-off-by: garyschulte <garyschulte@gmail.com> * Fix gradle verification (hyperledger#8435) Signed-off-by: Simon Dudley <simon.dudley@consensys.net> * eth_config prototype A prototype for eth_cofig, sharing operational client configuration data via RPC. Signed-off-by: Danno Ferrin <danno@numisight.com> * review comments Signed-off-by: Danno Ferrin <danno@numisight.com> * update precompile names Signed-off-by: Danno Ferrin <danno@numisight.com> * Refactor how next fork is calculated Signed-off-by: Danno Ferrin <danno@numisight.com> * copyright Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> * formatting Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> * changelog Signed-off-by: Simon Dudley <simon.dudley@consensys.net> --------- Signed-off-by: garyschulte <garyschulte@gmail.com> Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: Danno Ferrin <danno@numisight.com> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Co-authored-by: garyschulte <garyschulte@gmail.com> Co-authored-by: Simon Dudley <simon.dudley@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: jflo <justin+github@florentine.us>
PR description
A prototype for eth_cofig, sharing operational client configuration data
via RPC.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests