Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fixed block response limit check #9692

Merged
3 commits merged into from
Sep 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions client/network/src/block_request_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub fn generate_protocol_config(protocol_id: &ProtocolId) -> ProtocolConfig {
name: generate_protocol_name(protocol_id).into(),
max_request_size: 1024 * 1024,
max_response_size: 16 * 1024 * 1024,
request_timeout: Duration::from_secs(40),
request_timeout: Duration::from_secs(20),
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit off-topic for this PR, but this timeout is too high anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right it was too high in the first place, and since I've also decreased the number of requested blocks the timeout should be readjusted.

inbound_queue: None,
}
}
Expand Down Expand Up @@ -355,7 +355,8 @@ impl<B: BlockT> BlockRequestHandler<B> {
indexed_body,
};

total_size += block_data.body.len();
total_size += block_data.body.iter().map(|ex| ex.len()).sum::<usize>();
total_size += block_data.indexed_body.iter().map(|ex| ex.len()).sum::<usize>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Might this work well as a size() function on block_data?

Copy link
Member Author

Choose a reason for hiding this comment

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

BlockData struct is auto generated from protobuf description. Also, we don't need all of the field to be sized here, just the bodies. The rest do not vary much and should definitely fit in the margin.

blocks.push(block_data);

if blocks.len() >= max_blocks as usize || total_size > MAX_BODY_BYTES {
Expand Down
2 changes: 1 addition & 1 deletion client/network/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ impl<B: BlockT> Protocol<B> {
} else {
None
},
receipt: if !block_data.message_queue.is_empty() {
receipt: if !block_data.receipt.is_empty() {
Some(block_data.receipt)
} else {
None
Expand Down
19 changes: 11 additions & 8 deletions client/network/src/protocol/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ mod state;
mod warp;

/// Maximum blocks to request in a single packet.
const MAX_BLOCKS_TO_REQUEST: usize = 128;
const MAX_BLOCKS_TO_REQUEST: usize = 64;

/// Maximum blocks to store in the import queue.
const MAX_IMPORTING_BLOCKS: usize = 2048;
Expand Down Expand Up @@ -1054,12 +1054,14 @@ impl<B: BlockT> ChainSync<B> {
self.pending_requests.add(who);
if let Some(request) = request {
match &mut peer.state {
PeerSyncState::DownloadingNew(start_block) => {
PeerSyncState::DownloadingNew(_) => {
self.blocks.clear_peer_download(who);
let start_block = *start_block;
peer.state = PeerSyncState::Available;
validate_blocks::<B>(&blocks, who, Some(request))?;
self.blocks.insert(start_block, blocks, who.clone());
if let Some(start_block) =
validate_blocks::<B>(&blocks, who, Some(request))?
{
self.blocks.insert(start_block, blocks, who.clone());
}
self.drain_blocks()
},
PeerSyncState::DownloadingStale(_) => {
Expand Down Expand Up @@ -2315,13 +2317,14 @@ where
}

/// Validate that the given `blocks` are correct.
/// Returns the number of the first block in the sequence.
///
/// It is expected that `blocks` are in asending order.
/// It is expected that `blocks` are in ascending order.
fn validate_blocks<Block: BlockT>(
blocks: &Vec<message::BlockData<Block>>,
who: &PeerId,
request: Option<BlockRequest<Block>>,
) -> Result<(), BadPeer> {
) -> Result<Option<NumberFor<Block>>, BadPeer> {
if let Some(request) = request {
if Some(blocks.len() as _) > request.max {
debug!(
Expand Down Expand Up @@ -2415,7 +2418,7 @@ fn validate_blocks<Block: BlockT>(
}
}

Ok(())
Ok(blocks.first().and_then(|b| b.header.as_ref()).map(|h| *h.number()))
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion client/network/src/protocol/sync/blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl<B: BlockT> BlockCollection<B> {
for r in ranges {
self.blocks.remove(&r);
}
trace!(target: "sync", "Drained {} blocks", drained.len());
trace!(target: "sync", "Drained {} blocks from {:?}", drained.len(), from);
drained
}

Expand Down
29 changes: 29 additions & 0 deletions client/network/test/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1193,3 +1193,32 @@ fn syncs_indexed_blocks() {
.unwrap()
.is_some());
}

#[test]
fn syncs_huge_blocks() {
use sp_core::storage::well_known_keys::HEAP_PAGES;
use sp_runtime::codec::Encode;
use substrate_test_runtime_client::BlockBuilderExt;

sp_tracing::try_init_simple();
let mut net = TestNet::new(2);

// Increase heap space for bigger blocks.
net.peer(0).generate_blocks(1, BlockOrigin::Own, |mut builder| {
builder.push_storage_change(HEAP_PAGES.to_vec(), Some(256u64.encode())).unwrap();
builder.build().unwrap().block
});

net.peer(0).generate_blocks(32, BlockOrigin::Own, |mut builder| {
// Add 32 extrinsics 32k each = 1MiB total
for _ in 0..32 {
let ex = Extrinsic::IncludeData([42u8; 32 * 1024].to_vec());
builder.push(ex).unwrap();
}
builder.build().unwrap().block
});

net.block_until_sync();
assert_eq!(net.peer(0).client.info().best_number, 33);
assert_eq!(net.peer(1).client.info().best_number, 33);
}