Skip to content

feat(consensus): blobs feature#17

Merged
marcello33 merged 166 commits into
0xPolygon:developfrom
greg-szabo:greg/blobs
May 7, 2025
Merged

feat(consensus): blobs feature#17
marcello33 merged 166 commits into
0xPolygon:developfrom
greg-szabo:greg/blobs

Conversation

@greg-szabo
Copy link
Copy Markdown

The blobs feature allows a blob of bytes to be part of the consensus mechanism without including it in the block store. This blob can contain data interpreted by the application.

Alessandro Sforzin and others added 30 commits April 7, 2025 13:19
Tests that a call to `PrepareProposal` returns a blob inr
`PrepareProposalResponse`.
Type `Blob` is a `[]byte` slice, so there's no need for it to be a pointer.
It now returns a different string when the blob is nil, as type `Block` does.
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Signed-off-by: Alessandro Sforzin <alesforz@gmail.com>
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.
greg-szabo and others added 16 commits April 23, 2025 21:04
Context

The RoundState is a data structure keeping track of the status of the consensus algorithm at the current height.
Because CometBFT will gossip blobs separately rather than including them into blocks, the RoundState must keep track of them during consensus rounds at any given height.
Changes

This PR updates the RoundState and RoundStateSimple data structures to include fields handling the blob during rounds of consensus.

Note: another PR will take care of updating these new RoundState fields during consensus.
feat(state): Update `Proposal` with `BlobID`
feat(state): `RoundState` does not need `Valid*` and `Locked*` blob fields.
feat(state): Add `BlobPartMessage` and `HasBlobPartMessage` to consensus state and reactor
feat(abci): Add `Blob` to `ProcessProposalRequest`
feat(state): Split `Blob` into blob parts and send it to internal message queue
feat(state): Add blob-related fields to `PeerRoundState`
feat(consensus): Gossip block parts
feat(e2e): test app update to allow blob testing
feat(blob): Implement basic blob metrics
@marcello33
Copy link
Copy Markdown

LGTM generally speaking @greg-szabo
However I think we still miss smth from upstream (see failing tests) like t.BlobMaxBytes undefined (type Testnet has no field or method BlobMaxBytes), a bump to go version to fix govuln and the fixes to the linter (given that it has been migrated to v2 I believe)

@greg-szabo
Copy link
Copy Markdown
Author

Regarding the issues that surfaced on the first review:

  1. t.BlobMaxBytes undefined: this is a v1.x "mannerism" that is unrelated to the blob feature but the feature touched on it. In v1.x, the Testnet struct is derived from Manifest. (see PR 4308 in the cometbft repo) This was not backported to v0.38. In the Blobs feature, we introduce a new field for the Manifest struct, and because of inheritance, the field shows up in the Testnet struct too. In v0.38, that inheritance is not implemented, hence we have to explicitly add the field to the Testnet struct. (Alternatively, we could implement the inheritance in 4308 but you will see that it's a refactor to simplify these two structs.) I pushed a commit to update the Testnet struct.
  2. The Go bump version issue manifested in a third-party Docker image named cometbft-db-testing. That image is used in e2e testing but it still has (the now obsolete) Go 1.22.11 in it. I opened an issue and a PR to update the source code that creates that Docker image. I'm working on getting an approval, merging the change and getting a new version released as soon as possible. After that, we can update the Dockerfile to point to the new version of the image.

I can do a merge from develop into the branch to apply all new updates since the branch was cut.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 1, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
18.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@greg-szabo
Copy link
Copy Markdown
Author

I fixed the code issues and the e2e test bug. The nightly tests I ran locally all checked out.

@marcello33
Copy link
Copy Markdown

Thanks @greg-szabo
Do you think we can also fix the (new) linter:

  Error: consensus/byzantine_test.go:467:1: File is not properly formatted (gofmt)
  	blobID    := types.BlobID{
  ^
  Error: consensus/state_test.go:7:1: File is not properly formatted (goimports)
  	"github.com/cometbft/cometbft/mempool"
  ^
  Error: consensus/common_test.go:275:12: ineffectual assignment to propBlockID (ineffassign)
  	block, _, propBlockID, blob := createProposalBlockAndBlob(t, cs1)
  	          ^
  
  Error: issues found

And the govulncheck (go version bump and x/crypto bump)?

=== Symbol Results ===

Vulnerability #1: GO-2025-3563
  Request smuggling due to acceptance of invalid chunked data in net/http
More info: https://pkg.go.dev/vuln/GO-2025-3563
Standard library
  Found in: net/http/internal@go1.22.12
  Fixed in: net/http/internal@go1.23.8
  Example traces found:
Error:       #1: rpc/jsonrpc/client/http_json_client.go:235:34: client.Client.Call calls io.ReadAll, which eventually calls internal.chunkedReader.Read

Vulnerability #2: GO-2025-3487
  Potential denial of service in golang.org/x/crypto
More info: https://pkg.go.dev/vuln/GO-2025-3487
Module: golang.org/x/crypto
  Found in: golang.org/x/crypto@v0.32.0
  Fixed in: golang.org/x/crypto@v0.35.0
  Example traces found:
Error:       #1: libs/os/os.go:110:[18](https://github.com/0xPolygon/cometbft/actions/runs/14782484466/job/41642574301?pr=17#step:5:19): os.CopyFile calls io.Copy, which eventually calls ssh.extChannel.Read

Your code is affected by 2 vulnerabilities from 1 module and the Go standard library.
This scan also found 1 vulnerability in packages you import and 1 vulnerability
in modules you require, but your code doesn't appear to call these
vulnerabilities.

Then I think we're good to go 🚀

@marcello33 marcello33 merged commit 80ddb60 into 0xPolygon:develop May 7, 2025
15 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants