Final malleability changes: ServiceEventCount #90
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add configuration for multiple distinct sporks for testing
Remove unused createSpork() function and associated data. The data was moved to variable `Mainnet24_SporkVersion6`.
The test was not actually verifying anything, due to only checking resultIDs for current blocks against sealed results of previous blocks.
Remove TestExecutionResultConsistency_ChunkServiceEventCountField. As it relied on the internals of flow-go, this test would only be comparing one reimplementation of the old ID hashing against another reimplementation, for what I think is not much benefit. TestVerifyExecutionResultHash and TestVerifyBlockHash can still provide coverage for the ServiceEventCount field, provided the appropriate versions and block heights.
WalkthroughIntroduces version-aware chunk/internal representations and payload/execution-result hash derivations for protocol versions (v1/v7/v8), refactors ServiceEventCount handling, updates tests to per-spork verification, replaces consensus Badger usage with a storage.DB abstraction, and performs large dependency upgrades in go.mod and CI Go version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Switch as deriveExecutionResultSwitch
participant V2 as deriveExecutionResultV2
participant V7 as deriveExecutionResultV7
participant V8 as deriveExecutionResultV8
participant Conv as ChunksToV1/V7/V8
Caller->>Switch: deriveExecutionResult(exec, sporkVersion)
alt version 2–6
Switch->>V2: use V2 payload & chunks
V2-->>Caller: resultID
else version 7
Switch->>V7: handle *ServiceEventCount (nil or non-nil)
V7->>Conv: ChunksToV7 (when non-nil) or fallback to V2
V7-->>Caller: resultID
else version 8
Switch->>V8: ServiceEventCount as value
V8->>Conv: ChunksToV8
V8-->>Caller: resultID
end
sequenceDiagram
autonumber
participant Caller as Caller
participant PH as derivePayloadHash
participant PH7 as derivePayloadHashV7
Caller->>PH: derivePayloadHash(version, collection, seal, receipt, result, protocolStateId?)
alt version < 7
PH-->>Caller: hash(collection, seal, receipt, result)
else version ≥ 7
PH->>PH7: include protocolStateId in input
PH7-->>Caller: hash(collection, seal, receipt, result, protocolStateId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tim-barry
requested review from
jordanschalm and
peterargue
and removed request for
jordanschalm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go.mod (1)
3-3: Fix invalidgodirective (must bemajor.minor).
go 1.23.7will causeinvalid go versionerrors. Usego 1.23and keep thetoolchainfor the patch level.-go 1.23.7 +go 1.23state/convert.go (1)
262-274: Potential nil-pointer dereference in V8 block-hash derivation.
hdr.LastViewTC.ID()panics whenLastViewTCis nil. V5 code guarded this; do the same for V8.- LastViewTCID: hdr.LastViewTC.ID(), + LastViewTCID: func() flow.Identifier { + if hdr.LastViewTC == nil { + return flow.ZeroID + } + return hdr.LastViewTC.ID() + }(),
🧹 Nitpick comments (4)
go.mod (1)
19-19: Pre-release Flow-Go pin: confirm before merge to main.
github.com/onflow/flow-go v0.43.0-dev-malleability.3is a dev tag. Confirm CI reproducibility and plan to pin a stable tag before release.state/convert.go (3)
492-509: Defensive guard forChunksToV8against unexpected nilServiceEventCount.Current code dereferences
*chunk.ServiceEventCount. Your V8 conversion path sets it, but a future call path regression will panic. Add a safe assignment (no hash impact when non-nil).func ChunksToV8(chunks []*flowChunk) []*flow.Chunk { result := make([]*flow.Chunk, 0, len(chunks)) for _, chunk := range chunks { + svc := uint16(0) + if chunk.ServiceEventCount != nil { + svc = *chunk.ServiceEventCount + } else { + // Unexpected for V8+; conversion should have set it. Keeping 0 avoids a panic. + // Consider tightening this to return an error if this ever surfaces. + } result = append(result, &flow.Chunk{ ChunkBody: flow.ChunkBody{ CollectionIndex: chunk.CollectionIndex, StartState: chunk.StartState, EventCollection: chunk.EventCollection, - ServiceEventCount: *chunk.ServiceEventCount, + ServiceEventCount: svc, BlockID: chunk.BlockID, TotalComputationUsed: chunk.TotalComputationUsed, NumberOfTransactions: chunk.NumberOfTransactions, }, Index: chunk.Index, EndState: chunk.EndState, }) } return result }
781-786: Docstring mismatch: function is used for V7 and V8.Update the comment to reflect both versions.
-// derivePayloadHashV7 generates a payload hash for block version V7. -// It concatenates and hashes the provided collection, seal, receipt, result, and protocol state hashes. +// derivePayloadHashV7 generates a payload hash for block versions V7 and V8. +// It concatenates and hashes collection, seal, receipt, result, and protocol-state IDs.
26-27: Stale TODO.The V6 branching is already handled by versioned code elsewhere; either clarify the TODO or remove.
- // todo: add V6 version branching here directly after mainnet23 spork + // NOTE: Version branching handled via convertChunk/derive* functions; no action here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(8 hunks)state/convert.go(4 hunks)state/convert_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state/convert_test.go (2)
access/access.go (3)
NodeConfig(515-523)New(574-620)Client(66-69)config/config.go (2)
Chain(23-42)Spork(107-119)
Comment on lines
+39
to
+43
| AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet24.nodes.onflow.org:9000"}}, | ||
| Chain: config.Chain{Network: "mainnet"}, | ||
| RootBlock: 65264619, | ||
| Version: 6, | ||
| } |
There was a problem hiding this comment.
Use TLS for production Access nodes.
Without TLS: true (or PublicKey), access.New dials with insecure creds and will fail against mainnet endpoints.
var Mainnet24_SporkVersion6 = sporkTemplate{
- AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet24.nodes.onflow.org:9000"}},
+ AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet24.nodes.onflow.org:9000", TLS: true}},
Chain: config.Chain{Network: "mainnet"},
RootBlock: 65264619,
Version: 6,
}
var Mainnet26_SporkVersion7 = sporkTemplate{
- AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet26.nodes.onflow.org:9000"}},
+ AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet26.nodes.onflow.org:9000", TLS: true}},
Chain: config.Chain{Network: "mainnet"},
RootBlock: 125_000_000,
Version: 7,
}Also applies to: 46-50
🤖 Prompt for AI Agents
In state/convert_test.go around lines 39 to 43 (and similarly lines 46 to 50),
the AccessNodes NodeConfig for mainnet lacks TLS configuration causing
access.New to dial with insecure credentials and fail against production
endpoints; update each production AccessNodes entry to enable TLS (e.g., set
TLS: true) or provide a valid PublicKey in the NodeConfig so the client uses
secure creds when connecting to mainnet access nodes.
Comment on lines
+52
to
+63
| func TestVerifyBlockHash(t *testing.T) { | ||
| t.Run("mainnet24 spork version 6", func(t *testing.T) { | ||
| ctx := context.Background() | ||
| spork := Mainnet24_SporkVersion6.create(ctx) | ||
| VerifyBlocksForSpork(t, ctx, spork, 65264620, 65264630) | ||
| }) | ||
| t.Run("mainnet26 spork version 7", func(t *testing.T) { | ||
| ctx := context.Background() | ||
| spork := Mainnet26_SporkVersion7.create(ctx) | ||
| VerifyBlocksForSpork(t, ctx, spork, 125_000_001, 125_000_011) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard network-heavy tests and add timeouts.
Avoid CI flakes and indefinite hangs by (a) skipping under -short, and (b) using a context timeout.
func TestVerifyBlockHash(t *testing.T) {
+ if testing.Short() {
+ t.Skip("integration test requires network access")
+ }
t.Run("mainnet24 spork version 6", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
spork := Mainnet24_SporkVersion6.create(ctx)
VerifyBlocksForSpork(t, ctx, spork, 65264620, 65264630)
})
t.Run("mainnet26 spork version 7", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
spork := Mainnet26_SporkVersion7.create(ctx)
VerifyBlocksForSpork(t, ctx, spork, 125_000_001, 125_000_011)
})
}
func TestVerifyExecutionResultHash(t *testing.T) {
+ if testing.Short() {
+ t.Skip("integration test requires network access")
+ }
t.Run("mainnet24 spork version 6", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
spork := Mainnet24_SporkVersion6.create(ctx)
VerifyExecutionResultsForSpork(t, ctx, spork, 65264620, 65264630)
})
t.Run("mainnet26 spork version 7", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
spork := Mainnet26_SporkVersion7.create(ctx)
VerifyExecutionResultsForSpork(t, ctx, spork, 125_000_001, 125_000_011)
})
}
func TestDeriveEventsHash(t *testing.T) {
+ if testing.Short() {
+ t.Skip("integration test requires network access")
+ }
t.Run("mainnet24 / spork version 6", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
spork := Mainnet24_SporkVersion6.create(ctx)
VerifyEventsHashForSpork(t, ctx, spork, 65264620, 65264630)
})
t.Run("mainnet26 / spork version 7", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
spork := Mainnet26_SporkVersion7.create(ctx)
VerifyEventsHashForSpork(t, ctx, spork, 125_000_001, 125_000_011)
})
}Add imports:
import (
"bytes"
"context"
+ "time"
"testing"Also applies to: 81-92, 125-136
🤖 Prompt for AI Agents
In state/convert_test.go around lines 52-63 (and also apply same changes at
81-92 and 125-136), the tests run long network-heavy checks without guarding for
CI or timeouts; update each TestVerifyBlockHash subtest to first check
testing.Short() and call t.Skip("skipping network-heavy test in short mode")
when true, and replace context.Background() with context.WithTimeout(ctx,
<reasonableDuration>) (e.g., 30s or 1m) ensuring you cancel the context via
defer cancel(), and add necessary imports (context and time) to the file.
jordanschalm
approved these changes
Sep 8, 2025
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
zhangchiqing
approved these changes
Sep 8, 2025
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
state/state.go (1)
645-698: Close the Badger DB when the follower stops.
badger.Opencreates files and goroutines; not closing can leak resources and risk data corruption on restart.Apply:
- go follow.Run(ctx) + go func() { + defer func() { + if err := db.Close(); err != nil { + log.Errorf("Failed to close consensus database at %s: %s", dbDir, err) + } + }() + follow.Run(ctx) + }()
♻️ Duplicate comments (4)
state/convert_test.go (4)
3-14: Guard network-heavy tests and add timeouts.Prevents CI flakes and indefinite hangs; mirrors earlier guidance.
import ( "bytes" "context" + "time" "testing"func TestVerifyBlockHash(t *testing.T) { + if testing.Short() { + t.Skip("integration test requires network access") + } t.Run("mainnet24 spork version 6", func(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() spork := Mainnet24_SporkVersion6.create(ctx) VerifyBlocksForSpork(t, ctx, spork, 65264620, 65264630) }) t.Run("mainnet26 spork version 7", func(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() spork := Mainnet26_SporkVersion7.create(ctx) VerifyBlocksForSpork(t, ctx, spork, 125_000_001, 125_000_011) }) }Also applies to: 52-63
38-43: Use TLS for mainnet Access nodes.Production endpoints require TLS; without it, dials use insecure creds and fail.
var Mainnet24_SporkVersion6 = sporkTemplate{ - AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet24.nodes.onflow.org:9000"}}, + AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet24.nodes.onflow.org:9000", TLS: true}}, Chain: config.Chain{Network: "mainnet"}, RootBlock: 65264619, Version: 6, } var Mainnet26_SporkVersion7 = sporkTemplate{ - AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet26.nodes.onflow.org:9000"}}, + AccessNodes: []access.NodeConfig{{Address: "access-001.mainnet26.nodes.onflow.org:9000", TLS: true}}, Chain: config.Chain{Network: "mainnet"}, RootBlock: 125_000_000, Version: 7, }Also applies to: 45-50
81-92: Also guard and bound execution-result test runtime.func TestVerifyExecutionResultHash(t *testing.T) { + if testing.Short() { + t.Skip("integration test requires network access") + } t.Run("mainnet24 spork version 6", func(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() spork := Mainnet24_SporkVersion6.create(ctx) VerifyExecutionResultsForSpork(t, ctx, spork, 65264620, 65264630) }) t.Run("mainnet26 spork version 7", func(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() spork := Mainnet26_SporkVersion7.create(ctx) VerifyExecutionResultsForSpork(t, ctx, spork, 125_000_001, 125_000_011) }) }
130-141: Also guard and bound events-hash test runtime.func TestDeriveEventsHash(t *testing.T) { + if testing.Short() { + t.Skip("integration test requires network access") + } t.Run("mainnet24 / spork version 6", func(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() spork := Mainnet24_SporkVersion6.create(ctx) VerifyEventsHashForSpork(t, ctx, spork, 65264620, 65264630) }) t.Run("mainnet26 / spork version 7", func(t *testing.T) { - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() spork := Mainnet26_SporkVersion7.create(ctx) VerifyEventsHashForSpork(t, ctx, spork, 125_000_001, 125_000_011) }) }
🧹 Nitpick comments (6)
state/state.go (3)
56-56: Guard consensus reader usage when enabled.If
UseConsensusFolloweris true buti.consensusisn’t initialized (unexpected init/order issues), calls toi.consensus.Reader()will panic. Add a sanity check once at startup or before first use.
394-413: Avoid shadowingerrin consensus lookups.Shadowing hampers readability and can hide bugs.
- err := operation.LookupBlockHeight(i.consensus.Reader(), height, &blockID) + err = operation.LookupBlockHeight(i.consensus.Reader(), height, &blockID) ... - err := operation.LookupBySealedBlockID(i.consensus.Reader(), blockID, &sealID) + err = operation.LookupBySealedBlockID(i.consensus.Reader(), blockID, &sealID)Also applies to: 431-447
448-459: Fix typos in logs (“Acccess API”).User-facing log strings have a typo.
- "Unverifiable seal found in block %x at height %d: got seal for block %x via Acccess API, found seal for block %x via consensus", + "Unverifiable seal found in block %x at height %d: got seal for block %x via Access API, found seal for block %x via consensus", ... - "Unverifiable execution result for block %x found in block %x at height %d: got %x via Acccess API, got %x via consensus", + "Unverifiable execution result for block %x found in block %x at height %d: got %x via Access API, got %x via consensus",state/convert_test.go (2)
65-79: Mark helpers and tighten assertions.Add
t.Helper()to helper funcs for better failure locations and ensure non-trivial checks in events test.func VerifyBlocksForSpork(t *testing.T, ctx context.Context, spork *config.Spork, startHeight uint64, endHeight uint64) { + t.Helper()func VerifyExecutionResultsForSpork(t *testing.T, ctx context.Context, spork *config.Spork, startHeight uint64, endHeight uint64) { + t.Helper()func VerifyEventsHashForSpork(t *testing.T, ctx context.Context, spork *config.Spork, startHeight uint64, endHeight uint64) { + t.Helper() @@ - execResult, err = client.ExecutionResultForBlockID(ctx, block.Id) + execResult, err = client.ExecutionResultForBlockID(ctx, block.Id) require.NoError(t, err) + require.NotZero(t, len(execResult.Chunks), "range too short; no chunks verified")Also applies to: 99-128, 143-198
120-123: Compare identifiers directly instead of via String().Simpler and avoids unnecessary allocations.
- require.Equal(t, sealedResult.String(), computedResults[blockID].String(), "mismatched result ID") + require.Equal(t, sealedResult, computedResults[blockID], "mismatched result ID")go.mod (1)
21-29: Align OpenTelemetry modules to v1.38.0- go.opentelemetry.io/otel/sdk v1.36.0 - go.opentelemetry.io/otel/sdk/metric v1.36.0 + go.opentelemetry.io/otel/sdk v1.38.0 + go.opentelemetry.io/otel/sdk/metric v1.38.0After updating, run:
go mod tidy -v go list -m -u all | rg -n 'opentelemetry|otel'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(10 hunks)state/convert_test.go(2 hunks)state/state.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state/convert_test.go (2)
access/access.go (3)
NodeConfig(515-523)New(574-620)Client(66-69)config/config.go (2)
Chain(23-42)Spork(107-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
go.mod (1)
17-18: Flow-Go pinned to Pebble dev build — LGTM.Pin matches prior guidance for Pebble-backed storage while awaiting a release tag.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Successor to #86. Implements backward compatibility for the ServiceEventCount field of
flow.Chunk, and also contains the test improvements from #89.Removed the existing ServiceEventCount compatibility test - since it used flow-go's ID calculation to check a property of Chunks from sporkversion 7, I believe maintaining this test would just result in testing two reimplementations of the ID calculation against each other, and don't see how it would provide any benefit.
Closes #88
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores