Process WASM blocks from substreams #4916

mangas · 2023-10-13T13:02:36Z

Add support for running WASM mappings on incoming substreams blocks.

The block stream will emit the encoded result as a byte array
The mapping will need to handle the decoding of the input

Example is provided here

lutter

The biggest thing that I'd like to see addressed is the code duplication between handle_process_block and handle_process_wasm_block.

Since this touches and refactors some very sensitive core logic, it also needs a run in the integration cluster.

lutter · 2023-11-22T16:24:26Z

chain/arweave/src/chain.rs

+        _clock: Clock,
+        _cursor: FirehoseCursor,
+        _block: Vec<u8>,
+    ) -> Result<BlockStreamEvent<Chain>, Error> {
        unimplemented!()


Could this unimplemented be turned into an error so that we don't get a panic if we hit this?

If this code is reached it is a problem. Substreams is itself a chain so anything that calls into this code should be on another chain

My comment was more about that panics a lot of times are fairly invisible and easy to miss in our huge logs, errors are a bit easier to find. And since this already returns a Result, it wouldn't be hard to make this an error.

lutter · 2023-11-22T16:25:15Z

chain/cosmos/src/chain.rs

+        _clock: Clock,
+        _cursor: FirehoseCursor,
+        _block: Vec<u8>,
+    ) -> Result<BlockStreamEvent<Chain>, Error> {
        unimplemented!()


Same here - error would be preferable

lutter · 2023-11-22T16:26:42Z

chain/ethereum/src/chain.rs

+        _clock: Clock,
+        _cursor: FirehoseCursor,
+        _block: Vec<u8>,
+    ) -> Result<BlockStreamEvent<Chain>, Error> {
        unimplemented!()


Same here - error would be preferable

lutter · 2023-11-22T16:28:13Z

chain/near/src/chain.rs

 };

 const NEAR_FILTER_MODULE_NAME: &str = "near_filter";
-const SUBSTREAMS_TRIGGER_FILTER_BYTES: &[u8; 497306] = include_bytes!(
+const SUBSTREAMS_TRIGGER_FILTER_BYTES: &[u8; 510162] = include_bytes!(


A comment here what this is and what it is used for would be great. Also something about when/how this needs to be updated.

This is the binary produced by our other crate inside substreams, it will tell you when it needs to be updated :P

lutter · 2023-11-22T16:30:42Z

chain/substreams/src/block_stream.rs

+                logger,
+                chain.metrics_registry.clone(),
+            ),
+        };


Since most of the argument to SubstreamsBlockStream are the same in both branches, it would be clearer to just have something like let mapper = match .. { }; and then one statement SubstreamsBlockStream::new

I had that before and couldn't get it to compile, I can't remember what was the issue but I couldn't convince the compiler some specific type was correct. I will give it another go, I maybe have made more changes in the meantime

lutter · 2023-11-22T16:31:10Z

chain/substreams/src/data_source.rs

@@ -77,7 +77,7 @@ impl blockchain::DataSource<Chain> for DataSource {

    // runtime is not needed for substreams, it will cause the host creation to be skipped.


The comment doesn't seem accurate anymore

lutter · 2023-11-22T16:38:28Z

core/src/subgraph/context.rs

+        state = self
+            .instance
+            .hosts()
+            .first()


Maybe it's because I am not very familiar with this code, but why do we only pay attention to the first host here? What are the other ones for?

Hosts exist map 1:1 to DataSources, on substreams we only allow one data source since it's triggered on every block so we basically only care about the first one.

The code around the runner has all these points where context is required to understand what is going. It's something I would like to refactor at some point in the near future.

Added a comment

lutter · 2023-11-22T16:41:35Z

core/src/subgraph/runner.rs

+                    }
+                }
+
+                if matches!(action, Action::Restart) {


It would be better to make this a match action { .. } with all possibilities spelled out so that when anything is added to Action, the compiler points us to the places that require updating.

oh I think I see the issue, this is all old code, moved from elsewhere in the runner so it can be re-used so Idont want to change anything here

lutter · 2023-11-22T16:45:25Z

core/src/subgraph/runner.rs

+                    .start_section(PROCESS_WASM_BLOCK_SECTION_NAME);
+                self.handle_process_wasm_block(block_ptr, data, handler, cursor, cancel_handle)
+                    .await?
+            }


For a future refactor: why aren't the branches for Some(Ok(event)) a methond on BlockStreamEvent so that these three branches could simple be Some(Ok(event)) => event.handle(stopwatch, cancel_handle) ?

lutter · 2023-11-22T17:04:32Z

core/src/subgraph/runner.rs

@@ -880,197 +1077,247 @@ where
    C: Blockchain,
    T: RuntimeHostBuilder<C>,
 {
-    async fn handle_process_block(
+    async fn handle_process_wasm_block(


There seems to be a lot of code duplication between handle_process_wasm_block and handle_process_block which I fear will lead to subtle bugs as that logic evolves over time. Is there no way to avoid that duplication? I think handle_process_block would also be improved by introducing some helper structs that encapsulate some of the logic that is shared between the two; I know it's grown over time, but to the uninitiated, the code for handle_process_block is very hard to follow.

Similar to my other comment, I think this whole section requires a refactor btu I would first like to understand better how substream will work so I decided to duplicate a lot of the code instead of doing the refactor now but this certainly needs improvement in the near future

Can you then add an issue so this doesn't get completely forgotten?

lutter · 2023-12-04T19:05:32Z

I've approved this though I have pretty big reservations about the code duplication, and the fact that that code is pretty much undocumented (what's the high-level intended behavior of that code?) and very hard to follow.

mangas force-pushed the filipe/substreams-wasm-block branch 4 times, most recently from 670b89b to dce78e4 Compare November 7, 2023 15:59

mangas force-pushed the filipe/substreams-wasm-block branch 3 times, most recently from dc2fb4b to f3fea4f Compare November 13, 2023 15:13

mangas marked this pull request as ready for review November 13, 2023 15:30

mangas requested a review from lutter November 13, 2023 15:30

lutter requested changes Nov 22, 2023

View reviewed changes

lutter approved these changes Dec 4, 2023

View reviewed changes

mangas force-pushed the filipe/substreams-wasm-block branch 3 times, most recently from 01fcdf9 to 4d1012b Compare December 5, 2023 12:47

Process WASM blocks from substreams

bafda5b

mangas force-pushed the filipe/substreams-wasm-block branch from 4d1012b to bafda5b Compare December 5, 2023 14:01

mangas merged commit d06e1b5 into master Dec 5, 2023
7 checks passed

mangas deleted the filipe/substreams-wasm-block branch December 5, 2023 16:07

lutter mentioned this pull request Jun 7, 2024

Remove provider checks at startup #5337

Merged

mangas mentioned this pull request Jun 18, 2024

Refactor subgraph runner #5499

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process WASM blocks from substreams #4916

Process WASM blocks from substreams #4916

mangas commented Oct 13, 2023 •

edited

Loading

lutter left a comment

lutter Nov 22, 2023

mangas Dec 4, 2023

lutter Dec 4, 2023

lutter Nov 22, 2023

lutter Nov 22, 2023

lutter Nov 22, 2023

mangas Dec 4, 2023

lutter Nov 22, 2023

mangas Dec 4, 2023

lutter Nov 22, 2023

lutter Nov 22, 2023

mangas Dec 4, 2023

mangas Dec 4, 2023

lutter Nov 22, 2023

mangas Dec 5, 2023

lutter Nov 22, 2023

lutter Nov 22, 2023

mangas Dec 4, 2023

lutter Dec 4, 2023

lutter commented Dec 4, 2023

		@@ -77,7 +77,7 @@ impl blockchain::DataSource<Chain> for DataSource {

		// runtime is not needed for substreams, it will cause the host creation to be skipped.

Process WASM blocks from substreams #4916

Process WASM blocks from substreams #4916

Conversation

mangas commented Oct 13, 2023 • edited Loading

lutter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lutter commented Dec 4, 2023

mangas commented Oct 13, 2023 •

edited

Loading