Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execution tracing: GraphQL query to get storage inputs for past blocks #2491

Merged
merged 65 commits into from
Feb 18, 2025

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Dec 11, 2024

Description

To support local debugging and execution tracing, we need to add an API that returns the state that VM needs to execute transactions. This is done by recording all database accesses during tx validation, called storage read replay (bikeshedding in progress). This data is then exposed as-is through GraphQL. The API is non-trivial to consume, but a seperate client is proviced, see https://github.com/FuelLabs/execution-trace

The implementation exposes database table names and representations directly. Maintaining backwards compatibility with this could turn out to be quite hard.

Requires --historical-execution flag to enable, as this is otherwise quite expensive.

Follow-ups:

Example query:

mutation {
  storageReadReplay(height:"4") { column, key, value }
}

Open questions:

  • What should the query cost be?

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog: No breaking changes!
  • New behavior is reflected in tests
  • The specification matches the implemented behavior: This is outside spec scope.

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

See the VM PR FuelLabs/fuel-vm#881.

@Dentosal Dentosal added enhancement New feature or request fuel-block-executor wasm WASM-based block execution labels Dec 11, 2024
@Dentosal Dentosal self-assigned this Dec 11, 2024
@Dentosal Dentosal force-pushed the dento/execution-trace branch from 8d80d7b to 3ba2bc7 Compare December 19, 2024 15:18
@Dentosal Dentosal changed the title Execution tracing for past blocks Execution tracing: add GraphQL endpoint to get storage inputs for past blocks Jan 15, 2025
@Dentosal Dentosal changed the title Execution tracing: add GraphQL endpoint to get storage inputs for past blocks Execution tracing: GraphQL query to get storage inputs for past blocks Jan 15, 2025
Dentosal added a commit to FuelLabs/fuels-rs that referenced this pull request Jan 17, 2025
- Related to #1432

# Release notes

In this release, we:

- Changed `ABIDecoder` methods to take `std::io::Read` instead of
`&[u8]`, allowing it to be used in a streaming manner.

# Summary

`ABIDecoder` methods take `bytes: impl std::io::Read` instead of `bytes:
&[u8]`. This allows decoding abi types without having to know the size
in advance. This is particularly useful when reading them directly from
VM memory, which will be used by the indexer after
FuelLabs/fuel-core#2491 is done.

# Breaking Changes

`ABIDecoder` methods take `bytes: impl std::io::Read` instead of `bytes:
&[u8]`. Callers using arrays or `Vec` must change the argument from
`&value` to `value.as_slice()`.

# Checklist

- [x] All **changes** are **covered** by **tests** (or not applicable)
- [x] All **changes** are **documented** (or not applicable)
- [x] I **reviewed** the **entire PR** myself (preferably, on GH UI)
- [x] I **described** all **Breaking Changes** (or there's none)

---------

Co-authored-by: hal3e <git@hal3e.io>
Co-authored-by: Ahmed Sagdati <37515857+segfault-magnet@users.noreply.github.com>
Co-authored-by: segfault-magnet <ahmed.sagdati.ets@gmail.com>
crates/client/assets/schema.sdl Outdated Show resolved Hide resolved
@@ -762,6 +762,10 @@ type Mutation {
"""
dryRun(txs: [HexString!]!, utxoValidation: Boolean, gasPrice: U64, blockHeight: U32): [DryRunTransactionExecutionStatus!]!
"""
Get execution trace for an already-executed transaction.
"""
storageReadReplay(height: U32!): [StorageReadReplayEvent!]!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know, maybe @Voxelot knows. But I think it should be just Query since it doesn't modify the internal state and is used on the state at height, so it is deterministic.

crates/services/producer/src/block_producer.rs Outdated Show resolved Hide resolved
Comment on lines 43 to 51
fn get_full_block(&self, height: &BlockHeight) -> StorageResult<Block> {
let block = self.get_block(height)?;
let transactions = block
.transactions()
.iter()
.map(|id| self.get_transaction(id).map(|tx| tx.into_owned()))
.collect::<Result<Vec<_>, _>>()?;
Ok(block.into_owned().uncompress(transactions))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the implementation of the BlockProducerDatabase port in fuel-core.

image

crates/types/src/services/executor.rs Outdated Show resolved Hide resolved
tests/tests/storage_read_replay.rs Outdated Show resolved Hide resolved
@Dentosal Dentosal requested a review from xgreenx February 14, 2025 01:24
xgreenx
xgreenx previously approved these changes Feb 14, 2025
Comment on lines +517 to +529
let output = instance.run(module)?;

match output {
ReturnType::ExecutionV0(result) => {
let _ = convert_from_v0_execution_result(result)?;
}
ReturnType::ExecutionV1(result) => {
let _ = convert_from_v1_execution_result(result)?;
}
ReturnType::Validation(result) => {
let _ = result?;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for simplicity and performance we can avoid decoding the output type

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, maybe for performance we want to avoid deserialization of the output in the instance.run, but it is up to you. Because it required a new method like run_without_result

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we still need to ensure the execution was successful, and my understanding is that this is the way to do that. I expect ther performance impact of decoding the type to be neglible anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually you re-execute blocks that already were executed and validated in the past, so the result should be correct, unless storage has different state.

S: KeyValueInspect,
{
pub storage: S,
pub record: Arc<Mutex<Vec<StorageReadReplayEvent>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of Arc<Mutex<_>> I think you could use RefCell. I will make StorageAccessRecorder non Send, but I don't think that we have cases where you need it to be Send. But maybe I'm not right

Copy link
Member Author

Choose a reason for hiding this comment

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

We require it to be Send here:

S: KeyValueInspect<Column = Column> + Send + Sync + 'static,

Copy link
Contributor

@acerone85 acerone85 left a comment

Choose a reason for hiding this comment

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

Thanks for explanations and reply to all comments.
Happy to approve

@Dentosal Dentosal requested a review from xgreenx February 18, 2025 12:13
@Dentosal Dentosal enabled auto-merge (squash) February 18, 2025 12:13
Comment on lines +517 to +529
let output = instance.run(module)?;

match output {
ReturnType::ExecutionV0(result) => {
let _ = convert_from_v0_execution_result(result)?;
}
ReturnType::ExecutionV1(result) => {
let _ = convert_from_v1_execution_result(result)?;
}
ReturnType::Validation(result) => {
let _ = result?;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually you re-execute blocks that already were executed and validated in the past, so the result should be correct, unless storage has different state.

@Dentosal Dentosal merged commit b65f7b6 into master Feb 18, 2025
33 checks passed
@Dentosal Dentosal deleted the dento/execution-trace branch February 18, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fuel-block-executor wasm WASM-based block execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants