Fixes/improvements for disputes #3753
Conversation
eskimor
added
A0-please_review
| @@ -444,8 +444,7 @@ where | |||
| let (subchain_number, subchain_head) = rx | |||
| .await | |||
| .map_err(Error::OverseerDisconnected) | |||
| .map_err(|e| ConsensusError::Other(Box::new(e)))? | |||
| .unwrap_or_else(|| (subchain_number, subchain_head)); | |||
There was a problem hiding this comment.
Here we would previously use subchain_number/subchain_head in case we get None from the dispute coordinator.
| @@ -895,6 +895,32 @@ async fn issue_local_statement( | |||
| pending_confirmation, | |||
| ) | |||
| .await?; | |||
| match rx.await { | |||
| Err(_) => { | |||
| tracing::error!( | |||
There was a problem hiding this comment.
I suspect these will get noisy.
Metrics?
There was a problem hiding this comment.
actually no, those should not be noisy. The errors I would not expect to happen at all (hence errors) and the trace also only happens on IssueLocalVote, which should also not be noisy as this only happens on disputes and we will only vote once per dispute, so this should really be very low volume.
eskimor
force-pushed
the
rk-dispute-fixes
branch
from
e2e5ab2
to
e8aae8b
Compare
| @@ -582,12 +582,12 @@ async fn handle_incoming( | |||
| .await?; | |||
| }, | |||
| DisputeCoordinatorMessage::DetermineUndisputedChain { | |||
| base_number, | |||
| base: (base_number, base_hash), | |||
There was a problem hiding this comment.
nit: split this into base_number and base_hash instead on the messages already, base is never used as tuple.
There was a problem hiding this comment.
They really belong together, so I liked the grouping to highlight that relationship.
* master: dependabot: ignore yet another git dep (#3759) Bump serde_json from 1.0.66 to 1.0.67 (#3767) Bump syn from 1.0.74 to 1.0.75 (#3710) Companion for substrate #9371 (#3487) Fixes/improvements for disputes (#3753) chore: test helper arbitrary ordering for 2 (#3762) disputes: fix relay chain selection sanity check (#3750) technical committee is using the weight of council, but should have its own generated weight instead (#3511) new proxy for auctions, crowdloans, registrar, slots (#3683) Bump libc from 0.2.100 to 0.2.101 (#3726) Removed unneeded deps (#3658) Bump serde from 1.0.127 to 1.0.130 (#3739) Companion for Generate storage info for pallet authority_discovery #9428 (#3517) Return a Result in invert_location (#3730) XCM: Allow reclaim of assets dropped from holding (#3727)
In general things are looking good. I believe, I found one bug though - which might explain the memory leak we were seeing, at the very least it was very wrong: In case we could not find any undisputed block after the target block, we would use the best subchain block instead of the target block.
So disputes would not modify chain selection at all, although it is disputed and we should no longer build upon it ... I don't know the exact mechanisms in place here, but I could imagine that this leads to some code behaving weirdly.
To avoid such mistakes in the future, I adjusted the API to do the right thing, instead of leaving room for error to the caller.