Repository navigation
Devnet6 #4404
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
Devnet6 #4404
Changes from 154 commits
8d80200
9642ec0
b78a6e8
8403402
a17405a
22694d1
6f12df3
38e0994
66e09f4
25ff6e8
2f8c885
8618c30
195d802
0ad9fdf
374ec48
c714249
0cc1704
bacec52
3e854ae
d8ed811
b2a4c4e
269122f
03de817
93df0f5
381044a
b6531aa
274aba9
0560b7d
1d18756
76c09de
91594ad
b8708e3
53c0356
9c2acee
b5440f7
14e5bfa
ff58698
83c3ee1
46a9b3a
4390036
69e5e00
13f7217
d850788
0978848
2cad638
3c9fb78
7614abf
f226721
8e9c98e
d224fce
eadf293
185b761
a9b24bb
bfb5242
93bcd62
8edefb7
56b2365
e3f4218
af15789
f5facd6
a0f6159
6aff52c
51c4506
542e3c1
4dd1184
bef63e4
c7aad77
c59a3fc
8ea98c5
98d7a10
944c973
dc20155
4c17993
2bcdfc6
4ba3849
8c5c92b
f31c551
28b92cd
e503c8e
e617486
844920c
4b687c9
e6b0754
6970f7a
ee9578f
584eb6e
5d73a9b
ad422c3
c9727ae
185aecb
390b440
9359371
8f81558
7758e06
2809a84
1391905
8da95ac
0289804
59415cc
28576c9
9fa67cc
5f7610d
da25512
5fda708
b465530
069d13d
1ddd335
30fe57e
1f611fe
bb07e34
7478e75
d656d4b
7461731
453e038
c39ae51
61b1a11
a69b4af
ef8630c
2a1ea23
2d5a256
59df842
3b84894
db20e2e
52ed9ab
36309d3
b61772e
1c240fc
9a8f932
17592e7
4dd2130
5a2026a
7887d40
74a3c19
147d8a5
4553b40
e241bc8
238fa10
29b7737
db4d597
dfc46a4
97d5e90
602342f
ae5d355
18af05a
5a7b761
c81ef8c
57f55d5
03524ec
2259190
8c9cd29
49528c4
d12db6f
923baee
372f97f
3469283
bde5987
a515fd9
7d8106a
c2e045c
c9c49e6
b24cf80
32a4e7e
89306dd
afc4290
e51ad1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,8 @@ use state_processing::state_advance::partial_state_advance; | |
| use std::sync::Arc; | ||
|
|
||
| use crate::beacon_chain::{ | ||
| BeaconChain, BeaconChainTypes, MAXIMUM_GOSSIP_CLOCK_DISPARITY, | ||
| VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT, | ||
| BeaconChain, BeaconChainTypes, BLOCK_PROCESSING_CACHE_LOCK_TIMEOUT, | ||
| MAXIMUM_GOSSIP_CLOCK_DISPARITY, VALIDATOR_PUBKEY_CACHE_LOCK_TIMEOUT, | ||
| }; | ||
| use crate::data_availability_checker::{ | ||
| AvailabilityCheckError, AvailabilityPendingBlock, AvailableBlock, | ||
|
|
@@ -20,9 +20,9 @@ use ssz_types::FixedVector; | |
| use std::borrow::Cow; | ||
| use types::blob_sidecar::{BlobIdentifier, FixedBlobSidecarList}; | ||
| use types::{ | ||
| BeaconBlockRef, BeaconState, BeaconStateError, BlobSidecar, ChainSpec, CloneConfig, Epoch, | ||
| EthSpec, FullPayload, Hash256, KzgCommitment, RelativeEpoch, SignedBeaconBlock, | ||
| SignedBeaconBlockHeader, SignedBlobSidecar, Slot, | ||
| BeaconBlockRef, BeaconState, BeaconStateError, BlobSidecar, BlobSidecarList, ChainSpec, | ||
| CloneConfig, Epoch, EthSpec, FullPayload, Hash256, KzgCommitment, RelativeEpoch, | ||
| SignedBeaconBlock, SignedBeaconBlockHeader, SignedBlobSidecar, Slot, | ||
| }; | ||
|
|
||
| #[derive(Debug)] | ||
|
|
@@ -240,36 +240,72 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>( | |
| "block_root" => %block_root, | ||
| "index" => %blob_index, | ||
| ); | ||
| // The cached head state is in the same epoch as the blob or the state has already been | ||
| // advanced to the blob's epoch | ||
| let snapshot = &chain.canonical_head.cached_head().snapshot; | ||
| if snapshot.beacon_state.current_epoch() == blob_slot.epoch(T::EthSpec::slots_per_epoch()) { | ||
| ( | ||
| snapshot | ||
| .beacon_state | ||
| .get_beacon_proposer_index(blob_slot, &chain.spec)?, | ||
| snapshot.beacon_state.fork(), | ||
| ) | ||
| if let Some(mut snapshot) = chain | ||
| .snapshot_cache | ||
| .try_read_for(BLOCK_PROCESSING_CACHE_LOCK_TIMEOUT) | ||
| .and_then(|snapshot_cache| { | ||
| snapshot_cache.get_cloned(block_parent_root, CloneConfig::committee_caches_only()) | ||
| }) | ||
| { | ||
| if snapshot.beacon_state.slot() == blob_slot { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering why we need to clone the state if we don't need to advance it in this case.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tree states would end up making this a lot simpler... i'm not really sure how we should be handling this here.. |
||
| debug!( | ||
| chain.log, | ||
| "Cloning snapshot cache state for blob verification"; | ||
| "block_root" => %block_root, | ||
| "index" => %blob_index, | ||
| ); | ||
| ( | ||
| snapshot | ||
| .beacon_state | ||
| .get_beacon_proposer_index(blob_slot, &chain.spec)?, | ||
| snapshot.beacon_state.fork(), | ||
| ) | ||
| } else { | ||
| debug!( | ||
| chain.log, | ||
| "Cloning and advancing snapshot cache state for blob verification"; | ||
| "block_root" => %block_root, | ||
| "index" => %blob_index, | ||
| ); | ||
| let state = cheap_state_advance_to_obtain_committees( | ||
| &mut snapshot.beacon_state, | ||
| Some(snapshot.beacon_block_root), | ||
| blob_slot, | ||
| &chain.spec, | ||
| )?; | ||
| ( | ||
| state.get_beacon_proposer_index(blob_slot, &chain.spec)?, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we prime the proposer cache here as well?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we should... I'm wondering if we should separate this state-loading issue into a new task/pr.. what's here should work but is probably pretty suboptimal in practice. I'm not sure we should block merging this on this
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. |
||
| state.fork(), | ||
| ) | ||
| } | ||
| } | ||
| // Need to advance the state to get the proposer index | ||
| else { | ||
| // Reaching this condition too often might be an issue since we could theoretically have | ||
| // 5 threads (4 blob indices + 1 block) cloning the state. | ||
| // We shouldn't be seeing this condition a lot because we try to advance the state | ||
| // 3 seconds before the start of a slot. However, if this becomes an issue during testing, we should | ||
| // consider sending a blob for reprocessing to reduce the number of state clones. | ||
| warn!( | ||
| chain.log, | ||
| "Cached head not advanced for blob verification"; | ||
| "Snapshot cache miss for blob verification"; | ||
| "block_root" => %block_root, | ||
| "index" => %blob_index, | ||
| "action" => "contact the devs if you see this msg too often" | ||
| ); | ||
| // The state produced is only valid for determining proposer/attester shuffling indices. | ||
| let mut cloned_state = snapshot.clone_with(CloneConfig::committee_caches_only()); | ||
|
|
||
| let parent_block = chain | ||
| .get_blinded_block(&block_parent_root) | ||
| .map_err(BlobError::BeaconChainError)? | ||
| .ok_or_else(|| { | ||
| BlobError::from(BeaconChainError::MissingBeaconBlock(block_parent_root)) | ||
| })?; | ||
|
|
||
| let mut parent_state = chain | ||
| .get_state(&parent_block.state_root(), Some(parent_block.slot()))? | ||
| .ok_or_else(|| { | ||
| BeaconChainError::DBInconsistent(format!( | ||
| "Missing state {:?}", | ||
| parent_block.state_root() | ||
| )) | ||
| })?; | ||
| let state = cheap_state_advance_to_obtain_committees( | ||
| &mut cloned_state.beacon_state, | ||
| None, | ||
| &mut parent_state, | ||
| Some(parent_block.state_root()), | ||
| blob_slot, | ||
| &chain.spec, | ||
| )?; | ||
|
|
@@ -449,8 +485,9 @@ impl<T: EthSpec> KzgVerifiedBlob<T> { | |
| /// Returns an error if the kzg verification check fails. | ||
| pub fn verify_kzg_for_blob<T: EthSpec>( | ||
| blob: Arc<BlobSidecar<T>>, | ||
| kzg: &Kzg, | ||
| kzg: &Kzg<T::Kzg>, | ||
| ) -> Result<KzgVerifiedBlob<T>, AvailabilityCheckError> { | ||
| let _timer = crate::metrics::start_timer(&crate::metrics::KZG_VERIFICATION_SINGLE_TIMES); | ||
| //TODO(sean) remove clone | ||
| if validate_blob::<T>(kzg, blob.blob.clone(), blob.kzg_commitment, blob.kzg_proof) | ||
| .map_err(AvailabilityCheckError::Kzg)? | ||
|
|
@@ -468,8 +505,9 @@ pub fn verify_kzg_for_blob<T: EthSpec>( | |
| /// in a loop since this function kzg verifies a list of blobs more efficiently. | ||
| pub fn verify_kzg_for_blob_list<T: EthSpec>( | ||
| blob_list: Vec<Arc<BlobSidecar<T>>>, | ||
| kzg: &Kzg, | ||
| kzg: &Kzg<T::Kzg>, | ||
| ) -> Result<KzgVerifiedBlobList<T>, AvailabilityCheckError> { | ||
| let _timer = crate::metrics::start_timer(&crate::metrics::KZG_VERIFICATION_BATCH_TIMES); | ||
| let (blobs, (commitments, proofs)): (Vec<_>, (Vec<_>, Vec<_>)) = blob_list | ||
| .clone() | ||
| .into_iter() | ||
|
|
@@ -612,6 +650,15 @@ pub enum BlockWrapper<E: EthSpec> { | |
| } | ||
|
|
||
| impl<E: EthSpec> BlockWrapper<E> { | ||
| pub fn new(block: Arc<SignedBeaconBlock<E>>, blobs: Option<BlobSidecarList<E>>) -> Self { | ||
| match blobs { | ||
| Some(blobs) => { | ||
| let blobs = FixedVector::from(blobs.into_iter().map(Some).collect::<Vec<_>>()); | ||
| BlockWrapper::BlockAndBlobs(block, blobs) | ||
| } | ||
| None => BlockWrapper::Block(block), | ||
| } | ||
| } | ||
| pub fn deconstruct(self) -> (Arc<SignedBeaconBlock<E>>, Option<FixedBlobSidecarList<E>>) { | ||
| match self { | ||
| BlockWrapper::Block(block) => (block, None), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand why the
snapshot_cacheis better than the cacnonical head in this case. Is it because it has less lock contention?P.S. I don't remember why I used the
cached_headin the first place 😂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to make sure we are getting shuffling based on the parent state of this blob we're verifying, and our cached head might not be the one this blob builds on, but we still need to verify it. So we could use the cached head and fall back to the snapshot cache if the cached head isn't what the blob builds on but I figured this was simpler.