Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chunk Data Model supports per-chunk service event mapping #6744

Merged
merged 33 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e59dc76
add chunk model changes and encoding tests
jordanschalm Nov 20, 2024
869feff
add servcie event indices to chunk
jordanschalm Nov 20, 2024
6e4c8de
revert test package change
jordanschalm Nov 20, 2024
d383068
add test file
jordanschalm Nov 20, 2024
97515e8
rm indices from fixture
jordanschalm Nov 20, 2024
ec7c74e
add ServiceEventsByChunk method on result
jordanschalm Nov 20, 2024
e906251
fix chunk verifier test
jordanschalm Nov 20, 2024
b9849c6
add indices getter tests
jordanschalm Nov 20, 2024
8961e51
add ServiceEventsByChunk tests
jordanschalm Nov 21, 2024
5579f2a
chunk verifier tests
jordanschalm Nov 21, 2024
f1125b3
improve docs
jordanschalm Nov 21, 2024
718a5ee
improve docs
jordanschalm Nov 21, 2024
44f3c1a
adjust field and documentation
jordanschalm Nov 28, 2024
13aba21
update tests
jordanschalm Nov 28, 2024
defcdea
wip
jordanschalm Nov 29, 2024
405626e
update BlockResult tests
jordanschalm Dec 2, 2024
447017b
fix test name
jordanschalm Dec 2, 2024
e465bbc
fix chunk verifier test
jordanschalm Dec 2, 2024
140b324
backward compatible RLP encoding + tests
jordanschalm Dec 3, 2024
49bcbf6
note rpc change requirements
jordanschalm Dec 3, 2024
176100f
Merge branch 'feature/efm-recovery' into jord/6622-chunk-service-events
jordanschalm Dec 3, 2024
ee99871
Apply suggestions from code review
jordanschalm Dec 9, 2024
4d40dcf
add deprecated notes
jordanschalm Dec 9, 2024
9b7735e
Merge branch 'jord/6622-chunk-service-events' of github.com:onflow/fl…
jordanschalm Dec 9, 2024
ad346e8
RLP notes
jordanschalm Dec 9, 2024
b484203
rename var
jordanschalm Dec 9, 2024
a61e30e
add test case demonstrating rlp order dependence
jordanschalm Dec 9, 2024
6fe51f6
EncodeDecodeDifferentVersions docs
jordanschalm Dec 9, 2024
4a651b6
sanity check for service event count field
jordanschalm Dec 10, 2024
4a27b73
fix ER test
jordanschalm Dec 10, 2024
e1ee2f0
add context to chunkverifier test
jordanschalm Dec 10, 2024
9c1ef70
refactor generateEvents
jordanschalm Dec 10, 2024
9a10320
Update model/flow/chunk.go
jordanschalm Dec 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions engine/common/rpc/convert/execution_results_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/onflow/flow-go/utils/unittest"
)

// TODO: fails with input non-nil ChunkBody.ServiceEventCount
func TestConvertExecutionResult(t *testing.T) {
t.Parallel()

Expand All @@ -25,6 +26,7 @@ func TestConvertExecutionResult(t *testing.T) {
assert.Equal(t, er, converted)
}

// TODO: fails with input non-nil ChunkBody.ServiceEventCount
func TestConvertExecutionResults(t *testing.T) {
t.Parallel()

Expand All @@ -43,6 +45,7 @@ func TestConvertExecutionResults(t *testing.T) {
assert.Equal(t, results, converted)
}

// TODO: fails with input non-nil ChunkBody.ServiceEventCount
func TestConvertExecutionResultMetaList(t *testing.T) {
t.Parallel()

Expand Down
14 changes: 14 additions & 0 deletions engine/execution/block_result.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package execution

import (
"fmt"
"math"

"github.com/onflow/flow-go/fvm/storage/snapshot"
"github.com/onflow/flow-go/model/flow"
Expand Down Expand Up @@ -49,6 +50,18 @@ func (er *BlockExecutionResult) AllEvents() flow.EventsList {
return res
}

// ServiceEventCountForChunk returns the number of service events emitted in the given chunk.
func (er *BlockExecutionResult) ServiceEventCountForChunk(chunkIndex int) uint16 {
serviceEventCount := len(er.collectionExecutionResults[chunkIndex].serviceEvents)
if serviceEventCount > math.MaxUint16 {
// The current protocol demands that the ServiceEventCount does not exceed 65535.
// For defensive programming, we explicitly enforce this limit as 65k could be produced by a bug.
// Execution nodes would be first to realize that this bound is violated, and crash (fail early).
panic(fmt.Sprintf("service event count (%d) exceeds maximum value of 65535", serviceEventCount))
}
return uint16(serviceEventCount)
jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
}

func (er *BlockExecutionResult) AllServiceEvents() flow.EventsList {
res := make(flow.EventsList, 0)
for _, ce := range er.collectionExecutionResults {
Expand Down Expand Up @@ -199,6 +212,7 @@ func (ar *BlockAttestationResult) ChunkAt(index int) *flow.Chunk {
attestRes.startStateCommit,
len(execRes.TransactionResults()),
attestRes.eventCommit,
ar.ServiceEventCountForChunk(index),
attestRes.endStateCommit,
execRes.executionSnapshot.TotalComputationUsed(),
)
Expand Down
81 changes: 81 additions & 0 deletions engine/execution/block_result_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package execution

import (
"math/rand"
"testing"

"github.com/stretchr/testify/assert"

"github.com/onflow/flow-go/utils/slices"
"github.com/onflow/flow-go/utils/unittest"
)

// makeBlockExecutionResultFixture makes a BlockExecutionResult fixture
// with the specified allocation of service events to chunks.
func makeBlockExecutionResultFixture(serviceEventsPerChunk []int) *BlockExecutionResult {
fixture := new(BlockExecutionResult)
for _, nServiceEvents := range serviceEventsPerChunk {
fixture.collectionExecutionResults = append(fixture.collectionExecutionResults,
CollectionExecutionResult{
serviceEvents: unittest.EventsFixture(nServiceEvents),
convertedServiceEvents: unittest.ServiceEventsFixture(nServiceEvents),
})
}
return fixture
}

// Tests that ServiceEventCountForChunk method works as expected under various circumstances:
func TestBlockExecutionResult_ServiceEventCountForChunk(t *testing.T) {
t.Run("no service events", func(t *testing.T) {
nChunks := rand.Intn(10) + 1
jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
blockResult := makeBlockExecutionResultFixture(make([]int, nChunks))
// all chunks should have 0 service event count
for chunkIndex := 0; chunkIndex < nChunks; chunkIndex++ {
count := blockResult.ServiceEventCountForChunk(chunkIndex)
assert.Equal(t, uint16(0), count)
}
})
t.Run("service events only in system chunk", func(t *testing.T) {
nChunks := rand.Intn(10) + 2 // at least 2 chunks
// add between 1 and 10 service events, all in the system chunk
serviceEventAllocation := make([]int, nChunks)
nServiceEvents := rand.Intn(10) + 1
serviceEventAllocation[nChunks-1] = nServiceEvents

blockResult := makeBlockExecutionResultFixture(serviceEventAllocation)
// all non-system chunks should have zero service event count
for chunkIndex := 0; chunkIndex < nChunks-1; chunkIndex++ {
count := blockResult.ServiceEventCountForChunk(chunkIndex)
assert.Equal(t, uint16(0), count)
}
// the system chunk should contain all service events
assert.Equal(t, uint16(nServiceEvents), blockResult.ServiceEventCountForChunk(nChunks-1))
})
t.Run("service events only outside system chunk", func(t *testing.T) {
nChunks := rand.Intn(10) + 2 // at least 2 chunks
// add 1 service event to all non-system chunks
serviceEventAllocation := slices.Fill(1, nChunks)
serviceEventAllocation[nChunks-1] = 0

blockResult := makeBlockExecutionResultFixture(serviceEventAllocation)
// all non-system chunks should have 1 service event
for chunkIndex := 0; chunkIndex < nChunks-1; chunkIndex++ {
count := blockResult.ServiceEventCountForChunk(chunkIndex)
assert.Equal(t, uint16(1), count)
}
// the system chunk service event count should include all service events
assert.Equal(t, uint16(0), blockResult.ServiceEventCountForChunk(nChunks-1))
})
t.Run("service events in both system chunk and other chunks", func(t *testing.T) {
nChunks := rand.Intn(10) + 2 // at least 2 chunks
// add 1 service event to all chunks (including system chunk)
serviceEventAllocation := slices.Fill(1, nChunks)

blockResult := makeBlockExecutionResultFixture(serviceEventAllocation)
// all chunks should have service event count of 1
for chunkIndex := 0; chunkIndex < nChunks; chunkIndex++ {
count := blockResult.ServiceEventCountForChunk(chunkIndex)
assert.Equal(t, uint16(1), count)
}
})
}
29 changes: 29 additions & 0 deletions model/encoding/rlp/rlp_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package rlp_test

import (
"testing"

"github.com/onflow/go-ethereum/rlp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestRLPStructFieldOrder tests the field ordering property of RLP encoding.
// It provides evidence that RLP encoding depends on struct field ordering.
func TestRLPStructFieldOrder(t *testing.T) {
a := struct {
A uint32 // A first
B uint32
}{A: 2, B: 3}

b := struct {
B uint32 // B first
A uint32
}{A: 2, B: 3}

abin, err := rlp.EncodeToBytes(a)
require.NoError(t, err)
bbin, err := rlp.EncodeToBytes(b)
require.NoError(t, err)
assert.NotEqual(t, abin, bbin)
}
119 changes: 118 additions & 1 deletion model/flow/chunk.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package flow

import (
"fmt"
"io"
"log"

"github.com/ipfs/go-cid"
"github.com/onflow/go-ethereum/rlp"
"github.com/vmihailenco/msgpack/v4"
)

Expand All @@ -20,19 +22,111 @@ func init() {
}
}

// ChunkBodyV0 is the prior version of ChunkBody, used for computing backward-compatible IDs and tests.
// Compared to ChunkBody, ChunkBodyV0 does not have the ServiceEventCount field.
// Deprecated: to be removed in Mainnet27
// TODO(mainnet27): Remove this data structure https://github.com/onflow/flow-go/issues/6773
type ChunkBodyV0 struct {
jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
CollectionIndex uint
StartState StateCommitment
EventCollection Identifier
BlockID Identifier
TotalComputationUsed uint64
NumberOfTransactions uint64
}

type ChunkBody struct {
CollectionIndex uint

// execution info
StartState StateCommitment // start state when starting executing this chunk
EventCollection Identifier // Events generated by executing results
BlockID Identifier // Block id of the execution result this chunk belongs to
// ServiceEventCount defines how many service events were emitted in this chunk.
// By reading these fields from the prior chunks in the same ExecutionResult, we can
// compute exactly what service events were emitted in this chunk.
//
// Let C be this chunk, K be the set of chunks in the ExecutionResult containing C.
// Then the service event indices for C are given by:
// StartIndex = ∑Ci.ServiceEventCount : Ci ∈ K, Ci.Index < C.Index
// EndIndex = StartIndex + C.ServiceEventCount
// The service events for C are given by:
// ExecutionResult.ServiceEvents[StartIndex:EndIndex]
//
// BACKWARD COMPATIBILITY:
// (1) If ServiceEventCount is nil, this indicates that this chunk was created by an older software version
// which did support specifying a mapping between chunks and service events.
jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
// In this case, all service events are assumed to have been emitted in the system chunk (last chunk).
// This was the implicit behaviour prior to the introduction of this field.
// (2) Otherwise, ServiceEventCount must be non-nil.
// Within an ExecutionResult, all chunks must use either representation (1) or (2), not both.
// TODO(mainnet27): make this field non-pointer https://github.com/onflow/flow-go/issues/6773
ServiceEventCount *uint16
jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
BlockID Identifier // Block id of the execution result this chunk belongs to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we maybe want to move this to the beginning of the Chunk Body? I think conceptually, that would be more consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but RLP encoding depends on field ordering within structs, so doing this would change the ID computation (unless we over-rode again, using the RLP encoding).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case to validate this a61e30e


// Computation consumption info
TotalComputationUsed uint64 // total amount of computation used by running all txs in this chunk
NumberOfTransactions uint64 // number of transactions inside the collection
}

jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
// We TEMPORARILY implement the [rlp.Encoder] interface to implement backwards-compatible ID computation.
// TODO(mainnet27): remove EncodeRLP methods on Chunk and ChunkBody https://github.com/onflow/flow-go/issues/6773
var _ rlp.Encoder = &ChunkBody{}

// EncodeRLP defines custom encoding logic for the ChunkBody type.
// NOTE: For correct operation when encoding a larger structure containing ChunkBody,
// this method depends on Chunk also overriding EncodeRLP. Otherwise, since ChunkBody
// is an embedded field, the RLP encoder will skip Chunk fields besides those in ChunkBody.
//
// The encoding is defined for backward compatibility with prior data model version (ChunkBodyV0):
// - All new ChunkBody instances must have non-nil ServiceEventCount field
// - A nil ServiceEventCount field indicates a v0 version of ChunkBody
// - when computing the ID of such a ChunkBody, the ServiceEventCount field is omitted from the fingerprint
//
// No errors expected during normal operations.
// TODO(mainnet27): remove this method https://github.com/onflow/flow-go/issues/6773
func (ch ChunkBody) EncodeRLP(w io.Writer) error {
jordanschalm marked this conversation as resolved.
Show resolved Hide resolved
var err error
if ch.ServiceEventCount == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if we explicitly documented how this code is going to be updated.

Suggested change
if ch.ServiceEventCount == nil {
if ch.ServiceEventCount == nil { // TODO [Mainnet27] remove this branch (ServiceEventCount will be changed to value type)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the EncodeRLP method entirely instead. The other branch is essentially the default RLP behaviour.

err = rlp.Encode(w, struct {
CollectionIndex uint
StartState StateCommitment
EventCollection Identifier
BlockID Identifier
TotalComputationUsed uint64
NumberOfTransactions uint64
}{
CollectionIndex: ch.CollectionIndex,
StartState: ch.StartState,
EventCollection: ch.EventCollection,
BlockID: ch.BlockID,
TotalComputationUsed: ch.TotalComputationUsed,
NumberOfTransactions: ch.NumberOfTransactions,
})
} else {
err = rlp.Encode(w, struct {
CollectionIndex uint
StartState StateCommitment
EventCollection Identifier
ServiceEventCount *uint16
BlockID Identifier
TotalComputationUsed uint64
NumberOfTransactions uint64
}{
CollectionIndex: ch.CollectionIndex,
StartState: ch.StartState,
EventCollection: ch.EventCollection,
ServiceEventCount: ch.ServiceEventCount,
BlockID: ch.BlockID,
TotalComputationUsed: ch.TotalComputationUsed,
NumberOfTransactions: ch.NumberOfTransactions,
})
}
if err != nil {
return fmt.Errorf("failed to rlp encode ChunkBody: %w", err)
}
return nil
}

type Chunk struct {
ChunkBody

Expand All @@ -41,12 +135,34 @@ type Chunk struct {
EndState StateCommitment
}

// We TEMPORARILY implement the [rlp.Encoder] interface to implement backwards-compatible ID computation.
// TODO(mainnet27): remove EncodeRLP methods on Chunk and ChunkBody https://github.com/onflow/flow-go/issues/6773
var _ rlp.Encoder = &Chunk{}

// EncodeRLP defines custom encoding logic for the Chunk type.
// This method exists only so that the embedded ChunkBody's EncodeRLP method is
// not interpreted as the RLP encoding for the entire Chunk.
// No errors expected during normal operation.
// TODO(mainnet27): remove this method https://github.com/onflow/flow-go/issues/6773
func (ch Chunk) EncodeRLP(w io.Writer) error {
return rlp.Encode(w, struct {
ChunkBody ChunkBody
Index uint64
EndState StateCommitment
}{
ChunkBody: ch.ChunkBody,
Index: ch.Index,
EndState: ch.EndState,
})
}

func NewChunk(
blockID Identifier,
collectionIndex int,
startState StateCommitment,
numberOfTransactions int,
eventCollection Identifier,
serviceEventCount uint16,
endState StateCommitment,
totalComputationUsed uint64,
) *Chunk {
Expand All @@ -57,6 +173,7 @@ func NewChunk(
StartState: startState,
NumberOfTransactions: uint64(numberOfTransactions),
EventCollection: eventCollection,
ServiceEventCount: &serviceEventCount,
TotalComputationUsed: totalComputationUsed,
},
Index: uint64(collectionIndex),
Expand Down
Loading
Loading