Pass signatures of vote extensions in `RequestPrepareProposal` by sergio-mena · Pull Request #489 · cometbft/cometbft

sergio-mena · 2023-03-08T21:23:24Z

Contributes to #485

(please also see second part of the changes in #509)

Contents of this PR:

This PR introduces the changes in the ABCI protobufs to pass the vote extension signature together with the extensions in PrepareProposal
It demonstrates how to use this data at the application level
- In the unit tests, with a small example
- In the e2e app:
  - The e2e app can now find all the data needed to validate the signature
    - some extra functionality was introduced to keep track of the chain ID and the validators' public keys.
  - The e2e app adds all that data in a special transaction at PrepareProposal so that it is also available at ProcessProposal
  - The e2e app uses the data to validate the signature:
    - In PrepareProposal which panics if something is unexpected, as the data comes from the local CometBFT, which should have validated everything itself
    - In ProcessProposal which, rather than panicking, returns reject as the data cannot have come from a correct node (which, in this example, would have panicked while doing the same verification)

Note: The checks on PrepareProposal are done here because e2e app is for testing purposes, and also to demonstrate how to do it. However, real applications will probably be OK with skipping verification at PrepareProposal and thus trusting the local CometBFT. In that case PrepareProposal would just be forwarding the information in TXs to ProcessProposal (similarly to how it is done in this PR), and having ProcessProposal verify the extensions and signatures.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

sergio-mena · 2023-03-09T19:31:14Z

test/e2e/app/app.go

+	cryptoenc "github.com/cometbft/cometbft/crypto/encoding"
 	"github.com/cometbft/cometbft/libs/log"
+	"github.com/cometbft/cometbft/libs/protoio"
+	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"


Question to reviewers: Is it OK to import the core CometBFT proto types here? I'm really skeptic about that. I'm tempted to reshuffle the proto definitions to have what we need in ABCI protos.
Opinions?

Doesn't really matter very much right now, I think.

With the reorg that @mzabaluev will be doing, this will end up changing anyways and will be more consistent across all code that imports our generated Protobuf code.

Actually, I just did a quick search on the SDK repo and it's used in production code, even some protobufs import them (e.g., query, block, and evidence proto definitions).
So we should be good on e2e app.

thanethomson

One minor nit, but otherwise seems good to me!

I'll approve once the SDK team's happy that the solution works for them.

thanethomson · 2023-03-09T19:31:32Z

test/e2e/app/app.go

+	cfg               *Config
+	restoreSnapshot   *abci.Snapshot
+	restoreChunks     [][]byte
+	chanID            string


Good catch!

thanethomson · 2023-03-09T19:42:45Z

test/e2e/app/app.go

+		if !ok {
+			panic("PrepareProposal: received vote from unknown validator")
+		}
+
+		if !pub.VerifySignature(extSignBytes, vote.ExtensionSignature) {
+			panic("PrepareProposal: received vote with invalid signature")
+		}


What would the recommended behaviour be for correct applications here? I assume it's not to panic, right? (Just asking preemptively for app developers' sake - I can see how this is useful for picking up problems in our E2E tests)

So, my understanding is that the signature we're verifying is local (it comes from local CometBFT, which is supposed to have verified it itself when it received the info). A real app could even skip this verification in PrepareProposal under the argument it trusts that CometBFT has verified it.

The missing piece of this PR (hence it's still a draft) is shipping all necessary info into special transactions so that ProcessProposal can also verify (which is where a real app would verify). In that case it wouldn't be a panic, but a ResponseProcessProposal with reject.

In the case of the app, which verifies vote exts "injected" by the proposer, if any such verification fails, rather if more than 2/3 voting power fails, then it would REJECT the proposal.

I didn't get this last comment. The proposal contains extensions from 2/3 of the voting power from the previous round, out of which 1/3+1 is guaranteed to be from correct validators. What do you mean by "if more than 2/3 voting power fails"?
Maybe I am not understanding how you plan on using it. Is there anything you could share?

The proposal contains extensions from 2/3 of the voting power from the previous round, out of which 1/3+1 is guaranteed to be from correct validators.

Yes, given to the proposer during PrepareProposal. The remaining validators that receive the manually injected vote extensions during ProcessProposal have to ensure they get at least 2/3 from the proposer.

lasarojc · 2023-03-10T15:02:10Z

state/execution.go

 				panic(fmt.Errorf("commit at height %d received with missing vote extensions data", ec.Height))
 			}
 			ext = ecs.Extension
+			extSig = ecs.ExtensionSignature


in line 434, above, Tendermint -> CometBFT

Thanks for spotting. I'm changing this in an upcoming related PR, which I am doing separately to avoid bloating this one.

alexanderbez

LGTM :)

Thanks for also providing an example in the tests for how to perform verification app-side 👍

lasarojc · 2023-03-10T15:14:48Z

proto/tendermint/abci/types.proto

@@ -444,6 +444,8 @@ message ExtendedVoteInfo {
  bool signed_last_block = 2;
  // Non-deterministic extension provided by the sending validator's application.
  bytes vote_extension = 3;


would calling it just extension cause confusion?

I prefer to err on the safe side (long but clear name), unless someone has a strong opinion on this

lasarojc · 2023-03-10T15:15:26Z

proto/tendermint/abci/types.proto

  // Non-deterministic extension provided by the sending validator's application.
  bytes vote_extension = 3;
+  // Vote extension signature created by CometBFT
+  bytes extension_signature = 4;


and just signature?

Not possible, we shouldn't confuse it with the signature of the vote itself

lasarojc · 2023-03-10T16:27:47Z

test/e2e/app/app.go

-		// transaction that attempts to modify the "extensionSum" value.
-		txs := make([][]byte, len(req.Txs)+1)
-		for i, tx := range req.Txs {
+		extCommitBytes, err := req.LocalLastCommit.Marshal()


As this serves as an example app, this whole loop could be applied irrespective of extCount>0 to guard against transactions starting with extTxPrefix and reservedKey.
Then build and append the special transaction only if extCount>0.

I like your suggestion, let me see what I can do.
Also, for the future, we can strengthen the logic of this function, because we do know when vote extensions are active and when they are not. The way it is now is a bit loose, and, since this is a testing app we should probable make if panic if we receive extensions when we shouldn't (or if they are missing when they shouldn't)

Actually, I decided to go for the "tightening up" the checks we do on vote extensions. Everything is pretty optional in this PR. I have an upcoming PR lined up which does that, and simplifies this loop massively. Please take a look when it's up

test/e2e/app/app.go

lasarojc · 2023-03-10T16:37:32Z

test/e2e/app/app.go

 	appVersion                 = 1
 	voteExtensionKey    string = "extensionSum"
 	voteExtensionMaxVal int64  = 128
+	reservedKey         string = "reservedTxKey"


Eventhough addresses do not contain y, it might be a good idea to add a separator to the end of the key.

Suggested change

reservedKey string = "reservedTxKey"

reservedKey string = "reservedTxKey"

Yeah, good idea

Also, coming in the next PR

…gn-prepareproposal

…sign-prepareproposal

…gn-prepareproposal

@cmwaters

* Move `apphash`from `Commit`to `FinalizeBlock` (leftovers) (#478) * [partial cherry-pick] abci: Move `app_hash` parameter from `Commit` to `FinalizeBlock` (#8664) * Removed from proto * make proto-gen * make build works * make some tests pass * Fix TestMempoolTxConcurrentWithCommit * Minor change * Update abci/types/types.go * Update internal/state/execution.go * Update test/e2e/app/state.go Co-authored-by: Callum Waters <cmwaters19@gmail.com> * Updated changelog and `UPGRADING.md` * Fixed abci-cli tests, and doc * Addressed @cmwaters' comments * Addressed @cmwaters' comments, part 2 Co-authored-by: Callum Waters <cmwaters19@gmail.com> * Levftover typo in spec --------- Co-authored-by: Callum Waters <cmwaters19@gmail.com> * Compare `types.proto` and `feature/abci++vef` and tip of `v0.36.x` (#488) * CometBFT renaming in types.proto * AppHash * Make proto-gen * Trying 1.20.2 explicitly * Votes can't have zero height (#511) * [partial cherry-pick] e2e: programmable ABCI method times (#8638) (#515) * e2e: programmable ABCI method times * fix linting error Co-authored-by: Callum Waters <cmwaters19@gmail.com> * Pass signatures of vote extensions in `RequestPrepareProposal` (#489) * Pass vote extension signature in `PrepareProposal` * make proto-gen * Verify extensions at PrepareProposal * Addressed @thanethomson's comments * Fix vote extension activation in e2e * ProcessProposal: 1st try * Working.... * Refactoring * Verify signatrue in unit test * spacing * Addressed @lasarojc's comments * Fix test --------- Co-authored-by: Callum Waters <cmwaters19@gmail.com>

cason · 2024-11-08T09:27:53Z

This PR is missing a changelog entry. And it is a breaking change.

cason · 2024-11-08T09:38:14Z

Ref #4458.

sergio-mena added 2 commits March 8, 2023 22:16

Pass vote extension signature in PrepareProposal

a2c8ccc

make proto-gen

74e61b7

sergio-mena added the abci Application blockchain interface label Mar 8, 2023

sergio-mena self-assigned this Mar 8, 2023

sergio-mena mentioned this pull request Mar 8, 2023

A proposer needs to forward vote extension information to be verified in ProcessProposal #485

Closed

3 tasks

Verify extensions at PrepareProposal

bbcf6c8

sergio-mena commented Mar 9, 2023

View reviewed changes

thanethomson reviewed Mar 9, 2023

View reviewed changes

sergio-mena added 6 commits March 9, 2023 21:07

Addressed @thanethomson's comments

2c6746e

Fix vote extension activation in e2e

594c4cc

ProcessProposal: 1st try

c46262e

Working....

6e32d48

Refactoring

1ed677c

Verify signatrue in unit test

ddeadea

sergio-mena marked this pull request as ready for review March 10, 2023 14:09

sergio-mena requested a review from a team as a code owner March 10, 2023 14:09

lasarojc reviewed Mar 10, 2023

View reviewed changes

alexanderbez approved these changes Mar 10, 2023

View reviewed changes

spacing

ec139c2

lasarojc reviewed Mar 10, 2023

View reviewed changes

sergio-mena added 5 commits March 13, 2023 12:02

Addressed @lasarojc's comments

d4abc89

Fix test

dc08877

Merge branch feature/abci++vef into sergio/485-pass-vote-extension-si…

87d56e2

…gn-prepareproposal

Merge branch 'feature/abci++vef' into sergio/485-pass-vote-extension-…

e70d5e0

…sign-prepareproposal

Merge branch feature/abci++vef into sergio/485-pass-vote-extension-si…

b04adbf

…gn-prepareproposal

lasarojc approved these changes Mar 14, 2023

View reviewed changes

sergio-mena merged commit e6d9639 into feature/abci++vef Mar 14, 2023

sergio-mena deleted the sergio/485-pass-vote-extension-sign-prepareproposal branch March 14, 2023 15:13

sergio-mena mentioned this pull request Mar 16, 2023

Fix bugs: (1) another infinite loop in e2e tests (2) missing validator info from genesis #542

Merged

3 tasks

	reservedKey string = "reservedTxKey"
	reservedKey string = "reservedTxKey"

Conversation

sergio-mena commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

sergio-mena Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thanethomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderbez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cason commented Nov 8, 2024

Uh oh!

cason commented Nov 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sergio-mena commented Mar 8, 2023 •

edited

Loading

sergio-mena Mar 9, 2023 •

edited

Loading

sergio-mena Mar 9, 2023 •

edited

Loading

alexanderbez left a comment •

edited

Loading

sergio-mena Mar 13, 2023 •

edited

Loading