From d6e1d9c0d29c9e087839c9fe55baa6492d8acd69 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 21 Dec 2023 00:29:16 +0100 Subject: [PATCH] refactor: vote extensions refactoring --- internal/consensus/reactor.go | 4 +++ internal/consensus/vote_signer_test.go | 4 +++ internal/state/execution.go | 2 +- privval/dash_core_signer_client.go | 2 +- proto/tendermint/types/dash_test.go | 31 ++++++++++++++++++ test/e2e/app/app.go | 21 ++++++------ types/block.go | 8 ++--- types/canonical.go | 11 +++---- types/quorum.go | 8 ++--- types/signs_recoverer.go | 13 ++++++-- types/validation_test.go | 2 +- types/vote.go | 1 + types/vote_extension.go | 45 ++++++++++++++++++++++++-- types/vote_extension_test.go | 24 ++++++++++++++ 14 files changed, 141 insertions(+), 35 deletions(-) create mode 100644 proto/tendermint/types/dash_test.go create mode 100644 types/vote_extension_test.go diff --git a/internal/consensus/reactor.go b/internal/consensus/reactor.go index bd789bfeec..38c9063d3e 100644 --- a/internal/consensus/reactor.go +++ b/internal/consensus/reactor.go @@ -687,6 +687,10 @@ func (r *Reactor) handleVoteMessage(ctx context.Context, envelope *p2p.Envelope, if isValidator { // ignore votes on non-validator nodes; TODO don't even send it vMsg := msgI.(*VoteMessage) + if err := vMsg.Vote.ValidateBasic(); err != nil { + return fmt.Errorf("invalid vote received from %s: %w", envelope.From, err) + } + ps.EnsureVoteBitArrays(height, valSize) ps.EnsureVoteBitArrays(height-1, lastCommitSize) if err := ps.SetHasVote(vMsg.Vote); err != nil { diff --git a/internal/consensus/vote_signer_test.go b/internal/consensus/vote_signer_test.go index 89b2244c3d..38abd21c45 100644 --- a/internal/consensus/vote_signer_test.go +++ b/internal/consensus/vote_signer_test.go @@ -138,6 +138,10 @@ func TestVoteSigner_signAddVote(t *testing.T) { key, err := privVal.GetPubKey(ctx, valSet.QuorumHash) assert.NoError(t, err) + for _, ext := range vote.VoteExtensions { + assert.NotEmpty(t, ext.GetSignature()) + } + key1, err := bls.G1ElementFromBytes(key.Bytes()) assert.NoError(t, err) diff --git a/internal/state/execution.go b/internal/state/execution.go index 98ba693c40..463474da59 100644 --- a/internal/state/execution.go +++ b/internal/state/execution.go @@ -615,7 +615,7 @@ func buildLastCommitInfo(block *types.Block, initialHeight int64) abci.CommitInf Round: block.LastCommit.Round, QuorumHash: block.LastCommit.QuorumHash, BlockSignature: block.LastCommit.ThresholdBlockSignature, - ThresholdVoteExtensions: block.LastCommit.ThresholdVoteExtensions.ToProto(), + ThresholdVoteExtensions: block.LastCommit.ThresholdVoteExtensions, } } diff --git a/privval/dash_core_signer_client.go b/privval/dash_core_signer_client.go index 14a3d2704a..3dd46b7e32 100644 --- a/privval/dash_core_signer_client.go +++ b/privval/dash_core_signer_client.go @@ -275,7 +275,7 @@ func (sc *DashCoreSignerClient) SignVote( "voteType", protoVote.Type, "quorumType", quorumType, "quorumHash", quorumHash, - "signature", qs.sign, + "signature", hex.EncodeToString(qs.sign), "proTxHash", proTxHash, "coreBlockRequestId", qs.ID, "coreSignId", tmbytes.Reverse(qs.signHash), diff --git a/proto/tendermint/types/dash_test.go b/proto/tendermint/types/dash_test.go new file mode 100644 index 0000000000..4d430a690d --- /dev/null +++ b/proto/tendermint/types/dash_test.go @@ -0,0 +1,31 @@ +package types_test + +import ( + "testing" + + "github.com/dashpay/tenderdash/internal/libs/protoio" + "github.com/dashpay/tenderdash/proto/tendermint/types" + "github.com/stretchr/testify/assert" +) + +func TestMarshalVoteExtension(t *testing.T) { + + ve := &types.VoteExtension{ + Type: types.VoteExtensionType_THRESHOLD_RECOVER_RAW, + Extension: []byte("threshold"), + XSignRequestId: &types.VoteExtension_SignRequestId{ + SignRequestId: []byte("sign-request-id"), + }, + } + + v := types.Vote{ + Type: types.PrecommitType, + VoteExtensions: []*types.VoteExtension{ve}, + } + v.Marshal() + + ve.Marshal() + marshaled, err := protoio.MarshalDelimited(ve) + assert.NoError(t, err) + assert.NotEmpty(t, marshaled) +} diff --git a/test/e2e/app/app.go b/test/e2e/app/app.go index 027577b30a..f5862fa19b 100644 --- a/test/e2e/app/app.go +++ b/test/e2e/app/app.go @@ -6,7 +6,7 @@ import ( "encoding/binary" "errors" "fmt" - "math/rand" + "math/big" "strconv" "strings" "time" @@ -17,6 +17,7 @@ import ( "github.com/dashpay/tenderdash/abci/example/code" "github.com/dashpay/tenderdash/abci/example/kvstore" abci "github.com/dashpay/tenderdash/abci/types" + "github.com/dashpay/tenderdash/crypto" "github.com/dashpay/tenderdash/libs/log" types1 "github.com/dashpay/tenderdash/proto/tendermint/types" "github.com/dashpay/tenderdash/types" @@ -85,22 +86,18 @@ func (app *Application) ExtendVote(_ context.Context, req *abci.RequestExtendVot ) return &abci.ResponseExtendVote{}, nil } - ext := make([]byte, binary.MaxVarintLen64) - // We don't care that these values are generated by a weak random number - // generator. It's just for test purposes. - //nolint:gosec // G404: Use of weak random number generator - num := rand.Int63n(voteExtensionMaxVal) - extLen := binary.PutVarint(ext, num) + ext := make([]byte, crypto.DefaultHashSize) + copy(ext, big.NewInt(lastHeight+1).Bytes()) + app.logger.Info("generated vote extension", - "num", num, - "ext", fmt.Sprintf("%x", ext[:extLen]), - "state.Height", lastHeight, + "ext", fmt.Sprintf("%x", ext), + "state.Height", lastHeight+1, ) return &abci.ResponseExtendVote{ VoteExtensions: []*abci.ExtendVoteExtension{ { - Type: types1.VoteExtensionType_DEFAULT, - Extension: ext[:extLen], + Type: types1.VoteExtensionType_THRESHOLD_RECOVER_RAW, + Extension: ext, }, { Type: types1.VoteExtensionType_THRESHOLD_RECOVER, diff --git a/types/block.go b/types/block.go index 5bcec3d59e..05670545ec 100644 --- a/types/block.go +++ b/types/block.go @@ -737,7 +737,7 @@ type Commit struct { QuorumHash crypto.QuorumHash `json:"quorum_hash"` ThresholdBlockSignature []byte `json:"threshold_block_signature"` // ThresholdVoteExtensions keeps the list of recovered threshold signatures for vote-extensions - ThresholdVoteExtensions VoteExtensions `json:"threshold_vote_extensions"` + ThresholdVoteExtensions tmproto.VoteExtensions `json:"threshold_vote_extensions"` // Memoized in first call to corresponding method. // NOTE: can't memoize in constructor because constructor isn't used for @@ -764,7 +764,7 @@ func (commit *Commit) ToCommitInfo() types.CommitInfo { Round: commit.Round, QuorumHash: commit.QuorumHash, BlockSignature: commit.ThresholdBlockSignature, - ThresholdVoteExtensions: commit.ThresholdVoteExtensions.ToProto(), + ThresholdVoteExtensions: commit.ThresholdVoteExtensions, } } @@ -943,7 +943,7 @@ func (commit *Commit) ToProto() *tmproto.Commit { c.BlockID = commit.BlockID.ToProto() c.ThresholdBlockSignature = commit.ThresholdBlockSignature - c.ThresholdVoteExtensions = commit.ThresholdVoteExtensions.ToProto() + c.ThresholdVoteExtensions = commit.ThresholdVoteExtensions c.QuorumHash = commit.QuorumHash return c @@ -967,7 +967,7 @@ func CommitFromProto(cp *tmproto.Commit) (*Commit, error) { commit.QuorumHash = cp.QuorumHash commit.ThresholdBlockSignature = cp.ThresholdBlockSignature - commit.ThresholdVoteExtensions = VoteExtensionsFromProto(cp.ThresholdVoteExtensions...) + commit.ThresholdVoteExtensions = cp.ThresholdVoteExtensions commit.Height = cp.Height commit.Round = cp.Round diff --git a/types/canonical.go b/types/canonical.go index 53385d9039..1bdaf231b1 100644 --- a/types/canonical.go +++ b/types/canonical.go @@ -1,10 +1,9 @@ package types import ( - "errors" + "fmt" "time" - "github.com/dashpay/tenderdash/internal/libs/protoio" tmtime "github.com/dashpay/tenderdash/libs/time" tmproto "github.com/dashpay/tenderdash/proto/tendermint/types" ) @@ -33,7 +32,7 @@ func CanonicalizeProposal(chainID string, proposal *tmproto.Proposal) tmproto.Ca // CanonicalizeVoteExtension extracts the vote extension from the given vote // and constructs a CanonicalizeVoteExtension struct, whose representation in // bytes is what is signed in order to produce the vote extension's signature. -func CanonicalizeVoteExtension(chainID string, ext *tmproto.VoteExtension, height int64, round int32) ([]byte, error) { +func CanonicalizeVoteExtension(chainID string, ext *tmproto.VoteExtension, height int64, round int32) (tmproto.CanonicalVoteExtension, error) { switch ext.Type { case tmproto.VoteExtensionType_DEFAULT, tmproto.VoteExtensionType_THRESHOLD_RECOVER: { @@ -44,12 +43,10 @@ func CanonicalizeVoteExtension(chainID string, ext *tmproto.VoteExtension, heigh Round: int64(round), ChainId: chainID, } - return protoio.MarshalDelimited(&canonical) + return canonical, nil } - case tmproto.VoteExtensionType_THRESHOLD_RECOVER_RAW: - return ext.Extension, nil } - return nil, errors.New("provided vote extension type does not have canonical form for signing") + return tmproto.CanonicalVoteExtension{}, fmt.Errorf("provided vote extension type %s does not have canonical form for signing", ext.Type) } // CanonicalTime can be used to stringify time in a canonical way. diff --git a/types/quorum.go b/types/quorum.go index d8ea5e66d0..2ea285798f 100644 --- a/types/quorum.go +++ b/types/quorum.go @@ -17,7 +17,7 @@ type CommitSigns struct { func (c *CommitSigns) CopyToCommit(commit *Commit) { commit.QuorumHash = c.QuorumHash commit.ThresholdBlockSignature = c.BlockSign - commit.ThresholdVoteExtensions = c.ThresholdVoteExtensions + commit.ThresholdVoteExtensions = c.ThresholdVoteExtensions.ToProto() } // QuorumSigns holds all created signatures, block, state and for each recovered vote-extensions @@ -33,7 +33,7 @@ type QuorumSigns struct { func NewQuorumSignsFromCommit(commit *Commit) QuorumSigns { return QuorumSigns{ BlockSign: commit.ThresholdBlockSignature, - ThresholdVoteExtensions: commit.ThresholdVoteExtensions, + ThresholdVoteExtensions: VoteExtensionsFromProto(commit.ThresholdVoteExtensions...), } } @@ -135,8 +135,8 @@ func (q *QuorumSingsVerifier) verifyVoteExtensions( for i, sig := range thresholdSigs { if !pubKey.VerifySignatureDigest(signItems[i].ID, sig) { - return fmt.Errorf("vote-extension %d signature is invalid: %X", i, - signItems[i].Raw) + return fmt.Errorf("vote-extension %d signature is invalid: raw %X, signatrure %X", i, + signItems[i].Raw, sig) } } return nil diff --git a/types/signs_recoverer.go b/types/signs_recoverer.go index e97a437d92..d99bbf5de0 100644 --- a/types/signs_recoverer.go +++ b/types/signs_recoverer.go @@ -96,7 +96,6 @@ func (v *SignsRecoverer) addVoteExtensionSigs(voteExtensions VoteExtensions) { for i, ext := range thresholdExtensions { v.voteThresholdExtensionSigs[i] = append(v.voteThresholdExtensionSigs[i], ext.GetSignature()) } - } func (v *SignsRecoverer) recoverBlockSig(thresholdSigns *QuorumSigns) error { @@ -128,7 +127,17 @@ func (v *SignsRecoverer) recoverVoteExtensionSigs(quorumSigs *QuorumSigns) error thresholdExtIndex++ if len(extensionSignatures) > 0 { - thresholdSignature, err := bls12381.RecoverThresholdSignatureFromShares(extensionSignatures, v.validatorProTxHashes) + // as some nodes might have voted nil, we will have empty sigs from them; + // we need to filter them out before threshold-recovering + sigs := make([][]byte, 0, len(extensionSignatures)) + proTxHashes := make([][]byte, 0, len(extensionSignatures)) + for i, sig := range extensionSignatures { + if len(sig) > 0 { + sigs = append(sigs, sig) + proTxHashes = append(proTxHashes, v.validatorProTxHashes[i]) + } + } + thresholdSignature, err := bls12381.RecoverThresholdSignatureFromShares(sigs, proTxHashes) if err != nil { return fmt.Errorf("error recovering vote-extension %d threshold signature: %w", extIndex, err) } diff --git a/types/validation_test.go b/types/validation_test.go index 57541b768a..88fb57cff4 100644 --- a/types/validation_test.go +++ b/types/validation_test.go @@ -140,7 +140,7 @@ func TestValidatorSet_VerifyCommit_CheckThresholdSignatures(t *testing.T) { thresholdSigns, err := recoverer.Recover() require.NoError(t, err) commit.ThresholdBlockSignature = thresholdSigns.BlockSign - commit.ThresholdVoteExtensions = thresholdSigns.ThresholdVoteExtensions + commit.ThresholdVoteExtensions = thresholdSigns.ThresholdVoteExtensions.ToProto() err = valSet.VerifyCommit(chainID, blockID, h, commit) require.NoError(t, err) } diff --git a/types/vote.go b/types/vote.go index ec708b80ea..f2ba83fb73 100644 --- a/types/vote.go +++ b/types/vote.go @@ -380,6 +380,7 @@ func (vote *Vote) MarshalZerologObject(e *zerolog.Event) { e.Str("val_proTxHash", vote.ValidatorProTxHash.ShortString()) e.Int32("val_index", vote.ValidatorIndex) e.Bool("nil", vote.BlockID.IsNil()) + e.Array("extensions", vote.VoteExtensions) } func (vote *Vote) HasVoteMessage() *tmcons.HasVote { diff --git a/types/vote_extension.go b/types/vote_extension.go index 8e9b641647..3c6938cb94 100644 --- a/types/vote_extension.go +++ b/types/vote_extension.go @@ -10,9 +10,11 @@ import ( "github.com/dashpay/dashd-go/btcjson" abci "github.com/dashpay/tenderdash/abci/types" "github.com/dashpay/tenderdash/crypto" + "github.com/dashpay/tenderdash/internal/libs/protoio" tmbytes "github.com/dashpay/tenderdash/libs/bytes" tmproto "github.com/dashpay/tenderdash/proto/tendermint/types" "github.com/hashicorp/go-multierror" + "github.com/rs/zerolog" ) var ( @@ -191,6 +193,10 @@ func VoteExtensionsFromProto(pve ...*tmproto.VoteExtension) VoteExtensions { // Copy creates a deep copy of VoteExtensions func (e VoteExtensions) Copy() VoteExtensions { + if e.IsEmpty() { + return nil + } + copied := make(VoteExtensions, 0, len(e)) for _, ext := range e { copied = append(copied, ext.Copy()) @@ -237,6 +243,13 @@ func (e VoteExtensions) CopySignsToProto(dest tmproto.VoteExtensions) error { return nil } +// Marshal VoteExtensions as zerolog array +func (e VoteExtensions) MarshalZerologArray(a *zerolog.Array) { + for _, ext := range e { + a.Object(ext) + } +} + type VoteExtensionIf interface { // Return type of this vote extension GetType() tmproto.VoteExtensionType @@ -255,6 +268,8 @@ type VoteExtensionIf interface { Validate() error SetSignature(sig []byte) + + zerolog.LogObjectMarshaler } func VoteExtensionFromProto(ve tmproto.VoteExtension) VoteExtensionIf { @@ -282,7 +297,7 @@ func (e GenericVoteExtension) Copy() VoteExtensionIf { } func (e GenericVoteExtension) ToProto() tmproto.VoteExtension { - return e.VoteExtension + return e.VoteExtension.Clone() } func (e GenericVoteExtension) SignItem(chainID string, height int64, round int32, quorumType btcjson.LLMQType, quorumHash []byte) (crypto.SignItem, error) { @@ -297,10 +312,29 @@ func (e *GenericVoteExtension) SetSignature(sig []byte) { e.Signature = sig } +func (e GenericVoteExtension) MarshalZerologObject(o *zerolog.Event) { + o.Str("type", e.GetType().String()) + o.Hex("extension", e.GetExtension()) + o.Hex("signature", e.GetSignature()) + o.Hex("sign_request_id", e.GetSignRequestId()) +} + +//nolint:revive,stylecheck // name is the same as in protobuf-generated code +func (e GenericVoteExtension) GetSignRequestId() []byte { + if e.XSignRequestId == nil { + return nil + } + id, ok := e.XSignRequestId.(*tmproto.VoteExtension_SignRequestId) + if !ok || id == nil { + return nil + } + + return id.SignRequestId +} + // ThresholdVoteExtension is a threshold type of VoteExtension type ThresholdVoteExtension struct { GenericVoteExtension - ThresholdSignature []byte } func (e ThresholdVoteExtension) Copy() VoteExtensionIf { @@ -369,10 +403,15 @@ func voteExtensionRequestID(height int64, round int32) ([]byte, error) { // Similar to VoteSignBytes, the encoded Protobuf message is varint // length-prefixed for backwards-compatibility with the Amino encoding. func voteExtensionSignBytes(chainID string, height int64, round int32, ext *tmproto.VoteExtension) []byte { - bz, err := CanonicalizeVoteExtension(chainID, ext, height, round) + canonical, err := CanonicalizeVoteExtension(chainID, ext, height, round) if err != nil { panic(err) } + bz, err := protoio.MarshalDelimited(&canonical) + if err != nil { + panic(err) + } + return bz } diff --git a/types/vote_extension_test.go b/types/vote_extension_test.go new file mode 100644 index 0000000000..17ed25f02a --- /dev/null +++ b/types/vote_extension_test.go @@ -0,0 +1,24 @@ +package types + +import ( + "testing" + + tmproto "github.com/dashpay/tenderdash/proto/tendermint/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestVoteExtensionCopySignsFromProto(t *testing.T) { + src := tmproto.VoteExtensions{ + &tmproto.VoteExtension{ + Type: tmproto.VoteExtensionType_THRESHOLD_RECOVER, + Extension: []byte("threshold"), + Signature: []byte("signature"), + }, + } + + dst := VoteExtensions{&ThresholdVoteExtension{}} + err := dst.CopySignsFromProto(src) + require.NoError(t, err) + assert.EqualValues(t, src[0].GetSignature(), dst[0].GetSignature()) +}