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

EIP 7702 #7237

Merged
merged 100 commits into from
Jul 17, 2024
Merged

Conversation

daniellehrner
Copy link
Contributor

PR description

Fixed Issue(s)

fixes #7205

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@Override
public void addCodeToEOA(final Address address, final Bytes code) {
if (temporaryEOACode.containsKey(address)) {
return;
Copy link
Contributor

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(
Copy link
Contributor

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

@daniellehrner daniellehrner marked this pull request as ready for review June 25, 2024 21:24
Copy link
Contributor

@jflo jflo left a 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"));
Copy link
Contributor

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?

Copy link
Contributor

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++) {
Copy link
Contributor

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(
Copy link
Contributor

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);
Copy link
Contributor

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())) {
;
Copy link
Contributor

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;
Copy link
Contributor

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() {
Copy link
Contributor

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() {
Copy link
Contributor

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() {
Copy link
Contributor

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());
Copy link
Contributor

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 {
Copy link
Contributor

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. */
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
/** 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. */

daniellehrner and others added 23 commits July 16, 2024 14:01
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>
macfarla and others added 5 commits July 16, 2024 14:01
* removed PKI backed QBFT

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

* changelog

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

---------

Signed-off-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>
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();
Copy link
Contributor

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?

@daniellehrner daniellehrner requested a review from jflo July 16, 2024 15:25
… nonces

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Comment on lines 379 to 380
// TODO 7702
case SET_CODE -> null;
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

same re: TODO

Copy link
Contributor

@jflo jflo left a 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.

Signed-off-by: Justin Florentine <justin+github@florentine.us>
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.

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

// 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"));
Copy link
Contributor

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

daniellehrner and others added 3 commits July 17, 2024 16:01
…r at the end.

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Justin Florentine <justin+github@florentine.us>
daniellehrner and others added 7 commits July 17, 2024 17:57
…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: Justin Florentine <justin+github@florentine.us>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner daniellehrner enabled auto-merge (squash) July 17, 2024 18:59
@daniellehrner daniellehrner merged commit 895c17d into hyperledger:main Jul 17, 2024
40 checks passed
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.

EIP-7702