feat(state): Update `Proposal` with `BlobID` by alesforz · Pull Request #7 · alesforz/cometbft

alesforz · 2025-03-14T16:35:47Z

Part of the work of berachain#26.

Context

A Proposal represents a block proposal that peers vote on during consensus. To be valid, it must be signed by the correct proposer for the given height and round.

A Proposal includes a BlockID, which uniquely identifies the proposed block. This BlockID contains the Merkle root hash of the block’s parts, ensuring integrity as the block is split and gossiped across the network.

Since blocks can now have an associated blob, we introduce a unique identifier for the blob in the Proposal struct. This allows peers to correctly associate a blob with its corresponding block during gossip. Additionally, because the Proposal is signed, this prevents malicious actors from altering or replacing the blob linked to a specific block.

Changes

This PR extends the Proposal struct and its methods with a BlobID field.

…obID`

Regenerated the proto files as well. Moved the proto definition of `BlobID` to the `types.proto` file to fix an import cycle.

Since `BlobID` is not used in the consensus logic yet, this change is harmless. However, it allows us to keep this PR focused solely on `Proposal`-related changes. In a future PR, we’ll handle the proper creation of `BlobID` by implementing blob splitting into multiple parts. Additionally, this PR includes a refactor for improved readability.

…est.go

The `Proposal` sub-test was using an incorrect hex string to check the test's results. The string changed because we added the `BlobID` field to the `Proposal` type, thus changing its encoding to protobuf. Also rearrenged the code for better readability.

It was using an incorrect hex string to check the test's results. The string changed because we added the `BlobID` field to the `Proposal` type, thus changing its encoding to protobuf. Also rearrenged the code for better readability.

The sub-tests "Proposal Request", "Proposal Response", and "Proposal Response with error" were was using an incorrect hex string to check the test's results. The string changed because we added the `BlobID` field to the `Proposal` type, thus changing its encoding to protobuf. Also rearrenged the code for better readability.

alesforz · 2025-03-17T13:50:30Z

The linter is failing, but that's not a real problem.
It's complaining about not using the blob object, but in the next PR(s) the code will use it.

internal/consensus/state.go

types/canonical.go

types/proposal.go

jmalicevic · 2025-03-18T00:03:49Z

types/proposal.go

-	p.POLRound = pp.PolRound // FIXME: names do not match
-	p.Timestamp = pp.Timestamp
-	p.Signature = pp.Signature
+	blobID, err := BlobIDFromProto(&pp.BlobID)


Same as above, should we be checking if it is nil (maybe the check is performed inside BlobIDFromProto? ) ?

yes, BlobIDFromProto checks if the given argument is nil and returns an error if it is.

But can we not have a valid scenario where the blobID is nil (no blob in this proposal) and therefore it is ok that this is nil ? It should just be properly marshalled somehow.

The BlobID field in the Proposal struct is a concrete type, not a pointer. Therefore, it can't be nil. Similarly, the BlobID field of the

the ToProto() and FromProto() methods of type BlobID return a concrete type, not a pointer. So I don't see how it could be set to nil.

jmalicevic · 2025-03-18T00:07:42Z

Update isProposalComplete() to account for the blob. We need to handle this carefully, as a missing blob could mean either that it hasn’t arrived yet or that the block simply doesn’t have one. We can distinguish between these cases by checking the Proposal: if BlobID is empty, the block has no blob. Otherwise, a blob exists, and we should wait for it.

If this is not supposed to be part of this PR, I would remove it from the description

internal/consensus/state.go

melekes

LGTM minus comments from Jasmina

alesforz · 2025-03-18T08:12:28Z

Update isProposalComplete() to account for the blob. We need to handle this carefully, as a missing blob could mean either that it hasn’t arrived yet or that the block simply doesn’t have one. We can distinguish between these cases by checking the Proposal: if BlobID is empty, the block has no blob. Otherwise, a blob exists, and we should wait for it.

If this is not supposed to be part of this PR, I would remove it from the description

I was thinking whether to include this change or not in this PR 🤔
It can fit here, but I also see it as part of the PR dealing with consensus rounds updates.

Rationale: If a block has an associated blob, we set the `BlobID` in `Proposal` after returning from `PrepareProposal()`, causing the `IsNil()` check to return false. If the block does not have a blob, the `BlobID` field in `Proposal` will remain its zero value, making `IsNil()` return true. If a blob is present, `cs.ProposalBlob.IsNil()` will always return true until `cs.ProposalBlob` is assigned a non-empty `[]byte` slice. This assignment happens only after we have received all the blob parts and we have successfully reconstructed the blob.

alesforz · 2025-03-18T09:23:24Z

Update isProposalComplete() to account for the blob. We need to handle this carefully, as a missing blob could mean either that it hasn’t arrived yet or that the block simply doesn’t have one. We can distinguish between these cases by checking the Proposal: if BlobID is empty, the block has no blob. Otherwise, a blob exists, and we should wait for it.

If this is not supposed to be part of this PR, I would remove it from the description

I was thinking whether to include this change or not in this PR 🤔 It can fit here, but I also see it as part of the PR dealing with consensus rounds updates.

Update, I pushed the change in this PR. See the last commit.

alesforz · 2025-03-18T09:24:11Z

internal/consensus/state.go

+		// wait for the blob before marking the proposal as complete.
+		if !cs.Proposal.BlobID.IsNil() {
+			return !cs.ProposalBlob.IsNil()
+		}


Rationale: If a block has an associated blob, we set the BlobID in Proposal
after returning from PrepareProposal(), causing the IsNil() check to
return false. If the block does not have a blob, the BlobID field in
Proposal will remain its zero value, making IsNil() return true.

If a blob is present, cs.ProposalBlob.IsNil() will always return true until
cs.ProposalBlob is assigned a non-empty []byte slice. This assignment
happens only after we have received all the blob parts and we have successfully
reconstructed the blob.

Alessandro Sforzin added 8 commits March 14, 2025 13:25

Extend type Proposal with BlobID field

5a86188

Update Proposal methods ValidateBasic() and String() to use `Bl…

53c609d

…obID`

Add BlobID to Proposal protobuf.

bd9ce0f

Regenerated the proto files as well. Moved the proto definition of `BlobID` to the `types.proto` file to fix an import cycle.

Add function to canonalize BlobID

c310c03

Update Proposal to/from protobuf functions to include BlobID

7a8c3db

Update Proposal unit tests to use BlobID

bb03d72

Add method IsNil() to type BlobID

5ecf2f0

alesforz self-assigned this Mar 14, 2025

alesforz mentioned this pull request Mar 14, 2025

feat(state): CometBFT does not store blobs berachain/cometbft#26

Open

Alessandro Sforzin added 8 commits March 17, 2025 11:26

Add missing BlobID param to decideProposal()

d7f6af6

Add missing BlobID param in calls to NewProposal() in state_test.go

29ee908

Add missing BlobID param in calls to NewProposal() in replay_test.go

9acc70d

Add missing BlobID param in calls to NewProposal() in pbts_test.go

cfa4c98

Add missing BlobID param in calls to NewProposal() in byzantine_t…

30fce17

…est.go

Fixed unit test TestWALEncoder.

5e83844

It was using an incorrect hex string to check the test's results. The string changed because we added the `BlobID` field to the `Proposal` type, thus changing its encoding to protobuf. Also rearrenged the code for better readability.

Alessandro Sforzin added 2 commits March 17, 2025 14:50

Extend EventDataCompleteProposal with BlobID field

75c637e

Hash() method of type Blob checks return nil if empty blob.

576c737

alesforz marked this pull request as ready for review March 17, 2025 14:04

alesforz requested review from greg-szabo, jmalicevic and melekes March 17, 2025 14:04

jmalicevic reviewed Mar 18, 2025

View reviewed changes

internal/consensus/state.go Show resolved Hide resolved

melekes approved these changes Mar 18, 2025

View reviewed changes

alesforz requested review from jmalicevic and melekes March 18, 2025 08:13

Alessandro Sforzin added 2 commits March 18, 2025 09:53

updated docs

76616d4

alesforz commented Mar 18, 2025

View reviewed changes

jmalicevic approved these changes Mar 18, 2025

View reviewed changes

alesforz merged commit b5e984e into alesforz/feature/blobs Mar 18, 2025
18 of 19 checks passed

Conversation

alesforz commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

Uh oh!

alesforz commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmalicevic Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

alesforz Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

jmalicevic Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

alesforz Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

jmalicevic commented Mar 18, 2025

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

alesforz commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alesforz commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alesforz Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alesforz commented Mar 14, 2025 •

edited

Loading

alesforz commented Mar 18, 2025 •

edited

Loading

alesforz commented Mar 18, 2025 •

edited

Loading