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

Add PeerDAS RPC import boilerplate #6238

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Aug 7, 2024

Issue Addressed

Continue porting network changes of das branch. Extends #6224

Part of

Proposed Changes

  • Add a new Work:: RpcCustodyColumn event
  • Wire process function with importing columns into da_checker
  • Introduce new type DataColumnsSameBlock as downstream code assumes that all submitted columns are in the same block. The implementation in das just assumes this to be the case, but this PR makes this assumption explicit via a new type. I changed the gossip import method from accepting N columns to exactly 1, so we can assert that the single column is always part of the same block.

@@ -476,6 +477,24 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
})
}

/// Create a new `Work` event for some custody columns. `process_rpc_custody_columns` reports
/// the result back to sync.
pub fn send_rpc_custody_columns(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Future PR touching sync will consume this function

@jimmygchen jimmygchen changed the base branch from peerdas-network-boilerplate to unstable August 13, 2024 11:53
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed do-not-merge labels Aug 13, 2024
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Just some small feedback but looks good!

beacon_node/beacon_chain/src/beacon_chain.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/data_availability_checker.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/data_column_verification.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/data_column_verification.rs Outdated Show resolved Hide resolved
@dapplion
Copy link
Collaborator Author

@jimmygchen I have reverted the refactor of the *SameBlock types. This PR just extends same style as blobs

Also rebased on unstable to clean the commits :)

Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Looks like my feedback was related to the refactor, just had one nit!

@@ -181,6 +183,38 @@ impl<T: BeaconChainTypes> DataAvailabilityChecker<T> {
.put_kzg_verified_blobs(block_root, epoch, verified_blobs)
}

/// Put a list of custody columns received via RPC into the availability cache. This performs KZG
/// verification on the blobs in the list.
#[allow(clippy::type_complexity)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[allow(clippy::type_complexity)]

i think this is not necessary

@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented Aug 15, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 5169e03

mergify bot added a commit that referenced this pull request Aug 15, 2024
@mergify mergify bot merged commit 5169e03 into unstable Aug 15, 2024
28 checks passed
@mergify mergify bot deleted the peerdas-network-processor branch August 15, 2024 16:00
AgeManning pushed a commit to AgeManning/lighthouse that referenced this pull request Sep 3, 2024
* Add PeerDAS RPC import boilerplate

* revert refactor

* Remove allow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling das-unstable-merge ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants