[Storage Refactor] Refactor saving execution results by zhangchiqing · Pull Request #6906 · onflow/flow-go

zhangchiqing · 2025-01-16T16:31:50Z

This PR updates the execution node and access node’s saving result process to use the new storage abstraction.

The following database modules are refactored into new storage abstraction:

events
service events
light transaction results
transactionResultErrorMessages
commits
execution results
execution receipts
my execution receipts
transaction results

codecov-commenter · 2025-01-22T21:52:08Z

Codecov Report

Attention: Patch coverage is 38.90063% with 578 lines in your changes missing coverage. Please review.

Project coverage is 41.25%. Comparing base (cb8722b) to head (5fb21c8).

Files with missing lines	Patch %	Lines
storage/operation/transaction_results.go	0.00%	94 Missing ⚠️
engine/execution/state/state.go	49.25%	27 Missing and 7 partials ⚠️
storage/store/results.go	65.26%	27 Missing and 6 partials ⚠️
cmd/execution_builder.go	0.00%	31 Missing ⚠️
storage/store/receipts.go	57.35%	22 Missing and 7 partials ⚠️
storage/store/events.go	26.31%	28 Missing ⚠️
engine/testutil/nodes.go	0.00%	24 Missing ⚠️
storage/store/transaction_results.go	54.54%	17 Missing and 3 partials ⚠️
...ck-executed-height/cmd/rollback_executed_height.go	0.00%	19 Missing ⚠️
storage/store/light_transaction_results.go	57.14%	15 Missing and 3 partials ⚠️
... and 38 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6906      +/-   ##
==========================================
- Coverage   41.26%   41.25%   -0.02%     
==========================================
  Files        2155     2158       +3     
  Lines      189309   189421     +112     
==========================================
+ Hits        78125    78150      +25     
- Misses     104673   104754      +81     
- Partials     6511     6517       +6

Flag	Coverage Δ
unittests	`41.25% <38.90%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

zhangchiqing · 2025-02-19T00:28:48Z

cmd/access/node_builder/access_node_builder.go

+	events                         storage.Events
+	lightTransactionResults        storage.LightTransactionResults
+	transactionResultErrorMessages storage.TransactionResultErrorMessages


Access node syncs events, transaction results from EN with indexer engine, I'm refactoring them altogether along with the execution node's saving results process.

zhangchiqing · 2025-02-19T00:29:49Z

cmd/access/node_builder/access_node_builder.go

 			}).
 			Module("transaction results storage", func(node *cmd.NodeConfig) error {
-				builder.Storage.LightTransactionResults = bstorage.NewLightTransactionResults(node.Metrics.Cache, node.DB, bstorage.DefaultCacheSize)
+				builder.lightTransactionResults = store.NewLightTransactionResults(node.Metrics.Cache, node.ProtocolDB, bstorage.DefaultCacheSize)


ProtocolDB is a storage abstraction, so that we can change in a single place to decide using badger or pebble.

zhangchiqing · 2025-02-19T00:32:12Z

cmd/execution_builder.go

 		exeNode.txResults,
-		node.DB,
+		node.ProtocolDB,
+		getLatestFinalized,


This is for the execution state to implement getting highest finalized and executed block.

zhangchiqing · 2025-02-19T00:34:10Z

cmd/util/cmd/find-inconsistent-result/cmd.go


 	var blockID flow.Identifier
-	var lastExecuted uint64
-	err = db.View(procedure.GetLastExecutedBlock(&lastExecuted, &blockID))


The GetLastExecutedBlock procedure is replaced by RetrieveExecutedBlock which returns only the block ID, and getting the block with protocol State.

zhangchiqing · 2025-02-19T00:59:13Z

storage/operation/commits.go

+}
+
+// RemoveStateCommitment removes the state commitment by block ID
+func RemoveStateCommitment(w storage.Writer, blockID flow.Identifier) error {


This file is mostly identical with storage/badger/operation/commits.go, except BatchRemoveStateCommitment and BatchIndexStateCommitment are removed, since RemoveStateCommitment is always a batch updates.

zhangchiqing · 2025-02-19T01:10:37Z

storage/operation/results_test.go

@@ -0,0 +1,31 @@
+package operation_test


this file is identical to the same file in storage/badger/operation/

zhangchiqing · 2025-02-19T01:12:15Z

storage/operation/transaction_results.go

@@ -0,0 +1,166 @@
+package operation


this file is identical to the same file in storage/badger/operation/

Except the following functions are removed:

BatchInsertTransactionResult

BatchIndexTransactionResult

zhangchiqing · 2025-02-19T01:13:35Z

storage/store/commits.go

@@ -0,0 +1,82 @@
+package store


this file is identical to the same file in storage/badger/operation/

zhangchiqing · 2025-02-19T01:18:36Z

storage/store/my_receipts.go

+		blockID := receipt.ExecutionResult.BlockID
+		receiptID := receipt.ID()
+		var myOwnReceiptExecutedBefore flow.Identifier
+		err := operation.LookupOwnExecutionReceipt(rw.GlobalReader(), blockID, &myOwnReceiptExecutedBefore)


This is where we need to pay attention to, the original implementation can check if an own receipt already exist during insert.

if errors.Is(err, storage.ErrAlreadyExists) { var savedReceiptID flow.Identifier err := operation.LookupOwnExecutionReceipt(blockID, &savedReceiptID)(tx) if err != nil { return err }

The original implementation can do so because it's using badger transaction. However, the new storage abstraction uses badger updates, so we have to check it ourselves.

The new way it works is to acquire the indexingOwnReceipts and ensure no other process is storing own receipts, so that the check for if any own receipts for this block won't be stale.

zhangchiqing · 2025-02-19T01:19:29Z

storage/store/my_receipts_test.go

+		})
+	})
+
+	t.Run("store1 different receipt concurrent for same block should fail", func(t *testing.T) {


This file is the same as the original, except this added concurrent tests.

thread safe

zhangchiqing · 2025-02-21T05:00:54Z

storage/store/my_receipts_test.go

+			block := unittest.BlockFixture()
+			receipt1 := unittest.ReceiptForBlockFixture(&block)
+
+			err := db.WithReaderBatchWriter(func(rw storage.ReaderBatchWriter) error {


This file is the same as the deleted storage/badger/my_receipts_test.go, except the StoreMyReceipt call is replaced with BatchStoreMyReceipt, since we never call StoreMyReceipt alone.

peterargue

long, but straight forward. looks good to me

fxamacker

LGTM! I mostly focused on general Go programming.

fxamacker · 2025-02-28T16:47:42Z

cmd/util/cmd/find-inconsistent-result/cmd.go

+	reader := badgerimpl.ToDB(db).Reader()
+	err = operation.RetrieveExecutedBlock(reader, &blockID)


findLastExecutedAndSealedHeight() (not added in this PR) appears to be very similar to latest.LatestSealedAndExecutedHeight(). Maybe reuse that function.

flow-go/module/block_iterator/latest/sealed_and_executed.go

Line 63 in 516365e

func LatestSealedAndExecutedHeight(state protocol.State, db storage.DB) (uint64, error) {

zhangchiqing force-pushed the leo/db-ops-save-execution-results branch 2 times, most recently from f793dcf to 51b6fa0 Compare January 22, 2025 21:48

zhangchiqing force-pushed the leo/db-ops-save-execution-results branch 6 times, most recently from 7aaf9ae to 868a31e Compare February 12, 2025 23:13

zhangchiqing added 21 commits February 12, 2025 16:29

refactor events

0c5345f

add events

f62e8e7

transaction results and events

0276508

refactor receipts results transactions results

ae289b8

refactor commits operation

2be0689

refactor commits

7fee0e6

refactor state

80b08bf

refactor executed

9c88816

fix roll back

706a76c

fix execution state

56ddcaf

fix tests

ed6e97d

fix tests and mocks

bb99b85

fix tests

2fdc19b

fix transaction_results

23bcaf8

add receipts to be used by payloads

c5c9c9b

update badger.All

e020c45

move execution receipts

5c19365

fix bootstrap

a3aebb1

fix execution bootstrapper

e20d127

fix access tests

f6fa66b

implement execution storage layer

bd2f97c

zhangchiqing added 4 commits February 12, 2025 17:23

fix mocks

b7462c1

fix bootstrapping

dfc9b00

fix GetLastExecutedBlock

2532b07

fix tests

598a21a

zhangchiqing commented Feb 19, 2025

View reviewed changes

zhangchiqing mentioned this pull request Feb 19, 2025

Replacing Badger with Pebble DB #6515

Closed

17 tasks

zhangchiqing added 2 commits February 20, 2025 17:55

remove unused myreceipt.Store method, making save execution result

8eebce7

thread safe

fix mocks

930aacf

zhangchiqing changed the title ~~[WIP] [Storage Refactor] Refactor saving execution results~~ [Storage Refactor] Refactor saving execution results Feb 21, 2025

remove concurrent test

d96a8ce

zhangchiqing commented Feb 21, 2025

View reviewed changes

zhangchiqing added 2 commits February 21, 2025 10:16

reuse store methods in receipts and results

dedf906

remove test case

a323c20

zhangchiqing marked this pull request as ready for review February 21, 2025 19:15

zhangchiqing requested a review from a team as a code owner February 21, 2025 19:15

zhangchiqing requested review from fxamacker, janezpodhostnik and peterargue February 21, 2025 19:15

zhangchiqing added 3 commits February 25, 2025 15:29

Merge branch 'master' into leo/db-ops-save-execution-results

d49876b

refactor latest sealed and executed height

e58edd7

Merge branch 'master' into leo/db-ops-save-execution-results

516365e

peterargue approved these changes Feb 26, 2025

View reviewed changes

fxamacker approved these changes Feb 28, 2025

View reviewed changes

zhangchiqing added 2 commits March 4, 2025 13:42

reuse LatestSealedAndExecuted

43796c6

Merge branch 'master' into leo/db-ops-save-execution-results

5fb21c8

zhangchiqing added this pull request to the merge queue Mar 5, 2025

Merged via the queue into master with commit 109b7e2 Mar 5, 2025
56 checks passed

zhangchiqing deleted the leo/db-ops-save-execution-results branch March 5, 2025 21:55

j1010001 mentioned this pull request Mar 10, 2025

Badger -> Pebble DB M2 - DB access refactoring for low-risk data (AN, EN, VN) #6527

Closed

17 tasks

		reader := badgerimpl.ToDB(db).Reader()
		err = operation.RetrieveExecutedBlock(reader, &blockID)

Conversation

zhangchiqing commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterargue left a comment

Choose a reason for hiding this comment

Uh oh!

fxamacker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhangchiqing commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 22, 2025 •

edited

Loading