Modular plugable architecture

[phutchins]

No description provided.

[sergefdrv]

Note: most of the individual comments in this review are likely to be irrelevant, given the general feedback and recommendations below, but I keep them for future reference. My unfinished experiments (with broken build) can be found here.

Overall, I think the attempted module/plugin API brings in too much complexity and seems to demand substantial prior refactoring of the code base. Perhaps, this would make more sense to reconsidered after finishing the effort of cleaning up the generic IPC code and making it more modular (a.k.a. ipc-lib). The current plugin design in this PR has some serious limitations, some of which may be quite difficult to overcome:

• no support for multiple simultaneously enabled plugins/modules;
• unconditionally bundling all plugin/module traits together;
• combining additive hooks together with the executor, which can only be one;
• dirty hacks potentially compromising type safety;
• some plugin/module traits are not dyn-object-safe (could not be combined in a regular collection);
• additional generic type parameters spreading widely through the code base.

Therefore, I suggest the following approach for integrating the storage functionality into the IPC code base:

1. Rebasing and renaming:

• rebase the storage feature branch on top of main,
• apply renaming/moving files around (see 0cd2fc4...5a515cd);

2. Conditional compilation:

• compare the resulting storage feature branch against main with git to see all modifications of the generic IPC code introduced by the storage functionality,
• make those additions conditionally-compiled with cargo features (see 5a515cd...0e9ccb5);

3. Cleanup and CI:

• move any remaining storage-related code snippets from the generic code into storage-specific code to minimize intrusiveness of those additions (but keep things possibly simple),
• ensure CI compiles and runs tests with the storage feature enabled as well as disabled;

Once the above stages are complete, the storage feature branch could be merged into main (rebasing in case of merge conflicts).

After that, we could consider adding native support for sponsored transactions:

• implement support for Ethereum-style sponsored transaction fees in the generic IPC executor for FVM,
• adapt the storage code to use that capability instead of the custom executor hacks.

Then, and depending on the ipc-lib work, we could get back to reconsidering implementing a dynamic plugin/module system.

[sergefdrv]

This code seems to assume that there can only be a single enabled plugin and it's hard-coded in a way that it can only be the storage node plugin.

[sergefdrv]

I believe this won't compile if there's more that a single enabled plugin.

[sergefdrv]

This case could be easily handled by the normal path.

[sergefdrv]

So we can only support a single active plugin?

[sergefdrv]

We may want to have plugins enabled in cargo features, i.e. compiled into the binary, but nevertheless disabled in node's runtime configuration.

[sergefdrv]

Suggested change

	async fn handle_message<DB: Blockstore + Send + Sync>(
	&self,
	state: &mut dyn MessageHandlerState,
	msg: &IpcMessage,
	) -> Result<Option<ApplyMessageResponse>>;
	async fn handle_message(
	&self,
	state: &mut dyn MessageHandlerState,
	msg: &IpcMessage,
	) -> Result<Option<ApplyMessageResponse>>;

[sergefdrv]

This looks dangerous and fragile. We can avoid this by adding the following trait bound to this function:

DB: Send,
M: ModuleBundle<
    Kernel: fvm::Kernel<
        CallManager: fvm::call_manager::CallManager<
            Machine = DefaultMachine<DB, FendermintExterns<DB>>,
        >,
    >,
>,

Would also need to make NoOpModuleBundle generic in blockstore and externs types, and sprinkle here and there trait constraints like this:

M: ModuleBundle<
    Kernel: fvm::Kernel<
        CallManager: fvm::call_manager::CallManager<
            Machine = DefaultMachine<
                ReadOnlyBlockstore<DB>,
                FendermintExterns<ReadOnlyBlockstore<DB>>,
            >,
        >,
    >,
>,

[sergefdrv]

We wouldn't need this hack if we make NoOpModuleBundle generic in blockstore and externs types as suggested in another comment rather than hardcoding NoOpExterns.

Suggested change

	// SAFETY: We use transmute here because NoOpModuleBundle's RecallExecutor
	// uses MemoryBlockstore internally, but the state tree operations are
	// generic and work with any Blockstore. The memory layout is compatible.
	let state_tree_ptr = (*exec_state).state_tree_mut_with_deref() as *mut _ as *mut StateTree<MachineBlockstore<DB>>;
	unsafe {
	g(&mut *state_tree_ptr)
	}
	g(exec_state.state_tree_mut_with_deref())

[sergefdrv]

This is dangerous and fragile, consider instead wrapping Stage into Mutex or RwLock in FvmGenesisState.

[sergefdrv]

If we really want to have this method, we need to find a way of getting the circulating supply from the exec state.