Skip to content

Commit be68dd2

Browse files
dappliondapplion
authored
Fix wrong custody column count for lookup blocks (#7281)
Fixes - #7278 Don't assume 0 columns for `RpcBlockInner::Block`
1 parent 70f8ab9 commit be68dd2

File tree

13 files changed

+98
-76
lines changed

13 files changed

+98
-76
lines changed

‎beacon_node/beacon_chain/src/block_verification.rs‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1267,40 +1267,6 @@ impl<T: BeaconChainTypes> IntoExecutionPendingBlock<T> for SignatureVerifiedBloc
12671267
}
12681268
}
12691269

1270-
impl<T: BeaconChainTypes> IntoExecutionPendingBlock<T> for Arc<SignedBeaconBlock<T::EthSpec>> {
1271-
/// Verifies the `SignedBeaconBlock` by first transforming it into a `SignatureVerifiedBlock`
1272-
/// and then using that implementation of `IntoExecutionPendingBlock` to complete verification.
1273-
fn into_execution_pending_block_slashable(
1274-
self,
1275-
block_root: Hash256,
1276-
chain: &Arc<BeaconChain<T>>,
1277-
notify_execution_layer: NotifyExecutionLayer,
1278-
) -> Result<ExecutionPendingBlock<T>, BlockSlashInfo<BlockError>> {
1279-
// Perform an early check to prevent wasting time on irrelevant blocks.
1280-
let block_root = check_block_relevancy(&self, block_root, chain)
1281-
.map_err(|e| BlockSlashInfo::SignatureNotChecked(self.signed_block_header(), e))?;
1282-
let maybe_available = chain
1283-
.data_availability_checker
1284-
.verify_kzg_for_rpc_block(RpcBlock::new_without_blobs(Some(block_root), self.clone()))
1285-
.map_err(|e| {
1286-
BlockSlashInfo::SignatureNotChecked(
1287-
self.signed_block_header(),
1288-
BlockError::AvailabilityCheck(e),
1289-
)
1290-
})?;
1291-
SignatureVerifiedBlock::check_slashable(maybe_available, block_root, chain)?
1292-
.into_execution_pending_block_slashable(block_root, chain, notify_execution_layer)
1293-
}
1294-
1295-
fn block(&self) -> &SignedBeaconBlock<T::EthSpec> {
1296-
self
1297-
}
1298-
1299-
fn block_cloned(&self) -> Arc<SignedBeaconBlock<T::EthSpec>> {
1300-
self.clone()
1301-
}
1302-
}
1303-
13041270
impl<T: BeaconChainTypes> IntoExecutionPendingBlock<T> for RpcBlock<T::EthSpec> {
13051271
/// Verifies the `SignedBeaconBlock` by first transforming it into a `SignatureVerifiedBlock`
13061272
/// and then using that implementation of `IntoExecutionPendingBlock` to complete verification.

‎beacon_node/beacon_chain/src/block_verification_types.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,14 +103,14 @@ impl<E: EthSpec> RpcBlock<E> {
103103
pub fn new_without_blobs(
104104
block_root: Option<Hash256>,
105105
block: Arc<SignedBeaconBlock<E>>,
106+
custody_columns_count: usize,
106107
) -> Self {
107108
let block_root = block_root.unwrap_or_else(|| get_block_root(&block));
108109

109110
Self {
110111
block_root,
111112
block: RpcBlockInner::Block(block),
112-
// Block has zero columns
113-
custody_columns_count: 0,
113+
custody_columns_count,
114114
}
115115
}
116116

‎beacon_node/beacon_chain/src/test_utils.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2366,7 +2366,7 @@ where
23662366
.blob_kzg_commitments()
23672367
.is_ok_and(|c| !c.is_empty());
23682368
if !has_blobs {
2369-
return RpcBlock::new_without_blobs(Some(block_root), block);
2369+
return RpcBlock::new_without_blobs(Some(block_root), block, 0);
23702370
}
23712371

23722372
// Blobs are stored as data columns from Fulu (PeerDAS)
@@ -2417,7 +2417,7 @@ where
24172417
&self.spec,
24182418
)?
24192419
} else {
2420-
RpcBlock::new_without_blobs(Some(block_root), block)
2420+
RpcBlock::new_without_blobs(Some(block_root), block, 0)
24212421
}
24222422
} else {
24232423
let blobs = blob_items

‎beacon_node/beacon_chain/tests/block_verification.rs‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ fn build_rpc_block(
147147
RpcBlock::new_with_custody_columns(None, block, columns.clone(), columns.len(), spec)
148148
.unwrap()
149149
}
150-
None => RpcBlock::new_without_blobs(None, block),
150+
None => RpcBlock::new_without_blobs(None, block, 0),
151151
}
152152
}
153153

@@ -370,6 +370,7 @@ async fn chain_segment_non_linear_parent_roots() {
370370
blocks[3] = RpcBlock::new_without_blobs(
371371
None,
372372
Arc::new(SignedBeaconBlock::from_block(block, signature)),
373+
harness.sampling_column_count,
373374
);
374375

375376
assert!(
@@ -407,6 +408,7 @@ async fn chain_segment_non_linear_slots() {
407408
blocks[3] = RpcBlock::new_without_blobs(
408409
None,
409410
Arc::new(SignedBeaconBlock::from_block(block, signature)),
411+
harness.sampling_column_count,
410412
);
411413

412414
assert!(
@@ -434,6 +436,7 @@ async fn chain_segment_non_linear_slots() {
434436
blocks[3] = RpcBlock::new_without_blobs(
435437
None,
436438
Arc::new(SignedBeaconBlock::from_block(block, signature)),
439+
harness.sampling_column_count,
437440
);
438441

439442
assert!(
@@ -575,11 +578,16 @@ async fn invalid_signature_gossip_block() {
575578
.into_block_error()
576579
.expect("should import all blocks prior to the one being tested");
577580
let signed_block = SignedBeaconBlock::from_block(block, junk_signature());
581+
let rpc_block = RpcBlock::new_without_blobs(
582+
None,
583+
Arc::new(signed_block),
584+
harness.sampling_column_count,
585+
);
578586
let process_res = harness
579587
.chain
580588
.process_block(
581-
signed_block.canonical_root(),
582-
Arc::new(signed_block),
589+
rpc_block.block_root(),
590+
rpc_block,
583591
NotifyExecutionLayer::Yes,
584592
BlockImportSource::Lookup,
585593
|| Ok(()),
@@ -1541,12 +1549,13 @@ async fn add_base_block_to_altair_chain() {
15411549
));
15421550

15431551
// Ensure that it would be impossible to import via `BeaconChain::process_block`.
1552+
let base_rpc_block = RpcBlock::new_without_blobs(None, Arc::new(base_block.clone()), 0);
15441553
assert!(matches!(
15451554
harness
15461555
.chain
15471556
.process_block(
1548-
base_block.canonical_root(),
1549-
Arc::new(base_block.clone()),
1557+
base_rpc_block.block_root(),
1558+
base_rpc_block,
15501559
NotifyExecutionLayer::Yes,
15511560
BlockImportSource::Lookup,
15521561
|| Ok(()),
@@ -1564,7 +1573,7 @@ async fn add_base_block_to_altair_chain() {
15641573
harness
15651574
.chain
15661575
.process_chain_segment(
1567-
vec![RpcBlock::new_without_blobs(None, Arc::new(base_block))],
1576+
vec![RpcBlock::new_without_blobs(None, Arc::new(base_block), 0)],
15681577
NotifyExecutionLayer::Yes,
15691578
)
15701579
.await,
@@ -1677,12 +1686,13 @@ async fn add_altair_block_to_base_chain() {
16771686
));
16781687

16791688
// Ensure that it would be impossible to import via `BeaconChain::process_block`.
1689+
let altair_rpc_block = RpcBlock::new_without_blobs(None, Arc::new(altair_block.clone()), 0);
16801690
assert!(matches!(
16811691
harness
16821692
.chain
16831693
.process_block(
1684-
altair_block.canonical_root(),
1685-
Arc::new(altair_block.clone()),
1694+
altair_rpc_block.block_root(),
1695+
altair_rpc_block,
16861696
NotifyExecutionLayer::Yes,
16871697
BlockImportSource::Lookup,
16881698
|| Ok(()),
@@ -1700,7 +1710,7 @@ async fn add_altair_block_to_base_chain() {
17001710
harness
17011711
.chain
17021712
.process_chain_segment(
1703-
vec![RpcBlock::new_without_blobs(None, Arc::new(altair_block))],
1713+
vec![RpcBlock::new_without_blobs(None, Arc::new(altair_block), 0)],
17041714
NotifyExecutionLayer::Yes
17051715
)
17061716
.await,
@@ -1761,11 +1771,16 @@ async fn import_duplicate_block_unrealized_justification() {
17611771
// Create two verified variants of the block, representing the same block being processed in
17621772
// parallel.
17631773
let notify_execution_layer = NotifyExecutionLayer::Yes;
1764-
let verified_block1 = block
1774+
let rpc_block = RpcBlock::new_without_blobs(
1775+
Some(block_root),
1776+
block.clone(),
1777+
harness.sampling_column_count,
1778+
);
1779+
let verified_block1 = rpc_block
17651780
.clone()
17661781
.into_execution_pending_block(block_root, chain, notify_execution_layer)
17671782
.unwrap();
1768-
let verified_block2 = block
1783+
let verified_block2 = rpc_block
17691784
.into_execution_pending_block(block_root, chain, notify_execution_layer)
17701785
.unwrap();
17711786

‎beacon_node/beacon_chain/tests/payload_invalidation.rs‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#![cfg(not(debug_assertions))]
22

3+
use beacon_chain::block_verification_types::RpcBlock;
34
use beacon_chain::{
45
canonical_head::{CachedHead, CanonicalHead},
56
test_utils::{BeaconChainHarness, EphemeralHarnessType},
@@ -687,12 +688,14 @@ async fn invalidates_all_descendants() {
687688
assert_eq!(fork_parent_state.slot(), fork_parent_slot);
688689
let ((fork_block, _), _fork_post_state) =
689690
rig.harness.make_block(fork_parent_state, fork_slot).await;
691+
let fork_rpc_block =
692+
RpcBlock::new_without_blobs(None, fork_block.clone(), rig.harness.sampling_column_count);
690693
let fork_block_root = rig
691694
.harness
692695
.chain
693696
.process_block(
694-
fork_block.canonical_root(),
695-
fork_block,
697+
fork_rpc_block.block_root(),
698+
fork_rpc_block,
696699
NotifyExecutionLayer::Yes,
697700
BlockImportSource::Lookup,
698701
|| Ok(()),
@@ -788,12 +791,14 @@ async fn switches_heads() {
788791
let ((fork_block, _), _fork_post_state) =
789792
rig.harness.make_block(fork_parent_state, fork_slot).await;
790793
let fork_parent_root = fork_block.parent_root();
794+
let fork_rpc_block =
795+
RpcBlock::new_without_blobs(None, fork_block.clone(), rig.harness.sampling_column_count);
791796
let fork_block_root = rig
792797
.harness
793798
.chain
794799
.process_block(
795-
fork_block.canonical_root(),
796-
fork_block,
800+
fork_rpc_block.block_root(),
801+
fork_rpc_block,
797802
NotifyExecutionLayer::Yes,
798803
BlockImportSource::Lookup,
799804
|| Ok(()),
@@ -1057,8 +1062,10 @@ async fn invalid_parent() {
10571062
));
10581063

10591064
// Ensure the block built atop an invalid payload is invalid for import.
1065+
let rpc_block =
1066+
RpcBlock::new_without_blobs(None, block.clone(), rig.harness.sampling_column_count);
10601067
assert!(matches!(
1061-
rig.harness.chain.process_block(block.canonical_root(), block.clone(), NotifyExecutionLayer::Yes, BlockImportSource::Lookup,
1068+
rig.harness.chain.process_block(rpc_block.block_root(), rpc_block, NotifyExecutionLayer::Yes, BlockImportSource::Lookup,
10621069
|| Ok(()),
10631070
).await,
10641071
Err(BlockError::ParentExecutionPayloadInvalid { parent_root: invalid_root })
@@ -1380,11 +1387,13 @@ async fn recover_from_invalid_head_by_importing_blocks() {
13801387
} = InvalidHeadSetup::new().await;
13811388

13821389
// Import the fork block, it should become the head.
1390+
let fork_rpc_block =
1391+
RpcBlock::new_without_blobs(None, fork_block.clone(), rig.harness.sampling_column_count);
13831392
rig.harness
13841393
.chain
13851394
.process_block(
1386-
fork_block.canonical_root(),
1387-
fork_block.clone(),
1395+
fork_rpc_block.block_root(),
1396+
fork_rpc_block,
13881397
NotifyExecutionLayer::Yes,
13891398
BlockImportSource::Lookup,
13901399
|| Ok(()),

‎beacon_node/beacon_chain/tests/store_tests.rs‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#![cfg(not(debug_assertions))]
22

33
use beacon_chain::attestation_verification::Error as AttnError;
4+
use beacon_chain::block_verification_types::RpcBlock;
45
use beacon_chain::builder::BeaconChainBuilder;
56
use beacon_chain::data_availability_checker::AvailableBlock;
67
use beacon_chain::schema_change::migrate_schema;
@@ -2643,12 +2644,17 @@ async fn process_blocks_and_attestations_for_unaligned_checkpoint() {
26432644
assert_eq!(split.block_root, valid_fork_block.parent_root());
26442645
assert_ne!(split.state_root, unadvanced_split_state_root);
26452646

2647+
let invalid_fork_rpc_block = RpcBlock::new_without_blobs(
2648+
None,
2649+
invalid_fork_block.clone(),
2650+
harness.sampling_column_count,
2651+
);
26462652
// Applying the invalid block should fail.
26472653
let err = harness
26482654
.chain
26492655
.process_block(
2650-
invalid_fork_block.canonical_root(),
2651-
invalid_fork_block.clone(),
2656+
invalid_fork_rpc_block.block_root(),
2657+
invalid_fork_rpc_block,
26522658
NotifyExecutionLayer::Yes,
26532659
BlockImportSource::Lookup,
26542660
|| Ok(()),
@@ -2658,11 +2664,16 @@ async fn process_blocks_and_attestations_for_unaligned_checkpoint() {
26582664
assert!(matches!(err, BlockError::WouldRevertFinalizedSlot { .. }));
26592665

26602666
// Applying the valid block should succeed, but it should not become head.
2667+
let valid_fork_rpc_block = RpcBlock::new_without_blobs(
2668+
None,
2669+
valid_fork_block.clone(),
2670+
harness.sampling_column_count,
2671+
);
26612672
harness
26622673
.chain
26632674
.process_block(
2664-
valid_fork_block.canonical_root(),
2665-
valid_fork_block.clone(),
2675+
valid_fork_rpc_block.block_root(),
2676+
valid_fork_rpc_block,
26662677
NotifyExecutionLayer::Yes,
26672678
BlockImportSource::Lookup,
26682679
|| Ok(()),

‎beacon_node/http_api/src/publish_blocks.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use crate::metrics;
22
use std::future::Future;
33

44
use beacon_chain::blob_verification::{GossipBlobError, GossipVerifiedBlob};
5-
use beacon_chain::block_verification_types::AsBlock;
5+
use beacon_chain::block_verification_types::{AsBlock, RpcBlock};
66
use beacon_chain::data_column_verification::{GossipDataColumnError, GossipVerifiedDataColumn};
77
use beacon_chain::validator_monitor::{get_block_delay_ms, timestamp_now};
88
use beacon_chain::{
@@ -302,7 +302,11 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlock<T>>(
302302
);
303303
let import_result = Box::pin(chain.process_block(
304304
block_root,
305-
block.clone(),
305+
RpcBlock::new_without_blobs(
306+
Some(block_root),
307+
block.clone(),
308+
network_globals.custody_columns_count() as usize,
309+
),
306310
NotifyExecutionLayer::Yes,
307311
BlockImportSource::HttpApi,
308312
publish_fn,

‎beacon_node/network/src/network_beacon_processor/tests.rs‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -323,12 +323,22 @@ impl TestRig {
323323
}
324324
}
325325

326+
pub fn custody_columns_count(&self) -> usize {
327+
self.network_beacon_processor
328+
.network_globals
329+
.custody_columns_count() as usize
330+
}
331+
326332
pub fn enqueue_rpc_block(&self) {
327333
let block_root = self.next_block.canonical_root();
328334
self.network_beacon_processor
329335
.send_rpc_beacon_block(
330336
block_root,
331-
RpcBlock::new_without_blobs(Some(block_root), self.next_block.clone()),
337+
RpcBlock::new_without_blobs(
338+
Some(block_root),
339+
self.next_block.clone(),
340+
self.custody_columns_count(),
341+
),
332342
std::time::Duration::default(),
333343
BlockProcessType::SingleBlock { id: 0 },
334344
)
@@ -340,7 +350,11 @@ impl TestRig {
340350
self.network_beacon_processor
341351
.send_rpc_beacon_block(
342352
block_root,
343-
RpcBlock::new_without_blobs(Some(block_root), self.next_block.clone()),
353+
RpcBlock::new_without_blobs(
354+
Some(block_root),
355+
self.next_block.clone(),
356+
self.custody_columns_count(),
357+
),
344358
std::time::Duration::default(),
345359
BlockProcessType::SingleBlock { id: 1 },
346360
)

‎beacon_node/network/src/sync/block_lookups/common.rs‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ use crate::sync::block_lookups::{
66
};
77
use crate::sync::manager::BlockProcessType;
88
use crate::sync::network_context::{LookupRequestResult, SyncNetworkContext};
9-
use beacon_chain::block_verification_types::RpcBlock;
109
use beacon_chain::BeaconChainTypes;
1110
use lighthouse_network::service::api_types::Id;
1211
use parking_lot::RwLock;
@@ -97,13 +96,8 @@ impl<T: BeaconChainTypes> RequestState<T> for BlockRequestState<T::EthSpec> {
9796
seen_timestamp,
9897
..
9998
} = download_result;
100-
cx.send_block_for_processing(
101-
id,
102-
block_root,
103-
RpcBlock::new_without_blobs(Some(block_root), value),
104-
seen_timestamp,
105-
)
106-
.map_err(LookupRequestError::SendFailedProcessor)
99+
cx.send_block_for_processing(id, block_root, value, seen_timestamp)
100+
.map_err(LookupRequestError::SendFailedProcessor)
107101
}
108102

109103
fn response_type() -> ResponseType {

0 commit comments

Comments
 (0)