Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add methods param for RPC state_traceBlock #9642

Merged
6 commits merged into from
Aug 30, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion client/rpc-api/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ pub trait StateApi<Hash> {
///
/// ### Params
///
/// - `block_hash` (param index 0): Hash of the block to trace.
/// - `block` (param index 0): Hash of the block to trace.
/// - `targets` (param index 1): String of comma separated (no spaces) targets. Specified
/// targets match with trace targets by prefix (i.e if a target is in the beginning
/// of a trace target it is considered a match). If an empty string is specified no
Expand Down Expand Up @@ -263,6 +263,11 @@ pub trait StateApi<Hash> {
/// [2]: https://www.shawntabrizi.com/substrate/transparent-keys-in-substrate/
/// [3]: https://www.shawntabrizi.com/substrate/querying-substrate-storage-via-rpc/
///
/// - `methods` (param index 3): String of comma separated (no spaces) tracing event method.
/// If an empty string is specified no events will be filtered out. If anything other than
/// an empty string is specified, events will be filtered by method (so non-method events will
/// **not** show up).
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the caller specifies both storage_keys and methods? Is there a priority? Or Invalid Params error (this would be my preference)? Either way it'd be good to document the behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: what is the expected format of these methods? Is it the Rust name or some other convention (JS?)? A short example here would be valuable to newcomers I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if the caller specifies both storage_keys and methods? Is there a priority?

I prefer the priority of filter conditions to be determined according to the order of declaration of rpc parameters. (Currently, storage_keys is filtered first, then methods)

what is the expected format of these methods?

These methods are just ordinary string format, you could find these methods by rg "method = " primitives/state-machine/src/ext.rs

A short example here would be valuable to newcomers I think.

Yes, I will add a simple example into doc

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the priority of filter conditions to be determined according to the order of declaration of rpc parameters. (Currently, storage_keys is filtered first, then methods)

Fair. Can you document that behaviour please?

///
/// ### Maximum payload size
///
/// The maximum payload size allowed is 15mb. Payloads over this size will return a
Expand All @@ -277,5 +282,6 @@ pub trait StateApi<Hash> {
block: Hash,
targets: Option<String>,
storage_keys: Option<String>,
methods: Option<String>,
) -> FutureResult<sp_rpc::tracing::TraceBlockResponse>;
}
4 changes: 3 additions & 1 deletion client/rpc/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ where
block: Block::Hash,
targets: Option<String>,
storage_keys: Option<String>,
methods: Option<String>,
) -> FutureResult<sp_rpc::tracing::TraceBlockResponse>;
}

Expand Down Expand Up @@ -413,12 +414,13 @@ where
block: Block::Hash,
targets: Option<String>,
storage_keys: Option<String>,
methods: Option<String>,
) -> FutureResult<sp_rpc::tracing::TraceBlockResponse> {
if let Err(err) = self.deny_unsafe.check_if_safe() {
return async move { Err(err.into()) }.boxed()
}

self.backend.trace_block(block, targets, storage_keys)
self.backend.trace_block(block, targets, storage_keys, methods)
}
}

Expand Down
2 changes: 2 additions & 0 deletions client/rpc/src/state/state_full.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,12 +581,14 @@ where
block: Block::Hash,
targets: Option<String>,
storage_keys: Option<String>,
methods: Option<String>,
) -> FutureResult<sp_rpc::tracing::TraceBlockResponse> {
let block_executor = sc_tracing::block::BlockExecutor::new(
self.client.clone(),
block,
targets,
storage_keys,
methods,
self.rpc_max_payload,
);
let r = block_executor
Expand Down
1 change: 1 addition & 0 deletions client/rpc/src/state/state_light.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ where
_block: Block::Hash,
_targets: Option<String>,
_storage_keys: Option<String>,
_methods: Option<String>,
) -> FutureResult<sp_rpc::tracing::TraceBlockResponse> {
async move { Err(client_err(ClientError::NotAvailableOnLightClient)) }.boxed()
}
Expand Down
19 changes: 14 additions & 5 deletions client/tracing/src/block/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ pub struct BlockExecutor<Block: BlockT, Client> {
block: Block::Hash,
targets: Option<String>,
storage_keys: Option<String>,
methods: Option<String>,
rpc_max_payload: usize,
}

Expand All @@ -201,12 +202,13 @@ where
block: Block::Hash,
targets: Option<String>,
storage_keys: Option<String>,
methods: Option<String>,
rpc_max_payload: Option<usize>,
) -> Self {
let rpc_max_payload = rpc_max_payload
.map(|mb| mb.saturating_mul(MEGABYTE))
.unwrap_or(RPC_MAX_PAYLOAD_DEFAULT);
Self { client, block, targets, storage_keys, rpc_max_payload }
Self { client, block, targets, storage_keys, methods, rpc_max_payload }
}

/// Execute block, record all spans and events belonging to `Self::targets`
Expand Down Expand Up @@ -274,7 +276,13 @@ where
.filter(|e| {
self.storage_keys
.as_ref()
.map(|keys| event_key_filter(e, keys))
.map(|keys| event_values_filter(e, "key", keys))
.unwrap_or(false)
})
.filter(|e| {
self.methods
.as_ref()
.map(|methods| event_values_filter(e, "method", methods))
.unwrap_or(false)
})
.map(|s| s.into())
Expand All @@ -292,6 +300,7 @@ where
parent_hash: block_id_as_string(parent_id),
tracing_targets: targets.to_string(),
storage_keys: self.storage_keys.clone().unwrap_or_default(),
methods: self.methods.clone().unwrap_or_default(),
spans,
events,
})
Expand All @@ -301,12 +310,12 @@ where
}
}

fn event_key_filter(event: &TraceEvent, storage_keys: &str) -> bool {
fn event_values_filter(event: &TraceEvent, key: &str, values: &str) -> bool {
koushiro marked this conversation as resolved.
Show resolved Hide resolved
event
.values
.string_values
.get("key")
.and_then(|key| Some(check_target(storage_keys, key, &event.level)))
.get(key)
.and_then(|value| Some(check_target(values, value, &event.level)))
.unwrap_or(false)
}

Expand Down
3 changes: 3 additions & 0 deletions primitives/rpc/src/tracing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ pub struct BlockTrace {
/// Storage key targets used to filter out events that do not have one of the storage keys.
/// Empty string means do not filter out any events.
pub storage_keys: String,
/// Method targets used to filter out events that do not have one of the event method.
/// Empty string means do not filter out any events.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this empty string business is going to come back and bite us one day. Ideally we'd have an Option<String> here, but I understand why you went with the same we had before.

pub methods: String,
/// Vec of tracing spans
pub spans: Vec<Span>,
/// Vec of tracing events
Expand Down