Skip to content

op-program: Fix missing block re-exec preimages in host#14027

Merged
Inphi merged 2 commits intodevelopfrom
inphi/host-statedb
Jan 29, 2025
Merged

op-program: Fix missing block re-exec preimages in host#14027
Inphi merged 2 commits intodevelopfrom
inphi/host-statedb

Conversation

@Inphi
Copy link
Copy Markdown
Contributor

@Inphi Inphi commented Jan 28, 2025 •
edited
Loading

Make sure that the block data hint route persists all block data, including trie nodes in the host. Otherwise, the interop client won't be able to lookup, for example, receipts preimages needed for the consolidation step of the fault proof.

part of #14020

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 64.77273% with 31 lines in your changes missing coverage. Please review.

Project coverage is 42.68%. Comparing base (95cd71a) to head (28e6de9).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
op-program/host/common/l2_store.go 78.68% 11 Missing and 2 partials ⚠️
op-program/host/common/common.go 0.00% 8 Missing ⚠️
op-program/client/program.go 20.00% 3 Missing and 1 partial ⚠️
op-program/client/tasks/deposits_block.go 0.00% 2 Missing ⚠️
op-program/client/interop/interop.go 0.00% 1 Missing ⚠️
op-program/client/tasks/derive.go 0.00% 1 Missing ⚠️
op-program/host/host.go 0.00% 1 Missing ⚠️
op-program/host/prefetcher/reexec.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #14027      +/-   ##
===========================================
- Coverage    47.16%   42.68%   -4.48%     
===========================================
  Files          974      810     -164     
  Lines        81670    72433    -9237     
===========================================
- Hits         38516    30918    -7598     
+ Misses       40255    38793    -1462     
+ Partials      2899     2722     -177
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

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

Files with missing lines Coverage Δ
op-program/client/l2/db.go 54.76% <100.00%> (ø)
op-program/client/l2/engine_backend.go 79.23% <100.00%> (ø)
op-program/client/preinterop.go 82.35% <100.00%> (+1.10%) ⬆️
op-program/client/interop/interop.go 42.40% <0.00%> (-0.35%) ⬇️
op-program/client/tasks/derive.go 37.03% <0.00%> (ø)
op-program/host/host.go 5.63% <0.00%> (ø)
op-program/host/prefetcher/reexec.go 60.52% <0.00%> (ø)
op-program/client/tasks/deposits_block.go 0.00% <0.00%> (ø)
op-program/client/program.go 28.57% <20.00%> (-3.01%) ⬇️
op-program/host/common/common.go 0.00% <0.00%> (ø)
... and 1 more

... and 170 files with indirect coverage changes

@Inphi Inphi force-pushed the inphi/host-statedb branch 2 times, most recently from f97582c to 39579ec Compare January 28, 2025 21:52
@Inphi Inphi force-pushed the inphi/host-statedb branch from 39579ec to 1cf221b Compare January 29, 2025 00:08
Inphi
Inphi commented Jan 29, 2025
View reviewed changes
Comment thread op-program/host/common/l2_store.go
@Inphi Inphi requested a review from ajsutton January 29, 2025 00:53
@Inphi Inphi marked this pull request as ready for review January 29, 2025 00:54
@Inphi Inphi requested review from a team January 29, 2025 00:54
ajsutton
ajsutton approved these changes Jan 29, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM, but I suspect this won't capture the intermediate trie nodes for the transaction and receipt tries, because geth stores just the list of transactions and receipts, not the whole trie. We don't notice this normally because the prefetcher fetches the transactions and receipts using dedicated RPCs for fetching them (not debug_dbGet) and then rebuilds the trie and stores all the preimages:

optimism/op-program/host/prefetcher/prefetcher.go

Lines 412 to 437 in d895830

func (p *Prefetcher) storeReceipts(receipts types.Receipts) error {
opaqueReceipts, err := eth.EncodeReceipts(receipts)
if err != nil {
return err
}
return p.storeTrieNodes(opaqueReceipts)
}
func (p *Prefetcher) storeTransactions(txs types.Transactions) error {
opaqueTxs, err := eth.EncodeTransactions(txs)
if err != nil {
return err
}
return p.storeTrieNodes(opaqueTxs)
}
func (p *Prefetcher) storeTrieNodes(values []hexutil.Bytes) error {
_, nodes := mpt.WriteTrie(values)
for _, node := range nodes {
key := preimage.Keccak256Key(crypto.Keccak256Hash(node)).PreimageKey()
if err := p.kvStore.Put(key, node); err != nil {
return fmt.Errorf("failed to store node: %w", err)
}
}
return nil
}

This code is necessary, I just think we'll also have to capture the transactions and receipts being captured and also write those intermediate nodes somehow. Could be a follow up though and definitely something worth having a test demonstrating their missing before adding so we know for sure we are capturing them (plus I'd be happy to be pleasantly surprised that they are getting stored...)

@Inphi
Copy link
Copy Markdown
Contributor Author

Inphi commented Jan 29, 2025

LGTM, but I suspect this won't capture the intermediate trie nodes for the transaction and receipt tries, because geth stores just the list of transactions and receipts, not the whole trie. We don't notice this normally because the prefetcher fetches the transactions and receipts using dedicated RPCs for fetching them (not debug_dbGet) and then rebuilds the trie and stores all the preimages:

Thanks for checking. I'll follow up on making the txs and receipts intermediate nodes available. It should be pretty doable since the program has the full block. Just need to install a hook for the host to ingest it.

@Inphi Inphi added this pull request to the merge queue Jan 29, 2025
Merged via the queue into develop with commit d39eb24 Jan 29, 2025
@Inphi Inphi deleted the inphi/host-statedb branch January 29, 2025 01:54
Rjected pushed a commit to paradigmxyz/optimism that referenced this pull request Feb 25, 2025
@Inphi @Rjected
op-program: Fix missing block re-exec preimages in host (ethereum-opt…
c2023be 
…imism#14027)

* op-program: Fix missing block re-exec preimages in host

* check put key length
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajsutton ajsutton ajsutton approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Inphi @ajsutton