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

(In)correct Dispute Confirmation Thresholds #2721

Closed
2 tasks done
Overkillus opened this issue Dec 14, 2023 · 7 comments
Closed
2 tasks done

(In)correct Dispute Confirmation Thresholds #2721

Overkillus opened this issue Dec 14, 2023 · 7 comments
Assignees
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known. T0-node This PR/Issue is related to the topic “node”. T8-polkadot This PR/Issue is related to/affects the Polkadot network.

Comments

@Overkillus
Copy link
Contributor

Overkillus commented Dec 14, 2023

Implementer's guide:

The dispute is already confirmed: Meaning that 1/3+1 nodes already participated, as this suggests in our threat model that there was at least one honest node that already voted, so the dispute must be genuine.

As of now we deem the dispute confirmed when there is at least BZT (byzantine threshold) votes in it.

Problem:

Number of votes =/= number of nodes that believe the dispute is genuine/rightful. In short they are not equal because we count backing votes (and maybe approval votes under some circumstances).<

In more detail:

When you start a dispute and send a DisputeRequest you provide an invalid_vote and a valid_vote. BOTH those votes get added into dispute votes. That means that one of the valid_vote is counted there as well. This will lower the threshold to by one and effectively even exactly 1/3 malicious can confirm disputes.

It gets worse as in fact all backing votes (for that candidate) we perceive will also get added into the votes which effectively decreases the number of malicious validators needed to cross the confirmation threshold. This means that technically the confirmation threshold is not 1/3*vals+1 but more like 1/3*vals-4 as backing groups are of size 5 as of now.

Above I am pretty sure of and tested it recently in the new zombienet tests. #2184 (review)

What I'm unsure about is how it interacts with approval voting. Since approval voting statements can be used for disputes that would mean that generally malicious validators could also introduce approval messages into the dispute votes. If we'll be getting around 30 votes from approvals that would mean that achieving the confirmation threshold would be even easier.

This is not a big problem/vulnerability but goes against our assumptions.

TODOs:

  • Check if it's intended (per comment below I think it is but still confirm for a 100% safety @eskimor)
  • Check if approval votes can in fact be used for that purpose

Yes to both

@Overkillus Overkillus added T0-node This PR/Issue is related to the topic “node”. I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known. T8-polkadot This PR/Issue is related to/affects the Polkadot network. labels Dec 14, 2023
@Overkillus Overkillus self-assigned this Dec 14, 2023
@Overkillus
Copy link
Contributor Author

Overkillus commented Dec 15, 2023

Actually this might be perfectly fine. And this might just be my misunderstanding of what a "genuine dispute" should be. So now that I think about it seems that the goal was to NOT confirm disputes on completely imaginary candidates that never made it to backing/inclusion/approvals. If that is the goal counting backing and approvals is absolutely fine and achieves the goal.

Anyway will drop a clarification in the guide for posterity.

Although this is maybe somewhat problematic for disabling since we wanted to make it so confirmation trumps disablement. Issue is confirmation can be achieved on noticeably (if counting approvals) thresholds than the cap of disablement. In that case a network with a full f disabled nodes can still see disputes being started by the same f nodes if they all cooperate. We either need to revise if confirmation trumping disablement is needed or find a different way.

  • Check if affects disablement
  • Note in guide

@Overkillus Overkillus changed the title Incorrect Dispute Confirmation Thresholds (In)correct Dispute Confirmation Thresholds Dec 15, 2023
@eskimor
Copy link
Member

eskimor commented Dec 15, 2023

We should be counting voting validators not votes. Equivocations should count as 1, all else seems fine. (Counting backing votes and approval votes)

@Overkillus
Copy link
Contributor Author

We should be counting voting validators not votes.

We do~ I mean we count votes but the votes are only added for unique validators so it's exactly as you're saying

@eskimor
Copy link
Member

eskimor commented Dec 15, 2023

Perfect, then if I did not miss something, I think we are good :-) Let me know, if you disagree or something is unclear.

@Overkillus
Copy link
Contributor Author

Overkillus commented Dec 15, 2023

Although this is maybe somewhat problematic for disabling since we wanted to make it so confirmation trumps disablement. Issue is confirmation can be achieved on noticeably (if counting approvals) thresholds than the cap of disablement. In that case a network with a full f disabled nodes can still see disputes being started by the same f nodes if they all cooperate. We either need to revise if confirmation trumping disablement is needed or find a different way.

It still lowers the effective cap of disabling when it comes to disputes. Assuming 1/3 disabled and malicious nodes they will still be able to trigger spam disputes (invalid on valid). Malicious disabled nodes will need only f+1-5-30ish actors to trigger dispute confirmations.

That does not seem like a huge issue since achieving that still requires a LOT of nodes (generally nearly 1/3) that got disabled so the cost is real.

@Overkillus
Copy link
Contributor Author

Although the above might get maybe abused by... first malicious nodes no-show on purpose in approvals to trigger more honest nodes producing statements. Then instead of having the happy path 5 backers and 30ish approval voters we might get significantly more approval voters further lowering the barrier for confirming the dispute. Malicious nodes then start voting invalid on valid on some candidates and they don't even need 1/3rd to confirm them since there is a bunch of approval voters that participated anyway.

(Btw I am still not 100% sure if approval voters are counted like that or if we have some safeguards against it.)

@eskimor
Copy link
Member

eskimor commented Dec 15, 2023

So this would only be an issue for triggering disputes on candidates that have been finalized already. The import of approval votes will likely fail for those anyways, but we could be more careful and only import approval votes if is_potential_spam returns false. And indeed, for those finalized candidates we will also have backing votes .. so yes technically it would be better to not count those as well.

Not importing approval votes would be an easy fix and I don't see any downsides. For backing votes, I would not go that far - this is just something to document in the guide. If we had on chain finality, then we could prune (backing) votes for finalized blocks, also alleviating that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I2-bug The node fails to follow expected behavior. I10-unconfirmed Issue might be valid, but it's not yet known. T0-node This PR/Issue is related to the topic “node”. T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
Status: Completed
Development

No branches or pull requests

2 participants