Add dev_getBlockStats RPC

[athei]

This adds a new RPC:

pub struct BlockStats {
    pub witness_len: u64,
    pub witness_compact_len: u64,
    pub block_len: u64,
    pub block_num_extrinsics: u64,
}

#[rpc(name = "dev_getBlockStats")]
fn block_stats(&self, hash: Hash) -> Result<Option<BlockStats>>;

It is meant as a way to gain insides about the bottle necks of a given chain. While some information can only be learned through this RPC (witness related information) some of them are redundant: You can query the whole block and then measure the size of the block and the number of extrinsics. However, I think it still makes sense to include them as this saves us from transmitting the whole block to the RPC client (possibly over the internet) in order to learn about these statistics.

I have written a CLI tool that queries this draft version of the RPC:

cargo install --git https://github.com/athei/povstats.git --locked

# This will just connect to a locally running node and calls the RPC on each new block
povstats

The output when running this against a substrate --dev node without submitting any extrinsics to it looks like this:

27: Some(BlockStats { witness_len: 8179, witness_compact_len: 6124, witness_compressed_len: 5685, block_len: 226, block_num_extrinsics: 1 })
28: Some(BlockStats { witness_len: 7960, witness_compact_len: 5869, witness_compressed_len: 5412, block_len: 129, block_num_extrinsics: 1 })
29: Some(BlockStats { witness_len: 7894, witness_compact_len: 5901, witness_compressed_len: 5473, block_len: 129, block_num_extrinsics: 1 })
30: Some(BlockStats { witness_len: 8564, witness_compact_len: 6379, witness_compressed_len: 5918, block_len: 226, block_num_extrinsics: 1 })
31: Some(BlockStats { witness_len: 8535, witness_compact_len: 6382, witness_compressed_len: 5913, block_len: 226, block_num_extrinsics: 1 })
32: Some(BlockStats { witness_len: 8698, witness_compact_len: 6513, witness_compressed_len: 6065, block_len: 226, block_num_extrinsics: 1 })
33: Some(BlockStats { witness_len: 7842, witness_compact_len: 5913, witness_compressed_len: 5471, block_len: 129, block_num_extrinsics: 1 })
34: Some(BlockStats { witness_len: 7886, witness_compact_len: 5925, witness_compressed_len: 5473, block_len: 129, block_num_extrinsics: 1 })
35: Some(BlockStats { witness_len: 8520, witness_compact_len: 6497, witness_compressed_len: 6064, block_len: 226, block_num_extrinsics: 1 })

[kianenigma]

It happens inside: sp_api::Core::execute_block which is called by the new RPC.

I am not sure with the abstraction level that you are using, and I don't see any code that specifies on top of which state you are executing the given block. What I can say for sure is that to execute any arbitrary block, you certainly need its parent state, which may not be available. This error that you said also rings a bell for me in try-runtime, and I recall the reason was not using the proper parent state.

[athei]

I thought I did correctly specify the parent state by calling the runtime API at the block's parent's hash:

runtime_api.execute_block(&BlockId::Hash(*parent_hash), block)

[kianenigma]

I thought I did correctly specify the parent state by calling the runtime API at the block's parent's hash:

runtime_api.execute_block(&BlockId::Hash(*parent_hash), block)

that sounds correct to me. Perhaps to double check: if you pass an old-block's hash to a non-archive node, do you get an error?

[bkchr]

In generally I'm against adding new rpc methods that are added for everybody by default. However, the functionality here could maybe added to the state_traceBlock rpc method. Or if you want to keep it separate, it should be put into its own crate.

[athei]

In generally I'm against adding new rpc methods that are added for everybody by default. However, the functionality here could maybe added to the state_traceBlock rpc method.

Isn't modifying existing RPCs that are included for anyone even worse?

Or if you want to keep it separate, it should be put into its own crate.

This sounds sensible.

[bkchr]

Isn't modifying existing RPCs that are included for anyone even worse?

I mean we would just add some data to the output that you can just ignore on the calling site.

[athei]

Isn't modifying existing RPCs that are included for anyone even worse?

I mean we would just add some data to the output that you can just ignore on the calling site.

This is basically modifying the RPC in a backwards compatible way. So is adding a new RPC. Why is adding a new one worse?

[athei]

@dvdplm We are kind of unsure what would be the best place to put this new RPC. Asking for your opinion because I don't know how this ties into the jsonrpsee transition.

I think we should not append it to the state_traceBlock RPC as this would conflate two totally different things. The new RPC would be flagged as unsafe in any case.

Options are:

• Leave it as is: Additional RPC in the chain category
• Create a new category within sc-rpc
• Create a new crate within substrate to contain the new RPC
• Create a new crate within cumulus to contain the new RPC

[dvdplm]

My two cents:

• We don't have a good process to decide when/how/who to add RPCs (same for deprecation).
• Over time I've seen people think that "unsafe" RPCs are a way to signal that an RPC is experimental/debug-only/unstable. This is not the case, or rather, it's a bit of a semantic acrobatics. Unsafe means that an RPC should not be exposed to untrusted input, either because it does dangerous things or because it puts a lot of strain on the node and thus exposing a DoS veector.
• @tomaka 's approach when re-designing the RPCs is to be very explicit about which RPCs are stable and which aren't, by including "unstable" in the name. I think that's one way to go about it (preferably letting the proc macro do the heavy lifting).
• Given that up until now, adding or changing an RPC has been pretty much "do whatever you want", I think it's unfair to block this PR and I don't think the functionality fits very well as an extension to traceBlock

In other words: no objections to merge this from me (but it'd be good to start making up our minds about how to add RPCs, the sooner the better).

[bkchr]

In other words: no objections to merge this from me (but it'd be good to start making up our minds about how to add RPCs, the sooner the better).

This is relative easy. Don't add RPCs automatically to every node building on Substrate. Adding RPC functionality to Substrate isn't bad and we can name them unstable or whatever. However, people building on Substrate should decide on their own what kind of RPCs they want to expose.

[athei]

In other words: no objections to merge this from me (but it'd be good to start making up our minds about how to add RPCs, the sooner the better).

This is relative easy. Don't add RPCs automatically to every node building on Substrate. Adding RPC functionality to Substrate isn't bad and we can name them unstable or whatever. However, people building on Substrate should decide on their own what kind of RPCs they want to expose.

So a new category within sc-rpc would be fine? Putting it into a new crate just adds more boilerplate for no benefit.

[bkchr]

Yeah, a new category in sc-rpc is okay :) I mean, I have no idea how the structure with jsonrpsee looks, that was the reason I proposed to speak with @dvdplm :)

[athei]

@dvdplm said to me: The less boilerplate the better for our transition to jsonrpsee. So even adding a new category has a lot of boilerplate and makes the transition more work. That said, we need to balance this against how bad it is for RPCs to "just show up" when you bump substrate.

[dvdplm]

I mean, I have no idea how the structure with jsonrpsee looks, that was the reason I proposed to speak with @dvdplm :)

The amount of work for devs is quite similar, maybe slightly less but not by much.

[athei]

Okay so I will add a new category then. I still have no idea how to call this category. I was thinking about calling it dev to clearly indicate that it will contain some seriously wonky RPCs that are not really stable. We could then add new stuff there without much concern because people shouldn't add this category to their production node anyways.

What do you think?

[athei]

Ready for review now:

• Moved RPC to a new category.
• Added docs.
• Removed the witness_compressed_len. It is too assumptions. Client can compress itself if wanted.

[bkchr]

Most of the stuff are just nitpicks, otherwise it looks good.

[dvdplm]

Code LGTM. Left some nitpicks. :)

[athei]

bot merge