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

handling evidence when you don't have block history #5617

Closed
4 tasks
cmwaters opened this issue Nov 5, 2020 · 12 comments
Closed
4 tasks

handling evidence when you don't have block history #5617

cmwaters opened this issue Nov 5, 2020 · 12 comments
Labels
C:evidence Component: Evidence

Comments

@cmwaters
Copy link
Contributor

cmwaters commented Nov 5, 2020

Summary

Evidence follows a notion of expiration which gives a window of time that evidence can be submitted. In order to validate evidence a node must have the block and state when the infraction occurred. This all means that any node in consensus must have all the blocks and state (validator sets) from h to h - evidenceAge. If not then there is a chance that the node may panic on seeing valid evidence receiving 2/3 votes.

if err := cs.blockExec.ValidateBlock(cs.state, cs.ProposalBlock); err != nil {
	panic(fmt.Sprintf("enterPrecommit: +2/3 prevoted for an invalid block: %v", err))
}

Proposal

I see two ways of solving this:

  1. By implementing Backfill blocks and state metadata #4629 and enforcing that the retain height be at least the maximum evidence age. This should safely ensure that all nodes participating in consensus have all the necessary blocks. Note1: this restriction is already somewhat enforced by the cosmos SDK. Note2: This would need to be for full nodes, seed nodes and validators

  2. Separating out header validation logic and evidence validation logic where the former is mandatory (the nodes will panic if 2/3 vote on a block with say a different app hash) and the latter is only needed if you are to prevote/precommit for that block. If you see 2/3 vote for it then you skip the validating the evidence and by virtue of consensus (similarly to with txs or consensus param changes), accept it.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cmwaters cmwaters added the C:evidence Component: Evidence label Nov 5, 2020
@tac0turtle
Copy link
Contributor

tac0turtle commented Nov 5, 2020

Another option is the node requests the block needed for verification. This approach would require some sort of cross reactor communication. This is part of the p2p refactor, slated for phase 3 (6 months out).

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Nov 5, 2020

This is part of the p2p refactor, slated for phase 3 (6 months out).

Maybe. There was some opposition when we discussed this last time, so it may not happen, or we may not even have channels at all in phase 3 - all items in that phase are highly speculative and uncertain.

@tac0turtle
Copy link
Contributor

tac0turtle commented Nov 5, 2020

Maybe. There was some opposition when we discussed this last time, so it may not happen, or we may not even have channels at all in phase 3 - all items in that phase are highly speculative and uncertain.

Sorry for the miscommunication. I think in general we will need some sort of way for various modules to request info the node may not have. How its done is uncertain.

@cmwaters
Copy link
Contributor Author

With regards to the block retention height, if we leave this completely to the control of the app, then implementing back fill isn't really going to help because if the app sets a retain height > current height - evidence age then nodes will panic if they see a block with evidence coming from a height that is before the retain height (even if the evidence is invalid). Hence we should either:

  • Have a hard upper bound on pruning which is set as: current height - evidence age. When retention height from the abci response is greater than this height then it is ignored (nodes will just prune to the evidence age boundary).

  • Implement number 2: so nodes ignore evidence at heights they don't have blocks / validator sets for and just vote nil and if they see 2/3 for a block with evidence they will process the evidence blindly. The problem I have with this is that if the application allows more than 1/3 of nodes to set their retention height greater than the evidence age height then we could see a situation where genuine evidence is accidentally censored.

@alexanderbez
Copy link
Contributor

It seems like option (1) is more robust, but the UX is a bit weird of operators see blocks that are not pruned when they think they should've been.

@cmwaters
Copy link
Contributor Author

cmwaters commented Dec 7, 2020

You're right but I guess it's better to have blocks that you didn't think you'd have i.e. from pruning to close to the latest height than to not have the blocks that you thought you had. In other words, it's not too detrimental from a UX perspective.

@tac0turtle
Copy link
Contributor

(1) is the cleaner approach, for now, but it assumes an application wants to handle evidence.

How does this issue play into block backfilling? In block backfilling we assume we will backfill to the unbonding height so all nodes will have the history right?

@cmwaters
Copy link
Contributor Author

cmwaters commented Dec 7, 2020

it assumes an application wants to handle evidence.

Do we want to be promoting evidence as an optional feature? If you don't want to handle evidence then set the evidence age to 0 and then you can prune to whatever height and evidence will always be ignored

In block backfilling we assume we will backfill to the unbonding height so all nodes will have the history right?

Correct. Although applications if they wanted to could specify a height even older than the unbonding height that nodes have to retain history for.

@tac0turtle
Copy link
Contributor

Correct. Although applications if they wanted to could specify a height even older than the unbonding height that nodes have to retain history for.

This is in the backfill proposal right?

@cmwaters
Copy link
Contributor Author

cmwaters commented Dec 7, 2020

Yup

@alexanderbez
Copy link
Contributor

I do not think so. Apps that no not want to deal with evidence, simply just ignore them.

@cmwaters
Copy link
Contributor Author

We can close this issue. It was solved in #6463

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

No branches or pull requests

4 participants