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.
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.
should we check the nonce before we add code to eoa? i.e. why set code if the nonce is already invalid
jflo
left a comment
There was a problem hiding this comment.
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.
why not also assert that rugee now has zero balance?
There was a problem hiding this comment.
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.
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.
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.
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.
Empty statement in body of conditional?
|
|
||
| if (payload.nonces().size() == 1 | ||
| && !payload.nonces().getFirst().equals(accountNonce)) { | ||
| return; |
There was a problem hiding this comment.
I think this warrants a logging statement at DEBUG.
| class SetCodeTransactionDecoderTest { | ||
|
|
||
| @Test | ||
| void shouldDecodeInnerPayloadWithNonce() { |
There was a problem hiding this comment.
Could be used to implement the missing TODOs above.
| } | ||
|
|
||
| @Test | ||
| void shouldDecodeInnerPayloadWithMultipleNonces() { |
There was a problem hiding this comment.
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() { |
| 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.
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.
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.
| /** 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>
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 (besu-eth#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>
…esu-eth#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>
…u-eth#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>
… the transaction object (besu-eth#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.
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.
we should clear this TODO if we don't plan to implement this in tests [yet]
| } | ||
| break; | ||
| case SET_CODE: | ||
| // TODO 7702 |
jflo
left a comment
There was a problem hiding this comment.
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.
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.
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-requiredlabel 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