8000 privval: DO NOT require extension signature from privval if vote extension is disabled · Issue #2439 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

privval: DO NOT require extension signature from privval if vote extension is disabled #2439

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.

Sign up for GitHub

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

Jump to bottom
Closed
melekes opened this issue Feb 26, 2024 · 4 comments · Fixed by #2496
Closed

privval: DO NOT require extension signature from privval if vote extension is disabled #2439

melekes opened this issue Feb 26, 2024 · 4 comments · Fixed by #2496
Assignees
@melekes
Labels
enhancement New feature or request privval
Milestone
2024-Q1

Comments

@melekes
Copy link
Contributor
melekes commented Feb 26, 2024

Refs #2357

cometbft/types/vote.go

Lines 436 to 441 in 914e9d7

isNil := vote.BlockID.IsNil()
extSignature := (len(v.ExtensionSignature) > 0)
if extSignature == (!isPrecommit || isNil) {
// Non-recoverable because the vote is malformed
return false, &ErrVoteExtensionInvalid{ExtSignature: v.ExtensionSignature}
}

Currently, we require an extension signature from the privval EVEN IF vote extensions are disabled. I.e., if the app is not using vote extensions, validators are signing (through HSM or FilePV) empty Vote.Extension (plus other fields like height and round). Seems like a waste of computing resources. Refs iqlusioninc/tmkms#857 (comment)
@melekes melekes added bug Something isn't working needs-triage This issue/PR has not yet been triaged by the team. enhancement New feature or request privval and removed bug Something isn't working needs-triage This issue/PR has not yet been triaged by the team. labels Feb 26, 2024
@melekes melekes mentioned this issue Feb 26, 2024
Merged
@cason
Copy link
Contributor
cason commented Feb 26, 2024

if the app is not using vote extensions

If the application has enabled vote extensions (which is optional) and is not actually using it.

@melekes
Copy link
Contributor Author
melekes commented Feb 29, 2024 •
edited
Loading

After discussion with @sergio-mena, we've decided to add a boolean flag signExtension to SignVote, indicating if privval needs to sign extension bytes.

SignVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {

Protobuf declaration:

// SignVoteRequest is a request to sign a vote
message SignVoteRequest {
  cometbft.types.v1beta1.Vote vote     = 1;
  string                      chain_id = 2;
  bool sign_extension = 3;
}

melekes added a commit that referenced this issue Mar 2, 2024
@melekes
feat(privval)!: DO NOT require extension signature
54ebda3 
... from privval if vote extension is disabled

New API (added sign_extension bool flag):

```
SignVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {
```

New Protobuf message (added sign_extension field):

```
// SignVoteRequest is a request to sign a vote
message SignVoteRequest {
  cometbft.types.v1beta1.Vote vote     = 1;
  string                      chain_id = 2;
  bool sign_extension = 3;
}
```

[api-breaking]
Closes #2439
@melekes melekes mentioned this issue 8000 Mar 2, 2024
Merged
4 tasks
@melekes melekes self-assigned this Mar 2, 2024
@melekes melekes added this to CometBFT Mar 2, 2024
@github-project-automation github-project-automation bot moved this to Todo in CometBFT Mar 2, 2024
@melekes melekes moved this from Todo to Ready for Review in CometBFT Mar 2, 2024
melekes added a commit that referenced this issue Mar 4, 2024
@melekes
feat(privval)!: DO NOT require extension signature
ecd3fe8 
... from privval if vote extension is disabled

New API (added sign_extension bool flag):

```
SignVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {
```

New Protobuf message (added sign_extension field):

```
// SignVoteRequest is a request to sign a vote
message SignVoteRequest {
  cometbft.types.v1beta1.Vote vote     = 1;
  string                      chain_id = 2;
  bool sign_extension = 3;
}
```

[api-breaking]
Closes #2439
github-merge-queue bot pushed a commit that referenced this issue Mar 5, 2024
@melekes
feat(privval)!: DO NOT require extension signature (#2496)
cb177fe 
... from privval if vote extensions are disabled

New API (added sign_extension bool flag):

```
SignVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {
```

New Protobuf message (added sign_extension field):

```
// SignVoteRequest is a request to sign a vote
message SignVoteRequest {
  cometbft.types.v1beta1.Vote vote     = 1;
  string                      chain_id = 2;
  bool sign_extension = 3;
}
```

[api-breaking]
Closes #2439


---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in CometBFT Mar 5, 2024
mergify bot pushed a commit that referenced this issue Mar 5, 2024
@melekes @mergify
feat(privval)!: DO NOT require extension signature (#2496)
4495cf4 
... from privval if vote extensions are disabled

New API (added sign_extension bool flag):

```
SignVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {
```

New Protobuf message (added sign_extension field):

```
// SignVoteRequest is a request to sign a vote
message SignVoteRequest {
  cometbft.types.v1beta1.Vote vote     = 1;
  string                      chain_id = 2;
  bool sign_extension = 3;
}
```

[api-breaking]
Closes #2439

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit cb177fe)
@adizere adizere added this to the 2024-Q1 milestone Mar 6, 2024
tarcieri pushed a commit to iqlusioninc/tmkms that referenced this issue Mar 18, 2024
@tony-iqlusion
Add optional sign_extensions field to [[chain]] config
8497925 
This adds a new field to the `ChainConfig` structure which represents an
entry in the `[[chain]]` registry in tmkms.toml which indicates whether
or not vote extensions should be signed.

In the future we should be able to query this using the `signExtension`
field of `SignVote`. See cometbft/cometbft#2439.

However, for now this requires a manual configuration flag.
@tony-iqlusion tony-iqlusion mentioned this issue Mar 18, 2024
Merged
tony-iqlusion added a commit to iqlusioninc/tmkms that referenced this issue Mar 19, 2024
@tony-iqlusion
Add optional sign_extensions field to [[chain]] config (#882)
621bd41 
This adds a new field to the `ChainConfig` structure which represents an
entry in the `[[chain]]` registry in tmkms.toml which indicates whether
or not vote extensions should be signed.

In the future we should be able to query this using the `signExtension`
field of `SignVote`. See cometbft/cometbft#2439.

However, for now this requires a manual configuration flag.
@cason cason mentioned this issue Apr 8, 2024
Closed
@cason
Copy link
Contributor
cason commented Apr 22, 2024

Do we have a solution for this problem for v0.38.x? See #2711.

@melekes
Copy link
Contributor Author
melekes commented Apr 23, 2024

Do we have a solution for this problem for v0.38.x? See #2711.

The only possible solution is to not check extension signature if vote extensions are disabled. But this would change the default behavior (always check).

@fcecin fcecin mentioned this issue Apr 25, 2025
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@melekes melekes

Labels
enhancement New feature or request privval
Projects
No open projects
CometBFT
Status: Done
Milestone
2024-Q1
Development

Successfully merging a pull request may close this issue.

feat(privval)!: DO NOT require extension signature cometbft/cometbft
3 participants
@cason @melekes @adizere
0