Skip to content

Zm fork accountability#3840

Merged
tac0turtle merged 23 commits intomasterfrom
zm_fork-accountability
Sep 6, 2019
Merged

Zm fork accountability#3840
tac0turtle merged 23 commits intomasterfrom
zm_fork-accountability

Conversation

@mikeygbooth99
Copy link
Contributor

@mikeygbooth99 mikeygbooth99 commented Jul 29, 2019

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@cwgoes cwgoes assigned cwgoes and unassigned cwgoes Jul 29, 2019
@cwgoes cwgoes self-requested a review July 29, 2019 12:24
@codecov-io
Copy link

codecov-io commented Jul 29, 2019

Codecov Report

Merging #3840 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3840      +/-   ##
==========================================
+ Coverage   66.89%   66.93%   +0.04%     
==========================================
  Files         219      219              
  Lines       18470    18478       +8     
==========================================
+ Hits        12355    12369      +14     
+ Misses       5194     5189       -5     
+ Partials      921      920       -1
Impacted Files Coverage Δ
blockchain/v2/schedule.go 67.01% <0%> (-0.75%) ⬇️
p2p/pex/pex_reactor.go 82.6% <0%> (-0.58%) ⬇️
consensus/reactor.go 78.42% <0%> (+1.4%) ⬆️

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great write-up 👍

Should we expand on eclipse attacks, which are only mentioned briefly in "Fantom validators" section. Are there any other types of attacks?

Note that detecting this behavior require application knowledge. Detecting this behavior can probably be done by
referring to the block before the one in which height happen.

Q: can we say that in this case a validator ignore to check if proposed value is valid before voting for it?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, yes and we should punish for it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, voting on a block that can't be derived correctly from the previous one needs to be punishable: #3244 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I wonder if this is complex if combined with differing rounds, it seems like we might need some sort of challenge-response where a validator which signed a state that didn't end up being part of the canonical chain must submit the sequence of transactions which led to it in order to avoid being slashed.

Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great write up. But we should link to the google doc somewhere up front as having detailed spec

Note that we can have fantom validators based attacks as a follow up of equivocation or amnesia based attack where
forked state contains validators that are not part of the validator set at the main chain. In this case they keep
signing messages contributed to a forked chain although they are not part of the validator set on the main chain.
This attack can also be used to attack full node during a period of time he is eclipsed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't someone have to commit at least equivocation or amnesia to convince a full node? Ie. just fantom isn't enough

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is / was my understanding as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is what is meant by "as follow up of equivocation or amnesia", that is, until the fork is detected we can have phantom validators creating blocks. However, if we have a fork with different blocks with different next validator sets for the same height, the question is whether, from a technical viewpoint, it makes a difference whether one is a (faulty) phantom validator or whether one is correct. At the end, we will need social consensus to resolve which block to maintain, or where to roll-back to?

Example scenario: a lite client has a trust in a header for height h (and the corresponding validator set VS1).
As part of bisection header verification it verifies header at height h+k with new validator set VS2.
Some of the validators from VS2 signed the header at height h+k that is different from the corresponding header
at the main chain. Detecting these kind of attacks is easy as it just requires verifying if processes are signing messages in heights in which they are not part of the validator set.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description should be clarified to make clear that validators in VS2 are no longer in the val set at h+k (ie. use VS2 and VS'2, like the description in #3244 (comment)

Also note the attack still works even if the validators are still bonded, but just <1/3, so detecting fantom validators isn't enough: #3244 (comment)

Note that detecting this behavior require application knowledge. Detecting this behavior can probably be done by
referring to the block before the one in which height happen.

Q: can we say that in this case a validator ignore to check if proposed value is valid before voting for it?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, voting on a block that can't be derived correctly from the previous one needs to be punishable: #3244 (comment)


4. Fantom validators: faulty validators vote (sign prevote and precommit messages) in heights in which they are not part of the validator sets (at the main chain).

5. Lunatic validator: faulty validator that sign vote messages to support arbitrary application state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, we mean application states other than those which resulted from valid state transitions, right?

all transactions so it can verify if computed application state diverge from the proposed state. Therefore,
full node can be tricked to follow forked chain by committing different set of transactions to a blockchain.

In case of lite clients, attack surface is bigger as lite client only verified headers without executing transactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In case of lite clients, attack surface is bigger as lite client only verified headers without executing transactions.
In case of lite clients, the attack surface is bigger as a lite client only verifies headers without executing transactions.

Note that we can have fantom validators based attacks as a follow up of equivocation or amnesia based attack where
forked state contains validators that are not part of the validator set at the main chain. In this case they keep
signing messages contributed to a forked chain although they are not part of the validator set on the main chain.
This attack can also be used to attack full node during a period of time he is eclipsed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is / was my understanding as well.

Note that detecting this behavior require application knowledge. Detecting this behavior can probably be done by
referring to the block before the one in which height happen.

Q: can we say that in this case a validator ignore to check if proposed value is valid before voting for it?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I wonder if this is complex if combined with differing rounds, it seems like we might need some sort of challenge-response where a validator which signed a state that didn't end up being part of the canonical chain must submit the sequence of transactions which led to it in order to avoid being slashed.

@melekes melekes mentioned this pull request Aug 13, 2019
5 tasks
josef-widder and others added 10 commits August 14, 2019 15:53
Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-Authored-By: Anca Zamfir <ancazamfir@users.noreply.github.com>
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

@tac0turtle tac0turtle merged commit 22000c6 into master Sep 6, 2019
@tac0turtle tac0turtle deleted the zm_fork-accountability branch September 6, 2019 17:45
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

Successfully merging this pull request may close these issues.

9 participants