The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Summary of Changes

Hello @alpe, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on significant performance improvements for block execution and storage operations. By introducing caching mechanisms, optimizing data serialization, and refining key generation, the changes aim to reduce CPU and memory overhead, particularly in high-throughput scenarios. The modifications also streamline the block production pipeline by avoiding redundant computations and ensuring correct P2P broadcast ordering, contributing to a more efficient and robust node operation.

Highlights

• Performance Optimizations: Implemented several performance enhancements across block execution, storage, and serialization paths. This includes reducing memory allocations during key generation and protobuf marshalling, and optimizing data access patterns.
• Caching and Amortization: Introduced in-memory caching for last block data (header hash, data hash, signature) to avoid redundant store reads during block creation. Pending block checks are now amortized to reduce the frequency of expensive store scans.
• Streamlined Block Production: Refactored block production logic to use a new SaveBlockDataFromBytes method, allowing pre-serialized data to be saved directly. Validation for self-produced blocks now uses a lightweight AssertValidSequence to skip expensive re-computations.
• P2P Broadcast Ordering: Ensured sequential broadcasting of block headers before block data to the P2P network, addressing potential validation failures due to out-of-order delivery on peers.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Claude finished @alpe's task in 4m 36s —— View job

Code Review

Tasks

• Gather context and understand request
• Read changed files: executor.go, types, store packages
• Review code quality, correctness, and architecture
• Review performance and resource efficiency
• Review testing and reliability
• Post final review

Great set of optimizations with solid benchmarks backing them up. The ~3x throughput improvement and ~50% allocation reduction are impressive. A few issues need addressing before merge:

Bugs / Correctness

1. Optimization #6 is incomplete — signHeader bytes are discarded [block/internal/executing/executor.go:534]

signHeader was refactored to return (Signature, []byte, error) specifically to allow the caller to pass pre-serialized bytes to SaveBlockDataFromBytes. However the bytes are silently discarded with _ and the old SaveBlockData (which re-marshals) is used:

// line 534
signature, _, err := e.signHeader(&header.Header)
// ...
// line 553
if err := batch.SaveBlockData(header, data, &signature); err != nil {

The optimization is wired halfway — the interface change and signHeader change happened, but the caller was never updated to use SaveBlockDataFromBytes. Either complete the wiring or revert signHeader to return only (Signature, error) to avoid the misleading dead return value.

Note: this only works if AggregatorNodeSignatureBytesProvider produces the same bytes that header.MarshalBinary() would produce. If the signing payload is different (e.g., ev-abci uses a custom provider), then the bytes cannot be reused for storage and this optimization may not be universally applicable. Fix this →

2. lastBlockInfo cache not invalidated on rollback [block/internal/executing/executor.go:601–605]

e.lastBlockInfo.Store(&lastBlockInfo{
    headerHash: newState.LastHeaderHash,
    dataHash:   data.Hash(),
    signature:  signature,
})

If store.Rollback is triggered (e.g., during replay/sync), the in-memory lastBlockInfo and lastState caches would be stale. A subsequent CreateBlock using the cached values would build on a rolled-back block, producing a chain fork. Is there a guarantee that Rollback is never called while the executor is running? If not, this cache needs to be invalidated on rollback or the rollback path needs to update these caches.

Thread Safety

3. Signer.MarshalledPubKey() lazy init without synchronization [types/signer.go:19–32]

func (s *Signer) MarshalledPubKey() ([]byte, error) {
    if len(s.marshalledPubKey) > 0 {
        return s.marshalledPubKey, nil
    }
    // ...
    s.marshalledPubKey = bz  // unsynchronized write
    return bz, nil
}

This is a check-then-act without synchronization. If the same SignedHeader is ever accessed from multiple goroutines (e.g., passed to the P2P layer which may read it concurrently), this is a data race. In the current sequential broadcast path it's likely safe, but it's fragile. Using sync.Once or atomic.Pointer would be safer, or simply ensuring marshalledPubKey is always populated at construction time (as NewSigner does). Note that CreateBlock constructs types.Signer inline without calling NewSigner, so the cache is never pre-populated for locally produced blocks:

// executor.go:738-743
Signer: types.Signer{
    PubKey:  pubKey,        // marshalledPubKey is zero
    Address: e.genesis.ProposerAddress,
},

4. pendingCheckCounter not atomic [block/internal/executing/executor.go:73]

pendingCheckCounter uint64

This field is only accessed from the execution loop goroutine, so it's safe today. But it's a plain field adjacent to atomic.Pointer and atomic.Bool fields, which creates a misleading inconsistency. Consider making it atomic.Uint64 for clarity, or add a comment stating it's only accessed from executionLoop.

Design / Correctness Concerns

5. AssertValidSequence skips DataHash and ValidatorHash [types/state.go:70–94]

The new lightweight validation intentionally skips Validate(header, data) which checks DataHash. This is the right trade-off for self-produced blocks, but ValidatorHash is also not checked. If a validator rotation happened and produced an incorrect ValidatorHash, it would not be caught. The comment says "skip the expensive Validate() / DACommitment() re-computation" but DACommitment checks DataHash, not ValidatorHash. ValidatorHash check is cheap to add and would catch a class of bugs. Worth documenting explicitly which fields are intentionally skipped and why.

6. savePendingBlock never sets hasPendingBlock = true [block/internal/executing/executor.go:514]

if err := e.savePendingBlock(ctx, header, data); err != nil {
    return fmt.Errorf("failed to save block data: %w", err)
}
// hasPendingBlock is still false here

The flag only transitions false → true in initializeState. If the node crashes after savePendingBlock but before batch.Commit, on restart initializeState correctly finds and sets the flag. This is consistent with its purpose as a "crash recovery" indicator. However, the semantics are subtle: hasPendingBlock means "a pending block from a previous crashed cycle exists" not "a pending block is currently being produced". Consider renaming to recoveredPendingBlock or adding a comment to clarify this invariant.

Code Quality

7. signatureHash misleading name [pkg/store/batch.go:66]

signatureHash := *signature

signature is already a types.Signature (i.e., []byte). Naming the copy signatureHash implies it's a hash, but it's the raw signature bytes. This was noted in Gemini's review too. Rename to signatureCopy or sig. Fix this →

8. getHeightKey format inconsistency [pkg/store/keys.go:90–92]

func getHeightKey() string {
    return "/" + heightPrefix
}

All other height-related keys now use the heightKey() helper which produces /prefix/height. But getHeightKey returns /t directly (no height component). This is fine since it's the "current height" key, but the heightPrefix = "t" constant is otherwise only used here, while heightKey() is used for all block-indexed keys. The inconsistency is harmless but the heightPrefix constant is confusingly named given it's only used in getHeightKey.

9. GenerateKey drops path.Clean [pkg/store/kv.go:43–56]

The old implementation presumably used path.Clean. The new strings.Builder version is cleaner and more efficient, but if any caller passes fields with leading/trailing slashes or empty strings, the generated key format changes. All current callers appear to use well-behaved string constants, so this is low risk. A one-line comment noting the inputs are assumed clean would prevent future bugs.

10. timestamppb.Timestamp manual construction [types/serialization.go:377–378]

LastBlockTime: &timestamppb.Timestamp{Seconds: secs, Nanos: nanos},

timestamppb.New() validates that Nanos is in [0, 999999999]. Go's time.Nanosecond() is always in that range, so this is safe. Add a brief comment to make this invariant explicit, since a future refactor replacing time.Nanosecond() with something else could silently produce an invalid protobuf.

Performance (Minor)

11. Sequential broadcast adds latency to block cycle [block/internal/executing/executor.go:607–620]

The correctness argument for sequential broadcast is sound. But this adds the latency of header broadcast to the block production cycle, potentially increasing block times under slow P2P conditions. Consider whether the broadcast should happen in a background goroutine after the batch is committed (with the header-before-data ordering preserved). The current approach is the safest and likely acceptable, but worth profiling under network load.

Testing

12. No test for AssertValidSequence [types/state.go:70]

The new lightweight validation method has no dedicated unit tests. Given it's now on the critical path for every produced block (replacing ValidateBlock), it deserves table-driven tests covering: wrong chain ID, wrong height, time before last block, mismatched last header hash, mismatched app hash, and initial state (empty LastHeaderHash).

13. SaveBlockDataFromBytes has no test [pkg/store/batch.go:62]

The tracing wrapper and mock exist, but there's no test verifying that SaveBlockDataFromBytes produces the same stored result as SaveBlockData with the same inputs. A round-trip test would catch any hash computation differences.

Summary

The performance results are compelling and the architecture is sound. The most actionable items before merge:

• Must fix: Item ABCI interface #1 (incomplete optimization wiring / misleading return value)
• Should address: Item Peer discovery #2 (cache invalidation on rollback), Item Transaction gossiping #3 (MarshalledPubKey thread safety), Item Initial project setup #12 (test coverage for AssertValidSequence)
• Nice to have: Items Mempool #5–Fold the validator roots into the state tree #11

Codecov Report

❌ Patch coverage is 67.41573% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.91%. Comparing base (dc473e5) to head (985e9f9).
⚠️ Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3093      +/-   ##
==========================================
- Coverage   61.18%   60.91%   -0.27%     
==========================================
  Files         113      113              
  Lines       11444    11617     +173     
==========================================
+ Hits         7002     7077      +75     
- Misses       3654     3740      +86     
- Partials      788      800      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.