Skip to content

Partial processing#4

Merged
realbigsean merged 15 commits intopawanjay176:partial-processingfrom
realbigsean:partial-processing
Mar 24, 2023
Merged

Partial processing#4
realbigsean merged 15 commits intopawanjay176:partial-processingfrom
realbigsean:partial-processing

Conversation

@realbigsean
Copy link
Copy Markdown
Collaborator

@realbigsean realbigsean commented Mar 22, 2023 •
edited
Loading

A couple things I tried to sort out here:

  1. The networking layer tries to match up blocks and blobs very simplistically without doing any consensus checks. The block wrapper was what was used to do this pairing previously. We now use the block wrapper throughout consensus and there's no variant in it to denote "totally unchecked block and blobs". I added this variant. It's useful in sync to delay when we have to do the block vs blob verification.

  2. Related to this, I added a new KzgVerifiedBlob type which is useful to differentiate cached blobs from blobs we get during sync. I also added a IntoKzgVerifiedBlobs trait to make sure we can construct an AvailableBlock from either a Vec<KzgVerifiedBlob> (blobs cached and previously verified) or Vec<BlobSidecar> (sync or local so we can verify all blobs in the block at once).

  3. The BlockWrapper being nested inside the ExecutedBlock was causing issues with de contstructing and reconstructing the same object but altering the availability status of the BlockWrapper (the same is also true of ExecutionPendingBlock but I haven't touched that one.. yet). So I restructured the ExecutedBlock a bit, not sure if I like it but it made some things easier. I also wanted to gain back the AvailableBlock type safety on whatever we return from the blob cache. Hence creating the AvailableExecutedBlock. I kind of want to see if we can simplify these types still though.

Random note:
The AvailableBlockInner doesn’t seem to be serving a purpose anymore because all the fields of AvailableBlock are private now. 
 But we now have VerifiedBlobs::Available(BlobSidecarList<T>) which maybe should have an VerifiedBlobSidecarListInner type (because enum fields are always mutable and since this has been fully verified this should not be). Not sure on this one either but just something to think about.

@realbigsean
Copy link
Copy Markdown
Collaborator Author

realbigsean commented Mar 24, 2023

I think I resolved all this stuff I was mentioning yesterday.

  1. Moved the da_check_fn logic to the DataAvailabilityChecker
  2. Changed the ExecutedBlock into an enum
pub enum ExecutedBlock<E: EthSpec> {
    Available(AvailableExecutedBlock<E>),
    AvailabilityPending(AvailabilityPendingExecutedBlock<E>),
}

The reason for this being that we can now entirely circumvent any cache logic during block processing if we have an available block. Our cache will only accept blocks with type AvailabilityPendingExecutedBlock. This reduces the amount of scenarios we need to handle in the caching logic.

  1. I split the BlockWrapper into two types. This is more similar to what we had pre-decoupling. I think what you had originally with the BlockWrapper type is the way to go generally . Where we only have two variants, one denoting a fully available block and one denoting a block that we have checked whether or not we should have blocks and made it available if possible. I renamed this type to MaybeAvailableBlock though. And created a simple BlockWrapper for use in the networking layer that makes no claims whatsoever about consensus.
pub enum BlockWrapper<E: EthSpec> {
    Block(Arc<SignedBeaconBlock<E>>),
    BlockAndBlobs(Arc<SignedBeaconBlock<E>>, BlobSidecarList<E>),
}

The reason for also having this is to keep consensus logic out of networking and also because it keeps us from having unsafe type conversions while also reducing the amount of test-refactoring we have to do. We previously had this which circumvents checks around whether a block should have blobs, making the information provided by the variant less reliable:

impl <E: EthSpec> From<Arc<SignedBeaconBlock<E>>> for BlockWrapper<E> {
    fn from(b: Arc<SignedBeaconBlock<E>>) -> Self {
        BlockWrapper::AvailabilityPending(block)
    }
}

But removing this would have been a refactoring PITA. So by splitting up the type we can do conversions to the unsafe type in tests and put off converting to the safe type later in processing.

@realbigsean realbigsean merged commit aa10943 into pawanjay176:partial-processing Mar 24, 2023
pawanjay176 added a commit that referenced this pull request Mar 27, 2023
@pawanjay176 @realbigsean
Rework block processing (sigp#4092)
b276af9 
* introduce availability pending block

* add intoavailableblock trait

* small fixes

* add 'gossip blob cache' and start to clean up processing and transition types

* shard memory blob cache

* Initial commit

* Fix after rebase

* Add gossip verification conditions

* cache cleanup

* general chaos

* extended chaos

* cargo fmt

* more progress

* more progress

* tons of changes, just tryna compile

* everything, everywhere, all at once

* Reprocess an ExecutedBlock on unavailable blobs

* Add sus gossip verification for blobs

* Merge stuff

* Remove reprocessing cache stuff

* lint

* Add a wrapper to allow construction of only valid `AvailableBlock`s

* rename blob arc list to blob list

* merge cleanuo

* Revert "merge cleanuo"

This reverts commit 5e98326.

* Revert "Revert "merge cleanuo""

This reverts commit 3a40094.

* fix rpc methods

* move beacon block and blob to eth2/types

* rename gossip blob cache to data availability checker

* lots of changes

* fix some compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* cargo fmt

* use a common data structure for block import types

* fix availability check on proposal import

* refactor the blob cache and split the block wrapper into two types

* add type conversion for signed block and block wrapper

* fix beacon chain tests and do some renaming, add some comments

* Partial processing (#4)

* move beacon block and blob to eth2/types

* rename gossip blob cache to data availability checker

* lots of changes

* fix some compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* fix compilation issues

* cargo fmt

* use a common data structure for block import types

* fix availability check on proposal import

* refactor the blob cache and split the block wrapper into two types

* add type conversion for signed block and block wrapper

* fix beacon chain tests and do some renaming, add some comments

* cargo update (#6)

---------

Co-authored-by: realbigsean <sean@sigmaprime.io>
Co-authored-by: realbigsean <seananderson33@gmail.com>
@realbigsean realbigsean deleted the partial-processing branch November 21, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@realbigsean