EIP-2935 #6991

Gabriel-Trintinalia · 2024-04-23T21:40:05Z

PR description

fix #6531

Extract EIP-2935 from the verkle branch
Fix Engine Prague Acceptance tests
Implement tests for BlockHashOperation and HistoricalBlockHashProcessor

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

…into eip-2935

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

jflo

This is good, the requested changes are naming and contract addresses.

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

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java

jframe · 2024-05-07T05:50:37Z

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java

+      storeParentHash(account, currentBlockHeader);
+
+      BlockHeader ancestor =
+          blockchain.getBlockHeader(currentBlockHeader.getParentHash()).orElseThrow();


What will happen if an exception is thrown due to not finding a parent hash? Will Besu crash?

Agreed, should likely just replace this with a presence check, and no-op if it is missing

normally it's not possible to try import a block without having the parent it will be a critical issue. maybe we need to test to be sure the behavior is correct in this case and be sure we are considering the block as problematic to allow a retry ?

...reum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClearEmptyAccountStrategy.java

jframe · 2024-05-07T06:14:51Z

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

+            frame
+                .getWorldUpdater()
+                .get(HISTORY_STORAGE_ADDRESS)
+                .getStorageValue(UInt256.valueOf(soughtBlock % BLOCKHASH_OLD_WINDOW)));


I think we should be doing soughtBlock % HISTORY_SERVE_WINDOW not soughtBlock % BLOCKHASH_OLD_WINDOW

I think jason is right here, we may need to add a test where the block being sought is > BLOCKHASH_OLD_WINDOW but < HISTORY_SERVE_WINDOW

The last information I had was that this EIP was not supposed to serve more than 256 block hashes with the block hash operation, but this may have changed and needs to be verified. @Gabriel-Trintinalia

BLOCKHASH_OLD_WINDOW is the serving window pre-prague. HISTORY_SERVE_WINDOW is the new serving window. There was an error in that part. Fixed now

matkt

It seems to be fine for me as it's similar to the verkle implementation that is currently working in a devnet.

We just need to be sure we are catching correctly the case where we don't have the parent header and try this feature with reference tests when it will be available

and check the block hash operation implementation

...reum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClearEmptyAccountStrategy.java

matkt · 2024-05-07T13:08:21Z

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java

+      storeParentHash(account, currentBlockHeader);
+
+      BlockHeader ancestor =
+          blockchain.getBlockHeader(currentBlockHeader.getParentHash()).orElseThrow();


normally it's not possible to try import a block without having the parent it will be a critical issue. maybe we need to test to be sure the behavior is correct in this case and be sure we are considering the block as problematic to allow a retry ?

jflo

Agree with jasons comments

jflo · 2024-05-07T12:37:34Z

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java

+      storeParentHash(account, currentBlockHeader);
+
+      BlockHeader ancestor =
+          blockchain.getBlockHeader(currentBlockHeader.getParentHash()).orElseThrow();


Agreed, should likely just replace this with a presence check, and no-op if it is missing

jflo · 2024-05-07T13:06:47Z

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

+            frame
+                .getWorldUpdater()
+                .get(HISTORY_STORAGE_ADDRESS)
+                .getStorageValue(UInt256.valueOf(soughtBlock % BLOCKHASH_OLD_WINDOW)));


I think jason is right here, we may need to add a test where the block being sought is > BLOCKHASH_OLD_WINDOW but < HISTORY_SERVE_WINDOW

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

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

jflo

great work!

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java

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

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

shemnon

EVM implementation will cause problems with downstream users of the library. It should be refactored so the logic remains within the Block Hash Lookup function passed into the EVM, rather than bleeding into block hash operation logic. This is why the function was created.

shemnon · 2024-05-08T17:43:42Z

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

@@ -29,45 +32,105 @@
 /** The Block hash operation. */
 public class BlockHashOperation extends AbstractFixedCostOperation {

-  private static final int MAX_RELATIVE_BLOCK = 255;
+  /** The HISTORY_STORAGE_ADDRESS */


I really don't like this implementation, as it forces design decisions onto downstream users of the library.

This logic should live in a new BlockHashLookup function that is wired in in the mainnet protocol schedule, rather than re-writing the operation based on a flag.

Here's a WIP of how I think it should work - main...shemnon:besu:eip-2935-danno

Move all block hash lookup out of the operation, the operation only delegates to the data rpovider

the data provider is part of the protocol spec

merge in the beacon and block hash processing into the same class

MainnetProtocolSpecs is where it all gets loaded in.

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

Changes aren't needed

shemnon · 2024-05-09T04:44:29Z

Changes are needed, it damages the utility of the standalone EVM library. Here's my WIP to retain this utility - main...shemnon:besu:eip-2935-danno

shemnon · 2024-05-09T06:43:57Z

Here's a more fully worked version: #7080
I'll clean up the FIXMEs tomorrow. Acceptance and reference tests worked locally.

shemnon · 2024-05-09T21:42:52Z

PR #7080 is ready to review. It includes the changes in this PR but also containing the changes the EVM needs to remain a standalone library. @Gabriel-Trintinalia

Gabriel-Trintinalia · 2024-05-12T06:58:17Z

Closed in favour of #7080

Gabriel-Trintinalia added 20 commits April 17, 2024 18:08

extract into main

9f88959

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge main into 'eip-2935'

991b44a

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Add ClearEmptyAccountStrategy

1f17958

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Add ClearEmptyAccountStrategy

11f6122

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix javadoc

5cfbfbd

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix acceptance test

d866d67

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix spotless

c846e4b

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge branch 'main' into eip-2935

087817e

Fix missing instance

b34047a

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge branch 'eip-2935' of https://github.com/Gabriel-Trintinalia/besu …

48751d0

…into eip-2935

Fix integration test

eed222a

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix wrong strategy

7c10b05

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix wrong parameter

9f9a8cf

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix prague acceptance tests

ce1e7b9

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Refactoring plus testing

a762f79

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix spotless

3b90f1f

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge branch 'main' into eip-2935

1baba67

Add unit tests and fix historicalWindow - 1

223552c

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

merge main

1284c6f

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix header

67d34af

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Gabriel-Trintinalia marked this pull request as ready for review April 26, 2024 14:21

Gabriel-Trintinalia changed the title ~~WIP - EIP-2935~~ EIP-2935 Apr 26, 2024

jflo requested changes Apr 30, 2024

View reviewed changes

Gabriel-Trintinalia added 5 commits May 7, 2024 00:07

Update address and rename to match spec

56c7a7f

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Rename constant

f843e3f

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge main into eip-2935

4ce46e1

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge main into eip-2935

4150b18

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix acceptance tests

5068275

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

jframe reviewed May 7, 2024

View reviewed changes

matkt reviewed May 7, 2024

View reviewed changes

jflo requested changes May 7, 2024

View reviewed changes

Gabriel-Trintinalia and others added 3 commits May 7, 2024 11:02

PR recommendations

c4c0c90

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Fix wrong window

fd24617

Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>

Merge branch 'main' into eip-2935

b409d7f

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

jflo approved these changes May 7, 2024

View reviewed changes

jframe previously requested changes May 8, 2024

View reviewed changes

...m/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/HistoricalBlockHashProcessor.java Show resolved Hide resolved

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

jflo added 3 commits May 8, 2024 12:57

Merge branch 'main' into eip-2935

d1e27c1

adopts pr suggestions, allows for current block number in serve window

540221c

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

needs to duplicate and expose to avoid circular dependency

0292b24

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

shemnon requested changes May 8, 2024

View reviewed changes

nope, handling of current block is correctly zero

1a460c1

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

Gabriel-Trintinalia closed this May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-2935 #6991

EIP-2935 #6991

Gabriel-Trintinalia commented Apr 23, 2024 •

edited

jflo left a comment

jframe May 7, 2024

jflo May 7, 2024

matkt May 7, 2024

jframe May 7, 2024

jflo May 7, 2024

matkt May 7, 2024

Gabriel-Trintinalia May 7, 2024

matkt left a comment •

edited

matkt May 7, 2024

jflo left a comment

jflo May 7, 2024

jflo May 7, 2024

jflo left a comment

shemnon left a comment

shemnon May 8, 2024

shemnon May 9, 2024

shemnon commented May 9, 2024

shemnon commented May 9, 2024

shemnon commented May 9, 2024

Gabriel-Trintinalia commented May 12, 2024

EIP-2935 #6991

EIP-2935 #6991

Conversation

Gabriel-Trintinalia commented Apr 23, 2024 • edited

PR description

jflo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkt left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jflo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jflo left a comment

Choose a reason for hiding this comment

shemnon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shemnon commented May 9, 2024

shemnon commented May 9, 2024

shemnon commented May 9, 2024

Gabriel-Trintinalia commented May 12, 2024

Gabriel-Trintinalia commented Apr 23, 2024 •

edited

matkt left a comment •

edited