fix: Fix vote extension issues, add custom logic to handle secp256k1, fix rpc nilptr issues (#1717)

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

NOTE: PR titles should follow semantic commits:
https://www.conventionalcommits.org/en/v1.0.0/
-->

## Overview

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 

Ex: Closes #<issue number>
-->
Things fixed:
* getAddress does not work for secp256k1, hence using the address
directly from the validator set
* additional logic to handle secp256k1 signing
* move updating store height to before apply block so that, the rpc
(like Status) is able to use the updated height instead of old height
* change vote extension signing to sign on the bytes from
`VoteExtensionSignBytes` instead of plain extension
* fix rpc nilptr failures using normalized height
* add the missing vote extension enable height from genesis
* retain votes from proposed last commit

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Enhanced proposal voting process to support different private key
types.
  
- **Refactor**
  - Updated cryptographic package usage from `ed25519` to `cmcrypto`.
- Refactored key handling in multiple functions to support `secp256k1`.

- **Tests**
- Updated test cases to reflect changes in key handling and
cryptographic package usage.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Ganesha Upadhyaya <[email protected]>
Co-authored-by: Manav Aggarwal <[email protected]>

Author: 3 people

block/manager.go

@@ -10,6 +10,8 @@ import (
 	"sync/atomic"
 	"time"
 
+	secp256k1 "github.com/btcsuite/btcd/btcec/v2"
+	"github.com/btcsuite/btcd/btcec/v2/ecdsa"
 	abci "github.com/cometbft/cometbft/abci/types"
 	cmcrypto "github.com/cometbft/cometbft/crypto"
 	"github.com/cometbft/cometbft/crypto/merkle"
@@ -18,6 +20,7 @@ import (
 	cmtypes "github.com/cometbft/cometbft/types"
 	ds "github.com/ipfs/go-datastore"
 	"github.com/libp2p/go-libp2p/core/crypto"
+	"github.com/libp2p/go-libp2p/core/crypto/pb"
 	pkgErrors "github.com/pkg/errors"
 
 	goheaderstore "github.com/celestiaorg/go-header/store"
@@ -192,10 +195,7 @@ func NewManager(
 		conf.DAMempoolTTL = defaultMempoolTTL
 	}
 
-	proposerAddress, err := getAddress(proposerKey)
-	if err != nil {
-		return nil, err
-	}
+	proposerAddress := s.Validators.Proposer.Address.Bytes()
 
 	maxBlobSize, err := dalc.DA.MaxBlobSize(context.Background())
 	if err != nil {
@@ -264,14 +264,6 @@ func NewManager(
 	return agg, nil
 }
 
-func getAddress(key crypto.PrivKey) ([]byte, error) {
-	rawKey, err := key.GetPublic().Raw()
-	if err != nil {
-		return nil, err
-	}
-	return cmcrypto.AddressHash(rawKey), nil
-}
-
 // SetDALC is used to set DataAvailabilityLayerClient used by Manager.
 func (m *Manager) SetDALC(dalc *da.DAClient) {
 	m.dalc = dalc
@@ -749,8 +741,7 @@ func getRemainingSleep(start time.Time, interval, defaultSleep time.Duration) ti
 func (m *Manager) getCommit(header types.Header) (*types.Commit, error) {
 	// note: for compatibility with tendermint light client
 	consensusVote := header.MakeCometBFTVote()
-
-	sign, err := m.proposerKey.Sign(consensusVote)
+	sign, err := m.sign(consensusVote)
 	if err != nil {
 		return nil, err
 	}
@@ -851,7 +842,6 @@ func (m *Manager) publishBlock(ctx context.Context) error {
 		// if call to applyBlock fails, we halt the node, see https://github.com/cometbft/cometbft/pull/496
 		panic(err)
 	}
-
 	// Before taking the hash, we need updated ISRs, hence after ApplyBlock
 	block.SignedHeader.Header.DataHash, err = block.Data.Hash()
 	if err != nil {
@@ -875,7 +865,7 @@ func (m *Manager) publishBlock(ctx context.Context) error {
 	}
 
 	blockHeight := block.Height()
-	// Update the stored height before submitting to the DA layer and committing to the DB
+	// Update the store height before submitting to the DA layer and committing to the DB
 	m.store.SetHeight(ctx, blockHeight)
 
 	blockHash := block.Hash().String()
@@ -930,6 +920,28 @@ func (m *Manager) publishBlock(ctx context.Context) error {
 	return nil
 }
 
+func (m *Manager) sign(payload []byte) ([]byte, error) {
+	var sig []byte
+	switch m.proposerKey.Type() {
+	case pb.KeyType_Ed25519:
+		return m.proposerKey.Sign(payload)
+	case pb.KeyType_Secp256k1:
+		k := m.proposerKey.(*crypto.Secp256k1PrivateKey)
+		rawBytes, err := k.Raw()
+		if err != nil {
+			return nil, err
+		}
+		priv, _ := secp256k1.PrivKeyFromBytes(rawBytes)
+		sig, err = ecdsa.SignCompact(priv, cmcrypto.Sha256(payload), false)
+		if err != nil {
+			return nil, err
+		}
+		return sig[1:], nil
+	default:
+		return nil, fmt.Errorf("unsupported private key type: %T", m.proposerKey)
+	}
+}
+
 func (m *Manager) processVoteExtension(ctx context.Context, block *types.Block, newHeight uint64) error {
 	if !m.voteExtensionEnabled(newHeight) {
 		return nil
@@ -939,9 +951,17 @@ func (m *Manager) processVoteExtension(ctx context.Context, block *types.Block,
 	if err != nil {
 		return fmt.Errorf("error returned by ExtendVote: %w", err)
 	}
-	sign, err := m.proposerKey.Sign(extension)
+
+	vote := &cmproto.Vote{
+		Height:    int64(newHeight),
+		Round:     0,
+		Extension: extension,
+	}
+	extSignBytes := cmtypes.VoteExtensionSignBytes(m.genesis.ChainID, vote)
+
+	sign, err := m.sign(extSignBytes)
 	if err != nil {
-		return fmt.Errorf("error signing vote extension: %w", err)
+		return fmt.Errorf("failed to sign vote extension: %w", err)
 	}
 	extendedCommit := buildExtendedCommit(block, extension, sign)
 	err = m.store.SaveExtendedCommit(ctx, newHeight, extendedCommit)

block/manager_test.go

@@ -9,7 +9,9 @@ import (
 	"testing"
 	"time"
 
+	cmcrypto "github.com/cometbft/cometbft/crypto"
 	"github.com/cometbft/cometbft/crypto/ed25519"
+	"github.com/cometbft/cometbft/crypto/secp256k1"
 	cmtypes "github.com/cometbft/cometbft/types"
 	ds "github.com/ipfs/go-datastore"
 	"github.com/libp2p/go-libp2p/core/crypto"
@@ -54,7 +56,7 @@ func getBlockBiggerThan(blockHeight, limit uint64) (*types.Block, error) {
 
 func TestInitialStateClean(t *testing.T) {
 	require := require.New(t)
-	genesisDoc, _ := types.GetGenesisWithPrivkey()
+	genesisDoc, _ := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	genesis := &cmtypes.GenesisDoc{
 		ChainID:       "myChain",
 		InitialHeight: 1,
@@ -71,7 +73,7 @@ func TestInitialStateClean(t *testing.T) {
 
 func TestInitialStateStored(t *testing.T) {
 	require := require.New(t)
-	genesisDoc, _ := types.GetGenesisWithPrivkey()
+	genesisDoc, _ := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	valset := types.GetRandomValidatorSet()
 	genesis := &cmtypes.GenesisDoc{
 		ChainID:       "myChain",
@@ -102,8 +104,8 @@ func TestInitialStateStored(t *testing.T) {
 
 func TestInitialStateUnexpectedHigherGenesis(t *testing.T) {
 	require := require.New(t)
+	genesisDoc, _ := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	valset := types.GetRandomValidatorSet()
-	genesisDoc, _ := types.GetGenesisWithPrivkey()
 	genesis := &cmtypes.GenesisDoc{
 		ChainID:       "myChain",
 		InitialHeight: 2,
@@ -128,6 +130,31 @@ func TestInitialStateUnexpectedHigherGenesis(t *testing.T) {
 	require.EqualError(err, "genesis.InitialHeight (2) is greater than last stored state's LastBlockHeight (0)")
 }
 
+func TestSignVerifySignature(t *testing.T) {
+	require := require.New(t)
+	m := getManager(t, goDATest.NewDummyDA())
+	payload := []byte("test")
+	cases := []struct {
+		name  string
+		input cmcrypto.PrivKey
+	}{
+		{"ed25519", ed25519.GenPrivKey()},
+		{"secp256k1", secp256k1.GenPrivKey()},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			pubKey := c.input.PubKey()
+			signingKey, err := types.PrivKeyToSigningKey(c.input)
+			require.NoError(err)
+			m.proposerKey = signingKey
+			sig, err := m.sign(payload)
+			require.NoError(err)
+			ok := pubKey.VerifySignature(payload, sig)
+			require.True(ok)
+		})
+	}
+}
+
 func TestIsDAIncluded(t *testing.T) {
 	require := require.New(t)
 
@@ -426,7 +453,7 @@ func Test_isProposer(t *testing.T) {
 		{
 			name: "Signing key matches genesis proposer public key",
 			args: func() args {
-				genesisData, privKey := types.GetGenesisWithPrivkey()
+				genesisData, privKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 				s, err := types.NewFromGenesisDoc(genesisData)
 				require.NoError(err)
 				signingKey, err := types.PrivKeyToSigningKey(privKey)
@@ -442,7 +469,7 @@ func Test_isProposer(t *testing.T) {
 		{
 			name: "Signing key does not match genesis proposer public key",
 			args: func() args {
-				genesisData, _ := types.GetGenesisWithPrivkey()
+				genesisData, _ := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 				s, err := types.NewFromGenesisDoc(genesisData)
 				require.NoError(err)
 
@@ -460,7 +487,7 @@ func Test_isProposer(t *testing.T) {
 		{
 			name: "No validators found in genesis",
 			args: func() args {
-				genesisData, privKey := types.GetGenesisWithPrivkey()
+				genesisData, privKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 				genesisData.Validators = nil
 				s, err := types.NewFromGenesisDoc(genesisData)
 				require.NoError(err)

go.mod

@@ -36,6 +36,7 @@ require (
 
 require (
 	github.com/BurntSushi/toml v1.4.0
+	github.com/btcsuite/btcd/btcec/v2 v2.3.2
 	github.com/celestiaorg/go-header v0.6.2
 	github.com/ipfs/go-ds-badger4 v0.1.5
 	github.com/mitchellh/mapstructure v1.5.0
@@ -44,7 +45,6 @@ require (
 require (
 	github.com/benbjohnson/clock v1.3.5 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
-	github.com/btcsuite/btcd/btcec/v2 v2.3.2 // indirect
 	github.com/btcsuite/btcd/chaincfg/chainhash v1.0.2 // indirect
 	github.com/celestiaorg/go-libp2p-messenger v0.2.0 // indirect
 	github.com/cespare/xxhash v1.1.0 // indirect

node/full_client.go

@@ -707,9 +707,22 @@ func (c *FullClient) BlockSearch(ctx context.Context, query string, page, perPag
 
 // Status returns detailed information about current status of the node.
 func (c *FullClient) Status(ctx context.Context) (*ctypes.ResultStatus, error) {
-	latest, err := c.node.Store.GetBlock(ctx, c.node.Store.Height())
-	if err != nil {
-		return nil, fmt.Errorf("failed to find latest block: %w", err)
+	var (
+		latestBlockHash cmbytes.HexBytes
+		latestAppHash   cmbytes.HexBytes
+		latestBlockTime time.Time
+
+		latestHeight = c.node.Store.Height()
+	)
+
+	if latestHeight != 0 {
+		latest, err := c.node.Store.GetBlock(ctx, latestHeight)
+		if err != nil {
+			return nil, fmt.Errorf("failed to find latest block: %w", err)
+		}
+		latestBlockHash = cmbytes.HexBytes(latest.SignedHeader.DataHash)
+		latestAppHash = cmbytes.HexBytes(latest.SignedHeader.AppHash)
+		latestBlockTime = latest.Time()
 	}
 
 	initial, err := c.node.Store.GetBlock(ctx, uint64(c.node.GetGenesis().InitialHeight))
@@ -761,10 +774,10 @@ func (c *FullClient) Status(ctx context.Context) (*ctypes.ResultStatus, error) {
 			},
 		},
 		SyncInfo: ctypes.SyncInfo{
-			LatestBlockHash:     cmbytes.HexBytes(latest.SignedHeader.DataHash),
-			LatestAppHash:       cmbytes.HexBytes(latest.SignedHeader.AppHash),
-			LatestBlockHeight:   int64(latest.Height()),
-			LatestBlockTime:     latest.Time(),
+			LatestBlockHash:     latestBlockHash,
+			LatestAppHash:       latestAppHash,
+			LatestBlockHeight:   int64(latestHeight),
+			LatestBlockTime:     latestBlockTime,
 			EarliestBlockHash:   cmbytes.HexBytes(initial.SignedHeader.DataHash),
 			EarliestAppHash:     cmbytes.HexBytes(initial.SignedHeader.AppHash),
 			EarliestBlockHeight: int64(initial.Height()),
@@ -822,10 +835,11 @@ func (c *FullClient) CheckTx(ctx context.Context, tx cmtypes.Tx) (*ctypes.Result
 }
 
 // Header returns a cometbft ResultsHeader for the FullClient
-func (c *FullClient) Header(ctx context.Context, height *int64) (*ctypes.ResultHeader, error) {
-	blockMeta := c.getBlockMeta(ctx, *height)
+func (c *FullClient) Header(ctx context.Context, heightPtr *int64) (*ctypes.ResultHeader, error) {
+	height := c.normalizeHeight(heightPtr)
+	blockMeta := c.getBlockMeta(ctx, height)
 	if blockMeta == nil {
-		return nil, fmt.Errorf("block at height %d not found", *height)
+		return nil, fmt.Errorf("block at height %d not found", height)
 	}
 	return &ctypes.ResultHeader{Header: &blockMeta.Header}, nil
 }
@@ -919,8 +933,8 @@ func (c *FullClient) normalizeHeight(height *int64) uint64 {
 	return heightValue
 }
 
-func (rpc *FullClient) getBlockMeta(ctx context.Context, n int64) *cmtypes.BlockMeta {
-	b, err := rpc.node.Store.GetBlock(ctx, uint64(n))
+func (rpc *FullClient) getBlockMeta(ctx context.Context, n uint64) *cmtypes.BlockMeta {
+	b, err := rpc.node.Store.GetBlock(ctx, n)
 	if err != nil {
 		return nil
 	}

node/full_client_test.go

@@ -63,7 +63,7 @@ func getRPC(t *testing.T) (*mocks.Application, *FullClient) {
 	app.On("InitChain", mock.Anything, mock.Anything).Return(&abci.ResponseInitChain{}, nil)
 	key, _, _ := crypto.GenerateEd25519Key(crand.Reader)
 	ctx := context.Background()
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	node, err := newFullNode(
@@ -539,7 +539,7 @@ func TestTx(t *testing.T) {
 	mockApp.On("PrepareProposal", mock.Anything, mock.Anything).Return(prepareProposalResponse).Maybe()
 	mockApp.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil)
 	key, _, _ := crypto.GenerateEd25519Key(crand.Reader)
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	ctx, cancel := context.WithCancel(context.Background())
@@ -777,7 +777,7 @@ func TestMempool2Nodes(t *testing.T) {
 	assert := assert.New(t)
 	require := require.New(t)
 
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey1, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 
@@ -869,7 +869,7 @@ func TestStatus(t *testing.T) {
 	app.On("PrepareProposal", mock.Anything, mock.Anything).Return(prepareProposalResponse).Maybe()
 	app.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil)
 	key, _, _ := crypto.GenerateEd25519Key(crand.Reader)
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	pubKey := genesisDoc.Validators[0].PubKey
@@ -1013,7 +1013,7 @@ func TestFutureGenesisTime(t *testing.T) {
 	mockApp.On("Commit", mock.Anything, mock.Anything).Return(&abci.ResponseCommit{}, nil)
 	mockApp.On("CheckTx", mock.Anything, mock.Anything).Return(&abci.ResponseCheckTx{}, nil)
 	key, _, _ := crypto.GenerateEd25519Key(crand.Reader)
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	genesisTime := time.Now().Local().Add(time.Second * time.Duration(1))

node/full_node_integration_test.go

@@ -56,7 +56,7 @@ func TestAggregatorMode(t *testing.T) {
 	app.On("Commit", mock.Anything, mock.Anything).Return(&abci.ResponseCommit{}, nil)
 
 	key, _, _ := crypto.GenerateEd25519Key(rand.Reader)
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	blockManagerConfig := config.BlockManagerConfig{
@@ -170,7 +170,7 @@ func TestLazyAggregator(t *testing.T) {
 	app.On("Commit", mock.Anything, mock.Anything).Return(&abci.ResponseCommit{}, nil)
 
 	key, _, _ := crypto.GenerateEd25519Key(rand.Reader)
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	blockManagerConfig := config.BlockManagerConfig{
@@ -328,7 +328,7 @@ func TestChangeValSet(t *testing.T) {
 
 	// tmpubKey1
 	key, _, _ := crypto.GenerateEd25519Key(rand.Reader)
-	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey()
+	genesisDoc, genesisValidatorKey := types.GetGenesisWithPrivkey(types.DefaultSigningKeyType)
 	signingKey, err := types.PrivKeyToSigningKey(genesisValidatorKey)
 	require.NoError(err)
 	tmPubKey1, err := cryptoenc.PubKeyToProto(genesisDoc.Validators[0].PubKey)