Conversation
|
The latest Buf updates on your PR. Results from workflow Protobuf Lint / lint (pull_request).
|
4679819 to
3d10c38
Compare
1d5e119 to
316c364
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Aggregation across validator-set transitions, one-height commit catch-up, and conflicting persisted key encodings can cause panics, stalls, or divergent recovery.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Rebases the Berachain CometBFT fork onto v0.40 while preserving bera-v1.x consensus, wire, and storage compatibility.
Changes:
- Ports PBTS, BLS commit aggregation, and ADR-115 behavior.
- Adds dual validator-key encoding, WAL receive times, and sync-height propagation.
- Adds compatibility tests and updates CI workflows.
File summaries
| File | Description |
|---|---|
version/version.go |
Sets Berachain v0.40 version. |
types/vote.go |
Removes vote timestamps from consensus. |
types/vote_test.go |
Updates vote vectors. |
types/vote_set.go |
Adds BLS commit aggregation. |
types/vote_set_test.go |
Updates vote-set tests. |
types/validator.go |
Adds fork-compatible key encoding. |
types/validator_set_test.go |
Updates validator proto tests. |
types/time/time.go |
Adds injectable time source. |
types/time/mocks/source.go |
Adds generated time mock. |
types/test_util.go |
Updates vote test helpers. |
types/protobuf.go |
Supports dual validator encodings. |
types/protobuf_test.go |
Tests validator encoding compatibility. |
types/proposal.go |
Adds PBTS proposal timestamps. |
types/proposal_test.go |
Updates proposal tests. |
types/priv_validator.go |
Adds arbitrary-byte signing. |
types/params_test.go |
Updates feature parameter tests. |
types/evidence.go |
Removes vote timestamps from evidence. |
types/events.go |
Adds block-part event. |
types/canonical.go |
Excludes vote timestamps from sign bytes. |
types/block_test.go |
Updates commit tests. |
tests.mk |
Enables crypto build tags. |
test/e2e/runner/setup.go |
Uses feature parameters. |
test/e2e/networks/ci.toml |
Disables vote extensions. |
test/e2e/networks_regressions/blocksync_blocked.toml |
Updates regression configuration. |
test/e2e/generator/generate.go |
Disables generated vote extensions. |
test/e2e/app/app.go |
Migrates feature parameters. |
test/compat/README.md |
Documents compatibility testing. |
test/compat/gen-v1x/tools.go |
Tracks compatibility binary dependencies. |
test/compat/gen-v1x/go.mod |
Pins bera-v1.x dependencies. |
store/store_test.go |
Updates commit fixtures. |
state/validation.go |
Adds PBTS block-time validation. |
state/validation_test.go |
Tests PBTS validation. |
state/store.go |
Normalizes persisted validator updates. |
state/store_test.go |
Tests dual key persistence. |
state/state.go |
Persists delay and creates PBTS blocks. |
state/helpers_test.go |
Passes synchronization height. |
state/execution.go |
Adds delay, proposer, and sync metadata. |
state/execution_test.go |
Updates execution tests. |
spec/abci/abci++_methods.md |
Documents synchronization height. |
rpc/client/evidence_test.go |
Uses committed block time. |
proto/tendermint/types/validator.proto |
Adds aggregate flags and key fields. |
proto/tendermint/types/params.proto |
Adds synchrony and feature parameters. |
proto/tendermint/types/canonical.proto |
Reserves vote timestamp field. |
proto/tendermint/state/types.proto |
Persists next-block delay. |
proto/tendermint/privval/types.proto |
Adds SignBytes messages. |
proto/tendermint/consensus/wal.proto |
Persists message receive time. |
proto/tendermint/consensus/types.proto |
Adds commit gossip messages. |
proto/tendermint/consensus/message.go |
Wraps new consensus messages. |
proto/tendermint/abci/types.proto |
Extends ABCI fork fields. |
proto/buf.yaml |
Exempts intentional wire changes. |
privval/signer_requestHandler.go |
Handles byte-sign requests. |
privval/signer_client.go |
Implements remote byte signing. |
privval/signer_client_test.go |
Tests remote byte signing. |
privval/retry_signer_client.go |
Adds retrying byte signing. |
privval/msgs.go |
Wraps byte-sign messages. |
privval/file.go |
Implements local byte signing. |
privval/file_test.go |
Updates timestamp-signing tests. |
node/setup.go |
Adjusts seed disconnect timing. |
internal/test/params.go |
Migrates test feature parameters. |
internal/test/config.go |
Updates test genesis parameters. |
internal/test/commit.go |
Uses feature parameters. |
evidence/pool_test.go |
Updates evidence size expectation. |
crypto/encoding/codec.go |
Adds compressed BLS decoding. |
crypto/bls12381/key.go |
Extends disabled BLS stubs. |
crypto/bls12381/key_bls12381.go |
Adds compressed public keys. |
crypto/bls12381/const.go |
Defines compressed key size. |
crypto/bls12381/aggregation.go |
Implements signature aggregation. |
crypto/bls12381/aggregation_test.go |
Tests BLS aggregation. |
crypto/bls12381/aggregation_none.go |
Adds disabled aggregation stubs. |
consensus/types/round_state.go |
Supports whole last commits. |
consensus/types/peer_round_state.go |
Tracks catch-up commits. |
consensus/types/height_vote_set.go |
Stores aggregated commits. |
consensus/state.go |
Integrates PBTS and aggregation. |
consensus/state_test.go |
Updates consensus tests. |
consensus/state_ingest.go |
Updates verified-block ingestion. |
consensus/state_ingest_test.go |
Updates ingestion tests. |
consensus/replay.go |
Replays receive and sync metadata. |
consensus/replay_test.go |
Updates replay tests. |
consensus/reactor.go |
Gossips whole aggregated commits. |
consensus/reactor_test.go |
Tests commit gossip limits. |
consensus/msgs.go |
Converts commit and WAL messages. |
consensus/msgs_test.go |
Tests new wire messages. |
consensus/metrics.go |
Defines PBTS and delay metrics. |
consensus/metrics.gen.go |
Generates new metrics. |
consensus/mempool_test.go |
Updates proposal callback tests. |
consensus/invalid_test.go |
Removes vote timestamps. |
consensus/common_test.go |
Adds PBTS test helpers. |
consensus/byzantine_test.go |
Updates Byzantine scenarios. |
consensus/aggregation_replay_test.go |
Tests aggregated WAL replay. |
consensus/aggregation_reactor_test.go |
Tests aggregated consensus. |
consensus/aggregation_pbts_test.go |
Tests PBTS rejection. |
CHANGELOG.md |
Documents the fork. |
buf.gen.yaml |
Adds wrapper generation mapping. |
blocksync/reactor.go |
Propagates sync target height. |
blocksync/reactor_test.go |
Updates block-sync tests. |
.golangci.yml |
Allows BLS imports. |
.gitignore |
Ignores compatibility binary. |
.github/workflows/*.yml |
Pins actions and updates runners. |
.github/workflows/compat-rolling-upgrade.yml |
Adds rolling-upgrade CI. |
Review details
Files not reviewed (1)
- abci/types/types.pb.go: Generated file
- Files reviewed: 135/138 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _, blsKey := cs.privValidatorPubKey.(*bls12381.PubKey) | ||
| _, blsKey2 := cs.privValidatorPubKey.(bls12381.PubKey) | ||
| canBeAggregated := (blsKey || blsKey2) && | ||
| cs.state.Validators.AllKeysHaveSameType() |
| // Catchup logic | ||
| // If peer is lagging by more than 1, send Commit. | ||
| blockStoreBase := conS.blockStore.Base() | ||
| if blockStoreBase > 0 && prs.Height != 0 && rs.Height >= prs.Height+2 && prs.Height >= blockStoreBase { |
| if out[i].PubKeyType == "" { | ||
| out[i].PubKeyType = pub.Type() | ||
| out[i].PubKeyBytes = pub.Bytes() | ||
| } |
| return ic.extCommit.Round, ic.extCommit.ToExtendedVoteSet(chainID, vals), nil | ||
| } | ||
|
|
||
| return ic.commit.Round, ic.commit.ToVoteSet(chainID, vals), nil |
There was a problem hiding this comment.
spotted a call to ToVoteSet which has been removed on its other occurrence here
Line 702 in 316c364
This should cause a recovered panic on aggregated commits, only reachable via adaptive_sync. Just signaling it in case we also want to adapt this.
| "validator_address", val.Address.String(), | ||
| } | ||
| cs.metrics.ValidatorPower.With(label...).Set(float64(val.VotingPower)) | ||
| if commitSig.BlockIDFlag == types.BlockIDFlagCommit { |
There was a problem hiding this comment.
i think here we should port this fix 235ba078f if we care about those metrics
| var sum int64 | ||
| var extCount int | ||
| for _, vote := range extCommit.Votes { | ||
| if vote.BlockIdFlag == cmtproto.BlockIDFlagUnknown || vote.BlockIdFlag > cmtproto.BlockIDFlagNil { |
There was a problem hiding this comment.
Minor correction here to port correct case handling from bera-v1.x
| if vote.BlockIdFlag == cmtproto.BlockIDFlagUnknown || vote.BlockIdFlag > cmtproto.BlockIDFlagAggNilAbsent { |
| if vote.BlockIdFlag == cmtproto.BlockIDFlagUnknown || vote.BlockIdFlag > cmtproto.BlockIDFlagNil { | ||
| return 0, fmt.Errorf("vote with bad blockID flag value at height %d; blockID flag %d", currentHeight, vote.BlockIdFlag) | ||
| } | ||
| if vote.BlockIdFlag == cmtproto.BlockIDFlagAbsent || vote.BlockIdFlag == cmtproto.BlockIDFlagNil { |
There was a problem hiding this comment.
| if vote.BlockIdFlag == cmtproto.BlockIDFlagAbsent || vote.BlockIdFlag == cmtproto.BlockIDFlagNil || vote.BlockIdFlag == cmtproto.BlockIDFlagAggNil || vote.BlockIdFlag == cmtproto.BlockIDFlagAggNilAbsent { |
| timestamp = state.LastBlockTime // genesis time | ||
| } else { | ||
| default: | ||
| ts, err := MedianTime(lastCommit, state.LastValidators) |
There was a problem hiding this comment.
bera-v1.x was explicitly panicking here, shall we?
// Set time.
var timestamp time.Time
switch {
case state.ConsensusParams.Feature.PbtsEnabled(height):
timestamp = cmttime.Now()
default:
panic("PBTS has to be enabled")
}
maybe even rejecting PbtsEnableHeight != 1 at ValidateBasic (types/params.go:339) so the panic is directly at startup.
| @@ -1 +1 @@ | |||
| version: "2" | |||
There was a problem hiding this comment.
//go:build bls12381 files are excluded by the linting process.
| version: "2" | |
| run: | |
| build-tags: | |
| - bls12381 |
This PR moves the Berachain CometBFT re-fork from upstream v0.39.x to upstream v0.40.x. It establishes the new
bera-v0.40.xline, which starts from the pristine upstream tagv0.40.0and carries the same Berachain changes that the v0.39 re-fork (#51) carried. The v0.39 re-fork was still a draft and never reached testnet or mainnet, so this PR replaces it rather than building on it. Thebera-v0.39.xbranch was not merged here. Everything was recreated on top ofv0.40.0, so the branch keeps clean upstream ancestry.The goal is unchanged from #51. A
bera-v1.xnetwork can move to this binary with a rolling restart and no state migration, because the consensus sign bytes, hashes, wire format and on-disk state are byte identical tobera-v1.x. The difference this time is that the claim is backed by committed tests (see Testing) instead of an out-of-tree harness.Why v0.40
Upstream supports two release lines at a time, and v0.40 is now the newer of the two. Since the v0.39 re-fork had not been deployed anywhere yet, starting the Berachain line on v0.40.0 costs nothing today and gives us the longest runway before the next forced move. It also means the first upstream sync merges we do will be small v0.40.x patch releases rather than a jump from v0.39 to v0.40.
What is in it
The Berachain feature set is exactly the one described in #51, ported without behavior changes:
has_proposal_block_partat 10, wholecommitat 11).SignBytesrequest and compressed BLS public key support.MaxVotesCountcap on gossiped commits, race-freeCommit.String, vote gossip skipping a whole-commit LastCommit).On top of that, four small additions that came out of testing against a real bera-v1.x binary. All four are additive and wire compatible:
ValidatorUpdateunderstands both public key encodings. bera-v1.x writespub_key_bytespluspub_key_type, upstream writespub_key. FinalizeBlock responses are persisted in the state store and read back during crash recovery and for/block_results, so the node now reads either form and writes both. This keeps the store readable in both directions of a rolling upgrade.ubuntu-latestinstead of the upstream depot runners, the same fix that was applied tobera-v0.39.x.RequestFinalizeBlockgetssyncing_to_heightback (field 9, feat(proto): addSyncingToHeighttoFinalizeBlockRequestcometbft/cometbft#3134, native on bera-v1.x but absent from v0.40.0). Block sync passes the highest peer height, consensus and replay pass the block height. beacon-kit uses it to skip blobs outside the DA window and for/eth/v1/node/syncing; without it a node syncing from genesis reportedis_syncing=falseand processed every historical blob. Not persisted or gossiped, so vectors and wire format are unchanged.How this was built
The eight commits of #51 were cherry-picked one at a time onto
v0.40.0. Most of them applied cleanly. Conflicts came up in a handful of files where upstream moved between v0.39.4 and v0.40.0, and they were resolved by hand:state/execution.go. Upstream added a per-block validator set cache and turned thebuildLastCommitInfoFromStorehelpers into methods onBlockExecutor. The port keeps upstream's methods and threadsFeatureParamsandNextProposerAddressthrough them.types/params.goandcrypto/encoding/codec.go. Upstream added the ml-dsa-65 and secp256k1eth key types. They are kept, next to the synchrony and feature params from the port.privval/retry_signer_client.go. Upstream made retries abortable on close. The portedSignBytesfollows the same pattern.version/version.go,CHANGELOG.md, one test file.A file-by-file comparison with the #51 tree, after subtracting everything upstream changed between v0.39.4 and v0.40.0, leaves only the four additions above. In other words, the Berachain payload is the same diff as before, re-based by hand.
How to review
The bulk of the diff is the same code that was reviewed in #51, and the same split applies. The generated
.pb.gofiles,crypto/bls12381,verifyAggregatedCommit,MakeBLSCommit,IsTimelyand most ofconsensus/state.goare faithful ports and can be skimmed.Worth a closer look:
state/execution.gois the largest one.consensus/msgs.goandproto/tendermint/consensus/wal.protofor the WAL receive time.state/execution.goand its callers inblocksync/reactor.go,consensus/state.go,consensus/state_ingest.go,consensus/replay.goforsyncing_to_height. Thetypes.pb.godiff is justmake proto-genoutput.types/protobuf.go(PubKeyFromValidatorUpdate,NormalizeValidatorUpdates) andstate/store.gofor the validator update encoding.test/compat/, which is new. Start with its README.Upstream changes between v0.39.4 and v0.40.0
Every upstream commit in that range was checked for interaction with the port. The ones that matter:
MedianTimeno longer counts nil precommits (upstream marks this state-breaking). It does not change anything here.MedianTimeis only used on the BFT Time path, and this fork forces PBTS, exactly as bera-v1.x does.MaxSignatureSizegrew to fit ml-dsa-65 signatures, which shrinks the maximum tx bytes a proposer offers by a few kilobytes per validator. It is a local sizing value, not part of any hash or validation, and beacon-kit sets the block size to 100 MB, so it has no practical effect. Noted for completeness.keys.proto, and the privval noise listener. All defaults or additive.signVote. Vote extensions are disabled on Berachain.Compatibility with bera-v1.x
FinalizeBlockrequest has the same fields as bera-v1.x, includingsyncing_to_height.Two cosmetic differences remain and are asserted as such by the tests. The consensus params JSON written by this fork carries an empty
authorityobject that bera-v1.x ignores, andValidatorUpdate.pub_keyis a non-nullable message here so an unset key still encodes as an empty field that bera-v1.x skips.Testing
Unit tests and lint pass with and without
-tags bls12381, including-raceon the consensus, types, state and privval packages.make proto-genreproduces the committed generated files.New in this PR, under
test/compat/:vectors_test.goreplays a fixed set of inputs through this fork and compares every output byte for byte with vectors produced by the bera-v1.x code. It covers vote and proposal sign bytes and BLS signatures, aggregated and individual commits and their hashes, block and header hashes, consensus params, validator sets, evidence, the persisted state, the ABCI responses, every consensus p2p message, the WAL entries, and the genesis and key files. It runs in CI.gen-v1x/is the generator. It is a separate Go module pinned to the bera-v1.x head, so the vectors can be regenerated from scratch at any time. Its-verifymode does the reverse check, decoding and verifying what this fork produces with bera-v1.x code.rolling_upgrade.shstarts real nodes. Phase A runs one validator on bera-v1.x, then bera-v0.40.x, then bera-v1.x, then bera-v0.40.x, all on the same data directory. Phase B runs four BLS validators and moves them one at a time from bera-v1.x to bera-v0.40.x, restarts an upgraded node mid-soak, lets a node on each version block-sync the blocks it missed from mixed peers, sends a transaction through the upgraded network, rolls one node back to bera-v1.x and forward again. At every stage it checks that all validators signed every block, that both versions proposed, that commits stayed aggregated with round 0, that block hashes agree on every node, and that the logs are free of codec errors.All of the above passed locally against the bera-v1.x head. The devnet rolling-upgrade exercise from #51 was run twice with beacon-kit built from this branch (6 validators + fullnode: rolling upgrade, rollbacks both ways, crash replay, chaos, quorum loss, from-genesis sync, validator churn). All passed with identical hashes; the only regressions were the two beacon-kit effects of the missing
syncing_to_height, fixed by addition 4.test/compatis meant to stay in the tree after the upgrade, and its pieces have different lifetimes:gen-v1x/only exists to regenerate the vectors from the bera-v1.x code and to run the reverse check. It pins a bera-v1.x commit, so it is the one piece with a maintenance cost. It can be removed once bera-v1.x is retired from the network.rolling_upgrade.shtakes the two binaries as parameters, so after this upgrade it becomes a generic "previous release versus next release" check. Every futurebera-v0.40.xrelease is a rolling upgrade on mainnet, and the script proves data-dir reuse in both directions, a mixed validator set, block-sync across versions and rollback in a few minutes. A follow-up can rename the variables and drop the bera-v1.x defaults.Base and upstream syncing
The
bera-v0.40.xline is created from the upstream tag, as before:Future upstream releases are adopted by merging the release tag into
bera-v0.40.xthrough a normal PR. Using the next patch release as the example:Sync PRs must be merged with a merge commit, never squashed. Squashing breaks the shared ancestry with upstream, so every later sync re-conflicts on changes we already took. Merging rather than rebasing also keeps the branch append-only, so open PRs, clones and go module consumers survive a sync, and each conflict is resolved once inside a reviewed PR.
The Berachain changes stay easy to inspect.
git diff v0.40.0 bera-v0.40.xshows everything we maintain on top of the latest upstream tag, and thetest/compatvectors test fails if a sync ever touches the consensus bytes.Urgent upstream fixes can be cherry-picked between releases, and the next sync merge supersedes them.