Skip to content

Implement PeerDAS Fulu fork activation#6795

Merged
mergify[bot] merged 23 commits into
sigp:unstablefrom
jimmygchen:jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch
Jan 30, 2025
Merged

Implement PeerDAS Fulu fork activation#6795
mergify[bot] merged 23 commits into
sigp:unstablefrom
jimmygchen:jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

Conversation

@jimmygchen

@jimmygchen jimmygchen commented Jan 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Issue Addressed

Addresses #6706

Proposed Changes

This PR activates PeerDAS at the Fulu fork epoch instead of EIP_7594_FORK_EPOCH. This means we no longer support testing PeerDAS with Deneb / Electrs, as it's now part of a hard fork.

Additional Info

Apologies for the large diff - it was initially a relatively small change, but ended up being bigger because:

  • Fulu tests were failing since we now activate PeerDAS at the Fulu fork, so I had to refactor test utils to support PeerDAS - the good news is that PeerDAS/Fulu now gets the same test coverage as the other forks \o/
  • The tests revealed a range sync bug where range requests were sent to custody peers selected from the global peer list, rather than peers from the same syncing chain.
  • This PR also changes Fulu to use *_payload_v4 engine methods, as the v5 are not yet implemented, we should revert this commit once we're ready to switch over (I've add TODO(fulu) comments to the changes): 8cdf82e
  • I've also introduced a ForkName::latest_stable() which returns Electra, so we don't run tests on Fulu by default.

@jimmygchen jimmygchen requested a review from jxs as a code owner January 13, 2025 05:17
@jimmygchen jimmygchen added the das Data Availability Sampling label Jan 13, 2025
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from c1b0c91 to 2082c92 Compare January 13, 2025 05:39
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 2082c92 to b029342 Compare January 13, 2025 05:44
@jimmygchen jimmygchen added the ready-for-review The code is ready for review label Jan 13, 2025
jimmygchen
jimmygchen commented Jan 13, 2025
View reviewed changes
Comment thread beacon_node/lighthouse_network/src/rpc/protocol.rs
jimmygchen
jimmygchen commented Jan 13, 2025
View reviewed changes
Comment thread beacon_node/network/src/service.rs
jimmygchen
jimmygchen commented Jan 13, 2025
View reviewed changes
Comment thread testing/ef_tests/src/cases.rs Outdated
@michaelsproul michaelsproul mentioned this pull request Jan 14, 2025
Closed
dapplion
dapplion reviewed Jan 14, 2025
View reviewed changes
Comment thread testing/ef_tests/check_all_files_accessed.py Outdated
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 14, 2025
@jimmygchen jimmygchen requested a review from dapplion January 14, 2025 07:30
@jimmygchen
Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-act…
0c9d64b 
…ivate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

# Conflicts:
#	beacon_node/lighthouse_network/src/rpc/protocol.rs
#	testing/ef_tests/check_all_files_accessed.py
#	testing/ef_tests/src/handler.rs
@jimmygchen
Address review comments and fix lint.
4e25302
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 15, 2025
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch 3 times, most recently from d188403 to 11e9e13 Compare January 15, 2025 04:43
@jimmygchen
Use engine v4 methods for Fulu (v5 methods do not exist yet). Update …
8cdf82e 
…kurtosis config for PeerDAS as electra genesis is not yet supported.
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 11e9e13 to 8cdf82e Compare January 15, 2025 04:57
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 15, 2025
@jimmygchen
Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-act…
4d407fe 
…ivate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

# Conflicts:
#	beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
#	consensus/types/src/chain_spec.rs
#	testing/ef_tests/src/cases.rs
#	testing/ef_tests/src/cases/get_custody_groups.rs
#	testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs
#	testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs
#	testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs
#	testing/ef_tests/src/handler.rs
#	testing/ef_tests/tests/tests.rs
jimmygchen added a commit to jimmygchen/lighthouse that referenced this pull request Jan 23, 2025
@jimmygchen
Implement PeerDAS Fulu fork activation (sigp#6795)
bb335bd 
Squashed commit of the following:

commit b3da74b
Merge: e813532 a1b7d61
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Thu Jan 23 16:11:34 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/fetch_blobs.rs
    #	beacon_node/store/src/lib.rs
    #	beacon_node/store/src/memory_store.rs

commit e813532
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 21 17:44:19 2025 +1100

    Skip blob pruning tests for Fulu.

commit 0e8f671
Merge: 614f984 33c1648
Author: Jimmy Chen <jimmy@sigmaprime.io>
Date:   Tue Jan 21 16:03:53 2025 +1100

    Merge branch 'unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 614f984
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 21 15:59:22 2025 +1100

    Fix range sync to select custody peers from its syncing chain instead of the global peer list.

commit eff9a5b
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 21 10:02:19 2025 +1100

    More test fixes for Fulu.

commit b63a6c4
Merge: b7da075 7a0388e
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 20 23:41:13 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit b7da075
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 20 16:03:36 2025 +1100

    More test fixes for Fulu.

commit 6d5b5ed
Merge: 8980832 06329ec
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Jan 17 12:32:58 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 8980832
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Jan 17 11:41:46 2025 +1100

    Update Fulu spec tests. Revert back to testing Fulu as "feature", because all non-PeerDAS Fulu SSZ types are the same as Electra, and serde deserializes the vectors into Electra types.

commit 4d407fe
Merge: 8cdf82e b1a19a8
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Thu Jan 16 01:05:04 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
    #	consensus/types/src/chain_spec.rs
    #	testing/ef_tests/src/cases.rs
    #	testing/ef_tests/src/cases/get_custody_groups.rs
    #	testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs
    #	testing/ef_tests/src/handler.rs
    #	testing/ef_tests/tests/tests.rs

commit 8cdf82e
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Jan 15 14:31:29 2025 +1100

    Use engine v4 methods for Fulu (v5 methods do not exist yet). Update kurtosis config for PeerDAS as electra genesis is not yet supported.

commit 4e25302
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Jan 15 13:07:43 2025 +1100

    Address review comments and fix lint.

commit 0c9d64b
Merge: 64e44e1 587c3e2
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Jan 15 12:48:27 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/lighthouse_network/src/rpc/protocol.rs
    #	testing/ef_tests/check_all_files_accessed.py
    #	testing/ef_tests/src/handler.rs

commit 64e44e1
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 14 14:45:09 2025 +1100

    Fix failing tests now `fulu` fork is included.

commit b029342
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 13 16:30:34 2025 +1100

    Fix compilation and update Kurtosis test config for PeerDAS.

commit cd77b2c
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 13 16:16:18 2025 +1100

    Update spec tests.

commit 2e11554
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 13 14:45:55 2025 +1100

    Implement PeerDAS Fulu fork activation.
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 98863da to 492c1c6 Compare January 24, 2025 02:14
jimmygchen
jimmygchen commented Jan 24, 2025
View reviewed changes
Comment thread beacon_node/beacon_chain/tests/store_tests.rs
jimmygchen
jimmygchen commented Jan 24, 2025
View reviewed changes
Comment thread beacon_node/http_api/tests/broadcast_validation_tests.rs
jimmygchen added a commit that referenced this pull request Jan 24, 2025
@jimmygchen
Implement PeerDAS Fulu fork activation (#6795)
048bacc 
Squashed commit of the following:

commit e21b31e
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Jan 24 16:43:41 2025 +1100

    More beacon chain test fixes.

commit 492c1c6
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Jan 24 13:00:44 2025 +1100

    Use pre-computed data columns for testing and fix tests.

commit b3da74b
Merge: e813532 a1b7d61
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Thu Jan 23 16:11:34 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/fetch_blobs.rs
    #	beacon_node/store/src/lib.rs
    #	beacon_node/store/src/memory_store.rs

commit e813532
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 21 17:44:19 2025 +1100

    Skip blob pruning tests for Fulu.

commit 0e8f671
Merge: 614f984 33c1648
Author: Jimmy Chen <jimmy@sigmaprime.io>
Date:   Tue Jan 21 16:03:53 2025 +1100

    Merge branch 'unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 614f984
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 21 15:59:22 2025 +1100

    Fix range sync to select custody peers from its syncing chain instead of the global peer list.

commit eff9a5b
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 21 10:02:19 2025 +1100

    More test fixes for Fulu.

commit b63a6c4
Merge: b7da075 7a0388e
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 20 23:41:13 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit b7da075
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 20 16:03:36 2025 +1100

    More test fixes for Fulu.

commit 6d5b5ed
Merge: 8980832 06329ec
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Jan 17 12:32:58 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 8980832
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Fri Jan 17 11:41:46 2025 +1100

    Update Fulu spec tests. Revert back to testing Fulu as "feature", because all non-PeerDAS Fulu SSZ types are the same as Electra, and serde deserializes the vectors into Electra types.

commit 4d407fe
Merge: 8cdf82e b1a19a8
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Thu Jan 16 01:05:04 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
    #	consensus/types/src/chain_spec.rs
    #	testing/ef_tests/src/cases.rs
    #	testing/ef_tests/src/cases/get_custody_groups.rs
    #	testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs
    #	testing/ef_tests/src/handler.rs
    #	testing/ef_tests/tests/tests.rs

commit 8cdf82e
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Jan 15 14:31:29 2025 +1100

    Use engine v4 methods for Fulu (v5 methods do not exist yet). Update kurtosis config for PeerDAS as electra genesis is not yet supported.

commit 4e25302
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Jan 15 13:07:43 2025 +1100

    Address review comments and fix lint.

commit 0c9d64b
Merge: 64e44e1 587c3e2
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Wed Jan 15 12:48:27 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/lighthouse_network/src/rpc/protocol.rs
    #	testing/ef_tests/check_all_files_accessed.py
    #	testing/ef_tests/src/handler.rs

commit 64e44e1
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Tue Jan 14 14:45:09 2025 +1100

    Fix failing tests now `fulu` fork is included.

commit b029342
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 13 16:30:34 2025 +1100

    Fix compilation and update Kurtosis test config for PeerDAS.

commit cd77b2c
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 13 16:16:18 2025 +1100

    Update spec tests.

commit 2e11554
Author: Jimmy Chen <jchen.tc@gmail.com>
Date:   Mon Jan 13 14:45:55 2025 +1100

    Implement PeerDAS Fulu fork activation.
dapplion
dapplion requested changes Jan 25, 2025
View reviewed changes

@dapplion dapplion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fulu changes look good! I would remove the fix for selecting relevant peers in range sync and handle that in a separate PR

Comment thread beacon_node/beacon_chain/src/beacon_chain.rs Outdated
Comment thread beacon_node/beacon_chain/src/beacon_chain.rs
Comment thread beacon_node/beacon_chain/tests/store_tests.rs Outdated
Comment thread beacon_node/network/src/service.rs
Comment thread beacon_node/network/src/sync/network_context.rs Outdated
Comment thread beacon_node/network/src/sync/network_context.rs Outdated
@dapplion dapplion force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 38311f8 to d8cba4b Compare January 28, 2025 14:14
@dapplion

dapplion commented Jan 28, 2025

Copy link
Copy Markdown
Collaborator

I have reverted the change that draws custody peers from the chain's peer set. The test pause_and_resume_on_ee_offline broke as a result.

To fix it I followed a different path than you did initially @jimmygchen. I see why you needed to do the change to only draw peers from the sync chain but that's not the root cause of the failing test.

Let's break down pause_and_resume_on_ee_offline

  • Add peer A and create head chain A
  • Stop engine so nothing is sent for processing
  • Request to download blocks + columns on epoch 0 chain A
  • Complete the requests above, all sent to peer A
  • Chain A sends requests to peer A for epoch 1 chain A
  • Add peer B and create finalized chain B
  • Request to download blocks + columns on epoch 0 chain B - Now because we draw from the global pool there's a high chance that the custody requests will be split between peer A and peer B
  • Complete the requests above, but because we filter requests to peer B the batch download will not be complete
  • Enable engine, and expect batch epoch 0 chain A to be sent for processing (Ok, happens) + expect batch epoch 0 chain B to be sent for processing (failure, it's still on downloading mode)

To fix the tests I have made the find_blocks_by_range_request smarter with a better filter such that I can identify all column requests for a given chain even if split among different peers. We will need this improvement anyway when adding more tests. It's unrealistic for chains to have a single peer and in real networks custody by range requests are split among a lot of peers.

I also had to add an epoch filter as the extra request from chain A for epoch 1 was colliding with the complete attempt for chain B. As we add more tests we can make this filters more robust but I think it's good for now.

@dapplion dapplion self-requested a review January 28, 2025 19:18
jxs
jxs reviewed Jan 29, 2025
View reviewed changes

@jxs jxs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi Jimmy, the network parts look good to me!

@pawanjay176 pawanjay176 added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Jan 30, 2025
pawanjay176
pawanjay176 approved these changes Jan 30, 2025
View reviewed changes

@pawanjay176 pawanjay176 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, mostly nits

Comment thread beacon_node/lighthouse_network/src/rpc/protocol.rs Outdated
Comment thread beacon_node/lighthouse_network/src/rpc/protocol.rs Outdated
Comment thread beacon_node/network/src/service.rs
jimmygchen
jimmygchen commented Jan 30, 2025
View reviewed changes

@jimmygchen jimmygchen left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dapplion your change looks good to me!

@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 5f70c41 to ce8090d Compare January 30, 2025 04:44
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed under-review A reviewer has only partially completed a review. labels Jan 30, 2025
AgeManning
AgeManning approved these changes Jan 30, 2025
View reviewed changes

@AgeManning AgeManning left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pretend review, lgtm

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 30, 2025
mergify Bot added a commit that referenced this pull request Jan 30, 2025
@mergify
Merge of #6795
de98f16
@mergify mergify Bot mentioned this pull request Jan 30, 2025
Closed
10 tasks
@mergify mergify Bot merged commit 70194df into sigp:unstable Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jxs jxs jxs left review comments

@AgeManning AgeManning AgeManning approved these changes

@pawanjay176 pawanjay176 pawanjay176 approved these changes

@dapplion dapplion dapplion approved these changes

@mergify mergify[bot] mergify[bot] approved these changes

Assignees

No one assigned

Labels

das Data Availability Sampling ready-for-merge This PR is ready to merge.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jimmygchen @dapplion @jxs @AgeManning @pawanjay176