Dispute-coordinator: fix underflow panic #4557
Conversation
slumber
added
F1-panic
A0-please_review
| if idx == 0 { | ||
| // Return zero hash for the genesis block -- the same way | ||
| // Chain API does. | ||
| vec![Hash::zero()] |
There was a problem hiding this comment.
I am not convinced that this is sane. What would we ever do withthe zero hash? The genesis should not return ancestors, not a garbage one. The bug is in ancestry.
There was a problem hiding this comment.
I agree, although a check that we won't ever underflow at genesis is not a bad idea.
| if idx == 0 { | ||
| // Return zero hash for the genesis block -- the same way | ||
| // Chain API does. | ||
| vec![Hash::zero()] |
There was a problem hiding this comment.
I agree, although a check that we won't ever underflow at genesis is not a bad idea.
|
is this obsolete by fixing the chain-api instead? |
It's a good thing to check for underflow anyway, also could use this branch to add some early returns when looking for ancestors is not necessary. |
| 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) | ||
| }, | ||
| }; |
There was a problem hiding this comment.
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.
| head_number, | ||
| ); | ||
| return Ok(ancestors) | ||
| }, |
There was a problem hiding this comment.
It is a bit weird to return Ok for an error in a function returning Result.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We actually have a quite a lot of those (errors that should actually be unreachable), but you have a point.
|
bot merge |
|
Error: Approval by "Project Owners" is only attempted if other means defined in the criteria for merge are not satisfied first. The following errors might have affected the outcome of this attempt:
Merge failed. Check out the criteria for merge. If you're not meeting the approval count, check if the approvers are members of substrateteamleads or core-devs. |
|
bot merge |
|
Waiting for commit status. |
|
Merge cancelled due to error. Error: Checks failed for 0ac80cc |
* master: dispute-coordinator: fix underflow panic (#4557) Bump futures from 0.3.18 to 0.3.19 (#4567) Bump lru from 0.7.0 to 0.7.1 (#4566) Update Polkadot (#4561) Suggest installing graphviz before book building (#4565) [Zombienet] fix test creds (#4562) chain-api: stop ancestors lookup at block #0 (#4560) use v1.0.2 of zombienet (#4553) remove invalid dispute subsystem replace (#4559) Bump hyper from 0.14.15 to 0.14.16 (#4550) Create a README for XCMv2 detailing notable changes (#4059) enable disputes for known chains, except for polkadot (#4464) dispute statements node side limiting (#4541) Bump serde from 1.0.131 to 1.0.132 (#4551) Bump nix from 0.23.0 to 0.23.1 (#4552) Dispute coordinator: look for included candidates in non-finalized chain (#4508)
Resolves #4554
skip check-dependent-cumulus