Merge commit from fork

* add VaidateBasic to BitArray to ensure Bits and len(Elems) are valid

* call ValidateBasic on BitArrays when receiving as a msg from exteranl nodes

* enfore SetIndex is not setting out of bounds

* add guard to getNumTrueIndices

getNumTrueIndices will index out of bounds if Bits and Elems have a
mismatch where len(elems) != (bits+63)/64, this guard makes it simply
return 0 if this mismatch is present

* changelog

* fix missing import for v0.38.x

* update changelog for release of v0.38.19

* remove duplicate bug fixes from unreleased

* fix changelog date

* fix lint

* fix expected error string in test

Author: mattac21

CHANGELOG.md

@@ -58,7 +58,7 @@ team, including:
    details.
 5. The first officially supported release of the [data companion
    API](./docs/architecture/adr-101-data-companion-pull-api.md).
-6. Versioning of both the Protobuf definitions _and_ RPC. By versioning our
+6. Versioning of both the Protobuf definitions *and* RPC. By versioning our
    APIs, we aim to provide a level of commitment to API stability while
    simultaneously affording ourselves the ability to roll out substantial
    changes in non-breaking releases of CometBFT. See [ADR
@@ -88,7 +88,7 @@ the stability guarantees we provide for pre-releases.
 
 ### BREAKING CHANGES
 
- - `[abci/types]` Rename `UpdateValidator` to `NewValidatorUpdate`, remove
+- `[abci/types]` Rename `UpdateValidator` to `NewValidatorUpdate`, remove
    `Ed25519ValidatorUpdate` ([\#2843](https://github.com/cometbft/cometbft/pull/2843))
 - [`config`] deprecate boltdb and cleveldb. If you're using either of those,
   please reach out ([\#2775](https://github.com/cometbft/cometbft/pull/2775))
@@ -114,12 +114,12 @@ method is removed ([\#4040](https://github.com/cometbft/cometbft/pull/4040)).
   instances to follow the naming changes in the proto-derived
   `api/cometbft/abci/v1` package
   ([\#1533](https://github.com/cometbft/cometbft/pull/1533)):
-  * The prefixed naming pattern `RequestFoo`, `ReponseFoo` changed to
+    - The prefixed naming pattern `RequestFoo`, `ReponseFoo` changed to
     suffixed `FooRequest`, `FooResponse`.
-  * Each method gets its own unique request and response type to allow for
+    - Each method gets its own unique request and response type to allow for
     independent evolution with backward compatibility.
-  * `ABCIClient` renamed to `ABCIServiceClient`.
-  * `ABCIServer` renamed to `ABCIServiceServer`.
+    - `ABCIClient` renamed to `ABCIServiceClient`.
+    - `ABCIServer` renamed to `ABCIServiceServer`.
 - `[blocksync]` Move to `internal`
   ([\#1485](https://github.com/cometbft/cometbft/pull/1485))
 - `[cmd]` Remove `replay` and `replay-console` subcommands
@@ -232,14 +232,14 @@ method is removed ([\#4040](https://github.com/cometbft/cometbft/pull/4040)).
   data companion gRPC API as per
   [RFC 106](https://github.com/cometbft/cometbft/blob/main/docs/references/rfc/rfc-106-separate-stateful-methods.md)
   ([\#2230](https://github.com/cometbft/cometbft/issues/2230)):
-  * `GetLatest` from `cometbft.services.block.v1.BlockService`;
-  * `GetLatestBlockResults` from `cometbft.services.block_results.v1.BlockResultsService`.
+    - `GetLatest` from `cometbft.services.block.v1.BlockService`;
+    - `GetLatestBlockResults` from `cometbft.services.block_results.v1.BlockResultsService`.
 - `[rpc/grpc]` Remove support for stateful block data retrieval methods from the
   data companion APIs as per [RFC 106](https://github.com/cometbft/cometbft/blob/main/docs/references/rfc/rfc-106-separate-stateful-methods.md)
-  * `GetLatestBlock` method removed from the `BlockServiceClient` interface.
-  * `GetLatestBlockResults` method removed from the `BlockResultServiceClient` interface.
-  * `GetLatest` endpoint is no longer served by `BlockServiceServer` instances.
-  * `GetLatestBlockResults` endpoint is no longer served by `BlockResultServiceServer` instances.
+    - `GetLatestBlock` method removed from the `BlockServiceClient` interface.
+    - `GetLatestBlockResults` method removed from the `BlockResultServiceClient` interface.
+    - `GetLatest` endpoint is no longer served by `BlockServiceServer` instances.
+    - `GetLatestBlockResults` endpoint is no longer served by `BlockResultServiceServer` instances.
 - `[proto]` Renamed the packages from `tendermint.*` to `cometbft.*`
   and introduced versioned packages to distinguish between proto definitions
   released in `0.34.x`, `0.37.x`, `0.38.x`, and `1.x` versions.
@@ -256,26 +256,26 @@ method is removed ([\#4040](https://github.com/cometbft/cometbft/pull/4040)).
   ([#736](https://github.com/cometbft/cometbft/issues/736),
    [#1504](https://github.com/cometbft/cometbft/issues/1504),
    [#1530](https://github.com/cometbft/cometbft/issues/1530)):
-  * Names of request and response types used in gRPC changed by making
+    - Names of request and response types used in gRPC changed by making
     `Request`/`Response` the suffix instead of the prefix, e.g.
     `RequestCheckTx` ⭢ `CheckTxRequest`.
-  * The `Request` and `Response` multiplex messages are redefined accordingly.
-  * `CheckTxType` values renamed with the `CHECK_TX_TYPE_` prefix.
-  * `MisbehaviorType` values renamed with the `MISBEHAVIOR_TYPE_` prefix.
-  * `Result` enum formerly nested in `ResponseOfferSnapshot` replaced with the package-level
+    - The `Request` and `Response` multiplex messages are redefined accordingly.
+    - `CheckTxType` values renamed with the `CHECK_TX_TYPE_` prefix.
+    - `MisbehaviorType` values renamed with the `MISBEHAVIOR_TYPE_` prefix.
+    - `Result` enum formerly nested in `ResponseOfferSnapshot` replaced with the package-level
     `OfferSnapshotResult`, its values named with the
     `OFFER_SNAPSHOT_RESULT_` prefix.
-  * `Result` enum formerly nested in `ResponseApplyShapshotChunk` replaced with the package-level
+    - `Result` enum formerly nested in `ResponseApplyShapshotChunk` replaced with the package-level
     `ApplySnapshotChunkResult`, its values named with the
     `APPLY_SNAPSHOT_CHUNK_RESULT_` prefix.
-  * `Status` enum formerly nested in `ResponseProcessProposal` replaced with the package-level
+    - `Status` enum formerly nested in `ResponseProcessProposal` replaced with the package-level
     `ProcessProposalStatus`, its values named with the
     `PROCESS_PROPOSAL_STATUS_` prefix.
-  * `Status` enum formerly nested in `ResponseVerifyVoteExtension` replaced with the package-level
+    - `Status` enum formerly nested in `ResponseVerifyVoteExtension` replaced with the package-level
     `VerifyVoteExtensionStatus`, its values named with the
     `VERIFY_VOTE_EXTENSION_STATUS_` prefix.
-  * New definition of `Misbehavior` using the changed `MisbehaviorType`.
-  * The gRPC service is renamed `ABCIService` and defined using the types listed above.
+    - New definition of `Misbehavior` using the changed `MisbehaviorType`.
+    - The gRPC service is renamed `ABCIService` and defined using the types listed above.
 - `[proto]` In the `cometbft.state.v1` package, the definition for `ABCIResponsesInfo`
   is changed, renaming `response_finalize_block` field to `finalize_block`.
 - `[proxy]` Expand `ClientCreator` interface to allow
@@ -422,7 +422,7 @@ on the `/block_results` RPC endpoint.
 
 ### FEATURES
 
--  `[indexer]` Introduces configurable table names for the PSQL indexer.
+- `[indexer]` Introduces configurable table names for the PSQL indexer.
   ([\#3593](https://github.com/cometbft/cometbft/issues/3593))
 - `[config]` Add [`pebbledb`](https://github.com/cockroachdb/pebble). To use, build with
   `pebbledb` tag (`go build -tags pebbledb`) ([\#2132](https://github.com/cometbft/cometbft/pull/2132/))
@@ -489,6 +489,7 @@ on the `/block_results` RPC endpoint.
   ([\#1094](https://github.com/cometbft/cometbft/issues/1094))
 - `[light/store]` Added support for a different DB key representation within the light block store ([\#2327](https://github.com/cometbft/cometbft/pull/2327/))
 - `[mempool]` Add `nop` mempool ([\#1643](https://github.com/cometbft/cometbft/pull/1643)). If you want to use it, change mempool's `type` to `nop`:
+
 ```toml
       [mempool]
 
@@ -502,13 +503,14 @@ on the `/block_results` RPC endpoint.
       # is not supported.
       type = "nop"
 ```
+
 - `[metrics]` Add metric for mempool size in bytes `SizeBytes`.
   ([\#1512](https://github.com/cometbft/cometbft/pull/1512))
 - `[metrics]` Add metrics to monitor pruning and current available data in stores: `PruningServiceBlockRetainHeight`, `PruningServiceBlockResultsRetainHeight`, `ApplicationBlockRetainHeight`, `BlockStoreBaseHeight`, `ABCIResultsBaseHeight`.
   ([\#1234](https://github.com/cometbft/cometbft/pull/1234))
 - `[metrics]` Added metrics to monitor block store access. ([\#1974](https://github.com/cometbft/cometbft/pull/1974))
 - `[metrics]` Added metrics to monitor state store access. ([\#1974](https://github.com/cometbft/cometbft/pull/1974))
-- `[privval]` Add `key-type` flag to all command that _may_ generate a `privval` file,
+- `[privval]` Add `key-type` flag to all command that *may* generate a `privval` file,
   and make `GenFilePV` flexible to accept different key generators.
   ([\#3517](https://github.com/cometbft/cometbft/pull/3517))
 - `[proto]` Add definitions and generated code for
@@ -809,7 +811,7 @@ synchronous event bus subscription.
 This release includes the second part of ABCI++, called ABCI 2.0.
 ABCI 2.0 introduces ABCI methods `ExtendVote` and `VerifyVoteExtension`.
 These new methods allow the application to add data (opaque to CometBFT),
-called _vote extensions_ to precommit votes sent by validators.
+called *vote extensions* to precommit votes sent by validators.
 These vote extensions are made available to the proposer(s) of the next height.
 Additionally, ABCI 2.0 coalesces `BeginBlock`, `DeliverTx`, and `EndBlock`
 into one method, `FinalizeBlock`, whose `Request*` and `Response*`
@@ -901,7 +903,7 @@ for people who forked CometBFT and interact directly with the indexers kvstore.
 - `[light]` Fixed an edge case where a light client would panic when attempting
   to query a node that (1) has started from a non-zero height and (2) does
   not yet have any data. The light client will now, correctly, not panic
-  _and_ keep the node in its list of providers in the same way it would if
+  *and* keep the node in its list of providers in the same way it would if
   it queried a node starting from height zero that does not yet have data
   ([\#575](https://github.com/cometbft/cometbft/issues/575))
 - `[mempool/clist_mempool]` Prevent a transaction to appear twice in the mempool
@@ -1049,7 +1051,7 @@ See below for more details.
   queue is full: retry block request after a timeout
   ([\#9518](https://github.com/tendermint/tendermint/pull/9518))
 - `[consensus]` ([\#386](https://github.com/cometbft/cometbft/pull/386)) Short-term fix for the case when `needProofBlock` cannot find previous block meta by defaulting to the creation of a new proof block. (@adizere)
-  - Special thanks to the [Vega.xyz](https://vega.xyz/) team, and in particular to Zohar (@ze97286), for reporting the problem and working with us to get to a fix.
+    - Special thanks to the [Vega.xyz](https://vega.xyz/) team, and in particular to Zohar (@ze97286), for reporting the problem and working with us to get to a fix.
 - `[consensus]` Fixed a busy loop that happened when sending of a block part failed by sleeping in case of error.
   ([\#4](https://github.com/informalsystems/tendermint/pull/4))
 - `[consensus]` fix round number of `enterPropose`
@@ -1142,7 +1144,7 @@ to this release!
 - `[consensus]` Short-term fix for the case when `needProofBlock` cannot find
   previous block meta by defaulting to the creation of a new proof block.
   ([\#386](https://github.com/cometbft/cometbft/pull/386): @adizere)
-  - Special thanks to the [Vega.xyz](https://vega.xyz/) team, and in particular
+    - Special thanks to the [Vega.xyz](https://vega.xyz/) team, and in particular
     to Zohar (@ze97286), for reporting the problem and working with us to get to
     a fix.
 - `[p2p]` Correctly use non-blocking `TrySendEnvelope` method when attempting to
@@ -1168,7 +1170,7 @@ to this release!
 ### FEATURES
 
 - `[rpc]` Add `match_event` query parameter to indicate to the RPC that it
-  should match events _within_ attributes, not only within a height
+  should match events *within* attributes, not only within a height
   ([tendermint/tendermint\#9759](https://github.com/tendermint/tendermint/pull/9759))
 
 ### IMPROVEMENTS

internal/bits/bit_array.go

@@ -28,7 +28,7 @@ func NewBitArray(bits int) *BitArray {
 	}
 	return &BitArray{
 		Bits:  bits,
-		Elems: make([]uint64, (bits+63)/64),
+		Elems: make([]uint64, numElements(bits)),
 	}
 }
 
@@ -41,7 +41,7 @@ func NewBitArrayFromFn(bits int, fn func(int) bool) *BitArray {
 	}
 	bA := &BitArray{
 		Bits:  bits,
-		Elems: make([]uint64, (bits+63)/64),
+		Elems: make([]uint64, numElements(bits)),
 	}
 	for i := 0; i < bits; i++ {
 		v := fn(i)
@@ -90,7 +90,7 @@ func (bA *BitArray) SetIndex(i int, v bool) bool {
 }
 
 func (bA *BitArray) setIndex(i int, v bool) bool {
-	if i >= bA.Bits {
+	if i >= bA.Bits || i/64 >= len(bA.Elems) {
 		return false
 	}
 	if v {
@@ -121,7 +121,7 @@ func (bA *BitArray) copy() *BitArray {
 }
 
 func (bA *BitArray) copyBits(bits int) *BitArray {
-	c := make([]uint64, (bits+63)/64)
+	c := make([]uint64, numElements(bits))
 	copy(c, bA.Elems)
 	return &BitArray{
 		Bits:  bits,
@@ -282,6 +282,11 @@ func (bA *BitArray) PickRandom(r *rand.Rand) (int, bool) {
 }
 
 func (bA *BitArray) getNumTrueIndices() int {
+	if bA.Size() == 0 || len(bA.Elems) == 0 || len(bA.Elems) != numElements(bA.Size()) {
+		// size and elements must be valid to do this calc
+		return 0
+	}
+
 	count := 0
 	numElems := len(bA.Elems)
 	// handle all elements except the last one
@@ -500,3 +505,22 @@ func (bA *BitArray) FromProto(protoBitArray *cmtprotobits.BitArray) {
 		bA.Elems = protoBitArray.Elems
 	}
 }
+
+// ValidateBasic validates a BitArray. Note that a nil BitArray and BitArray of
+// size 0 bits is valid. However the number of Bits and Elems be valid based on
+// each other.
+func (bA *BitArray) ValidateBasic() error {
+	if bA == nil {
+		return nil
+	}
+
+	expectedElems := numElements(bA.Size())
+	if expectedElems != len(bA.Elems) {
+		return fmt.Errorf("mismatch between specified number of bits %d, and number of elements %d, expected %d elements", bA.Size(), len(bA.Elems), expectedElems)
+	}
+	return nil
+}
+
+func numElements(bits int) int {
+	return (bits + 63) / 64
+}

internal/bits/bit_array_test.go

@@ -177,6 +177,28 @@ func TestGetNumTrueIndices(t *testing.T) {
 	}
 }
 
+func TestGetNumTrueIndicesInvalidStates(t *testing.T) {
+	testCases := []struct {
+		name string
+		bA1  *BitArray
+		exp  int
+	}{
+		{"empty", &BitArray{}, 0},
+		{"explicit 0 bits nil elements", &BitArray{Bits: 0, Elems: nil}, 0},
+		{"explicit 0 bits 0 len elements", &BitArray{Bits: 0, Elems: make([]uint64, 0)}, 0},
+		{"nil", nil, 0},
+		{"with elements", NewBitArray(10), 0},
+		{"more elements than bits specifies", &BitArray{Bits: 0, Elems: make([]uint64, 5)}, 0},
+		{"less elements than bits specifies", &BitArray{Bits: 200, Elems: make([]uint64, 1)}, 0},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			n := tc.bA1.getNumTrueIndices()
+			require.Equal(t, n, tc.exp)
+		})
+	}
+}
+
 func TestGetNthTrueIndex(t *testing.T) {
 	type testcase struct {
 		Input          string
@@ -230,7 +252,7 @@ func TestGetNthTrueIndex(t *testing.T) {
 	}
 }
 
-func TestBytes(_ *testing.T) {
+func TestBytes(t *testing.T) {
 	bA := NewBitArray(4)
 	bA.SetIndex(0, true)
 	check := func(bA *BitArray, bz []byte) {
@@ -257,6 +279,10 @@ func TestBytes(_ *testing.T) {
 	check(bA, []byte{0x80, 0x01})
 	bA.SetIndex(9, true)
 	check(bA, []byte{0x80, 0x03})
+
+	bA = NewBitArray(4)
+	bA.Elems = nil
+	require.False(t, bA.SetIndex(1, true))
 }
 
 func TestEmptyFull(t *testing.T) {
@@ -374,6 +400,28 @@ func TestBitArrayProtoBuf(t *testing.T) {
 	}
 }
 
+func TestBitArrayValidateBasic(t *testing.T) {
+	testCases := []struct {
+		name    string
+		bA1     *BitArray
+		expPass bool
+	}{
+		{"valid empty", &BitArray{}, true},
+		{"valid explicit 0 bits nil elements", &BitArray{Bits: 0, Elems: nil}, true},
+		{"valid explicit 0 bits 0 len elements", &BitArray{Bits: 0, Elems: make([]uint64, 0)}, true},
+		{"valid nil", nil, true},
+		{"valid with elements", NewBitArray(10), true},
+		{"more elements than bits specifies", &BitArray{Bits: 0, Elems: make([]uint64, 5)}, false},
+		{"less elements than bits specifies", &BitArray{Bits: 200, Elems: make([]uint64, 1)}, false},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.bA1.ValidateBasic()
+			require.Equal(t, err == nil, tc.expPass)
+		})
+	}
+}
+
 // Tests that UnmarshalJSON doesn't crash when no bits are passed into the JSON.
 // See issue https://github.com/cometbft/cometbft/issues/2658
 func TestUnmarshalJSONDoesntCrashOnZeroBits(t *testing.T) {

internal/consensus/reactor.go

@@ -1758,6 +1758,9 @@ func (m *NewValidBlockMessage) ValidateBasic() error {
 	if err := m.BlockPartSetHeader.ValidateBasic(); err != nil {
 		return cmterrors.ErrWrongField{Field: "BlockPartSetHeader", Err: err}
 	}
+	if err := m.BlockParts.ValidateBasic(); err != nil {
+		return fmt.Errorf("validating BlockParts: %w", err)
+	}
 	if m.BlockParts.Size() == 0 {
 		return cmterrors.ErrRequiredField{Field: "blockParts"}
 	}
@@ -1812,6 +1815,9 @@ func (m *ProposalPOLMessage) ValidateBasic() error {
 	if m.ProposalPOLRound < 0 {
 		return cmterrors.ErrNegativeField{Field: "ProposalPOLRound"}
 	}
+	if err := m.ProposalPOL.ValidateBasic(); err != nil {
+		return fmt.Errorf("validating ProposalPOL: %w", err)
+	}
 	if m.ProposalPOL.Size() == 0 {
 		return cmterrors.ErrRequiredField{Field: "ProposalPOL"}
 	}
@@ -1957,6 +1963,9 @@ func (m *VoteSetBitsMessage) ValidateBasic() error {
 	if err := m.BlockID.ValidateBasic(); err != nil {
 		return cmterrors.ErrWrongField{Field: "BlockID", Err: err}
 	}
+	if err := m.Votes.ValidateBasic(); err != nil {
+		return fmt.Errorf("validating Votes: %w", err)
+	}
 	// NOTE: Votes.Size() can be zero if the node does not have any
 	if m.Votes.Size() > types.MaxVotesCount {
 		return fmt.Errorf("votes bit array is too big: %d, max: %d", m.Votes.Size(), types.MaxVotesCount)

internal/consensus/reactor_test.go

@@ -882,6 +882,14 @@ func TestNewValidBlockMessageValidateBasic(t *testing.T) {
 			func(msg *NewValidBlockMessage) { msg.BlockParts = bits.NewBitArray(int(types.MaxBlockPartsCount) + 1) },
 			"blockParts bit array size 1602 not equal to BlockPartSetHeader.Total 1",
 		},
+		{
+			func(msg *NewValidBlockMessage) { msg.BlockParts.Elems = nil },
+			"mismatch between specified number of bits 1, and number of elements 0, expected 1 elements",
+		},
+		{
+			func(msg *NewValidBlockMessage) { msg.BlockParts.Bits = 500 },
+			"mismatch between specified number of bits 500, and number of elements 1, expected 8 elements",
+		},
 	}
 
 	for i, tc := range testCases {
@@ -917,6 +925,14 @@ func TestProposalPOLMessageValidateBasic(t *testing.T) {
 			func(msg *ProposalPOLMessage) { msg.ProposalPOL = bits.NewBitArray(types.MaxVotesCount + 1) },
 			"proposalPOL bit array is too big: 10001, max: 10000",
 		},
+		{
+			func(msg *ProposalPOLMessage) { msg.ProposalPOL.Elems = nil },
+			"mismatch between specified number of bits 1, and number of elements 0, expected 1 elements",
+		},
+		{
+			func(msg *ProposalPOLMessage) { msg.ProposalPOL.Bits = 500 },
+			"mismatch between specified number of bits 500, and number of elements 1, expected 8 elements",
+		},
 	}
 
 	for i, tc := range testCases {
@@ -1069,6 +1085,14 @@ func TestVoteSetBitsMessageValidateBasic(t *testing.T) {
 			func(msg *VoteSetBitsMessage) { msg.Votes = bits.NewBitArray(types.MaxVotesCount + 1) },
 			"votes bit array is too big: 10001, max: 10000",
 		},
+		{
+			func(msg *VoteSetBitsMessage) { msg.Votes.Elems = nil },
+			"mismatch between specified number of bits 1, and number of elements 0, expected 1 elements",
+		},
+		{
+			func(msg *VoteSetBitsMessage) { msg.Votes.Bits = 500 },
+			"mismatch between specified number of bits 500, and number of elements 1, expected 8 elements",
+		},
 	}
 
 	for i, tc := range testCases {