-
Notifications
You must be signed in to change notification settings - Fork 838
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
EIP 7702 #7237
EIP 7702 #7237
Conversation
9e9311c
to
5aeb067
Compare
@Override | ||
public void addCodeToEOA(final Address address, final Bytes code) { | ||
if (temporaryEOACode.containsKey(address)) { | ||
return; |
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.
what happens if there are multiple 7702 transactions in a block for a single address? should we be doing a codeHash check here? or a nonce check?
return; | ||
} | ||
|
||
worldUpdater.addCodeToEOA( |
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.
should we check the nonce before we add code to eoa? i.e. why set code if the nonce is already invalid
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.
Suggest some refactoring, which will also correct some invalid tests.
// The transaction sponsor balance should be greater than 170000 ETH. We don't do an exact | ||
// balance check to avoid | ||
// having to calculate the exact amount of gas used. | ||
assertThat(transactionSponsorBalance).isGreaterThan(new BigInteger("170000000000000000000000")); |
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.
why not also assert that rugee now has zero balance?
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 this can be resolved, it is currently on line 111
final long noncesSize = input.nextSize(); | ||
|
||
input.enterList(); | ||
for (int i = 0; i < noncesSize; i++) { |
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 will fail under fuzzing, when someone sends a list of nonces of length > 1, suggest throwing a decode exception of some sort should that arise. See above point about treating the nonce as an Optional in SetCodeAuthorization but wrapping it with a list only upon encoding/decoding.
* @param nonces the list of nonces | ||
* @param signature the signature of the EOA account which will be used to set the code | ||
*/ | ||
public record SetCodeAuthorization( |
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 domain object describes the nonce as a list, however it is really an Optional of a single value. The fact that we use a List to represent that is an implementation detail of how the RLP is encoded, and should only be necessary in the Encoders/Decoders.
rlpOutput.writeBigIntegerScalar(payload.chainId()); | ||
rlpOutput.writeBytes(payload.address().copy()); | ||
rlpOutput.startList(); | ||
payload.nonces().forEach(rlpOutput::writeLongScalar); |
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 should never run more than once, and if we make the changes suggested above, we can just do an optional check instead of running the risk that there is more than one.
if (!chainId.equals(BigInteger.ZERO) | ||
&& !payload.chainId().equals(BigInteger.ZERO) | ||
&& !chainId.equals(payload.chainId())) { | ||
; |
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.
Empty statement in body of conditional?
|
||
if (payload.nonces().size() == 1 | ||
&& !payload.nonces().getFirst().equals(accountNonce)) { | ||
return; |
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 this warrants a logging statement at DEBUG.
class SetCodeTransactionDecoderTest { | ||
|
||
@Test | ||
void shouldDecodeInnerPayloadWithNonce() { |
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.
Could be used to implement the missing TODOs above.
} | ||
|
||
@Test | ||
void shouldDecodeInnerPayloadWithMultipleNonces() { |
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.
Invalid test per EIP-7702:
When the list length is zero, consider the authorization nonce to be null. When the list length is one, consider the single integer value to be the provided nonce authorization. Other lengths and value types in this optional are invalid and the transaction as a whole should be considered malformed.
} | ||
|
||
@Test | ||
void shouldEncodeSingleSetCodeWithMultipleNonces() { |
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.
Same as above, invalid test.
@@ -374,6 +386,9 @@ public TransactionProcessingResult processTransaction( | |||
if (transaction.getVersionedHashes().isPresent()) { | |||
commonMessageFrameBuilder.versionedHashes( | |||
Optional.of(transaction.getVersionedHashes().get().stream().toList())); | |||
} else if (transaction.setCodeTransactionPayloads().isPresent()) { | |||
setCodeTransactionProcessor.addContractToAuthority(worldUpdaterService, transaction); | |||
addressList.addAll(worldUpdaterService.getAuthorities()); |
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.
Suggest renaming this emptyAccounts
, had to trace this back to the declaration and luckily it was documented there.
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class SetCodeTransactionProcessor { |
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.
Suggest renaming. Perhaps AuthorityProcessor or something, this does not process the transaction, but rather prepares it for processing elsewhere.
import org.apache.tuweni.bytes.Bytes32; | ||
import org.apache.tuweni.units.bigints.UInt256; | ||
|
||
/** Wraps another account that has authorized code to be loaded into 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.
/** Wraps another account that has authorized code to be loaded into it. */ | |
/** Wraps an EOA account and includes authorized code to be run on behalf of it. */ |
9c610ec
to
7ff9983
Compare
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Container verification step in release process automated with the container verify GitHub workflow. New workflow is triggered at the end of the release workflow which will check the release container images starts successfully. Verification test only checks container starts and reach the Ethereum main loop Signed-off-by: Chaminda Divitotawela <cdivitotawela@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Fix some reasons for chain download halts when syncing Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net> Signed-off-by: Stefan Pingel <16143240+pinges@users.noreply.github.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Check and test for the unused container rule, and only returncontract targets can have truncated data rule. Also test the other subcontainer rules in unit tests. Signed-off-by: Danno Ferrin <danno@numisight.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…#7247) Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…transaction validation and tx pool logic Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…to be necessary before final fork choice update Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…ting accounts as well Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…only available in the prague hard fork Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…yet exist Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…rty (hyperledger#7222) * Add build version option to prefix git hash with custom version property * Refactor to make appending the git hash a boolean property. Include a commented-out example of how to use the properties in the gradle file Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…yperledger#7221) Signed-off-by: Jason Frame <jason.frame@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Ties <71668189+TiesD@users.noreply.github.com> Co-authored-by: Matt Nelson <85905982+non-fungible-nelson@users.noreply.github.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
* Add option for LUKSO network Signed-off-by: Wolmin <lamonos123@gmail.com> * Add tests for LUKSO Signed-off-by: Wolmin <lamonos123@gmail.com> * Apply spotless Signed-off-by: Wolmin <lamonos123@gmail.com> * Add changelog entry Signed-off-by: Wolmin <lamonos123@gmail.com> * Fix duplicate func Signed-off-by: Wolmin <lamonos123@gmail.com> * Fix changelog Signed-off-by: Wolmin <lamonos123@gmail.com> * Add bootnodes to genesis Signed-off-by: Wolmin <lamonos123@gmail.com> --------- Signed-off-by: Wolmin <lamonos123@gmail.com> Signed-off-by: Wolmin <44748271+Wolmin@users.noreply.github.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…erledger#7245) Make the max code size and max initcode size a part of the EVM configuration. As part of the change we need to move the tasks CodeFactory once handled as a static class and move it into the EVM. This has a nice follow on effect that we don't need to pass in max EOF versions or max code sizes anymore. Signed-off-by: Danno Ferrin <danno@numisight.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
* fix the synchronizer usage Signed-off-by: Leni <leniram159@gmail.com> * fix Identifier usage Signed-off-by: Leni <leniram159@gmail.com> --------- Signed-off-by: Leni <leniram159@gmail.com> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net> Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
* add request type for consolidations, encoder, decoder and tests * added raw tx for consolidation * add consolidation reqs to EngineGetPayloadResultV4 * set storage slot value to 0 initially and value for tx * updates plugin api Signed-off-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> --------- Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com> Signed-off-by: Justin Florentine <justin+github@florentine.us> Co-authored-by: Justin Florentine <justin+github@florentine.us> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
… the transaction object (hyperledger#7323) * fix: Use Builder for JsonCallParameter * changelog * add additional unit tests * fix: Update builder to withGas to match the json eth_call --------- Signed-off-by: Usman Saleem <usman@usmans.info> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net> Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
… isn't set Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
7ff9983
to
4eeb5f0
Compare
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
* | ||
* @return all the nonces | ||
*/ | ||
List<Long> nonces(); |
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.
Shouldn't this be Optional<Long> nonce()
instead since only 1 can be in the RLP list?
… nonces Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
// TODO 7702 | ||
case SET_CODE -> null; |
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.
we should clear this TODO if we don't plan to implement this in tests [yet]
@@ -92,6 +92,9 @@ public Transaction createTransaction(final KeyPair keys) { | |||
builder.versionedHashes(versionedHashes.get()); | |||
} | |||
break; | |||
case SET_CODE: | |||
// TODO 7702 |
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.
same re: TODO
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.
Still doesn't stop the encoding detail of the List from leaking upward. Happy to open a PR into this branch to better illustrate what I mean.
...ore/src/main/java/org/hyperledger/besu/ethereum/core/encoding/SetCodeTransactionEncoder.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/SetCodeAuthorization.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Justin Florentine <justin+github@florentine.us>
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.
Big PR. Still haven't gotten through all of it, so I am posting comments early.
I think we need more checks around boundary cases for authorizations to ensure we (continue to) correctly handle duplicated authorities, multiple nonces in authorizations, etc
...rc/test/java/org/hyperledger/besu/tests/acceptance/ethereum/PragueAcceptanceTestService.java
Outdated
Show resolved
Hide resolved
// The transaction sponsor balance should be greater than 170000 ETH. We don't do an exact | ||
// balance check to avoid | ||
// having to calculate the exact amount of gas used. | ||
assertThat(transactionSponsorBalance).isGreaterThan(new BigInteger("170000000000000000000000")); |
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 this can be resolved, it is currently on line 111
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/SetCodeAuthorization.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetProtocolSpecs.java
Outdated
Show resolved
Hide resolved
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/AuthorityProcessor.java
Outdated
Show resolved
Hide resolved
evm/src/main/java/org/hyperledger/besu/evm/worldstate/AbstractWorldUpdater.java
Show resolved
Hide resolved
plugs leaky abstraction
…r at the end. Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
…Account Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…/besu into feat/issue-7205/eip-7702v1 Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
…/besu into feat/issue-7205/eip-7702v1
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
PR description
Fixed Issue(s)
fixes #7205
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests