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

abci++: Sync implementation and spec for vote extensions #8141

Merged
merged 95 commits into from
Apr 19, 2022

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Mar 16, 2022

Closes #7655

This PR needs some in-depth review, since it's the most substantial change I've implemented in Tendermint in quite some time now.

internal/state/execution.go Show resolved Hide resolved
internal/consensus/common_test.go Outdated Show resolved Hide resolved
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>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson changed the title WIP: abci++: Sync implementation and spec for vote extensions abci++: Sync implementation and spec for vote extensions Mar 20, 2022
@thanethomson thanethomson marked this pull request as ready for review March 20, 2022 18:04
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Comment on lines +328 to +331
Hash: vote.BlockID.Hash,
ValidatorAddress: vote.ValidatorAddress,
Height: vote.Height,
VoteExtension: vote.Extension,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sanity check here please?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no extension (i.e., len(vote.Extension) == 0 or similar), are we relying on the application to correctly say "ok", or should we skip calling it entirely in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Perhaps we should skip this application call and automatically accept any vote that's missing a vote extension? cc @sergio-mena

Not sure if I missed this in the spec or if we didn't state it explicitly, but right now I can't imagine any harm coming from automatically accepting in the absence of an extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming it doesn't violate the spec in some other way, this would also potentially avoid giving a buggy application the chance to incorrectly(?) reject the vote.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the spec implies that VerifyVoteExtension will be called even with empty extension, although it doesn't state this clearly.
I think the best would be to discuss this on Wednesday's ABCI++ meeting. I am not 100% sure what is better from the App's perspective, so would like us to have stakeholders' input before we make a decision.
I just opened in Issue to keep track of this decision. Until then, I guess either solution should do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I missed this in the spec or if we didn't state it explicitly, but right now I can't imagine any harm coming from automatically accepting in the absence of an extension.

If an application needs the vote extension data to make progress, then an empty vote extension is probably a sign that the vote is 'bad' and the application may wish to reject the vote+extension. I'm not sure we can automatically accept empty vote extensions because apps do not have a way to specify that a vote extension is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding on to William's reasoning (that I agree with). Also think about the difference between:

  • An empty, signed, vote extension
  • No vote extension

These may seem very similar but are different. The latter can be tampered with in the current vote extension model, the former cannot.
As a conclusion, I think that, when in doubt, we need to give the application as much visibility as possible. But, as stated previously, I'd like to have the App developers' input in tomorrow's ABCI++ meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

If an application needs the vote extension data to make progress, then an empty vote extension is probably a sign that the vote is 'bad' and the application may wish to reject the vote+extension.

This seems related to the concern we previously discussed, where a bad vote extension could implicate liveness of the consensus algorithm. I had thought we'd concluded we didn't want to allow the application to stall consensus for such cases: Once the transaction is accepted, the application could use the presence/absence/validity of the extension data to determine whether to execute the transaction, but not to fail the block itself. Did I misunderstand that, or is that not the issue that we're facing here.

It's not clear to me how the distinction between "empty vote extension" and "no vote extension" can be meaningful: If the extension is empty, then it has no effect on the hash of the enclosing message. That's the case regardless whether the application intended for there to be an empty extension or not, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

From todays ABCi++ call, the decision was made to always pass the extension to the application, even if it is empty. Applications that do not wish to use the vote extension mechanism must just return the ACCEPT enum value.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
abci/example/kvstore/persistent_kvstore.go Show resolved Hide resolved
Timestamp: vs.clock.Now(),
ValidatorAddress: pubKey.Address(),
ValidatorIndex: vs.Index,
Extension: []byte("extension"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably I missed something, but why does this need to be set at all, if we don't have an explicit extension payload from the caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my current understanding of the tests that use this, this was meant as a way of implicitly testing vote extensions in existing tests. We could always turn it into a function parameter and let the individual tests decide whether to extend the vote.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, but then won't we need to add explicit cases to ensure we're also testing the cases where no extension was provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes definitely.

internal/state/execution.go Show resolved Hide resolved
internal/state/execution.go Show resolved Hide resolved
Comment on lines +328 to +331
Hash: vote.BlockID.Hash,
ValidatorAddress: vote.ValidatorAddress,
Height: vote.Height,
VoteExtension: vote.Extension,
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no extension (i.e., len(vote.Extension) == 0 or similar), are we relying on the application to correctly say "ok", or should we skip calling it entirely in that case?

privval/file.go Outdated Show resolved Hide resolved
ResponseInfo info = 4;
ResponseInitChain init_chain = 5;
ResponseQuery query = 6;
ResponseBeginBlock begin_block = 7 [deprecated = true];
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we've discussed this a few times, but I'm not sure what the outcome was.

I think we decided that we're keeping the OG ABCI types here so that the SDK can play bridge for existing ABCi applications that have not yet been updated to use the ABCI++ requests. If that's correct (which would make sense), we should double-check that we have not changed any of the existing type tags on the request and response oneof wrappers. Otherwise, an existing out-of-process application will get the wrong tag numbers for the existing messages and is likely to be confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The tag numbers have been kept consistent with the old version.
As for what to do with these, it was indeed discussed here, where I proposed to follow @creachadair's proposal 3).
My view hasn't changed on this: once this PR is in (meaning: the bulk of ABCI++ implementation is in) we should remove this entirely to avoid confusion (Apps might think we still support the old API, which is not the case).

Copy link
Contributor

Choose a reason for hiding this comment

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

The tag numbers have been kept consistent with the old version.

Ok, great, in that case I have no concerns. I just wasn't sure because of how the new ones are mixed in. ✔️

@@ -35,3 +35,12 @@ message CanonicalVote {
google.protobuf.Timestamp timestamp = 5 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
string chain_id = 6 [(gogoproto.customname) = "ChainID"];
}

// CanonicalVoteExtension
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should either give this comment more context or remove it. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We (i.e. I 🙂) should definitely add more context. While going through the protos I had to hold back from adding comments to all the messages and fields defined in the spec. I think that deserves its own PR.

types/vote_test.go Outdated Show resolved Hide resolved
[]byte{
0x38, // length
0x2e, // length
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pre-existing, but I find it deeply weird that we have hardcoded golden wire-format messages in this test. It seems like that makes a change-detector rather than a real test, since if the real marshaling code emits something different, the signatures won't verify correctly in the real code but will still pass these hardwired tests.

Not something we need to resolve right now, but I'm not sure what we imagine to be testing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this does ultimately seem like a change detector. But perhaps this is something that Buf should be detecting for us instead?

I found this nifty protobuf decoder that I used with the hex values from our test structs to ensure I understood what I was changing before I changed it. I'm sure there's a CLI-based tool for this somewhere (and if there isn't there should be).

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more in the other direction: We can populate the (generated) structs with the data we expect to verify, marshal them, and then verify that the results plumb through correctly. But that would work too.

I'm slightly biased toward making the test cases easier for a human to read, which would militate against hex strings, but I'm sure this is something we can revisit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@creachadair, I had the same thought when I came across these tests in January. Probably something we could improve on if we all agree it'd be more readable and there are no drawbacks we may be missing here

Copy link
Contributor

Choose a reason for hiding this comment

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

In my understanding these tests effectively do live as 'change detectors'.
The goal of these tests is to ensure that our different types continue to marshal to the exact same set of bytes across different versions of the code. The purpose of that being to ensure that our signed/hashed data isn't changing as we make updates to Tendermint.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my understanding these tests effectively do live as 'change detectors'. The goal of these tests is to ensure that our different types continue to marshal to the exact same set of bytes across different versions of the code. The purpose of that being to ensure that our signed/hashed data isn't changing as we make updates to Tendermint.

If indeed the main purpose of these is to catch hash changes, we should probably not have a bunch of different test cases: We only need one representative of each message with all the relevant fields filled in to detect a hash divergence.

If we think that's valuable to have, I'd suggest we make the golden data not the message inputs but the hash values. That way, we can write the messages in a way that's human-readable (e.g., using struct literals) and the test can compute the bits. That'd also make it easier to show which message diverged (log output can include a text dump of the input message rather than a bunch of hex).

That said, this isn't something we need to resolve right now.

thanethomson and others added 7 commits April 16, 2022 09:22
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
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.
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>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@@ -321,3 +359,15 @@ func newProposal(height int64, round int32, blockID types.BlockID, t time.Time)
Timestamp: t,
}
}

func newTestFilePV(t *testing.T) (*FilePV, string, string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the 2nd and 3rd args being used, I don't think we should return them if they aren't used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

privVal, _, _ := newTestFilePV(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use this function in places where this same logic exists in these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

ABCI++ automation moved this from Review in progress to Reviewer approved Apr 18, 2022
@williambanfield
Copy link
Contributor

Approving this PR. It looks like we have the main cases covered and issues opened to continue discussion and ongoing work.

@thanethomson thanethomson merged commit a1104b9 into master Apr 19, 2022
ABCI++ automation moved this from Reviewer approved to Done Apr 19, 2022
@thanethomson thanethomson deleted the thane/7655-vote-extensions branch April 19, 2022 12:05
@sergio-mena
Copy link
Contributor

Yes! Happy to see this merged! :-)

@sergio-mena
Copy link
Contributor

Closes #8174

@cmwaters cmwaters mentioned this pull request Oct 24, 2022
3 tasks
sergio-mena added a commit that referenced this pull request Nov 12, 2022
* 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>
sergio-mena added a commit that referenced this pull request Nov 28, 2022
…ons (#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>
sergio-mena added a commit that referenced this pull request Nov 30, 2022
…ons (#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>
sergio-mena added a commit that referenced this pull request Dec 6, 2022
* [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

ABCI++: Synchronize ExtendVote and VerifyVoteExtension spec and implementation
7 participants