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

SignableMsg consensus step is hardcoded to "6" #8

Open
tarcieri opened this issue Aug 9, 2019 · 4 comments
Open

SignableMsg consensus step is hardcoded to "6" #8

tarcieri opened this issue Aug 9, 2019 · 4 comments
Labels
enhancement New feature or request kms

Comments

@tarcieri
Copy link
Contributor

tarcieri commented Aug 9, 2019

The KMS needs to know the correct round step of the consensus state for double signing detection:

https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/amino_types/vote.rs#L170

See this PR for how the KMS is parsing it:

tendermint/tmkms#348

@zmanian
Copy link
Contributor

zmanian commented Aug 12, 2019

A little bit of where I've been stuck on fixing it is that there seems to be some inconsistency in the code base about what consensus states map to a given step.

I've seen propose 0, prevote 1 and preccommit 2.

I've also seen propose 0, prevote 2, preccommit 3.

@tarcieri
Copy link
Contributor Author

tarcieri commented Aug 12, 2019

There's this mapping, where PreVote is 4 and PreCommit is 6 (ostensibly where the hardcoded 6 comes from):

https://github.com/tendermint/tendermint/blob/f39138aa2e543438548a150bdad45304ccc3296b/consensus/types/round_state.go#L18

There's also this mapping which is already in tendermint-rs:

https://github.com/interchainio/tendermint-rs/blob/566dfb6/src/amino_types/signature.rs#L40

    match self {
        // Votes
        SignedMsgType::PreVote => 0x01,
        SignedMsgType::PreCommit => 0x02,
        // Proposals
        SignedMsgType::Proposal => 0x20,
    }

The one I used in this PR was suggested by @ebuchman:

https://github.com/tendermint/kms/pull/348/files#diff-4c9ae8d0d599659f5476e14edb82cbaeR250

   match msg_type {
        SignedMsgType::Proposal => 0,
        SignedMsgType::PreVote => 1,
        SignedMsgType::PreCommit => 2,
    }

I think it really doesn't matter too much so long as it moves Propose -> PreVote -> PreCommit and the step increments between each, and moving between a block ID and <nil> is allowed between steps.

@ebuchman ebuchman added the enhancement New feature or request label Jun 3, 2020
@thanethomson thanethomson added this to the v0.18 milestone Oct 6, 2020
@ebuchman ebuchman added the kms label Nov 21, 2020
@thanethomson
Copy link
Contributor

Is this still relevant? The code on master at the moment seems like it's deprecated: https://github.com/informalsystems/tendermint-rs/blob/master/tendermint/src/vote.rs#L148

@tarcieri
Copy link
Contributor Author

KMS isn't seeing any deprecation warnings, so I think this can be safely removed?

@thanethomson thanethomson removed this from the v0.19 milestone Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request kms
Projects
None yet
Development

No branches or pull requests

4 participants