Datahash opcode #4933

jflo · 2023-01-15T21:31:03Z

Don't merge till https://github.com/hyperledger/besu/pull/4931/files has been merged.

fab-10

looks good, please check my comments/questions

CHANGELOG.md

fab-10 · 2023-01-16T11:50:01Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java

@@ -753,6 +768,7 @@ private static Bytes32 computeSenderRecoveryHash(
        preimage = frontierPreimage(nonce, gasPrice, gasLimit, to, value, payload, chainId);
        break;
      case EIP1559:
+      case EIP4844:


idk too much about the recovery hash, so just asking if the versionedHashes should be ignored or taken in consideration here

I'm also unclear, but i think this will make more sense when we merge in the SSZ serialization

fab-10 · 2023-01-16T11:56:02Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java

@@ -979,6 +995,7 @@ public static class Builder {
    protected Optional<BigInteger> chainId = Optional.empty();

    protected Optional<BigInteger> v = Optional.empty();
+    protected Optional<List<Hash>> versionedHashes;


it seems simpler and more efficient to treat this likewise maxPriorityFeePerGas and similar optional field, keeping it null by default and using Optional.ofNullable in the build method

evm/src/main/java/org/hyperledger/besu/evm/operation/DataHashOperation.java

shemnon

One thing is unclear to me, if the chain is post-cancun, and they pass in a legacy or 1559 transaction should the datahash opcode be available? i.e. should it act as though there were zero hashes and always return zero or should it throw an invalid opcode if a 1559 tx calls a contract with the opcode?

Currently it looks like we are coded for the latter. This doesn't seem like the valid set of opcodes should be a function of the transaction format.

evm/src/main/java/org/hyperledger/besu/evm/operation/DataHashOperation.java

jflo · 2023-01-17T13:03:08Z

One thing is unclear to me, if the chain is post-cancun, and they pass in a legacy or 1559 transaction should the datahash opcode be available? i.e. should it act as though there were zero hashes and always return zero or should it throw an invalid opcode if a 1559 tx calls a contract with the opcode?

Currently it looks like we are coded for the latter. This doesn't seem like the valid set of opcodes should be a function of the transaction format.

Yes! I struggled with this as well while implementing it, and in hindsight i think I like returning zero instead. That way at least contract authors using DATAHASH can choose to accept or not pre-4844 transactions without a halt. I will change this and bring it up for discussion and see if test cases to cover this are planned.

CHANGELOG.md

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java

...um/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetTransactionProcessor.java

gradle.properties

…ventsImplTest Signed-off-by: Justin Florentine <justin+github@florentine.us>

Signed-off-by: Justin Florentine <justin+github@florentine.us>

…s, wtf Signed-off-by: Justin Florentine <justin+github@florentine.us>

… dns Signed-off-by: Justin Florentine <justin+github@florentine.us>

Signed-off-by: Justin Florentine <justin+github@florentine.us>

…blic constructor. Signed-off-by: Justin Florentine <justin+github@florentine.us>

Signed-off-by: Justin Florentine <justin+github@florentine.us>

evm/src/main/java/org/hyperledger/besu/evm/operation/DataHashOperation.java

evm/src/test/java/org/hyperledger/besu/evm/operations/DataHashOperationTest.java

shemnon · 2023-01-19T14:20:19Z

evm/src/test/java/org/hyperledger/besu/evm/operations/DataHashOperationTest.java

+  }
+
+  @Test
+  public void failsOnVersionIndexOutOFBounds() {


fail in tests indicates to me an exception raised, rather than a zero push.

public void pushesZeroOnVersionInexOutOfBounds() {

shemnon

Return need to be null exceptional halt reason, and some minor test changes (perhaps we should add the null exceptional halt to the last two unit tests for the data hash)

…peration.java Co-authored-by: Danno Ferrin <danno.ferrin@shemnon.com> Signed-off-by: Justin Florentine <justin+github@florentine.us>

…OperationTest.java Co-authored-by: Danno Ferrin <danno.ferrin@shemnon.com> Signed-off-by: Justin Florentine <justin+github@florentine.us>

diega

Doesn't o.h.b.e.m.MainnetProtocolSpecs#cancunDefinition need to set the transactionValidatorBuilder to add support for the new BLOB tx?

For the future, maybe we could came up with a different design for the Transaction/MessageFrame that doesn't carry so many Optionals all around.

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java

Signed-off-by: Justin Florentine <justin+github@florentine.us>

Implements new DATAHASH opcode in EVM Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo requested review from gezero and fab-10 January 15, 2023 21:34

fab-10 reviewed Jan 16, 2023

View reviewed changes

shemnon reviewed Jan 16, 2023

View reviewed changes

evm/src/main/java/org/hyperledger/besu/evm/operation/DataHashOperation.java Outdated Show resolved Hide resolved

jflo force-pushed the DATAHASH_opcode branch from 1a57ede to 67e969e Compare January 17, 2023 19:10

jflo marked this pull request as ready for review January 17, 2023 19:18

jflo requested review from shemnon and fab-10 January 17, 2023 19:18

fab-10 reviewed Jan 18, 2023

View reviewed changes

jflo added 20 commits January 18, 2023 13:37

replace iteration over tx type enums with explicit lists, fixes BesuE…

aadd8e7

…ventsImplTest Signed-off-by: Justin Florentine <justin+github@florentine.us>

fixes more tests

f62cc77

Signed-off-by: Justin Florentine <justin+github@florentine.us>

fixes BlockchainQueriesTest by changing the ORDER of transaction type…

aae68e4

…s, wtf Signed-off-by: Justin Florentine <justin+github@florentine.us>

fixes test that won't pass if local hostname unresolvable via reverse…

8a6ef6d

… dns Signed-off-by: Justin Florentine <justin+github@florentine.us>

removed commented code

997596d

Signed-off-by: Justin Florentine <justin+github@florentine.us>

removed commented code

9bc7bfa

Signed-off-by: Justin Florentine <justin+github@florentine.us>

changelog blurb

8157269

Signed-off-by: Justin Florentine <justin+github@florentine.us>

naive impl, passes a happy path test

60adb09

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds error condition coverage

6e07e73

Signed-off-by: Justin Florentine <justin+github@florentine.us>

updating TX construction signatures

f331684

Signed-off-by: Justin Florentine <justin+github@florentine.us>

updates plugin api hash since we've got a new TX type.

fb76b2f

Signed-off-by: Justin Florentine <justin+github@florentine.us>

changelog blurb

2f23a2f

Signed-off-by: Justin Florentine <justin+github@florentine.us>

naive impl, passes a happy path test

753d975

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adds error condition coverage

448ce9e

Signed-off-by: Justin Florentine <justin+github@florentine.us>

unit tests passing

27c0fe5

Signed-off-by: Justin Florentine <justin+github@florentine.us>

rebased off of 4844_tx_type

b364cda

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adopts pr suggestions

dc5bd41

Signed-off-by: Justin Florentine <justin+github@florentine.us>

never throw an invalid operation

6c1ae0c

Signed-off-by: Justin Florentine <justin+github@florentine.us>

more memory

bd14bb6

Signed-off-by: Justin Florentine <justin+github@florentine.us>

fix broken integration test that doesn't use the builder, uses the pu…

f03af6f

…blic constructor. Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo added 2 commits January 18, 2023 13:37

adopts pr suggestions

9e90c0a

Signed-off-by: Justin Florentine <justin+github@florentine.us>

adopts pr suggestions

70beaef

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo force-pushed the DATAHASH_opcode branch from b4efa48 to 70beaef Compare January 18, 2023 19:15

jflo requested a review from fab-10 January 18, 2023 20:55

fab-10 approved these changes Jan 19, 2023

View reviewed changes

Merge branch 'main' into DATAHASH_opcode

aaa9290

shemnon reviewed Jan 19, 2023

View reviewed changes

evm/src/main/java/org/hyperledger/besu/evm/operation/DataHashOperation.java Outdated Show resolved Hide resolved

shemnon reviewed Jan 19, 2023

View reviewed changes

evm/src/test/java/org/hyperledger/besu/evm/operations/DataHashOperationTest.java Outdated Show resolved Hide resolved

shemnon reviewed Jan 19, 2023

View reviewed changes

evm/src/test/java/org/hyperledger/besu/evm/operations/DataHashOperationTest.java Outdated Show resolved Hide resolved

shemnon reviewed Jan 19, 2023

View reviewed changes

shemnon suggested changes Jan 19, 2023

View reviewed changes

jflo and others added 3 commits January 19, 2023 09:28

Update evm/src/main/java/org/hyperledger/besu/evm/operation/DataHashO…

429588b

…peration.java Co-authored-by: Danno Ferrin <danno.ferrin@shemnon.com> Signed-off-by: Justin Florentine <justin+github@florentine.us>

Update evm/src/test/java/org/hyperledger/besu/evm/operations/DataHash…

b7d5380

…OperationTest.java Co-authored-by: Danno Ferrin <danno.ferrin@shemnon.com> Signed-off-by: Justin Florentine <justin+github@florentine.us>

Update evm/src/test/java/org/hyperledger/besu/evm/operations/DataHash…

79d9d26

…OperationTest.java Co-authored-by: Danno Ferrin <danno.ferrin@shemnon.com> Signed-off-by: Justin Florentine <justin+github@florentine.us>

diega reviewed Jan 19, 2023

View reviewed changes

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java Outdated Show resolved Hide resolved

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Transaction.java Outdated Show resolved Hide resolved

adds test coverage to ensure halt reason is null

534c1a0

Signed-off-by: Justin Florentine <justin+github@florentine.us>

shemnon approved these changes Jan 19, 2023

View reviewed changes

jflo added 2 commits January 19, 2023 10:45

Merge branch 'main' into DATAHASH_opcode

1aaf58c

minimized new param impact to legacy Transaction constructors

d16cee5

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo enabled auto-merge (squash) January 19, 2023 16:42

diega approved these changes Jan 19, 2023

View reviewed changes

jflo added 3 commits January 19, 2023 14:10

Merge branch 'main' into DATAHASH_opcode

081edaa

Merge branch 'main' into DATAHASH_opcode

742a111

Signed-off-by: Justin Florentine <justin+github@florentine.us>

fixes conflict

bb448be

Signed-off-by: Justin Florentine <justin+github@florentine.us>

jflo merged commit 42d67de into hyperledger:main Jan 20, 2023

elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023

Datahash opcode (hyperledger#4933)

0fe4786

Implements new DATAHASH opcode in EVM Signed-off-by: Justin Florentine <justin+github@florentine.us>

eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023

Datahash opcode (hyperledger#4933)

32a505b

Implements new DATAHASH opcode in EVM Signed-off-by: Justin Florentine <justin+github@florentine.us>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datahash opcode #4933

Datahash opcode #4933

jflo commented Jan 15, 2023

fab-10 left a comment

fab-10 Jan 16, 2023

jflo Jan 16, 2023

fab-10 Jan 16, 2023

shemnon left a comment

jflo commented Jan 17, 2023

shemnon Jan 19, 2023

shemnon left a comment

diega left a comment

Datahash opcode #4933

Datahash opcode #4933

Conversation

jflo commented Jan 15, 2023

fab-10 left a comment

Choose a reason for hiding this comment

fab-10 Jan 16, 2023

Choose a reason for hiding this comment

jflo Jan 16, 2023

Choose a reason for hiding this comment

fab-10 Jan 16, 2023

Choose a reason for hiding this comment

shemnon left a comment

Choose a reason for hiding this comment

jflo commented Jan 17, 2023

shemnon Jan 19, 2023

Choose a reason for hiding this comment

shemnon left a comment

Choose a reason for hiding this comment

diega left a comment

Choose a reason for hiding this comment