From 63fc6911c6f99034482ade3185263479fcc9c3e4 Mon Sep 17 00:00:00 2001 From: Maks Pawlak <120831+m4ksio@users.noreply.github.com> Date: Tue, 13 Oct 2020 20:40:59 -0700 Subject: [PATCH 1/6] Split Snapshot into Snapshot and SpockSnapshot to ensure spock secret is not stored and relied upon --- .../delta_snapshot_exporter.go | 3 --- .../computation/computer/computer.go | 2 +- engine/execution/ingestion/engine.go | 26 ++++++++++++------- engine/execution/ingestion/engine_test.go | 6 ++++- engine/execution/messages.go | 2 +- engine/execution/state/delta/view.go | 20 +++++++++----- .../execution/state/mock/execution_state.go | 14 ++++++++++ engine/execution/state/state.go | 14 +++++++--- engine/execution/state/unittest/fixtures.go | 6 ++--- storage/badger/operation/interactions_test.go | 2 +- utils/unittest/fixtures.go | 2 +- 11 files changed, 65 insertions(+), 32 deletions(-) diff --git a/cmd/util/cmd/exec-data-json-export/delta_snapshot_exporter.go b/cmd/util/cmd/exec-data-json-export/delta_snapshot_exporter.go index 1d729400259..8d44bb8b8f8 100644 --- a/cmd/util/cmd/exec-data-json-export/delta_snapshot_exporter.go +++ b/cmd/util/cmd/exec-data-json-export/delta_snapshot_exporter.go @@ -2,7 +2,6 @@ package jsonexporter import ( "bufio" - "encoding/hex" "encoding/json" "fmt" "os" @@ -19,7 +18,6 @@ import ( type dSnapshot struct { DeltaJSONStr string `json:"delta_json_str"` Reads []string `json:"reads"` - SpockSecret string `json:"spock_secret_data"` } // ExportDeltaSnapshots exports all the delta snapshots @@ -79,7 +77,6 @@ func ExportDeltaSnapshots(blockID flow.Identifier, dbPath string, outputPath str data := dSnapshot{ DeltaJSONStr: string(m), Reads: reads, - SpockSecret: hex.EncodeToString(snap[0].SpockSecret), } jsonData, err := json.Marshal(data) diff --git a/engine/execution/computation/computer/computer.go b/engine/execution/computation/computer/computer.go index c4c1a52e9aa..c7e492e3bf9 100644 --- a/engine/execution/computation/computer/computer.go +++ b/engine/execution/computation/computer/computer.go @@ -104,7 +104,7 @@ func (e *blockComputer) executeBlock( var gasUsed uint64 - interactions := make([]*delta.Snapshot, len(collections)+1) + interactions := make([]*delta.SpockSnapshot, len(collections)+1) events := make([]flow.Event, 0) blockTxResults := make([]flow.TransactionResult, 0) diff --git a/engine/execution/ingestion/engine.go b/engine/execution/ingestion/engine.go index baa1aa9bc85..e44b2da0b5e 100644 --- a/engine/execution/ingestion/engine.go +++ b/engine/execution/ingestion/engine.go @@ -718,17 +718,23 @@ func (e *Engine) handleComputationResult( // There is one result per transaction e.metrics.ExecutionTotalExecutedTransactions(len(result.TransactionResult)) - receipt, err := e.saveExecutionResults( + snapshots := make([]*delta.Snapshot, len(result.StateSnapshots)) + for i, stateSnapshot := range result.StateSnapshots { + snapshots[i] = &stateSnapshot.Snapshot + } + + executionResult, err := e.saveExecutionResults( ctx, result.ExecutableBlock, - result.StateSnapshots, + snapshots, result.Events, result.TransactionResult, startState, ) + receipt, err := e.generateExecutionReceipt(ctx, executionResult, result.StateSnapshots) if err != nil { - return nil, err + return nil, fmt.Errorf("could not generate execution receipt: %w", err) } err = e.providerEngine.BroadcastExecutionReceipt(ctx, receipt) @@ -752,7 +758,7 @@ func (e *Engine) saveExecutionResults( events []flow.Event, txResults []flow.TransactionResult, startState flow.StateCommitment, -) (*flow.ExecutionReceipt, error) { +) (*flow.ExecutionResult, error) { span, childCtx := e.tracer.StartSpanFromContext(ctx, trace.EXESaveExecutionResults) defer span.Finish() @@ -824,9 +830,9 @@ func (e *Engine) saveExecutionResults( return nil, fmt.Errorf("could not generate execution result: %w", err) } - receipt, err := e.generateExecutionReceipt(childCtx, executionResult, stateInteractions) + err = e.execState.PersistExecutionResult(ctx, executionResult) if err != nil { - return nil, fmt.Errorf("could not generate execution receipt: %w", err) + return nil, fmt.Errorf("could not persist execution result: %w", err) } // not update the highest executed until the result and receipts are saved. @@ -872,7 +878,7 @@ func (e *Engine) saveExecutionResults( Hex("final_state", endState). Msg("saved computation results") - return receipt, nil + return executionResult, nil } // logExecutableBlock logs all data about an executable block @@ -957,7 +963,7 @@ func (e *Engine) generateExecutionResultForBlock( func (e *Engine) generateExecutionReceipt( ctx context.Context, result *flow.ExecutionResult, - stateInteractions []*delta.Snapshot, + stateInteractions []*delta.SpockSnapshot, ) (*flow.ExecutionReceipt, error) { spocks := make([]crypto.Signature, len(stateInteractions)) @@ -1384,7 +1390,7 @@ func (e *Engine) applyStateDelta(delta *messages.ExecutionStateDelta) { // TODO - validate state delta, reject invalid messages - executionReceipt, err := e.saveExecutionResults( + executionResult, err := e.saveExecutionResults( e.unit.Ctx(), &delta.ExecutableBlock, delta.StateInteractions, @@ -1397,7 +1403,7 @@ func (e *Engine) applyStateDelta(delta *messages.ExecutionStateDelta) { log.Fatal().Err(err).Msg("fatal error while processing sync message") } - finalState, ok := executionReceipt.ExecutionResult.FinalStateCommitment() + finalState, ok := executionResult.FinalStateCommitment() if !ok { // set to start state next line will fail anyways finalState = delta.StartState diff --git a/engine/execution/ingestion/engine_test.go b/engine/execution/ingestion/engine_test.go index a0b8c90908e..b2d55e2fab6 100644 --- a/engine/execution/ingestion/engine_test.go +++ b/engine/execution/ingestion/engine_test.go @@ -216,6 +216,10 @@ func (ctx *testingContext) assertSuccessfulBlockComputation(executableBlock *ent On("UpdateHighestExecutedBlockIfHigher", mock.Anything, executableBlock.Block.Header). Return(nil) + ctx.executionState. + On("PersistExecutionResult", mock.Anything, executableBlock.Block.Header). + Return(nil) + ctx.executionState. On( "PersistExecutionReceipt", @@ -500,7 +504,7 @@ func TestExecuteScriptAtBlockID(t *testing.T) { func Test_SPOCKGeneration(t *testing.T) { runWithEngine(t, func(ctx testingContext) { - snapshots := []*delta.Snapshot{ + snapshots := []*delta.SpockSnapshot{ { SpockSecret: []byte{1, 2, 3}, }, diff --git a/engine/execution/messages.go b/engine/execution/messages.go index 6d910ddfee7..5a7c0c2d51d 100644 --- a/engine/execution/messages.go +++ b/engine/execution/messages.go @@ -17,7 +17,7 @@ type ComputationOrder struct { type ComputationResult struct { ExecutableBlock *entity.ExecutableBlock - StateSnapshots []*delta.Snapshot + StateSnapshots []*delta.SpockSnapshot Events []flow.Event TransactionResult []flow.TransactionResult GasUsed uint64 diff --git a/engine/execution/state/delta/view.go b/engine/execution/state/delta/view.go index cda9f49c54d..b9004ecff88 100644 --- a/engine/execution/state/delta/view.go +++ b/engine/execution/state/delta/view.go @@ -25,10 +25,14 @@ type View struct { readFunc GetRegisterFunc } -// Snapshot is set of interactions with the register type Snapshot struct { - Delta Delta - Reads []flow.RegisterID + Delta Delta + Reads []flow.RegisterID +} + +// Snapshot is state of interactions with the register +type SpockSnapshot struct { + Snapshot SpockSecret []byte } @@ -43,7 +47,7 @@ func NewView(readFunc GetRegisterFunc) *View { } // Snapshot returns copy of current state of interactions with a View -func (v *View) Interactions() *Snapshot { +func (v *View) Interactions() *SpockSnapshot { var delta = Delta{ Data: make(map[string]flow.RegisterEntry, len(v.delta.Data)), @@ -63,9 +67,11 @@ func (v *View) Interactions() *Snapshot { var spockSecret = make([]byte, len(spockSecHashSum)) copy(spockSecret, spockSecHashSum) - return &Snapshot{ - Delta: delta, - Reads: reads, + return &SpockSnapshot{ + Snapshot: Snapshot{ + Delta: delta, + Reads: reads, + }, SpockSecret: spockSecret, } } diff --git a/engine/execution/state/mock/execution_state.go b/engine/execution/state/mock/execution_state.go index 2cd43309d70..faad0cea705 100644 --- a/engine/execution/state/mock/execution_state.go +++ b/engine/execution/state/mock/execution_state.go @@ -230,6 +230,20 @@ func (_m *ExecutionState) PersistExecutionReceipt(_a0 context.Context, _a1 *flow return r0 } +// PersistExecutionResult provides a mock function with given fields: ctx, result +func (_m *ExecutionState) PersistExecutionResult(ctx context.Context, result *flow.ExecutionResult) error { + ret := _m.Called(ctx, result) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, *flow.ExecutionResult) error); ok { + r0 = rf(ctx, result) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // PersistStateCommitment provides a mock function with given fields: _a0, _a1, _a2 func (_m *ExecutionState) PersistStateCommitment(_a0 context.Context, _a1 flow.Identifier, _a2 []byte) error { ret := _m.Called(_a0, _a1, _a2) diff --git a/engine/execution/state/state.go b/engine/execution/state/state.go index 84f530e9c0e..d150c193213 100644 --- a/engine/execution/state/state.go +++ b/engine/execution/state/state.go @@ -67,6 +67,8 @@ type ExecutionState interface { // PersistChunkDataPack stores a chunk data pack by chunk ID. PersistChunkDataPack(context.Context, *flow.ChunkDataPack) error + PersistExecutionResult(ctx context.Context, result *flow.ExecutionResult) error + PersistExecutionReceipt(context.Context, *flow.ExecutionReceipt) error PersistStateInteractions(context.Context, flow.Identifier, []*delta.Snapshot) error @@ -92,6 +94,14 @@ type state struct { db *badger.DB } +func (s *state) PersistExecutionResult(ctx context.Context, executionResult *flow.ExecutionResult) error { + err := s.results.Index(executionResult.BlockID, executionResult.ID()) + if err != nil { + return fmt.Errorf("could not index execution result: %w", err) + } + return nil +} + func RegisterIDToKey(reg flow.RegisterID) ledger.Key { return ledger.NewKey([]ledger.KeyPart{ ledger.NewKeyPart(KeyPartOwner, []byte(reg.Owner)), @@ -332,10 +342,6 @@ func (s *state) PersistExecutionReceipt(ctx context.Context, receipt *flow.Execu if err != nil { return fmt.Errorf("could not index execution receipt: %w", err) } - err = s.results.Index(receipt.ExecutionResult.BlockID, receipt.ExecutionResult.ID()) - if err != nil { - return fmt.Errorf("could not index execution result: %w", err) - } return nil } diff --git a/engine/execution/state/unittest/fixtures.go b/engine/execution/state/unittest/fixtures.go index cd9e45185c8..323688c0088 100644 --- a/engine/execution/state/unittest/fixtures.go +++ b/engine/execution/state/unittest/fixtures.go @@ -8,12 +8,12 @@ import ( "github.com/onflow/flow-go/utils/unittest" ) -func StateInteractionsFixture() *delta.Snapshot { +func StateInteractionsFixture() *delta.SpockSnapshot { return delta.NewView(nil).Interactions() } func ComputationResultFixture(collectionsSignerIDs [][]flow.Identifier) *execution.ComputationResult { - stateViews := make([]*delta.Snapshot, len(collectionsSignerIDs)) + stateViews := make([]*delta.SpockSnapshot, len(collectionsSignerIDs)) for i := 0; i < len(collectionsSignerIDs); i++ { stateViews[i] = StateInteractionsFixture() } @@ -25,7 +25,7 @@ func ComputationResultFixture(collectionsSignerIDs [][]flow.Identifier) *executi func ComputationResultForBlockFixture(completeBlock *entity.ExecutableBlock) *execution.ComputationResult { n := len(completeBlock.CompleteCollections) - stateViews := make([]*delta.Snapshot, n) + stateViews := make([]*delta.SpockSnapshot, n) for i := 0; i < n; i++ { stateViews[i] = StateInteractionsFixture() } diff --git a/storage/badger/operation/interactions_test.go b/storage/badger/operation/interactions_test.go index 6f6f190abee..da8f6df57b4 100644 --- a/storage/badger/operation/interactions_test.go +++ b/storage/badger/operation/interactions_test.go @@ -35,7 +35,7 @@ func TestStateInteractionsInsertCheckRetrieve(t *testing.T) { _, err = d1.Get(string([]byte{3}), "", "") require.NoError(t, err) - interactions := []*delta.Snapshot{d1.Interactions(), d2.Interactions()} + interactions := []*delta.Snapshot{&d1.Interactions().Snapshot, &d2.Interactions().Snapshot} blockID := unittest.IdentifierFixture() diff --git a/utils/unittest/fixtures.go b/utils/unittest/fixtures.go index 54ec70727b7..d60d03aae89 100644 --- a/utils/unittest/fixtures.go +++ b/utils/unittest/fixtures.go @@ -128,7 +128,7 @@ func BlockWithParentFixture(parent *flow.Header) flow.Block { } func StateInteractionsFixture() *delta.Snapshot { - return delta.NewView(nil).Interactions() + return &delta.NewView(nil).Interactions().Snapshot } func BlockWithParentAndProposerFixture(parent *flow.Header, proposer flow.Identifier) flow.Block { From 7c449d4b32ecf75c7370cbcbe53042fe3c057ed9 Mon Sep 17 00:00:00 2001 From: Maks Pawlak <120831+m4ksio@users.noreply.github.com> Date: Tue, 13 Oct 2020 21:34:05 -0700 Subject: [PATCH 2/6] Fix missing storage of ExecutionResults --- engine/execution/ingestion/engine_test.go | 8 +++++++- engine/execution/state/state.go | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/engine/execution/ingestion/engine_test.go b/engine/execution/ingestion/engine_test.go index b2d55e2fab6..19bd7efdd18 100644 --- a/engine/execution/ingestion/engine_test.go +++ b/engine/execution/ingestion/engine_test.go @@ -217,7 +217,13 @@ func (ctx *testingContext) assertSuccessfulBlockComputation(executableBlock *ent Return(nil) ctx.executionState. - On("PersistExecutionResult", mock.Anything, executableBlock.Block.Header). + On( + "PersistExecutionResult", + mock.Anything, + mock.MatchedBy(func(executionResult *flow.ExecutionResult) bool { + return executionResult.BlockID == executableBlock.Block.ID() && executionResult.PreviousResultID == previousExecutionResultID + }), + ). Return(nil) ctx.executionState. diff --git a/engine/execution/state/state.go b/engine/execution/state/state.go index d150c193213..6eb8229b00d 100644 --- a/engine/execution/state/state.go +++ b/engine/execution/state/state.go @@ -95,7 +95,13 @@ type state struct { } func (s *state) PersistExecutionResult(ctx context.Context, executionResult *flow.ExecutionResult) error { - err := s.results.Index(executionResult.BlockID, executionResult.ID()) + + err := s.results.Store(executionResult) + if err != nil { + return fmt.Errorf("could not store result: %w", err) + } + + err = s.results.Index(executionResult.BlockID, executionResult.ID()) if err != nil { return fmt.Errorf("could not index execution result: %w", err) } From a4a86b72b2ff55bf79f666deeb2ea0965bcb81cd Mon Sep 17 00:00:00 2001 From: Maks Pawlak <120831+m4ksio@users.noreply.github.com> Date: Wed, 14 Oct 2020 09:00:14 -0700 Subject: [PATCH 3/6] Linting --- engine/execution/ingestion/engine.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/engine/execution/ingestion/engine.go b/engine/execution/ingestion/engine.go index e44b2da0b5e..9d03078a470 100644 --- a/engine/execution/ingestion/engine.go +++ b/engine/execution/ingestion/engine.go @@ -731,6 +731,9 @@ func (e *Engine) handleComputationResult( result.TransactionResult, startState, ) + if err != nil { + return nil, fmt.Errorf("could not save execution results: %w", err) + } receipt, err := e.generateExecutionReceipt(ctx, executionResult, result.StateSnapshots) if err != nil { From c0dc548418d36f74fe87956ee9edd000139b2214 Mon Sep 17 00:00:00 2001 From: Maks Pawlak <120831+m4ksio@users.noreply.github.com> Date: Fri, 23 Oct 2020 11:13:14 -0700 Subject: [PATCH 4/6] Use proper context --- engine/execution/ingestion/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/execution/ingestion/engine.go b/engine/execution/ingestion/engine.go index 9d03078a470..08a7d3df9ff 100644 --- a/engine/execution/ingestion/engine.go +++ b/engine/execution/ingestion/engine.go @@ -833,7 +833,7 @@ func (e *Engine) saveExecutionResults( return nil, fmt.Errorf("could not generate execution result: %w", err) } - err = e.execState.PersistExecutionResult(ctx, executionResult) + err = e.execState.PersistExecutionResult(childCtx, executionResult) if err != nil { return nil, fmt.Errorf("could not persist execution result: %w", err) } From 83c490ed326da563634e1d0e9da2f398517e33f0 Mon Sep 17 00:00:00 2001 From: Maks Pawlak <120831+m4ksio@users.noreply.github.com> Date: Tue, 27 Oct 2020 11:27:21 -0700 Subject: [PATCH 5/6] Fix merge --- engine/execution/ingestion/engine.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/engine/execution/ingestion/engine.go b/engine/execution/ingestion/engine.go index 2af2a4a2a69..4c3fae19f09 100644 --- a/engine/execution/ingestion/engine.go +++ b/engine/execution/ingestion/engine.go @@ -746,6 +746,20 @@ func (e *Engine) handleComputationResult( return nil, fmt.Errorf("could not generate execution receipt: %w", err) } + err = func() error { + span, _ := e.tracer.StartSpanFromContext(ctx, trace.EXESaveExecutionReceipt) + defer span.Finish() + + err = e.execState.PersistExecutionReceipt(ctx, receipt) + if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { + return fmt.Errorf("could not persist execution receipt: %w", err) + } + return nil + }() + if err != nil { + return nil, err + } + err = e.providerEngine.BroadcastExecutionReceipt(ctx, receipt) if err != nil { return nil, fmt.Errorf("could not send broadcast order: %w", err) @@ -881,20 +895,6 @@ func (e *Engine) saveExecutionResults( return nil, err } - err = func() error { - span, _ := e.tracer.StartSpanFromContext(childCtx, trace.EXESaveExecutionReceipt) - defer span.Finish() - - err = e.execState.PersistExecutionReceipt(ctx, receipt) - if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { - return fmt.Errorf("could not persist execution receipt: %w", err) - } - return nil - }() - if err != nil { - return nil, err - } - e.log.Debug(). Hex("block_id", logging.Entity(executableBlock)). Hex("start_state", originalState). From a9698a95044bf845bcf49954c8b120742f85639a Mon Sep 17 00:00:00 2001 From: Kay-Zee Date: Mon, 2 Nov 2020 18:29:16 -0800 Subject: [PATCH 6/6] resolve merge issue --- engine/execution/ingestion/engine.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/execution/ingestion/engine.go b/engine/execution/ingestion/engine.go index 3344110d73d..e17346f35ac 100644 --- a/engine/execution/ingestion/engine.go +++ b/engine/execution/ingestion/engine.go @@ -989,12 +989,12 @@ func (e *Engine) handleComputationResult( startState, ) if err != nil { - return nil, fmt.Errorf("could not save execution results: %w", err) + return nil, nil, fmt.Errorf("could not save execution results: %w", err) } receipt, err := e.generateExecutionReceipt(ctx, executionResult, result.StateSnapshots) if err != nil { - return nil, fmt.Errorf("could not generate execution receipt: %w", err) + return nil, nil, fmt.Errorf("could not generate execution receipt: %w", err) } err = func() error {