Skip to content
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

dispute-distribution and dispute-coordinator should be a noop for full nodes #747

Open
eskimor opened this issue Dec 21, 2022 · 4 comments

Comments

@eskimor
Copy link
Member

eskimor commented Dec 21, 2022

We seem to be importing votes in the dispute-coordinator, even on simply full nodes (e.g. collators). This in itself is unnecessary and should likely be removed. One artifact of this is that dispute-distribution learns about those unknown disputes and tries to find the node's vote in order to send it out to validators.

Both subsystems should ignore disputes further up the stack in order to not waste resources. In particular dispute-distribution is effectively doing nothing already, but it does "nothing" in a loop which is wasteful, instead the full loop should be skipped.

In particular we should check whether the node has validator keys in the keystore and only if it has at least one, we enable functionality on active leaves updates. Keys being present is enough to enable functionality. In particular, we should not check whether we are an active validator in the current session. This is because, we could become active in the next session for example and then having recorded dispute votes is beneficial to the network's security.

@bkchr
Copy link
Member

bkchr commented Mar 6, 2023

@skunert wants to clean up subsystems usage in Cumulus. We should probably have dispute distribution not even registered on collators/full nodes if possible?

@eskimor
Copy link
Member Author

eskimor commented Mar 6, 2023

Yes, that would be even better, although not as straight forward. For example relay-chain-selection is sending messages to the dispute-coordinator to find out about disputed. Given that full nodes don't vote on finality this in itself is also pointless. So yes, cleaning this up on a deeper level makes a lot of sense, but might also be a bit involved.

@eskimor
Copy link
Member Author

eskimor commented Mar 6, 2023

Even better than checking for keys, would be to just tell the subsystem whether the node is meant to be an authority or not, like we do it for network protocols.

@eskimor
Copy link
Member Author

eskimor commented Mar 6, 2023

@BradleyOlson64 also an easier one to tackle. While a larger scale cleanup as suggested by @bkchr is a very good idea, I think an intermediate solution where we just make sure to not do any unnecessary work would still be worthwhile.

@eskimor eskimor changed the title dispute-distribution should be a noop for full nodes dispute-distribution and dispute-coordinator should be a noop for full nodes Mar 6, 2023
@BradleyOlson64 BradleyOlson64 self-assigned this Mar 9, 2023
@BradleyOlson64 BradleyOlson64 removed their assignment Mar 20, 2023
@Sophia-Gold Sophia-Gold transferred this issue from paritytech/polkadot Aug 24, 2023
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
* Fix typos and add more comments

* More comments

* Merge with main branch

* Minor fix

* test: speed up e2e

* polish comments

* fix missing chmod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

5 participants