Conversation
williambanfield
left a comment
There was a problem hiding this comment.
Am I understanding correctly that the extended vote information will not be present in the block?
| >**TODO**: Add the changes needed in LastCommitInfo for vote extensions | ||
|
|
||
| >**TODO**: DISCUSS: We need to make clear whether a proposer is also running the logic of a non-proposer node (in particular "ProcessProposal") | ||
| >**TODO**: DISCUSS: We need to make clear whether a proposer is also running the logic of a non-proposer node (in particular "ProcessProposal"). |
There was a problem hiding this comment.
My sense is that this would be a bit a tricky, since most of the consensus code is a bit agnostic to if the node is the proposer of the current height/round. It treats all proposed blocks the same way, regardless of proposer. This could certainly be amended and bookkeeping could be implemented to track the needed state.
There was a problem hiding this comment.
Hmmm. Interesting.
From the spec's perspective, it's "simpler" to not distinguish between proposer and non-proposer. So everyone would receive the ProcessProposal call. If the proposer has already done the work as part of PrepareProposal, it can simply return "accept" to ProcessProposal. in the vast majority of Applications, this is expected to be the case.
The reason for this dilemma (and so, the TODO) is implementation/performance focused. Sending the whole proposed block over ABCI API is not a trivial thing to do in terms of performance if the Application is not built in the same process as Tendermint.
Now, the fact that the Tendermint logic currently doesn't distinguish between proposer and non-proposer is another variable to take into account; together with the fact that most Apps will be "in-process", so negligible performance penalty for unneeded ProcessProposal API calls.
After thinking about this for a while, I have a proposal, @williambanfield, @cmwaters, let me know what you think:
- For v0.36, we adopt the least disruptive way for the consensus core logic, and we don't distinguish between proposer and non-proposer. So the proposer's Application will also be called
ProcessProposal. - For v0.37 (or later, depending on priorities), we define a new Consensus param (or ".toml" param), which allows an Application to disable calling
ProcessProposalon the proposer. The implementation of such parameter will be the added bookkeeping suggested by @williambanfield. This option will help with the performance of those Applications producing big blocks, which are not built in-process (although the help wouldn't be huge, as only the proposer would stop receiving the blocks over serialized ABCI).
There was a problem hiding this comment.
The reason for this dilemma (and so, the TODO) is implementation/performance focused. Sending the whole proposed block over ABCI API is not a trivial thing to do in terms of performance if the Application is not built in the same process as Tendermint.
I'm personally not sure that the performance savings is worth it. Proto serialization is reasonably fast and this isn't something a node has to do simultaneously or even all that often. When Tendermint calls ProcessProposal on the proposer, it does so at the same time as ProcessProposal is being called on the other nodes as well because they're all at the same step in the algorithm. (in reality, it happens a bit earlier on the proposer because it's gossiping the block directly to itself). Everyone then has to move through the propose->prevote step together before the network can make progress, so letting the proposer be 'faster' here doesn't seem to confer much benefit. All this to say, I'm not sure yet that a performance improvement in this step is that meaningful, but maybe there is something I'm missing.
we define a new Consensus param (or ".toml" param), which allows an Application to disable calling ProcessProposal on the proposer
As far as adding new parameters for this kind of thing, I think that we should try to reduce the number of knobs unless they are incredibly meaningful to application developers. They add maintenance burden and confusion for new users of the software.
I know that we could add this change, but it will add a bit more unavoidable conditional logic that I think we should ensure is necessary before jumping into adding.
There was a problem hiding this comment.
OK, so do we agree that:
- for v0.36
ProcessProposalis always called on all validators, including the proposer - for later, we wait for ABCI++ to start being adopted and, if there is demand for the (potential) optimization, then we can write an ADR to see how we add the option... and to discuss -- in line with your comments above -- whether it makes sense at all. That ADR could be reviewed by those that may ask for the optimization.
There was a problem hiding this comment.
My sense is that this would be a bit a tricky, since most of the consensus code is a bit agnostic to if the node is the proposer of the current height/round.
I kind of disagree. There's a function, isProposer, that you can use to work out if a validator is the proposer or not. It shouldn't be difficult to only call ProcessProposal when a validator is not the proposer and if the validator is the proposer, then automatically consider it valid. I don't think this is a disruptive change to the codebase.
Perhaps people disagree but I find it more intuitive if the proposer doesn't have to process their own proposal since they proposed it in the first place
There was a problem hiding this comment.
Let me give this a quick look, my worry was that there may be more complexity related to reproposal, but perhaps that's not a problem.
There was a problem hiding this comment.
Looked at this together with @cmwaters. We can indeed call isProposer quite easily at that part of the code.
We have given this a spin: see this patch
I've run "make build" and "make test", and everything is passing.
As a result, I'd align with @cmwaters and add this "if" in the code, which isn't complex and makes more sense for the Application; and keep the spec as it is (by removing this TODO; the spec is already aligned to not calling ProcessProposal at the proposer).
There was a problem hiding this comment.
Hmmm, actually, the condition in the patch is the opposite of what we want 😊
But the point of the patch was to show it can be done easily.
There was a problem hiding this comment.
My secondary concern here was about the creation of candidate state. If an application relied on ProcessProposal for the creation of candidate state, then it wouldn't be run on proposers that re-propose locked blocks. As long as we make clear to application developers that they may not have candidate state in FinalizeBlock because these methods may not have been called, then there shouldn't be any danger in foregoing the call to ProcessProposal on the proposer, even if the proposer is re-proposing its locked block.
There was a problem hiding this comment.
I see where you are trying to get at here. My answer to that has two parts:
- As you say above, the Application can never assume that it will always have a candidate state ready for a block delivered by
FinalizeBlock. An Application trying to enforce that, will need an unbounded amount of memory just to keep all candidate states. - In my understanding, a locked value in a proposer got to that state (a) either because that proposer proposed it in a previous round, or (b) because it received the proposal and 2/3+ prevotes from other validators. In case (a) the Application received the proposal via
PrepareProposalpreviously. In case (b) the Application received it viaProcessProposalin a previous round.
Sorry, had missed this comment. |
Awesome, that was my recollection, wanted to check that I was understanding that correctly. |
This reverts commit 4566f1e.
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
1f8c30f to
b01f5bd
Compare
* Changed the spec text to agreed VoteExtension solution * Revert "Removed protobufs related to vote extensions" This reverts commit 4566f1e. * Changes to ABCI protocol buffers * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Fix dangling link in ABCI++ readme * Addressed comments Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
* Changed the spec text to agreed VoteExtension solution * Revert "Removed protobufs related to vote extensions" This reverts commit 4566f1e. * Changes to ABCI protocol buffers * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Fix dangling link in ABCI++ readme * Addressed comments Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
* Changed the spec text to agreed VoteExtension solution * Revert "Removed protobufs related to vote extensions" This reverts commit 4566f1e. * Changes to ABCI protocol buffers * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Fix dangling link in ABCI++ readme * Addressed comments Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
* [cherry-picked] abci: Vote Extension 1 (#6646) * add proto, add boilerplates * add canonical * fix tests * add vote signing test * Update internal/consensus/msgs_test.go * modify state execution in progress * add extension signing * VoteExtension -> ExtendVote * apply review * update data structures * Add comments * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * *Signed -> *ToSign * add Vote to RequestExtendVote * apply reviews * Apply suggestions from code review Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * fix typo, modify proto Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * [cherry-picked] ABCI Vote Extension 2 (#6885) * add proto, add boilerplates * add canonical * fix tests * add vote signing test * Update internal/consensus/msgs_test.go * modify state execution in progress * add extension signing * add extension signing * VoteExtension -> ExtendVote * modify state execution in progress * add extension signing * verify in progress * modify CommitSig * fix test * apply review * update data structures * Apply suggestions from code review * Add comments * fix test * VoteExtensionSigned => VoteExtensionToSigned * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * *Signed -> *ToSign * add Vote to RequestExtendVote * add example VoteExtension * apply reviews * fix vote * Apply suggestions from code review Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * fix typo, modify proto * add abcipp_kvstore.go * add extension test * fix test * fix test * fix test * fit lint * uncomment test * refactor test in progress * gofmt * apply review * fix lint Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> * [cheryy-picked] abci: PrepareProposal-VoteExtension integration [2nd try] (#7821) * PrepareProposal-VoteExtension integration (#6915) * make proto-gen * Fix protobuf crash in e2e nightly tests * Update types/vote.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Addressed @creachadair's comments Co-authored-by: mconcat <monoidconcat@gmail.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * [cherry-picked] Vote extensions: new design (#8031) * Changed the spec text to agreed VoteExtension solution * Revert "Removed protobufs related to vote extensions" This reverts commit 4566f1e. * Changes to ABCI protocol buffers * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update spec/core/data_structures.md Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Fix dangling link in ABCI++ readme * Addressed comments Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * [cherry-picked] abci++: Sync implementation and spec for vote extensions (#8141) * Refactor so building and linting works This is the first step towards implementing vote extensions: generating the relevant proto stubs and getting the build and linter to pass. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix typo Signed-off-by: Thane Thomson <connect@thanethomson.com> * Better describe method given vote extensions Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix types tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * Move CanonicalVoteExtension to canonical types proto defs Signed-off-by: Thane Thomson <connect@thanethomson.com> * Regenerate protos including latest PBTS synchrony params update Signed-off-by: Thane Thomson <connect@thanethomson.com> * Inject vote extensions into proposal Signed-off-by: Thane Thomson <connect@thanethomson.com> * Thread vote extensions through code and fix tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove extraneous empty value initialization Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix lint Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix missing VerifyVoteExtension request data Signed-off-by: Thane Thomson <connect@thanethomson.com> * Explicitly ensure length > 0 to sign vote extension Signed-off-by: Thane Thomson <connect@thanethomson.com> * Explicitly ensure length > 0 to sign vote extension Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove extraneous comment Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update privval/file.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Update types/vote_test.go Co-authored-by: M. J. Fromberger <fromberger@interchain.io> * Format Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix ABCI proto generation scripts for Linux Signed-off-by: Thane Thomson <connect@thanethomson.com> * Sync intermediate and goal protos Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update internal/consensus/common_test.go Co-authored-by: Sergio Mena <sergio@informal.systems> * Use dummy value with clearer meaning Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rewrite loop for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Panic on ABCI++ method call failure Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add strong correctness guarantees when constructing extended commit info for ABCI++ Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add strong guarantee in extendedCommitInfo that the number of votes corresponds Signed-off-by: Thane Thomson <connect@thanethomson.com> * Make extendedCommitInfo function more robust At first extendedCommitInfo expected votes to be in the same order as their corresponding validators in the supplied CommitInfo struct, but this proved to be rather difficult since when a validator set's loaded from state it's first sorted by voting power and then by address. Instead of sorting the votes in the same way, this approach simply maps votes to their corresponding validator's address prior to constructing the extended commit info. This way it's easy to look up the corresponding vote and we don't need to care about vote order. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove extraneous validator address assignment Signed-off-by: Thane Thomson <connect@thanethomson.com> * Sign over canonical vote extension Signed-off-by: Thane Thomson <connect@thanethomson.com> * Validate vote extension signature against canonical vote extension Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update privval tests for more meaningful dummy value Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add vote extension capability to E2E test app Signed-off-by: Thane Thomson <connect@thanethomson.com> * Disable lint for weak RNG usage for test app Signed-off-by: Thane Thomson <connect@thanethomson.com> * Use parseVoteExtension instead of custom parsing in PrepareProposal Signed-off-by: Thane Thomson <connect@thanethomson.com> * Only include extension if we have received txs It's unclear at this point why this is necessary to ensure that the application's local app_hash matches that committed in the previous block. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Require app_hash from app to match that from last block Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add contrived (possibly flaky) test to check that vote extensions code works Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove workaround for problem now solved by #8229 Signed-off-by: Thane Thomson <connect@thanethomson.com> * add tests for vote extension cases * Fix spelling mistake to appease linter Signed-off-by: Thane Thomson <connect@thanethomson.com> * Collapse redundant if statement Signed-off-by: Thane Thomson <connect@thanethomson.com> * Formatting Signed-off-by: Thane Thomson <connect@thanethomson.com> * Always expect an extension signature, regardless of whether an extension is present Signed-off-by: Thane Thomson <connect@thanethomson.com> * Votes constructed from commits cannot include extensions or signatures Signed-off-by: Thane Thomson <connect@thanethomson.com> * Pass through vote extension in test helpers Signed-off-by: Thane Thomson <connect@thanethomson.com> * Temporarily disable vote extension signature requirement Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand on vote equality test errors for clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Expand on vote matching error messages in testing Signed-off-by: Thane Thomson <connect@thanethomson.com> * Allow for selective subscription by vote type This is an attempt to fix the intermittently failing `TestPrepareProposalReceivesVoteExtensions` test in the internal consensus package. Occasionally we get prevote messages via the subscription channel, and we're not interested in those. This change allows us to specify what types of votes we're interested in (i.e. precommits) and discard the rest. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Read lock consensus state mutex in test helper to avoid data race Signed-off-by: Thane Thomson <connect@thanethomson.com> * Revert BlockIDFlag parameter in node test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Perform additional check in ProcessProposal for special txs generated by vote extensions Signed-off-by: Thane Thomson <connect@thanethomson.com> * e2e: check that our added tx does not cause all txs to exceed req.MaxTxBytes Signed-off-by: Thane Thomson <connect@thanethomson.com> * Only set vote extension signatures when signing is successful Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove channel capacity constraint in test helper to avoid missing messages Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add TODO to always require extension signatures in vote validation Signed-off-by: Thane Thomson <connect@thanethomson.com> * e2e: reject vote extensions if the request height does not match what we expect Signed-off-by: Thane Thomson <connect@thanethomson.com> * types: remove extraneous call to voteWithoutExtension in test Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove unnecessary address parameter from CanonicalVoteExtension Signed-off-by: Thane Thomson <connect@thanethomson.com> * privval: change test vote type to precommit since we use an extension Signed-off-by: Thane Thomson <connect@thanethomson.com> * privval: update signing logic to cater for vote extensions Signed-off-by: Thane Thomson <connect@thanethomson.com> * proto: update field descriptions for vote message Signed-off-by: Thane Thomson <connect@thanethomson.com> * proto: update field description for vote extension sig in vote message Signed-off-by: Thane Thomson <connect@thanethomson.com> * proto/types: use fixed-length 64-bit integers for rounds in CanonicalVoteExtension Signed-off-by: Thane Thomson <connect@thanethomson.com> * consensus: fix flaky TestPrepareProposalReceivesVoteExtensions Signed-off-by: Thane Thomson <connect@thanethomson.com> * consensus: remove previously added test helper functionality Signed-off-by: Thane Thomson <connect@thanethomson.com> * e2e: add error logs when we get an unexpected height in ExtendVote or VerifyVoteExtension requests Signed-off-by: Thane Thomson <connect@thanethomson.com> * node_test: get validator addresses from privvals Signed-off-by: Thane Thomson <connect@thanethomson.com> * privval/file_test: optimize filepv creation in tests Signed-off-by: Thane Thomson <connect@thanethomson.com> * privval: add test to check that vote extensions are always signed Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add a script to check documentation for ToC entries. (#8356) This script verifies that each document in the docs and architecture directory has a corresponding table-of-contents entry in its README file. It can be run manually from the command line. - Hook up this script to run in CI (optional workflow). - Update ADR ToC to include missing entries this script found. * build(deps): Bump async from 2.6.3 to 2.6.4 in /docs (#8357) Bumps [async](https://github.com/caolan/async) from 2.6.3 to 2.6.4. - [Release notes](https://github.com/caolan/async/releases) - [Changelog](https://github.com/caolan/async/blob/v2.6.4/CHANGELOG.md) - [Commits](caolan/async@v2.6.3...v2.6.4) --- updated-dependencies: - dependency-name: async dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * privval/file_test: reset vote ext sig before signing Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: William Banfield <wbanfield@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix cherry-picks * make proto-gen * make mockery * fix build * All units tests passing * linter error * Update consensus/state_test.go Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com> * Addressed @williambanfield's comments * Go, not C! Co-authored-by: mconcat <monoidconcat@gmail.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: M. J. Fromberger <fromberger@interchain.io> Co-authored-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: William Banfield <wbanfield@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
This PR contains the new design agreed in ABCI++ meetings on 2022-02-03 and 2022-10-17, and documented in #7664 which this PR is addressing.