-
Notifications
You must be signed in to change notification settings - Fork 741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gossip-support: add unittests for update authorities #3258
Conversation
…overed later (#2981) The previous fix was actually incomplete because we update the authorties only on the situation where we decided to reconnect because we had a low connectivity issue. Now the problem is that update_authority_ids use the list of connected peers, so on restart that does contain anything, so calling immediately after issue_connection_request won't detect all authorties, so we need to also check every block as the comment said, but that did not match the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a unit test please? :)
The CI pipeline was cancelled due to failure one of the required jobs. |
I tested with a zombienet, which seems like an overkill for this, will try to see how hard is to add an unittest. |
In order to make the action `Required` it should run always. cc @ggwpez
Signed-off-by: Alexandru Gheorghe <[email protected]>
Signed-off-by: Alexandru Gheorghe <[email protected]>
Added a unittest as well. |
Signed-off-by: Alexandru Gheorghe <[email protected]>
Okay, I got fooled by the very short session on Zombienet, it turns out the fix was actually complete see my updated description, I ended keeping up just the test, I guess it is not an wasted effort :D. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval, but there are few questions/nits I'd like to see addressed. Thanks for adding the tests!
Signed-off-by: Alexandru Gheorghe <[email protected]>
Signed-off-by: Alexandru Gheorghe <[email protected]>
Signed-off-by: Alexandru Gheorghe <[email protected]>
* master: (41 commits) Add Coretime to Westend (#3319) removed `pallet::getter` from `pallet-sudo` (#3370) gossip-support: add unittests for update authorities (#3258) [FRAME Core] remove unnecessary overrides while using derive_impl for frame_system (#3317) Update coretime-westend bootnodes (#3380) `im-online` removal cleanup: remove off-chain storage (#2290) Bump the known_good_semver group with 1 update (#3379) Fix documentation dead link (#3372) Make `sp-keystore` `no_std`-compatible and fix the `build-runtimes-polkavm` CI job (#3363) Remove unused `im-online` weights (#3373) Ensure referenda `TracksInfo` is sorted (#3325) rpc server: add rate limiting middleware (#3301) do not block finality for "disabled" disputes (#3358) fix(zombienet): docker `img` version to use in merge queues for bridges (#3337) Various nits and alignments for SP testnets found during bumping `polkadot-fellows` repo (#3359) Add broker pallet to `coretime-westend` (#3272) remove recursion limit (#3348) Update subkey README.md (#3355) Bump the known_good_semver group with 6 updates (#3347) Document LocalKeystore insert method (#3336) ...
The previous fix was actually incomplete because we update the authorties only on the situation where we decided to reconnect because we had a low connectivity issue. Now the problem is that update_authority_ids use the list of connected peers, so on restart that does contain anything, so calling immediately after issue_connection_request won't detect all authorities, so we need to also check every block as the comment said, but that did not match the code.Actually the fix was correct the flow is follow if more than 1/3 of the authorities can not be resolved we set last_failure and call
ConnectToResolvedValidators
.We will call UpdateAuthorities for all the authorities already connected and for which we already know the address and for the ones that will connect later on
PeerConnected
will have the AuthorityId field set, because it is already known, so approval-distribution will update its cache topology.