fix(internal/consensus/reactor.go): reject oversized proposals by arsushi · Pull Request #5309 · cometbft/cometbft

arsushi · 2025-08-25T14:20:13Z

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

aljo242 · 2025-08-25T14:29:58Z

internal/consensus/reactor.go

 		}
 		switch msg := msg.(type) {
 		case *ProposalMessage:
+			maxBytes := conR.conS.state.ConsensusParams.Block.MaxBytes


Is there a way we can test this?

Added test case in 6639886

almk-dev

Can you explain the -1/+1 logic in a comment?

internal/consensus/reactor.go

arsushi · 2025-08-26T06:02:23Z

Can you explain the -1/+1 logic in a comment?

It is based on the validation in

cometbft/internal/consensus/state.go

Lines 2075 to 2082 in 88e9ea6

    
           // Validate the proposed block size, derived from its PartSetHeader 
        
           maxBytes := cs.state.ConsensusParams.Block.MaxBytes 
        
           if maxBytes == -1 { 
        
           	maxBytes = int64(types.MaxBlockSizeBytes) 
        
           } 
        
           if int64(proposal.BlockID.PartSetHeader.Total) > (maxBytes-1)/int64(types.BlockPartSizeBytes)+1 { 
        
           	return ErrProposalTooManyParts 
        
           }

Co-authored-by: Abdul Malek <me@almk.dev>

aljo242 · 2025-08-26T18:17:17Z

@arsushi looks like we have some CI failing!

aljo242 · 2025-08-26T18:26:04Z

@arsushi can we also get a changelog entry for this?

mattac21 · 2025-09-15T21:08:29Z

internal/consensus/byzantine_test.go

+	}
+	proposal.Signature = p.Signature
+
+	go sendProposalAndParts(height, round, cs, targetPeer, proposal, block, block.Hash(), blockParts)


I'm not sure this test case is actually testing anything. This looks to me that it creates the oversized proposal, and then sends the proposal and parts to the peer (where it will be rejected), but it doesnt actually asset that the peer rejected it, it returns before anything is asserted about the peer state.

--- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here #5309, but reopened since the contributor stopped replying. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com>

--- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here #5309, but reopened since the contributor stopped replying. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com> (cherry picked from commit 9ff47d0)

…5407) --- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here #5309, but reopened since the contributor stopped replying. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request #5324 done by [Mergify](https://mergify.com). Co-authored-by: Alex | Interchain Labs <alex@cosmoslabs.io> Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com>

…#5324) (cometbft#5407) --- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here cometbft#5309, but reopened since the contributor stopped replying. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request cometbft#5324 done by [Mergify](https://mergify.com). Co-authored-by: Alex | Interchain Labs <alex@cosmoslabs.io> Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com>

…#5324) (cometbft#5407) --- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here cometbft#5309, but reopened since the contributor stopped replying. - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request cometbft#5324 done by [Mergify](https://mergify.com). Co-authored-by: Alex | Interchain Labs <alex@cosmoslabs.io> Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com>

@odeke-em

* fix(bits): prevent BitArray.UnmarshalJSON from crashing on 0 bits in the JSON (backport cometbft#2774) (cometbft#2778) This change fixes a bug in which BitArray.UnmarshalJSON hadn't accounted for the fact that invoking NewBitArray(<=0) returns nil and hence when dereferenced would crash with a runtime nil pointer dereference. This bug was found by my security analysis and fuzzing too. Author: @odeke-em Fixes cometbft#2658 --- - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] ~~Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request cometbft#2774 done by [Mergify](https://mergify.com). --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> * fix(consensus/reactor): reject oversized proposals (backport cometbft#5324) (cometbft#5407) --- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here cometbft#5309, but reopened since the contributor stopped replying. - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request cometbft#5324 done by [Mergify](https://mergify.com). Co-authored-by: Alex | Interchain Labs <alex@cosmoslabs.io> Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com> * 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 * add necessary test constants --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Alex | Interchain Labs <alex@cosmoslabs.io> Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com> Co-authored-by: Matt Acciai <matt@cosmoslabs.io>

…#5324) (cometbft#5407) --- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here cometbft#5309, but reopened since the contributor stopped replying. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request cometbft#5324 done by [Mergify](https://mergify.com). Co-authored-by: Alex | Interchain Labs <alex@cosmoslabs.io> Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com>

--- Updates the consensus reactor to validate that a received proposal will not contain more parts than the amount of chunks that it would take to build a block whos size is equal to `ConsensusParams.Block.MaxBytes`. Original PR is here #5309, but reopened since the contributor stopped replying. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: arsushi <richie@asymmetric.re> Co-authored-by: Abdul Malek <me@almk.dev> Co-authored-by: Matt Acciai <matt@skip.money> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com>

reject oversized proposals

88e9ea6

arsushi requested review from a team as code owners August 25, 2025 14:20

arsushi changed the title ~~Reject oversized proposals~~ fix(internal/consensus/reactor.go): reject oversized proposals Aug 25, 2025

aljo242 added backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x backport-to-v1.x labels Aug 25, 2025

aljo242 self-assigned this Aug 25, 2025

aljo242 reviewed Aug 25, 2025

View reviewed changes

almk-dev reviewed Aug 25, 2025

View reviewed changes

internal/consensus/reactor.go Outdated Show resolved Hide resolved

arsushi and others added 2 commits August 26, 2025 14:02

Update internal/consensus/reactor.go

621956b

Co-authored-by: Abdul Malek <me@almk.dev>

add TestRejectOversizedProposals test

6639886

aljo242 assigned Eric-Warehime Aug 26, 2025

Merge branch 'main' into main

a8f765c

aljo242 removed backport-to-v1.x labels Sep 11, 2025

mattac21 reviewed Sep 15, 2025

View reviewed changes

mattac21 mentioned this pull request Sep 16, 2025

fix(consensus/reactor): reject oversized proposals #5324

Merged

3 tasks

mattac21 closed this Sep 16, 2025

mergify bot mentioned this pull request Sep 17, 2025

fix(consensus/reactor): reject oversized proposals (backport #5324) #5407

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(internal/consensus/reactor.go): reject oversized proposals#5309

fix(internal/consensus/reactor.go): reject oversized proposals#5309
arsushi wants to merge 4 commits intocometbft:mainfrom
arsushi:main

arsushi commented Aug 25, 2025

Uh oh!

aljo242 Aug 25, 2025

Uh oh!

arsushi Aug 26, 2025

Uh oh!

almk-dev left a comment

Uh oh!

Uh oh!

arsushi commented Aug 26, 2025 •

edited

Loading

Uh oh!

aljo242 commented Aug 26, 2025

Uh oh!

aljo242 commented Aug 26, 2025

Uh oh!

mattac21 Sep 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

arsushi commented Aug 25, 2025

PR checklist

Uh oh!

aljo242 Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

arsushi Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

almk-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arsushi commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aljo242 commented Aug 26, 2025

Uh oh!

aljo242 commented Aug 26, 2025

Uh oh!

mattac21 Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arsushi commented Aug 26, 2025 •

edited

Loading