RFC017: ABCI++ Vote Extension Propagation

[sergio-mena]

This RFC reports on all discussions and recent findings on the matter of ABCI++ vote extensions as defined in our current spec.
The problems and situations we need to face and possible solutions are discussed.
There are two missing sections I'll update shortly; they will explain how the blocksync and consensus reactors do for catching up, but these are not really needed to review the rest if you are familiar with those reactors.

Rendered

[williambanfield]

Would you mind expanding on this a bit? I think I'm not clear how light blocks apply here.

[sergio-mena]

My understanding of statesync is limited, but I saw in the code that statesync uses light block (i.e., the minimal information needed to light-client-verify the blockchain up to a certain height... I guess that of the snapshot). This minimal information, called signed header, contains the header and the commit, but e.g., not the transactions.

However, someone more knowledgeable on statesync might want to keep me honest here.

Anyway, back to your comment, I think I'll add a couple of sentences briefly explaining what a light block is and why we don't consider them.

[cason]

There is also an unlikely corner case to address here: state sync puts the blockchain/application in height h and the latest height is h+1.

[sergio-mena]

That's a good point. Let me add a TODO in the doc.
My first thought on this is: if a node statesyncs to h in your example, that means:

• 2/3+ validators have got to h+1
• some node has made a snapshot of the state just after applying h

Now, independently of statesync, you need (at least) those 2/3+ validators to keep on making progress, otherwise the chain halts anyway.
So, with our "Base Implementation" we are good, as we will stay in blocksync until we have, at least, blocksynced one block.

Again, these are my first thoughts, so take it with a pinch of salt.

[sergio-mena]

@cason, after revisiting this, I see your comment is referring to the section describing the base solution. In this case there is no problem because we are protected by the exit condition imposed on blocksync (to switch to consensus). See the code, or the text in this RFC explaining it.

Also, in Solution 3 the condition that governs switching from consensus mode to catch-up mode, defined here, is also protecting us against this case.

Let me know if you still have concerns.

[sergio-mena]

Thanks a lot @williambanfield for your thorough review

[cason]

The approach proposed in this RFC works. However, it seems too conservative to me, in terms of the amount of information that nodes need not only to store (this part seems unavoidable) but also to forward to lagging peers.

[cason]

Commits do not contain nil votes.

[sergio-mena]

You're right. While going through the code (MakeCommit method of VoteSet) I somehow confused "absent" and "nil".
I'll make a pass over this text with this in mind.

[sergio-mena]

@cason, just coming back to this comment after some findings in the code...
If commits don't have nil votes, then why is BlockIDFlagNil declared here for CommitSig?
I've looked a bit into the code and, when a precommit Vote is converted into a CommitSig here, we can set it to nil.
Should the code be fixed IYO?

[cason]

Another point to make clear after our discussion: we need some for of pruning, but not detached from block store pruning. That means that we assume that every block in the block store should have the associated vote extensions.

[cmwaters]

I think this is a really well written document on what is quite a challenging problem.

Given its complexity, I generally want to lean hard into whatever is the simplest solution, especially in regards to v0.36. With the potential opportunity of breaking the protocol in v0.37, the time to more deeply analyse the different edges, and feedback from the community using it, I'm far more comfortable with trying to formulate more cleverer solutions then.

Based on this, I'm currently of the opinion that:

• We should have a consensus param to enable and disable the use of vote extensions
• The catch up protocol in consensus should gossip Votes with VoteExtensions i.e. we should store all ExtendedCommits until the application wants us to prune it (RetainHeight)
• Blocksync should not require VoteExtensions to function and should terminate at peer.MaxHeight - 1.

[sergio-mena]

Closes #8174

[thanethomson]

We should have a consensus param to enable and disable the use of vote extensions

Any preferences as to what we should we call these params? Perhaps an ApplicationParams or ABCIParams struct with a single boolean field RequireVoteExtensions?

[josef-widder]

The changes to case (h) look great. Thanks for the clarifications.