disputes: fix relay chain selection sanity check #3750
Conversation
drahnr
commented
Aug 31, 2021
•
drahnr
commented
- adjust tests
|
duplicate of #3749 |
Lldenaurois
added
A0-please_review
|
I've burned in this on Roccoco, and the warnings immediately disappeared: https://grafana.parity-mgmt.parity.io/goto/9tpTG147z?orgId=1 |
|
It would be good to add a comment why the sanity check has this +-1 |
drahnr
changed the title
Lldenaurois
force-pushed
the
bernhard-fix-relay-chain-sel-check
branch
from
0d24180
to
77b3527
Compare
|
Good morning gents... I stayed up all night and fixed the issue. There were a few things wrong, but the main one was actually a bug in the Therefore, if all blocks are approved, the sanity check is correct as it is in master; but if the tip is unapproved, the sanity-check is off by one and we need to make the adjustment above. After realizing this, I realized that the logic was somewhat copy/pasted into the After that I just had to fix the tests to reflect the fact that the |
| @@ -521,7 +508,7 @@ fn chain_0() -> CaseVars { | |||
|
|
|||
| let a1 = builder.fast_forward_approved(0xA0, head, 1); | |||
| let a2 = builder.fast_forward_approved(0xA0, a1, 2); | |||
| let a3 = builder.fast_forward_approved(0xA0, a2, 3); | |||
| let a3 = builder.fast_forward_disputed(0xA0, a2, 3); | |||
There was a problem hiding this comment.
If 0xA3 is not disputed, then it is automatically undisputed_chain, since chain.disputes.is_empty() == true. However, this is contrary to the expectation that 0xA2 is the undisputed_chain.
There was a problem hiding this comment.
Same as before, I'd prefer to keep the test case input the same as before, if there is a need for additional ones, C&P :)
node/service/src/tests.rs
Outdated
| ); | ||
|
|
||
| let expected = chain.undisputed_chain( | ||
| target_blocknumber, |
There was a problem hiding this comment.
Undisputed chain computes the parent of the first dispute after the target block. If there is no dispute after the target block, then highest_approved_ancestor == expected.
There was a problem hiding this comment.
I believe this is the correct fix considering #3484 (comment), block_descriptions should only represent fully approved range of blocks with no gaps.
drahnr
changed the title
There was a problem hiding this comment.
LGTM 👍 , since I opened the PR initially, I cannot approve. Had a call with @Lldenaurois to walk me through the details, thanks!
|
bot merge |
|
Waiting for commit status. |
|
bot merge |
|
Waiting for commit status. |
ghost
merged commit * 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)
