eip-7709 implement BLOCKHASH opcode from system contract state #7971

lu-pinto · 2024-12-02T23:08:20Z

PR description

Refactor BlockHashLookup to store it in BlockHashProcessor so one can have different implementations for it. These changes are supposedly side-effect free so no changes to integration test assertions are required.
Also BlockHashLookup now takes the Frame so that it can lookup the hash in the system contract storage.
Implementation for EIP-7709 BlockHashLookup is provided but still with no updated costs.

Gas costs update and wiring will be done in the verkle branch.

Fixed Issue(s)

fixes #7811

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

…rom system contract state Signed-off-by: Luis Pinto <[email protected]>

ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/StateTestSubCommand.java

ethereum/evmtool/src/main/java/org/hyperledger/besu/evmtool/T8nExecutor.java

evm/src/main/java/org/hyperledger/besu/evm/blockhash/BlockHashLookup.java

Gabriel-Trintinalia · 2024-12-10T23:24:06Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/ContractBasedBlockHashLookup.java

+    final long currentBlockNumber = blockHeader.getNumber();
+    final long minBlockServe = Math.max(0, currentBlockNumber - blockHashServeWindow);
+    if (blockNumber >= currentBlockNumber || blockNumber < minBlockServe) {
+      LOG.debug("failed to read hash from system account for block {}", blockNumber);


Maybe trace instead of debug?

Sure i can change. What is debug used for then? When this happens there's a problem with the smart contract's code but no consensus issue occurs since all clients should behave like this from spec

Gabriel-Trintinalia · 2024-12-10T23:26:27Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/ContractBasedBlockHashLookup.java

+    final WorldUpdater worldUpdater = frame.getWorldUpdater();
+    Account account = worldUpdater.get(contractAddress);
+    if (account == null) {
+      LOG.error("cannot query system contract {}", contractAddress);


This a valid scenario, correct? In case a network does not have this contract, should we log a warn/trace instead here?

This would be a problem that we are not storing hashes in the system contract storage or we are storing in wrong account. I feel it should be an error as it will cause a consensus issue

Per EIP-7709 The BLOCKHASH opcode semantics remains the same as before.
Then it lists three options:

ONLY if the arg is within the correct BLOCKHASH window, clients can choose to either - do a direct SLOAD from state, or - do a system call to [EIP-2935](https://eips.ethereum.org/EIPS/eip-2935) contract via its get mechanism (caller other than SYSTEM_ADDRESS) or - serve from memory or as per current designs if maintaining requisite history (full clients for e.g.)

None of those depend on if the system contract is deployed, and if there is no deployed system contract we can get get different results if different paths are chosen. Rather than warn/trace I think Fatal should be considered instead as this is attempting to resolve a non-determinstic blockchain.

In normal cases it should not occur as the PragueBlockHashProcessor that stores hashes in system contract storage should ship with Pectra and populate the contract storage within a day or so, but I would code defensively against it.

serve from memory or as per current designs if maintaining requisite history (full clients for e.g.)

There is always this option though. If I get that sentence right, it mentions that we could fallback to the previous approach of finding hashes from blockchain building. That was something I thought about actually but wasn't sure whether add the previous approach as fallback. But I agree with @shemnon, that this is not totally clear. By fatal, did you mean halting execution if this happens? I would agree with that

Not halting execution, but a level of warning that is not usually muted. Fatal indicates that recovery should not be expected, whereas "error" typically means an error has occured, but a reliable recovery has been done. Fatal also indicates that intervention is generally required.

Decided privately to leave it as is, since there's no higher logging than error and EIP-7709 does not provide clarity on what to do in case the contract or its storage does not exist. Hopefully there will be more clarity when EIP is closer to production.

shemnon

Just a sanity check, this isn't wired into any existing forks yet, right? Is it still just a Verkle thing? Would it be better to work it out in the Verkle branch?

shemnon · 2024-12-16T17:27:36Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/ContractBasedBlockHashLookup.java

+ * Retrieves block hashes from system contract storage and caches hashes by number, used by
+ * BLOCKHASH operation.
+ */
+public class ContractBasedBlockHashLookup implements BlockHashLookup {


The specific semantics may change in the future, and are tightly aligned with the needs of EIP7709BlockHashProcessor. Consider EIP7709BlockHashLookup as a name instead.

shemnon · 2024-12-16T17:34:30Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/ContractBasedBlockHashLookup.java

+    final WorldUpdater worldUpdater = frame.getWorldUpdater();
+    Account account = worldUpdater.get(contractAddress);
+    if (account == null) {
+      LOG.error("cannot query system contract {}", contractAddress);


Per EIP-7709 The BLOCKHASH opcode semantics remains the same as before.
Then it lists three options:

ONLY if the arg is within the correct BLOCKHASH window, clients can choose to either - do a direct SLOAD from state, or - do a system call to [EIP-2935](https://eips.ethereum.org/EIPS/eip-2935) contract via its get mechanism (caller other than SYSTEM_ADDRESS) or - serve from memory or as per current designs if maintaining requisite history (full clients for e.g.)

None of those depend on if the system contract is deployed, and if there is no deployed system contract we can get get different results if different paths are chosen. Rather than warn/trace I think Fatal should be considered instead as this is attempting to resolve a non-determinstic blockchain.

shemnon · 2024-12-16T18:02:25Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/vm/ContractBasedBlockHashLookup.java

+   */
+  @VisibleForTesting
+  ContractBasedBlockHashLookup(
+      final ProcessableBlockHeader currentBlock,


Why pass the processable block header into the constructor? We have the MessageFrame and can get the current block number from that value.

Then we can re-use this object across multiple blocks, and even use it concurrently across multiple blocks.

lu-pinto · 2024-12-17T11:32:37Z

Just a sanity check, this isn't wired into any existing forks yet, right? Is it still just a Verkle thing? Would it be better to work it out in the Verkle branch?

That's right, EIP is still in draft and is a hard dependency for Verkle. Don't think there's any case to have it sooner. We have a Verkle branch but have decided to start bringing stable parts of Verkle into main because of the high amount of conflicts while merging and time it takes to review these merges. But I'll defer to @matkt on that.

matkt · 2024-12-17T14:34:38Z

Just a sanity check, this isn't wired into any existing forks yet, right? Is it still just a Verkle thing? Would it be better to work it out in the Verkle branch?

That's right, EIP is still in draft and is a hard dependency for Verkle. Don't think there's any case to have it sooner. We have a Verkle branch but have decided to start bringing stable parts of Verkle into main because of the high amount of conflicts while merging and time it takes to review these merges. But I'll defer to @matkt on that.

We're trying to merge things step by step into the main branch to reduce the size of the Verkle branch and also minimize futur conflicts, etc. The goal is to avoid having a branch that keeps growing if certain things can be merged without impacting Besu's codebase. Maybe we can add a comment to indicate that it's part of the Verkle work to prevent a future PR from trying to remove it as not used.

Gabriel-Trintinalia

lgtm

Signed-off-by: Luis Pinto <[email protected]>

reimplement blockhashlookup with MessageFrame instead of WorldUpdater Signed-off-by: Luis Pinto <[email protected]>

address review comments Signed-off-by: Luis Pinto <[email protected]>

add comment about unused BlockHashProcessor Signed-off-by: Luis Pinto <[email protected]>

* eip-7709 implement BLOCKHASH opcode from system contract state Signed-off-by: Luis Pinto <[email protected]> * fixup! eip-7709 implement BLOCKHASH opcode from system contract state reimplement blockhashlookup with MessageFrame instead of WorldUpdater Signed-off-by: Luis Pinto <[email protected]> * fixup! eip-7709 implement BLOCKHASH opcode from system contract state address review comments Signed-off-by: Luis Pinto <[email protected]> * fixup! eip-7709 implement BLOCKHASH opcode from system contract state add comment about unused BlockHashProcessor Signed-off-by: Luis Pinto <[email protected]> --------- Signed-off-by: Luis Pinto <[email protected]>

lu-pinto marked this pull request as ready for review December 2, 2024 23:11

lu-pinto force-pushed the eip-7709-implement-blockhash-system-contract branch from 6bb3371 to 04678ff Compare December 4, 2024 11:10

lu-pinto mentioned this pull request Dec 4, 2024

Eip 7709 implement gas costs #7983

Draft

8 tasks

lu-pinto added a commit to lu-pinto/besu that referenced this pull request Dec 10, 2024

[REMOVE AFTER hyperledger#7971] eip-7709 implement BLOCKHASH opcode f…

836137a

…rom system contract state Signed-off-by: Luis Pinto <[email protected]>

lu-pinto force-pushed the eip-7709-implement-blockhash-system-contract branch from 04678ff to e310147 Compare December 10, 2024 14:02

lu-pinto requested a review from Gabriel-Trintinalia December 10, 2024 14:05

Gabriel-Trintinalia reviewed Dec 10, 2024

View reviewed changes

shemnon reviewed Dec 16, 2024

View reviewed changes

lu-pinto force-pushed the eip-7709-implement-blockhash-system-contract branch from e310147 to 71e2965 Compare December 17, 2024 16:44

lu-pinto requested review from Gabriel-Trintinalia and matkt December 17, 2024 16:45

lu-pinto force-pushed the eip-7709-implement-blockhash-system-contract branch from 515bc57 to a4c1b7f Compare December 17, 2024 17:13

Gabriel-Trintinalia approved these changes Dec 19, 2024

View reviewed changes

lu-pinto added 4 commits December 20, 2024 10:02

eip-7709 implement BLOCKHASH opcode from system contract state

cc00332

Signed-off-by: Luis Pinto <[email protected]>

fixup! eip-7709 implement BLOCKHASH opcode from system contract state

3ce9bb1

reimplement blockhashlookup with MessageFrame instead of WorldUpdater Signed-off-by: Luis Pinto <[email protected]>

fixup! eip-7709 implement BLOCKHASH opcode from system contract state

8723494

address review comments Signed-off-by: Luis Pinto <[email protected]>

fixup! eip-7709 implement BLOCKHASH opcode from system contract state

069096f

add comment about unused BlockHashProcessor Signed-off-by: Luis Pinto <[email protected]>

lu-pinto force-pushed the eip-7709-implement-blockhash-system-contract branch from a4c1b7f to 069096f Compare December 20, 2024 10:02

lu-pinto enabled auto-merge (squash) December 20, 2024 10:03

lu-pinto merged commit b3b33da into hyperledger:main Dec 20, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eip-7709 implement BLOCKHASH opcode from system contract state #7971

eip-7709 implement BLOCKHASH opcode from system contract state #7971

lu-pinto commented Dec 2, 2024 •

edited by Gabriel-Trintinalia

Loading

Gabriel-Trintinalia Dec 10, 2024

lu-pinto Dec 16, 2024

Gabriel-Trintinalia Dec 10, 2024

lu-pinto Dec 16, 2024

shemnon Dec 16, 2024

lu-pinto Dec 17, 2024

shemnon Dec 17, 2024

lu-pinto Dec 17, 2024

shemnon left a comment

shemnon Dec 16, 2024

shemnon Dec 16, 2024

shemnon Dec 16, 2024

lu-pinto commented Dec 17, 2024

matkt commented Dec 17, 2024

Gabriel-Trintinalia left a comment

eip-7709 implement BLOCKHASH opcode from system contract state #7971

eip-7709 implement BLOCKHASH opcode from system contract state #7971

Conversation

lu-pinto commented Dec 2, 2024 • edited by Gabriel-Trintinalia Loading

PR description

Fixed Issue(s)

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

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

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

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

Choose a reason for hiding this comment

lu-pinto commented Dec 17, 2024

matkt commented Dec 17, 2024

Gabriel-Trintinalia left a comment

Choose a reason for hiding this comment

lu-pinto commented Dec 2, 2024 •

edited by Gabriel-Trintinalia

Loading