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

Update EIP-2935: Move to Draft #8166

Merged
merged 20 commits into from
Apr 20, 2024
Merged

Conversation

gballet
Copy link
Member

@gballet gballet commented Feb 5, 2024

This heretofore stagnant eip is now necessary for stateless clients to be able to execute the BLOCKHASH opcode. This PR:

  • moves the eip from a stagnant status to a draft status
  • change how the PR is activated since forks are now timestamp-based
  • adds additional history persist step on fork activation so that new retrieval logic can be transitioned to from first block itself

@gballet gballet requested a review from eth-bot as a code owner February 5, 2024 12:11
@github-actions github-actions bot added c-status Changes a proposal's status s-draft This EIP is a Draft t-core labels Feb 5, 2024
@eth-bot
Copy link
Collaborator

eth-bot commented Feb 5, 2024

✅ All reviewers have approved.

@eth-bot eth-bot added the a-review Waiting on author to review label Feb 5, 2024
@eth-bot eth-bot changed the title verkle-friendly eip2935 Update EIP-2935: Move to Draft Feb 5, 2024
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Feb 5, 2024
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Feb 6, 2024
g11tech
g11tech previously approved these changes Feb 6, 2024
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm pending previous authors' approval

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Great updates, I left some comments

EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
Co-authored-by: Jochem Brouwer <[email protected]>
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
EIPS/eip-2935.md Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 9, 2024

The commit 8e4a928 (as a parent of b82ee83) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Feb 9, 2024
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Feb 9, 2024
EIPS/eip-2935.md Outdated Show resolved Hide resolved
…add eip 158 handling strategy (#3)

* Update the eip to store in ring buffer instead of serial storage and add eip 158 handling strategy

* address feedback

* typo

* more typos

* fix a reference

* further cleanup

* apply feedback

* apply feedback

* fix typo
EIPS/eip-2935.md Outdated

## Specification

| Parameter | Value |
| - | - |
| `FORK_BLKNUM` | TBD |
| `FORK_TIMESTAMP` | TBD |
| `HISTORY_STORAGE_ADDRESS` | `0xfffffffffffffffffffffffffffffffffffffffe`|
Copy link
Contributor

Choose a reason for hiding this comment

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

This conflicts with the system address from EIP-4788.

if block.parent.timestamp < FORK_TIMESTAMP:
ancestor = block.parent
for i in range(HISTORY_SERVE_WINDOW - 1):
# stop at genesis block
Copy link
Member Author

Choose a reason for hiding this comment

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

The wording here is confusing, I suggest the following:

Suggested change
# stop at genesis block
# if ancestor.number == 0, this means we inserted the
# genesis block hash in the previous iteration. Stop here.

Comment on lines +51 to +52
ancestor = ancestor.parent
state.insert_slot(HISTORY_STORAGE_ADDRESS, ancestor.number % HISTORY_SERVE_WINDOW, ancestor.hash)
Copy link
Contributor

@g11tech g11tech Apr 9, 2024

Choose a reason for hiding this comment

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

Suggested change
ancestor = ancestor.parent
state.insert_slot(HISTORY_STORAGE_ADDRESS, ancestor.number % HISTORY_SERVE_WINDOW, ancestor.hash)
state.insert_slot(HISTORY_STORAGE_ADDRESS, (ancestor.number - 1) % HISTORY_SERVE_WINDOW, ancestor.parent.hash)
ancestor = ancestor.parent

Copy link
Contributor

Choose a reason for hiding this comment

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

this makes it look more analogous to L41

Copy link
Member Author

@gballet gballet Apr 9, 2024

Choose a reason for hiding this comment

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

actually, I think we should just rewrite it more clearly:

for i in range(block.number-1, min(block.number-HISTORY_SERVE_WINDOW, 0), -1):
    state.insert_slot(HISTORY_STORAGE_ADDRESS, i % HISTORY_SERVE_WINDOW, ancestor.parent.hash)
    ancestor = ancestor.parent

then, the way it's implemented is left to the implementer.

Copy link
Contributor

@g11tech g11tech Apr 9, 2024

Choose a reason for hiding this comment

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

i think this is not correct : storing ancestor's parent at i since 'i` is ancestor as per the logic

or we shouldn't use i, just ancestor.number -1 since that relationship is more clear (we always store ancestor's parent)

    state.insert_slot(HISTORY_STORAGE_ADDRESS, (ancestor.number -1) % HISTORY_SERVE_WINDOW, ancestor.parent.hash)

…ional sload gas (#4)

* extend history retention to 8192 and modify blockhash to charge additional sload gas

* title

* fix cleanup

* add missing contributor names

Co-authored-by: Ignacio Hagopian <[email protected]>

* further update on discussion

* update

* update

* update

* add contract assembley

* apply feedback

* add helpful comments

* fix the comment vals

* update the assembely as per feedback

* improv

* add and generate deployment tx, inputdata and sender/contract address

---------

Co-authored-by: Guillaume Ballet <[email protected]>
Co-authored-by: Ignacio Hagopian <[email protected]>
@lightclient
Copy link
Member

@g11tech @gballet how are the block hashes written to this contract? The assembly I see has no sstore. I think it should follow the 4788 pattern where it is spec'd as the system makes a call as 0xff..ff and the update logic is gated on that.

@eth-bot eth-bot enabled auto-merge (squash) April 20, 2024 02:09
Copy link
Collaborator

@eth-bot eth-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eth-bot eth-bot merged commit d9dbe9c into ethereum:master Apr 20, 2024
11 checks passed
@gballet
Copy link
Member Author

gballet commented Apr 20, 2024

@g11tech @gballet how are the block hashes written to this contract? The assembly I see has no sstore. I think it should follow the 4788 pattern where it is spec'd as the system makes a call as 0xff..ff and the update logic is gated on that.

We already addressed this question during ACD: if anything, the 4788 system should be changed as it unnecessarily adds code chunks to the witness. We do not want to do that, and so the 2935 contract retains its original mode of operation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-review Waiting on author to review c-status Changes a proposal's status s-draft This EIP is a Draft t-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants