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

Add max_fee_per_data_gas field to transaction #4970

Merged
merged 12 commits into from
Jan 24, 2023

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Jan 20, 2023

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Fixed Issue(s)

Add max_fee_per_data_gas field to transaction

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@@ -184,6 +185,11 @@
}
}

public static void writeBlobVersionedHashes(
final RLPOutput rlpOutput, final List<Hash> versionedHashes) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'rlpOutput' is never used.
@@ -184,6 +185,11 @@
}
}

public static void writeBlobVersionedHashes(
final RLPOutput rlpOutput, final List<Hash> versionedHashes) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'versionedHashes' is never used.
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 marked this pull request as ready for review January 20, 2023 19:18
# Conflicts:
#	ethereum/core/src/test-support/java/org/hyperledger/besu/ethereum/core/BlockDataGenerator.java
#	evm/src/main/java/org/hyperledger/besu/evm/frame/MessageFrame.java
Optional.empty(),
Optional.empty(),
unsignedLong(gas),
Optional.ofNullable(address(toAddress)),
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 normal to remove the optional for the to address ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes because it is done in the TransactionBuilder

@@ -1349,6 +1349,7 @@ public static class Builder {
private Multimap<Address, Bytes32> accessListWarmStorage = HashMultimap.create();

private Optional<List<Hash>> versionedHashes;

Copy link
Contributor

Choose a reason for hiding this comment

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

we can remove this modification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is voluntary, to add a missing space to separate 2 methods

@@ -72,13 +74,16 @@

public static final BigInteger TWO = BigInteger.valueOf(2);

public static final int DATA_GAS_PER_BLOB = 131072; // 2^17
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a good idea to move that in the protocol schedule part in a future PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, and will remove it from this review, since it will go in the data gas accounting PR

}

public BlockOptions setMaxFeePerDataGas(final Optional<Wei> maxFeePerDataGas) {
this.maybeMaxFeePerDataGas = Optional.of(maxFeePerDataGas);
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 optional of optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the same pattern that was used for the optional baseFee, that is also optional of optional here

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I saw that. it's confusing though!

it depends on the protocol schedule, and will managed in a next PR.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

👍

@fab-10 fab-10 enabled auto-merge (squash) January 24, 2023 07:40
@fab-10 fab-10 merged commit 617c14a into hyperledger:main Jan 24, 2023
@fab-10 fab-10 deleted the max_fee_per_data_gas branch January 24, 2023 08:12
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Jan 24, 2023
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
ensi321 pushed a commit to ensi321/besu that referenced this pull request Feb 19, 2023
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
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.

4 participants