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

Check offchain for equivocations in accepted GRANDPA finality proofs #2400

Closed
serban300 opened this issue Jul 18, 2023 · 10 comments
Closed

Check offchain for equivocations in accepted GRANDPA finality proofs #2400

serban300 opened this issue Jul 18, 2023 · 10 comments
Assignees

Comments

@serban300
Copy link
Collaborator

Related to #2496

We should have a process (maybe in the relayers) for checking if the GRANDPA finality proofs that were submitted via submit_finality_proof contain equivocations.

For each accepted finality proof on the chain we should query a node for that block's finality and look for equivocations.

@serban300 serban300 self-assigned this Jul 18, 2023
@serban300
Copy link
Collaborator Author

serban300 commented Jul 18, 2023

There is one problem

When we get the justifications via grandpa_subscribeJustifications, we get:

Chain Polkadot => Block 16452795 justification: GrandpaJustification { round: 17735, commit: Commit { target_hash: 0x5d0484e714bdc581b1b3c3c50c49a5f66baa374340ed478d39673b108b14d0d8, target_number: 16452795, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452796 justification: GrandpaJustification { round: 17736, commit: Commit { target_hash: 0x85005485781f8e8ad13802e9d792745db34c0a13e5f7619ddafc3bf9c7da1b07, target_number: 16452796, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452797 justification: GrandpaJustification { round: 17738, commit: Commit { target_hash: 0xcff2990a57cccab2907b4d2033f89da7f9af692c94557e5a9624b12ca033c36b, target_number: 16452797, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452798 justification: GrandpaJustification { round: 17739, commit: Commit { target_hash: 0xb6d2e66a6c8b19644235527c937a758362612bfc98685dbf4c617c47c4efb8bf, target_number: 16452798, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452799 justification: GrandpaJustification { round: 17741, commit: Commit { target_hash: 0x03e35f716cf02c4f631037e6f372e037aa320c5f4b10a5c390d95cbd735f116a, target_number: 16452799, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452800 justification: GrandpaJustification { round: 17742, commit: Commit { target_hash: 0xa70a1ce928cc5f5f8a33c2c5d1c93e84c11eb5ed4b667737f9c8f3fd40fbadaf, target_number: 16452800, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452801 justification: GrandpaJustification { round: 17744, commit: Commit { target_hash: 0x8eb8f145e064118bd287a6ea14acc5b8085ce45b016e30051338eab06ceb1e5f, target_number: 16452801, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452802 justification: GrandpaJustification { round: 17745, commit: Commit { target_hash: 0xd6acf1704967001258f996bae9e44ee9482844a98eaa2f300a24ad7f4e2bba8b, target_number: 16452802, precommits: [...] }, votes_ancestries: [] }
Chain Polkadot => Block 16452803 justification: GrandpaJustification { round: 17746, commit: Commit { target_hash: 0x247123cfa64a0e2c27e0ec9578dbb1774e23cc1924d4b4275016dfaa19099060, target_number: 16452803, precommits: [...] }, votes_ancestries: [] }

via grandpa_proveFinality we get:

Chain Polkadot => Block 16452795 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 45
Chain Polkadot => Block 16452796 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 44
Chain Polkadot => Block 16452797 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 43
Chain Polkadot => Block 16452798 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 42
Chain Polkadot => Block 16452799 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 41
Chain Polkadot => Block 16452800 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 40
Chain Polkadot => Block 16452801 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 39
Chain Polkadot => Block 16452802 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 38
Chain Polkadot => Block 16452803 justification: GrandpaJustification { round: 17800, commit: Commit { target_hash: 0xd5ab74d6ee2832e02664316c092ae2e02b80545f37b1d75a43b282445ecf272b, target_number: 16452840, precommits: [...] }, votes_ancestries: [] }, unknown headers: 37

and via chain_getBlock we get

16452795...16453120 - no justification found in the `SignedBlock`
Chain Polkadot => Block 16453120 justification: GrandpaJustification { round: 18205, commit: Commit { target_hash: 0x24f628b361fede0f324c56b2b3e891fa19368b4f66ac807ade6f252f59b11cfd, target_number: 16453120, precommits: [...] }, votes_ancestries: [] }

Basically the problem is that when we subscribe to finality notifications we get a notification for each block, or small number of blocks. And each justification has a different round, in this case 17735..17746. While for grandpa_proveFinality, we get the same justification for an entire batch of blocks (a lot of blocks, in my experiments over 10k), by proving the last block in the set, => a single round number, in this case 17800. The same for chain_getBlock, but here we get 1 justification for 512 blocks, still a single round number, in this case 18205.

And in order to check that 2 finality proofs for the same block don't contain equivocations, we need them to target the same round. For the moment I couldn't find a way to get the justification for a certain block number and a certain round.

I think the only option that would cover this problem so far would be to use grandpa_subscribeJustifications() and store the received notifications for a while, indexed by target_number and round. Still thinking.

@andresilva do you know if there is any API for getting the GRANDPA justification for a certain block number + round number ?

@svyatonik
Copy link
Contributor

With grandpa_proveFinality and chain_getBlock you can only get access to persistent justifications, i.e. justifications that are stored in the client database. There are mandatory justifications (for authorities-set-change blocks) and justifications that are simply saved to db from time to time (don't know if we still have them - but your "round number" justifications look exactly like that.

Couple of things to think of:

  1. if the attacker is able to split the network, he could "shadow" the relayer from the "proper" network fork. Do we need to consider this option? Maybe this code need to be run on collators? Maybe some inherent?;
  2. given possible (1), we can't simply save all ephemeral GRANDPA justifications and use them to detect misbehavior - the node we connected to may be on a "wrong" fork and we won't see other votes;
  3. the node currently have all required code to detect and report equivocations. So if we'll be able to implement original suggestion from Detect and report validators misbehavior #2496 then everything should work just fine. But given (2) (i.e. if our node is a part of the "wrong" fork), when we need to gossip? Will node accept votes for old rounds? Will node multicast votes for old rounds?

@serban300
Copy link
Collaborator Author

@svyatonik thank you for the details and the comment !

  1. if the attacker is able to split the network, he could "shadow" the relayer from the "proper" network fork. Do we need to consider this option?

Very good point ! Thank you for pointing this out ! Yes, I think we should take into consideration the possibility that the relayers could follow the wrong fork. So, maybe checking for equivocations in the relayers is not a good option.

  1. Maybe this code need to be run on collators? Maybe some inherent?;

Could you expand on this please ? You mean the bridge hub collators ? To have them subscribe to grandpa_subscribeJustifications and check for equivocations ? Couldn't they follow the wrong fork as well ?

I am not familiar with collators and inherents. But I can take a look on this.

  1. But given (2) (i.e. if our node is a part of the "wrong" fork), when we need to gossip? Will node accept votes for old rounds? Will node multicast votes for old rounds?

I can try to understand what happens if we gossip votes for old rounds. And try to understand gossiping in more detail in general.

  1. the node currently have all required code to detect and report equivocations.

Sounds right. Also I'm assuming that the gossiping logic is resilient to cases where there's a network split. If it's so, I guess gossiping would be the safest solution to catch the offenders.

But another thing: I guess gossiping will catch and report individual equivocations one by one. But will we also be able to tell that the vote that we gossiped was part of a bad finalized fork, and get the right justification ? In order to send it to the bridge and make it halt or take some action

@svyatonik
Copy link
Contributor

Very good point ! Thank you for pointing this out ! Yes, I think we should take into consideration the possibility that the relayers could follow the wrong fork. So, maybe checking for equivocations in the relayers is not a good option.

I'd say that checking using single instance is wrong - no matter is it relay or some other process. So e.g. if we'll have relayers running at BH collator nodes then maybe it is fine.

Could you expand on this please ? You mean the bridge hub collators ? To have them subscribe to grandpa_subscribeJustifications and check for equivocations ? Couldn't they follow the wrong fork as well ?

Yes - BH collators. I do not have some final solution here - just thinking that we may explore that. I imagine that maybe collators should include some mandatory extrinsic into every block (aka inherent) - this would lead to misconsensus if hashes read by them are different. But as I said - this is just some early stage idea. Maybe if we'll be running relayers on collators (also one of early ideas for #2486) then we wouldn't need this inherent at all.

Sounds right. Also I'm assuming that the gossiping logic is resilient to cases where there's a network split. If it's so, I guess gossiping would be the safest solution to catch the offenders.

👍

But another thing: I guess gossiping will catch and report individual equivocations one by one. But will we also be able to tell that the vote that we gossiped was part of a bad finalized fork, and get the right justification ? In order to send it to the bridge and make it halt or take some action

Right, that's the other side of the "attack" - i.e. to halt the bridge (or do something else) if it follows the wrong fork. We may e.g. to add the separate call to the GRANDPA pallet, similar to original pallet report_equivocation. It should accept the vote and switch bridge to halted state if it sees that different vote in the last (or several last?) justifications. This needs some more thinking + needs to be discussed with other team members first. Maybe the fact that the GRANDPA validator will be slashed when misbehaving (and will lose its stake) is a good enough protection?

@serban300
Copy link
Collaborator Author

Right, that's the other side of the "attack" - i.e. to halt the bridge (or do something else) if it follows the wrong fork. We may e.g. to add the separate call to the GRANDPA pallet, similar to original pallet report_equivocation. It should accept the vote and switch bridge to halted state if it sees that different vote in the last (or several last?) justifications. This needs some more thinking + needs to be discussed with other team members first.

It would be great to discuss this with the team.

Maybe the fact that the GRANDPA validator will be slashed when misbehaving (and will lose its stake) is a good enough protection?

If we decide that this is enough, I think that BEEFY + paritytech/substrate#14520 would solve this. The problem is that with GRANDPA in case of a network split a set of malicious validators would be able to sign one fork on the chain and a different fork for the bridge, without gossiping this different fork that they sent to the bridge. But if I understand correctly, with paritytech/substrate#14520, this couldn't happen with BEEFY, right ? Because this would prevent the appearance of a network parition that votes non-finalized grandpa blocks in BEEFY.

@acatangiu
Copy link
Collaborator

acatangiu commented Jul 20, 2023

cc @andresilva knows GRANDPA well, and may have more ideas

@serban300
Copy link
Collaborator Author

  1. But given (2) (i.e. if our node is a part of the "wrong" fork), when we need to gossip? Will node accept votes for old rounds? Will node multicast votes for old rounds?

I can try to understand what happens if we gossip votes for old rounds. And try to understand gossiping in more detail in general.

According to the description in the GRANDPA gossip module and to this code I think that the nodes won't accept gossiped votes for rounds older then the current round - 1.

So I don't know, maybe participating in the gossip is not the solution either.

@Lederstrumpf
Copy link

If we decide that this is enough, I think that BEEFY + paritytech/substrate#14520 would solve this. The problem is that with GRANDPA in case of a network split a set of malicious validators would be able to sign one fork on the chain and a different fork for the bridge, without gossiping this different fork that they sent to the bridge. But if I understand correctly, with paritytech/substrate#14520, this couldn't happen with BEEFY, right ? Because this would prevent the appearance of a network partition that votes non-finalized grandpa blocks in BEEFY.

If #14520 checks votes only, then in a network split where a relayer is not aware of the gossiped vote, they could not report an equivocation for slashing. I'm changing it to be generic over the source of an equivocating signature on a commitment, so we could also for instance fish for equivocations detected on Ethereum (https://github.com/Snowfork/snowbridge/blob/8d01a0529270815532110d94e53d44e38c10650d/core/packages/contracts/src/BeefyClient.sol#L73).

@acatangiu
Copy link
Collaborator

acatangiu commented Jul 20, 2023

if the attacker is able to split the network, he could "shadow" the relayer from the "proper" network fork. Do we need to consider this option?

Yes, this could be an issue, but I don't think it's ultimately a blocker - it's unrealistic to be able to shadow all relayers for entire bonding period.

To get away without slashing, attackers would have to maintain a global network attack splitting/shadowing the network for an entire (28 days) bonding period.

So I think, adding "fishermen" capabilities to relayers (and making sure we run multiple relayer instances) should be enough. We can ping Jakob with this question if you think otherwise.

@serban300
Copy link
Collaborator Author

Closed via all the issues linked above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants