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

Dispute-coordinator: fix underflow panic #4557

Merged
merged 3 commits into from
Dec 20, 2021
Merged

Dispute-coordinator: fix underflow panic #4557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7793490
Handle invalid block ranges in ordering provider
slumber Dec 17, 2021
43df86e
Merge remote-tracking branch 'origin/master' into slumber-ordering-pr…
slumber Dec 18, 2021
0ac80cc
Improve block numbers handling
slumber Dec 18, 2021
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
22 changes: 18 additions & 4 deletions node/core/dispute-coordinator/src/real/ordering/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,12 +256,12 @@ impl OrderingProvider {
) -> Result<Vec<Hash>> {
let mut ancestors = Vec::new();

if self.last_observed_blocks.get(&head).is_some() {
let finalized_block_number = get_finalized_block_number(sender).await?;

if self.last_observed_blocks.get(&head).is_some() || head_number <= finalized_block_number {
return Ok(ancestors)
}

let finalized_block_number = get_finalized_block_number(sender).await?;

loop {
let (tx, rx) = oneshot::channel();
let hashes = {
Expand All @@ -279,7 +279,20 @@ impl OrderingProvider {
rx.await.or(Err(Fatal::ChainApiSenderDropped))?.map_err(Fatal::ChainApi)?
};

let earliest_block_number = head_number - hashes.len() as u32;
let earliest_block_number = match head_number.checked_sub(hashes.len() as u32) {
Some(number) => number,
None => {
// It's assumed that it's impossible to retrieve
// more than N ancestors for block number N.
tracing::error!(
target: LOG_TARGET,
"Received {} ancestors for block number {} from Chain API",
hashes.len(),
head_number,
);
return Ok(ancestors)
},
Copy link
Member

Choose a reason for hiding this comment

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

It is a bit weird to return Ok for an error in a function returning Result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This branch is considered to be unreachable, so we log this at error level and return valid ancestors that we managed to fetch.
It does feel weird but still better than introducing an error variant for the match arm that can only happen in case of a bug in chain API

Copy link
Member

Choose a reason for hiding this comment

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

We actually have a quite a lot of those (errors that should actually be unreachable), but you have a point.

};
Comment on lines +282 to +295
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing saturated_sub is bad since we will have invalid block number for the next iteration in case of underflow, so it's best to assume this is unreachable.

// The reversed order is parent, grandparent, etc. excluding the head.
let block_numbers = (earliest_block_number..head_number).rev();

Expand All @@ -295,6 +308,7 @@ impl OrderingProvider {

ancestors.push(*hash);
}

match hashes.last() {
Some(last_hash) => {
head = *last_hash;
Expand Down
12 changes: 5 additions & 7 deletions node/core/dispute-coordinator/src/real/ordering/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,11 @@ impl TestState {
let chain = vec![get_block_number_hash(0)];

let finalized_block_number = 0;
let expected_ancestry_len = 1;
let overseer_fut = overseer_process_active_leaves_update(
&mut ctx_handle,
&chain,
finalized_block_number,
expected_ancestry_len,
);
let overseer_fut = async {
assert_finalized_block_number_request(&mut ctx_handle, finalized_block_number).await;
// No requests for ancestors since the block is already finalized.
assert_candidate_events_request(&mut ctx_handle, &chain).await;
};

let ordering_provider =
join(OrderingProvider::new(ctx.sender(), leaf.clone()), overseer_fut)
Expand Down