feat: add flush_all_blocks option to also write intermediate blocks #2101

rvagg · 2025-01-11T07:18:29Z

No description provided.

codecov-commenter · 2025-01-11T07:23:45Z

Codecov Report

Attention: Patch coverage is 13.63636% with 19 lines in your changes missing coverage. Please review.

Project coverage is 74.50%. Comparing base (7f8986c) to head (b18a544).

Files with missing lines	Patch %	Lines
fvm/src/blockstore/buffered.rs	0.00%	6 Missing ⚠️
fvm/src/executor/default.rs	0.00%	4 Missing ⚠️
fvm/src/executor/threaded.rs	0.00%	3 Missing ⚠️
fvm/src/machine/boxed.rs	0.00%	3 Missing ⚠️
fvm/src/machine/default.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2101      +/-   ##
==========================================
- Coverage   75.96%   74.50%   -1.46%     
==========================================
  Files         154      153       -1     
  Lines       15771    15170     -601     
==========================================
- Hits        11980    11302     -678     
- Misses       3791     3868      +77

Files with missing lines	Coverage Δ
fvm/src/executor/mod.rs	`21.21% <ø> (ø)`
fvm/src/kernel/default.rs	`82.26% <100.00%> (+0.06%)`	⬆️
fvm/src/machine/mod.rs	`71.21% <ø> (ø)`
fvm/src/executor/threaded.rs	`71.42% <0.00%> (-11.91%)`	⬇️
fvm/src/machine/boxed.rs	`0.00% <0.00%> (ø)`
fvm/src/machine/default.rs	`68.42% <0.00%> (-3.46%)`	⬇️
fvm/src/executor/default.rs	`76.92% <0.00%> (-0.80%)`	⬇️
fvm/src/blockstore/buffered.rs	`14.07% <0.00%> (-68.46%)`	⬇️

... and 17 files with indirect coverage changes

* Plumb through dump_cache from fvm4 to access intermediate blocks: - filecoin-project/filecoin-ffi#512 - filecoin-project/ref-fvm#2101 * Enable cache dumping in StateReplay with LOTUS_REPLAY_DUMP_CACHED_BLOCKS * Add optional "Blocks" field InvocResult * Handle ExecutionEvent::Log's and add "Logs" field to ExecutionTrace * Dump intermediate cache blocks to CAR in /tmp when they appear while using `lotus-shed msg --exec-trace`.

rvagg · 2025-01-13T01:45:01Z

#1994

Stebalien · 2025-01-21T06:01:54Z

This seems like a reasonable approach but I'm not super happy adding this to the Executor/Machine traits. What about adding a "flush intermediate blocks to" option to either:

The DefaultMachine::new constructor. That is, instead of taking a single blockstore, it would take a blockstore for the "real" blocks and an Option<Blockstore> for everything else (although we'd likely want to erase the types to avoid adding a new generic?).
Ditto, but just make it a flag? I.e., if some "write intermediates" flag is set, flush writes all blocks?
Maybe specify it as an option in the machine context? I.e., a flag next to the tracing flag?

This will also impact how it's integrated is writing these intermediate blocks to the real blockstore going to be an issue? IMO, probably not for debugging but definitely if we want to use it when tracing.

- Update url dependency. We don't use it directly, but wasmtime does. - Update the builtin actors (dev) dependency to remove some crufty dependencies. - Add licenses everywhere.

* chore: prep release v4.5.4 chore: prep release v4.5.4 * chore: fix changelogs * chore: bump minor version instead of patch version Unfortunately, we have a tiny breaking change in `fvm_shared` renaming something. I merged it because we left it sitting for ages waiting for a "good time" and, in my infinite wisdom, I picked a releas that contained no other major breaking changes... * chore: release fvm_ipld_encoding 0.5.2 --------- Co-authored-by: Steven Allen <[email protected]>

This didn't get updated in the lockfile correctly. It doesn't really matter (it's just the lock file) but it was causing issues when publishing.

rvagg · 2025-03-21T04:54:15Z

I'd really like to specify a separate blockstore for this, there's no good reason to pollute the main blockstore with a ton of garbage.

I thought maybe I could put this on the MachineContext: pub flush_intermediates_to: Option<Arc<dyn Blockstore>>, but that's pretty janky and other stuff has to break just to make the compiler happy with it being there. I could make another argument and then if it's Some take that to mean that they should always be flushed. But then I run into problems with the FFI interface:

https://github.com/filecoin-project/filecoin-ffi/blob/037aa5ce11958eb2b47799b4cf16922874f6e2e1/cgo/interface.go#L30-L38

Currently the blockstore is the whole Externs object as well, so we exHandle := cgo.Register(context.TODO(), opts.Externs) and then use exHandle for both the Externs and the Blockstore. So another blockstore needs another handle, and then I have to figure out how nil->None works for cgo. Maybe I can hang it off the existing Externs, do a GetIntermediatBlockstore?

rvagg · 2025-03-21T04:55:11Z

i.e. it was significantly easier to have an entirely separate function and code path that doesn't touch FVM initialisation thanks to the complexity of the FFI layer

rvagg · 2025-03-27T03:44:12Z

Changed to just be a flush_all_blocks option on the context. Still depends on #2109

Matches builtin actors and fixes cargo-deny.

rvagg · 2025-03-28T08:17:08Z

fvm/src/kernel/default.rs

+        // TODO: remove this, it's just for debugging.
+        self.call_manager.log(format!("block_link({})", k));


@Stebalien I removed this in my latest version but realised I now lose the ability to see what blocks are created during which part of a message execution - with it, I can look in the trace, find these, and know which blocks were created during a particular call or subcall exec. Now I only get all the blocks for the entirety of a message's execution. Do you have any clever ideas for how I might restore the functionality?

I'm happy to add a new event type to the execution log. But that'll be a bit of a larger change (we'll probably need to generalize CallManager::log.

I've also considered changing how our gas events work a bit: ideally we'd log syscalls/actor calls then associate gas charges with those syscalls. I'd leave this out of this change and chat about it sync next week.

Ref: filecoin-project/ref-fvm#2101

* Plumb through dump_cache from fvm4 to access intermediate blocks: - filecoin-project/filecoin-ffi#512 - filecoin-project/ref-fvm#2101 * Enable cache dumping in StateReplay with LOTUS_REPLAY_DUMP_CACHED_BLOCKS * Add optional "Blocks" field InvocResult * Handle ExecutionEvent::Log's and add "Logs" field to ExecutionTrace * Dump intermediate cache blocks to CAR in /tmp when they appear while using `lotus-shed msg --exec-trace`.

Stebalien

This seems like a reasonable approach.

Stebalien · 2025-03-28T19:50:27Z

fvm/src/blockstore/buffered.rs

+            self.write.borrow().len()
+        );
+        self.base
+            .put_many_keyed(self.write.borrow().iter().map(|(k, v)| (*k, v.as_slice())))


nit: flush actually removes them from this blockstore to avoid writing twice (just in case). I'd do that here as well. I.e., put_many_keyed(self.write.borrow_mut().drain()....

ahh good point, I've made it do this and added some tests to assert the behaviour of this vs flush()

fvm/src/blockstore/buffered.rs

* add tests

rvagg · 2025-03-31T03:57:30Z

Converting to draft because it's stacked on #2109 plus a master rebase so this branch is a bit messy and shouldn't be merged as is

rvagg added a commit to filecoin-project/filecoin-ffi that referenced this pull request Jan 11, 2025

feat: support DumpCache, handle Logs in traces

e08bbe8

Ref: filecoin-project/ref-fvm#2101

rvagg mentioned this pull request Jan 11, 2025

feat: support DumpCache, handle Logs in traces filecoin-project/filecoin-ffi#512

Draft

This was referenced Jan 11, 2025

feat(exec): dump intermediate cache blocks from FVM exec in StateReplay filecoin-project/lotus#12822

Draft

Expose intermediate state #1994

Open

Stebalien and others added 4 commits March 14, 2025 19:11

feat: support tock (#2110)

ac25034

chore: switch to cargo-deny and fix some lints (#2111)

e7f0bc6

- Update url dependency. We don't use it directly, but wasmtime does. - Update the builtin actors (dev) dependency to remove some crufty dependencies. - Add licenses everywhere.

chore: update fvm_ipld_encoding

280d805

This didn't get updated in the lockfile correctly. It doesn't really matter (it's just the lock file) but it was causing issues when publishing.

rvagg mentioned this pull request Mar 21, 2025

chore: update to rust 1.81.0 #2109

Draft

rvagg force-pushed the rvagg/dump-cache branch from b18a544 to 924222c Compare March 27, 2025 03:42

rvagg changed the base branch from master to steb/update-rust March 27, 2025 03:42

rvagg changed the title ~~add dump_cache() to write out intermediate blocks to a blockstore~~ feat: add flush_all_blocks option to also write intermediate blocks Mar 27, 2025

rvagg force-pushed the rvagg/dump-cache branch 2 times, most recently from 398c205 to c038a4a Compare March 27, 2025 03:43

rvagg marked this pull request as ready for review March 27, 2025 03:43

rvagg force-pushed the rvagg/dump-cache branch from c038a4a to 2c7c2ba Compare March 27, 2025 03:50

Stebalien and others added 5 commits March 27, 2025 17:27

chore: update to rust 1.81.0

12cae1a

Matches builtin actors and fixes cargo-deny.

chore: remove references to wasmtime features not enabled

b776189

chore: clippy

976d0bf

add dump_cache() to write out intermediate blocks to a blockstore

0eaebbb

feat: add flush_all_blocks option to also write intermediate blocks

784aa42

rvagg force-pushed the rvagg/dump-cache branch from 2c7c2ba to 784aa42 Compare March 27, 2025 06:44

rvagg commented Mar 28, 2025

View reviewed changes

rvagg added a commit to filecoin-project/filecoin-ffi that referenced this pull request Mar 28, 2025

feat: support DumpCache, handle Logs in traces

8659288

Ref: filecoin-project/ref-fvm#2101

rvagg added a commit to filecoin-project/filecoin-ffi that referenced this pull request Mar 28, 2025

feat: support DumpCache, handle Logs in traces

0579984

Ref: filecoin-project/ref-fvm#2101

Stebalien approved these changes Mar 28, 2025

View reviewed changes

fix: flush_all removes blocks from the BufferedBlockstore

e1c2407

* add tests

rvagg marked this pull request as draft March 31, 2025 03:57

rvagg mentioned this pull request Mar 31, 2025

feat: log(LogEntry) to handle different log types #2113

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flush_all_blocks option to also write intermediate blocks #2101

feat: add flush_all_blocks option to also write intermediate blocks #2101

rvagg commented Jan 11, 2025

codecov-commenter commented Jan 11, 2025 •

edited

Loading

rvagg commented Jan 13, 2025

Stebalien commented Jan 21, 2025

rvagg commented Mar 21, 2025

rvagg commented Mar 21, 2025

rvagg commented Mar 27, 2025

rvagg Mar 28, 2025

Stebalien Mar 28, 2025

Stebalien left a comment

Stebalien Mar 28, 2025

rvagg Mar 31, 2025

rvagg commented Mar 31, 2025

		// TODO: remove this, it's just for debugging.
		self.call_manager.log(format!("block_link({})", k));

feat: add flush_all_blocks option to also write intermediate blocks #2101

Are you sure you want to change the base?

feat: add flush_all_blocks option to also write intermediate blocks #2101

Conversation

rvagg commented Jan 11, 2025

codecov-commenter commented Jan 11, 2025 • edited Loading

Codecov Report

rvagg commented Jan 13, 2025

Stebalien commented Jan 21, 2025

rvagg commented Mar 21, 2025

rvagg commented Mar 21, 2025

rvagg commented Mar 27, 2025

rvagg Mar 28, 2025

Choose a reason for hiding this comment

Stebalien Mar 28, 2025

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Mar 28, 2025

Choose a reason for hiding this comment

rvagg Mar 31, 2025

Choose a reason for hiding this comment

rvagg commented Mar 31, 2025

codecov-commenter commented Jan 11, 2025 •

edited

Loading