Rebase EIP-4844 on Capella by Inphi · Pull Request #3052 · ethereum/consensus-specs

Inphi · 2022-10-20T14:36:55Z

Also adds a "testing" specification that allows withdrawals to be disabled during testing.

This also introduces an `ENABLE_WITHDRAWALS` feature-flag to allow implementers test EIP-4844 without including Capella-specific state changes.

hwwhww · 2022-10-20T16:05:19Z

address #3044

hwwhww

Had a quick scan, @Inphi well done on the rebasing!

Note that #3042 changed the consensus containers & epoch processing functions. So if we include it, you'd need to update that part.
From pyspec's point of view, having ENABLE_WITHDRAWALS is kinda annoying 😬 but I'd defer to client devs if they really want this flag control in spec.

hwwhww · 2022-10-20T16:18:42Z

+    if ENABLE_WITHDRAWALS:  # [New in EIP-4844]
+        for_ops(body.bls_to_execution_changes, process_bls_to_execution_change)


That means we have to replace @with_capella_and_later decorator in tests with @with_phases([CAPELLA]) if we treat ENABLE_WITHDRAWALS == 0 as a constant.

And if we want to have the flexibility to test ENABLE_WITHDRAWALS == 1, we need to write a new decorator to check preset before running the test.

hwwhww · 2022-10-20T16:19:48Z

+Since the `eip4844.BeaconState` format is equal to the `Capella.BeaconState` format, we only have to update `BeaconState.fork`.

 ```python
-def upgrade_to_eip4844(pre: bellatrix.BeaconState) -> BeaconState:
-    # TODO: if Capella gets scheduled, add sync it with Capella.BeaconState
-    epoch = bellatrix.get_current_epoch(pre)
+def upgrade_to_eip4844(pre: Capella.BeaconState) -> BeaconState:


It's lowercase capella for the pyspec library.

terencechain · 2022-10-20T17:45:57Z

Had a quick scan, @Inphi well done on the rebasing!

* Note that #3042 changed the consensus containers & epoch processing functions. So if we include it, you'd need to update that part.

* From pyspec's point of view, having `ENABLE_WITHDRAWALS` is kinda annoying 😬  but I'd defer to client devs if they really want this flag control in spec.

Ditto. I don't think we should enshrine test-only flag in the spec. The decision on whether withdrawal is enabled or in the protocol for testing and testnet can be reached out of the band

Inphi · 2022-10-20T18:04:37Z

The goal is to align the behavior of disabling withdrawals across all clients. ENABLE_WITHDRAWALS will be removed eventually upon further testing. But right now, we're trying to avoid any withdrawal bugs or changes in the spec from influencing 4844.

Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>

realbigsean · 2022-10-20T20:08:47Z

In testnets we can avoid worrying too much about the impact of withdrawals on consensus by stubbing out these functions:

Epoch processing:
-process_full_withdrawals
-process_partial_withdrawals

Block processing:

process_withdrawals
for_ops(body.bls_to_execution_changes)

But I think we can figure this out on a per-testnet basis, by saying "target this spec commit but stub out these functions" rather than ingraining this in the spec here

I think it's simpler if we don't touch anything else, for example:

Actually get the root of an empty withdrawals list
Keep an empty withdrawals list in payload attributes
Use EIP4844 + withdrawal types in all APIs

And remove the ENABLE_WITHDRAWALS feature-flag. The Testing section in the spec has been updated to specify how withdrawals is to be disabled

Inphi · 2022-10-24T16:56:31Z

@realbigsean Thanks for the suggestions. I've made the changes to the spec. ENABLE_WITHDRAWALS has been removed and I've updated the Testing section to specify how clients can disable withdrawals.

Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>

hwwhww · 2022-10-26T15:49:16Z

Marked it "DO NOT MERGE" since we have to swap the no-op functions in pyspec internally + fix tests. I can work on it but I'm pretty afk this week. 🙏

hwwhww

I managed to make withdrawal functions no-op & add basic tests, and find okay Wi-Fi somewhere 2km far from Machu Picchu. 😄

The functionalities/correctness LGTM now!

Edited: I'll defer to client devs to make the decision in testing. Although I personally found that the no-op stub is still somewhat annoying. 😅

Inphi · 2022-11-10T19:32:03Z

I've updated the PR for #3068. Reviewers please take another look.

terencechain

Great work @Inphi ! Just one question

terencechain · 2022-11-11T02:48:43Z

 | `GENESIS_FORK_VERSION`   | `phase0.SignedBeaconBlock` |
 | `ALTAIR_FORK_VERSION`    | `altair.SignedBeaconBlock` |
 | `BELLATRIX_FORK_VERSION` | `bellatrix.SignedBeaconBlock` |
+| `CAPELLA_FORK_VERSION`   | `capella.SignedBeaconBlock`   |


Potential merge conflict with #3089. I can fix that after this is merged

fixed in f1d4c90

terencechain · 2022-11-11T02:49:54Z

+#### `get_payload`

-```python
-def get_blobs_and_kzg_commitments(payload_id: PayloadId) -> Tuple[Sequence[BLSFieldElement], Sequence[KZGCommitment]]:


get_blobs_and_kzg_commitments is still mentioned below, do we want to remove this?

Good catch. This was a bad merge from dev. I've added it back in. Thanks.

hwwhww

lgtm!

I have a test vector generator branch (https://github.com/ethereum/consensus-specs/compare/pr3052-testgen) based on this PR. I'll open a PR once this one is merged.

djrtwo · 2022-11-15T14:14:52Z

 ```
+
+### Disabling Withdrawals
+During testing we avoid Capella-specific updates to the state transition. We do this by replacing the following functions with a no-op implementation:


This no-op works fine for clients?
I'm out of the loop, but I thought they would use the full functionality here (yes, complicating testing)

We want to avoid submitted withdrawals from interfering with the testnet. By no-op'ing these functions, withdrawals won't have any effect.

djrtwo · 2022-11-15T14:19:38Z



-@with_capella_and_later
+@with_phases([CAPELLA])


This should be capella and later, no?

Ah, the no-op change will break this test... If we do this, we need to remember to go back and change these all :/
When we were rebasing here, I didn't realize we were rebasing and mutating. How much will we be blocked on 4844 if we don't do the capella no-op within 4844 spec?

My understanding is to prevent random semi-public testnet users/validators from invoking any withdrawals?

Agreed with this complicating testing but given that we have 3-4 in-discussion changes on Capella, it makes sense that EIP-4844 devs want to disable Capella now... 😭

Btw I also added the tests to test the Capella no-op. So we must undo it altogether.

My understanding is to prevent random semi-public testnet users/validators from invoking any withdrawals?

Yes it's exactly this.

djrtwo

okay, I'm fine with it, assuming that clients can no-op functions for a fork without too much issue.

approved

Rebase Capella on EIP-4844

242e1b7

This also introduces an `ENABLE_WITHDRAWALS` feature-flag to allow implementers test EIP-4844 without including Capella-specific state changes.

timbeiko mentioned this pull request Oct 20, 2022

Consensus-layer Call 97 ethereum/pm#630

Closed

Inphi changed the title ~~Rebase Capella on EIP-4844~~ Rebase EIP-4844 on Capella Oct 20, 2022

hwwhww added the deneb label Oct 20, 2022

terencechain reviewed Oct 20, 2022

View reviewed changes

Comment thread specs/eip4844/beacon-chain.md

hwwhww reviewed Oct 20, 2022

View reviewed changes

Update specs/eip4844/beacon-chain.md

f6f2474

Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>

timbeiko mentioned this pull request Oct 24, 2022

EIP-4844 Implementers' Call #1 ethereum/pm#647

Closed

Inphi added 2 commits October 24, 2022 12:44

Fix py setup

2ac57c7

And remove the ENABLE_WITHDRAWALS feature-flag. The Testing section in the spec has been updated to specify how withdrawals is to be disabled

remove unchanged epoch processing section

0488c0c

Inphi added 2 commits October 24, 2022 16:25

Fix test_process_execution_payload

459310f

Add CAPELLA_FORK_EPOCH overrides

6d270cd

ralexstokes reviewed Oct 25, 2022

View reviewed changes

Comment thread specs/eip4844/beacon-chain.md Outdated

Update specs/eip4844/beacon-chain.md

ca538f5

Co-authored-by: Alex Stokes <r.alex.stokes@gmail.com>

Inphi marked this pull request as ready for review October 25, 2022 16:06

hwwhww added the DO NOT MERGE label Oct 26, 2022

hwwhww added 3 commits October 26, 2022 15:06

Make pyspec disable withdrawal-functions in EIP4844

3172095

Add tests for no-op functions

e460005

Add test_process_withdrawals no-op test

60187e5

hwwhww removed the DO NOT MERGE label Oct 26, 2022

hwwhww approved these changes Oct 26, 2022

View reviewed changes

hwwhww requested a review from ralexstokes October 26, 2022 20:39

dgcoffman mentioned this pull request Nov 1, 2022

EIP-4844 dgcoffman/lodestar#1

Draft

21 of 25 tasks

jessepollak mentioned this pull request Nov 1, 2022

EIP-4844 Implementers' Call #2 ethereum/pm#650

Closed

Inphi added 2 commits November 10, 2022 14:11

remove eip4844 partial/full withdrawwals tests

fcafdc1

remove eip4844 epoch_processing package

67ba28c

Inphi requested review from djrtwo, hwwhww, realbigsean and terencechain and removed request for djrtwo, ralexstokes, realbigsean and terencechain November 10, 2022 19:32

terencechain reviewed Nov 11, 2022

View reviewed changes

replace get_blobs_and_kzg_commitments

104cba0

hwwhww approved these changes Nov 11, 2022

View reviewed changes

ralexstokes reviewed Nov 11, 2022

View reviewed changes

Comment thread specs/eip4844/beacon-chain.md

Inphi added 4 commits November 11, 2022 17:24

rename excess_blobs

6327ffa

excess_data_gas uint256

cd1e113

Merge remote-tracking branch 'origin/dev' into inphi/eip4844-rebase

3df1371

Fix merge conflict

3714446

timbeiko mentioned this pull request Nov 14, 2022

EIP-4844 Implementers' Call #4 ethereum/pm#663

Closed

djrtwo reviewed Nov 15, 2022

View reviewed changes

tbenr mentioned this pull request Nov 16, 2022

Fix latest_execution_payload_header field in eip4844 state upgrade #3106

Merged

hwwhww added 2 commits November 18, 2022 02:12

Merge branch 'dev' into pr3052

f1d4c90

Merge branch 'dev' into pr3052

ee0e2a0

djrtwo approved these changes Nov 17, 2022

View reviewed changes

hwwhww merged commit 78f0e03 into ethereum:dev Nov 17, 2022

hwwhww mentioned this pull request Nov 17, 2022

Rebase 4844 on Capella #3044

Closed

tbenr mentioned this pull request Nov 21, 2022

change excess_blobs: uint64 to excess_data_gas: uint256 Consensys-Incorporated/teku#6479

Closed

protolambda mentioned this pull request Nov 22, 2022

Shanghai Planning ethereum/pm#450

Closed

Inphi deleted the inphi/eip4844-rebase branch February 20, 2024 15:59

		if ENABLE_WITHDRAWALS: # [New in EIP-4844]
		for_ops(body.bls_to_execution_changes, process_bls_to_execution_change)



		@with_capella_and_later
		@with_phases([CAPELLA])

Conversation

Inphi commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwwhww commented Oct 20, 2022

Uh oh!

Uh oh!

hwwhww left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terencechain commented Oct 20, 2022

Uh oh!

Inphi commented Oct 20, 2022

Uh oh!

realbigsean commented Oct 20, 2022

Uh oh!

Inphi commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hwwhww commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwwhww left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Inphi commented Nov 10, 2022

Uh oh!

terencechain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hwwhww left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djrtwo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Inphi commented Oct 20, 2022 •

edited

Loading

Inphi commented Oct 24, 2022 •

edited

Loading

hwwhww commented Oct 26, 2022 •

edited

Loading

hwwhww left a comment •

edited

Loading