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.

Sign up for GitHub

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

Jump to bottom

EIP - 4844 #5536

Closed
wants to merge 149 commits into from
Closed

EIP - 4844 #5536

wants to merge 149 commits into from

Conversation

jflo
Copy link
Contributor

@jflo jflo commented Jun 5, 2023 •
edited
Loading

Implements EIP-4844.

  • introduces a Hardfork class to the protocol schedule system
  • new Engine APIs required for CL to work on 4844
  • new DataGas type for tracking block cost for 4844 data
  • new VersionedHash type to reflect that a versioned hash is not quite a pure sha256
  • incorporates wrapped jc-kzg library for KZG point evaluations
  • New transaction type, and domain objects for constituent parts to represent the Blobs, KZGCommitments, and Proofs used for 4844
  • RLP encoders and decoders to support new transaction type
  • gas pricing calculators for the new type of gas
  • plugin-api version was changed

@github-actions
Copy link

github-actions bot commented Jun 5, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

macfarla
macfarla reviewed Jun 6, 2023
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

got 1/3 through! can we remove the ssz tuweni snapshot lib?

@jflo jflo force-pushed the 4844-devnet-5b branch from 268f33a to 904d001 Compare June 7, 2023 00:26
macfarla
macfarla reviewed Jun 7, 2023
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

some more comments :)

@jflo
Copy link
Contributor Author

jflo commented Jun 8, 2023

Build has sync'd with mainnet here:

https://grafana.observability.consensys.net/d/_Mqt4ksnz/besu-full?orgId=19&var-system=dev-besu-clc-teku-ohio-mainnet-dev_4844_devnet6&var-sync_task_quantile=All&var-executor=All&var-vertx_pool_name=All&var-protocol_inbound=All&var-protocol_outbound=All&var-diskdevices=%5Ba-z%5D%2B%7Cnvme%5B0-9%5D%2Bn%5B0-9%5D%2B&var-overview_engine_percentiles=All&from=now-24h&to=now&refresh=1m

@non-fungible-nelson non-fungible-nelson mentioned this pull request Jun 13, 2023
Closed
@jflo jflo force-pushed the 4844-devnet-5b branch from 19d949e to fabe2cb Compare June 13, 2023 18:23
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 13, 2023
View reviewed changes
...ava/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/UnsignedLongParameter.java
@@ -24,7 +24,7 @@

@JsonCreator
public UnsignedLongParameter(final String value) {
this.value = Long.decode(value);
this.value = value != null ? Long.decode(value) : 0;

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException

Potential uncaught 'java.lang.NumberFormatException'.
.../main/java/org/hyperledger/besu/ethereum/referencetests/BlockchainReferenceTestCaseSpec.java
@@ -188,6 +189,7 @@
Hash.fromHexString(mixHash), // mixHash
Bytes.fromHexStringLenient(nonce).toLong(),
withdrawalsRoot != null ? Hash.fromHexString(withdrawalsRoot) : null,
dataGasUsed != null ? Long.parseLong(dataGasUsed) : 0,

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException

Potential uncaught 'java.lang.NumberFormatException'.
@jflo jflo force-pushed the 4844-devnet-5b branch 3 times, most recently from 5e2098f to 51d9ae5 Compare June 16, 2023 20:35
@jflo jflo marked this pull request as ready for review June 16, 2023 21:08
@jflo jflo force-pushed the 4844-devnet-5b branch from 7b3df74 to ec9c2ce Compare June 21, 2023 14:57
@jflo jflo force-pushed the 4844-devnet-5b branch from 4a2ce92 to 6ff611b Compare June 29, 2023 13:03
shemnon
shemnon previously requested changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

Most of my comments are nits and non blockers, except the bit about VersionedHash I think we need to make it type in the datatypes module and ensure we consistently use it instead of a mix of Hash and Bytes32.

@jflo jflo requested a review from shemnon June 29, 2023 19:51
@shemnon shemnon dismissed their stale review June 29, 2023 21:14

Issues resolved

@shemnon
Copy link
Contributor

shemnon commented Jun 29, 2023

I've reviewed datatypes, evm, plugin, and build code and I am satisfied. However I have not reviewed any of the code in besu, acceptance-tests, consensus, or ethereum directories as I expect other maintainers are better positioned to review those. Ping me next week if I still need to review those directories.

I would tag this as "approved" however that alone would clear it for merge when I feel other areas need different eye, so I'll hold off.

@jflo jflo force-pushed the 4844-devnet-5b branch from 7a26a4c to 4913d61 Compare June 30, 2023 14:26
siladu
siladu previously requested changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Reviewed acceptance-tests, consensus and part of engine API so far.

Requesting changes due to what I believe is a ZeroBaseFeeMarket bug

fab-10
fab-10 reviewed Jul 5, 2023
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

in progress 52/155, please check my comments

jflo and others added 26 commits July 19, 2023 16:08
@jflo
adds spdx header
ec80a6e 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
javadoc and more cleanup
175e38a 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
fixes test
8cc0c3b 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
tests unsupported hash versions
31d7413 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
tests unsupported hash versions
60b9a17 
Signed-off-by: Justin Florentine <[email protected]>
@Gabriel-Trintinalia @jflo
[4844] Check params earlier and move blob validation to a new method (#…
be4c893 
…5663)

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@jflo
adds test to cover blob txs not allowed to create contracts
4174d97 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
adopts pr suggestions
84f94e6 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
adopts pr suggestions
89e59af 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
default deposits is null
41dec98 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
default deposits is null
8405c05 
Signed-off-by: Justin Florentine <[email protected]>
@Gabriel-Trintinalia @jflo
[4844] NewPayloadV3 - Add fork validation and fixes (#5667)
66e4998 
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia @jflo
[4844] Swap parameters order and remove unused method (#5668)
fef3660 
* Swap parameters order

* Remove method

* Move blob validation to EngineNewPayloadV3 class

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia @spencer-tb @jflo
[4844] [Hive] Fix for genesis mismatch (#5677)
decc6d5 
* fix for genesis mismatch

Signed-off-by: spencer-tb <[email protected]>

* Fix tests and spotless

Signed-off-by: Gabriel-Trintinalia <[email protected]>

---------

Signed-off-by: spencer-tb <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Co-authored-by: spencer-tb <[email protected]>
@Gabriel-Trintinalia @jflo
[4844] Fix Data Gas Price Calculation (#5680)
4306b12 
* [4844] Fix Data Gas Price Calculation (#1)

Signed-off-by: Gabriel-Trintinalia <[email protected]>

* Fix spotless

Signed-off-by: Gabriel-Trintinalia <[email protected]>

* Fix mock unit tests

Signed-off-by: Gabriel-Trintinalia <[email protected]>

---------

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@jflo
default empty optionals
537a0b1 
Signed-off-by: Justin Florentine <[email protected]>
@Gabriel-Trintinalia @jflo
[4844] Add excessDataGas and dataGasUsed validation to EngineNewPaylo…
c611766 
…adV3 (#5683)

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@jflo
simplify VersionedHash
91aa138 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
create operations need the parents versionedHashes
25b4129 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
adopts more pr comments
57245bd 
Signed-off-by: Justin Florentine <[email protected]>
@Gabriel-Trintinalia @jflo
Use PrecompileContractResult halt and success instead of new Precompi…
00e9f2f 
…leContractResult (#5691)

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia @jflo
[4844] New Payload V3 - Return Invalid Payload instead of Invalid Par…
5f94f3f 
…ams for invalid versionedHash version (#5690)

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@jflo
moves 4844 classes to datatypes
ae51c6b 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
cleanup
f4b6dc3 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
ignoring 6110 test for now
2d87370 
Signed-off-by: Justin Florentine <[email protected]>
@jflo
updated from main, incorporates txpool refactoring
f145e87 
Signed-off-by: Justin Florentine <[email protected]>
@jflo jflo force-pushed the 4844-devnet-5b branch from d95ad30 to f145e87 Compare July 20, 2023 13:42
fab-10
fab-10 reviewed Jul 20, 2023
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

another pass

.../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java

LOG.atTrace()
.setMessage("blockparam: {}")
.addArgument(() -> Json.encodePrettily(blockParam))
.log();

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

is this required? if yes could you add a comment to explain

.../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java
@@ -172,7 +198,10 @@ public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext)
blockParam.getPrevRandao(),
0,
maybeWithdrawals.map(BodyValidation::withdrawalsRoot).orElse(null),
null,
blockParam.getDataGasUsed() == null ? null : blockParam.getDataGasUsed(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
blockParam.getDataGasUsed() == null ? null : blockParam.getDataGasUsed(),
blockParam.getDataGasUsed(),

...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineGetPayloadV3.java
final EngineCallListener engineCallListener,
final ProtocolSchedule schedule) {
super(vertx, protocolContext, mergeMiningCoordinator, blockResultFactory, engineCallListener);
this.shanghai = schedule.hardforkFor(s -> s.fork().name().equalsIgnoreCase("Shanghai"));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to use enums instead of strings here?

...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java
@@ -169,10 +171,10 @@ protected BlockCreationResult createBlock(
createPendingBlockHeader(timestamp, maybePrevRandao, newProtocolSpec);
final Address miningBeneficiary =
miningBeneficiaryCalculator.getMiningBeneficiary(processableBlockHeader.getNumber());
final Wei dataGasPrice =
Wei dataGasPrice =
Copy link
Contributor

Choose a reason for hiding this comment

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

why no more final?

...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java
@@ -228,11 +230,11 @@ protected BlockCreationResult createBlock(

throwIfStopped();

final DataGas newExcessDataGas = computeExcessDataGas(transactionResults, newProtocolSpec);
final GasUsage usage = computeExcessDataGas(transactionResults, newProtocolSpec);
Copy link
Contributor

Choose a reason for hiding this comment

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

usage is quire generic here, perhaps rename to dataGasUsage?

...creation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/AbstractBlockCreator.java
@@ -280,8 +285,11 @@ List<Deposit> findDepositsFromReceipts(final TransactionSelectionResults transac
.toList();
}

private DataGas computeExcessDataGas(
TransactionSelectionResults transactionResults, ProtocolSpec newProtocolSpec) {
record GasUsage(DataGas excessDataGas, DataGas used) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to DataGasUsage?

@jflo
Copy link
Contributor Author

jflo commented Jul 26, 2023

this work has been ported to https://github.com/jflo/besu/tree/EIP-4844 and already merged into main via #5724

Thanks to all who contributed comments!

@jflo jflo closed this Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shemnon shemnon shemnon left review comments

@macfarla macfarla macfarla left review comments

@siladu siladu siladu left review comments

@Gabriel-Trintinalia Gabriel-Trintinalia Gabriel-Trintinalia left review comments

@fab-10 fab-10 fab-10 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jflo @shemnon @macfarla @siladu @Gabriel-Trintinalia @fab-10 @gezero @gfukushima @pinges