Skip to content

[ADR][DRAFT] 024: SignBytes and validator types in privval#2445

Merged
ebuchman merged 5 commits intodevelopfrom
adr_sign_bytes
Oct 5, 2018
Merged

[ADR][DRAFT] 024: SignBytes and validator types in privval#2445
ebuchman merged 5 commits intodevelopfrom
adr_sign_bytes

Conversation

@liamsi
Copy link
Contributor

@liamsi liamsi commented Sep 19, 2018 •
edited
Loading

Work in progress: Quick fledged draft summarizing #1622

If time allows, please review for overall sanity (not typos/nitpicks):
https://github.com/tendermint/tendermint/blob/5d46a2d49dba4da726463ba43058c6b230e3d24a/docs/architecture/adr-02x-sign-bytes.md

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@liamsi liamsi added C:docs Component: Documentation T:design Type: Design work is needed labels Sep 19, 2018
@liamsi liamsi changed the base branch from master to develop September 19, 2018 13:21
@liamsi liamsi mentioned this pull request Sep 19, 2018
Closed
@liamsi
Copy link
Contributor Author

liamsi commented Sep 19, 2018

@tarcieri also suggested as while ago to include the public-key / fingerprint thereof into each "SignRequest" sent by tendermint. E.g. the KMS holds a bunch of key-pairs and needs to know which priv-key of those to use to sign Votes, Proposals, or Hearbeats. This is probably quite important. I still left it away here as I assume the implied changes to be quite involved already.

@tendermint tendermint deleted a comment from codecov-io Sep 19, 2018
@liamsi
fix link and add comment about pub-key per message; also:
19ded87 
 - fix link
 - add little diagram
 - fix typo
@ebuchman
Copy link
Contributor

ebuchman commented Sep 20, 2018

Thanks for taking this on!!

Just noting it should be ADR-024. See number assignment in #2313

@tendermint tendermint deleted a comment from codecov-io Sep 20, 2018
@zmanian
Copy link
Contributor

zmanian commented Sep 21, 2018

IMO there is a lot of scope for improving the validator socket protocol lets focus on getting the MVP working with KMS before we add important features like key ids and and error handling.

@tendermint tendermint deleted a comment from codecov-io Sep 21, 2018
@liamsi
Copy link
Contributor Author

liamsi commented Sep 21, 2018

@zmanian: OK, I'll open an issue and a PR that just moves SignBytes(chainId string) to be amino encoded and leave everything else as it is now (including leaving the fields variable sized). We can change this incrementally instead. It is still worthwhile to have this discussion earlier than later. As I assume we will change the structs before launch (and the kms needs to catch up with these changes then).

liamsi added a commit that referenced this pull request Sep 21, 2018
@liamsi
WIP: exploring changes related to proposed ADR:
d54c09d 
#2445

- still contains a lot of failing tests etc
@xla
Copy link
Contributor

xla commented Sep 21, 2018

Error handling should be considered part of an MVP.

@liamsi liamsi mentioned this pull request Sep 21, 2018
Closed
@xla xla changed the title [ADR][WIP] sign bytes, sign-requests and types [ADR][DRAFT] sign bytes, sign-requests and types Sep 25, 2018
@xla xla changed the title [ADR][DRAFT] sign bytes, sign-requests and types [ADR][DRAFT] 024: sign bytes, sign-requests and types Sep 25, 2018
@xla xla changed the title [ADR][DRAFT] 024: sign bytes, sign-requests and types [ADR][DRAFT] 024: SignBytes and validator types in privval Sep 25, 2018
ebuchman
ebuchman approved these changes Oct 5, 2018
View reviewed changes
Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Will merge this and open an issue to follow up

docs/architecture/adr-024-sign-bytes.md

### Vote

As explained in [issue#1622] `Vote` will be changed to contain the following fields
Copy link
Contributor

@ebuchman ebuchman Oct 5, 2018

Choose a reason for hiding this comment

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

Let's add a summary of why here (ie. fixed points for jumping to to check fields for preventing double signing), and note that from Timestamp and on we lose the fixed lengths.

docs/architecture/adr-024-sign-bytes.md
// registered with "tendermint/socketpv/SignedVoteReply"
message SignedVoteReply {
Vote Vote
Signature Signature
Copy link
Contributor

@ebuchman ebuchman Oct 5, 2018

Choose a reason for hiding this comment

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

Might be nice to ultimately get to this, but we should probably separate it out last, since so much expects a Vote to have a Signature.

docs/architecture/adr-024-sign-bytes.md

```proto
// vanilla protobuf / amino encoded
message Proposal {
Copy link
Contributor

@ebuchman ebuchman Oct 5, 2018

Choose a reason for hiding this comment

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

missing Version?

docs/architecture/adr-024-sign-bytes.md
Round sfixed32
Timestamp Timestamp // << using protobuf definition
BlockPartsHeader PartSetHeader // as already defined
POLRound sfixed32
Copy link
Contributor

@ebuchman ebuchman Oct 5, 2018

Choose a reason for hiding this comment

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

lets move this into the fixed length section (ie. above Timestamp).

docs/architecture/adr-024-sign-bytes.md

### Heartbeat

**TODO**: clarify if heartbeat also needs a fixed offset and update the fields accordingly:
Copy link
Contributor

@ebuchman ebuchman Oct 5, 2018

Choose a reason for hiding this comment

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

It doesn't, it's just for debugging

@ebuchman ebuchman mentioned this pull request Oct 5, 2018
Closed
@ebuchman ebuchman merged commit d2be748 into develop Oct 5, 2018
@ebuchman ebuchman deleted the adr_sign_bytes branch October 5, 2018 00:28
@ebuchman ebuchman mentioned this pull request Oct 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@melekes melekes Awaiting requested review from melekes

@xla xla Awaiting requested review from xla

@zramsay zramsay Awaiting requested review from zramsay

1 more reviewer

@ebuchman ebuchman ebuchman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C:docs Component: Documentation T:design Type: Design work is needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@liamsi @ebuchman @zmanian @xla