Add RPC call 'getblockstats' #157

gcomte · 2021-01-19T12:42:26Z

getblockstats is available in Bitcoin Core since version 0.17.0 .

It won't work for some heights with pruning.
It won't work without -txindex for utxo_size_inc, *fee or *feerate stats.

More info (here)[https://bitcoincore.org/en/doc/0.17.0/rpc/blockchain/getblockstats/]

I'm completely new to Rust and this is my first Rust-PR ever, so please have mercy.

stevenroose · 2021-01-25T00:02:47Z

It seems that in 0.18 this call needs -txindex=1. You can add this to fix that:

diff --git a/integration_test/run.sh b/integration_test/run.sh
index b40aa78..97925fd 100755
--- a/integration_test/run.sh
+++ b/integration_test/run.sh
@@ -29,6 +29,7 @@ if bitcoind -version | grep -q "v0\.2"; then
 fi
 
 bitcoind -regtest $BLOCKFILTERARG $FALLBACKFEEARG \
+    -txindex=1 \
     -datadir=${TESTDIR}/2 \
     -connect=127.0.0.1:12348 \
     -rpcport=12349 \

stevenroose · 2021-01-25T00:02:53Z

The rest looks good to me!

shesek · 2021-01-25T05:47:00Z

client/src/client.rs

@@ -439,6 +439,10 @@ pub trait RpcApi: Sized {
        self.call("getblockhash", &[height.into()])
    }

+    fn get_block_stats(&self, height: u64) -> Result<json::GetBlockStatsResult> {


This can also accept the block hash instead of the height, maybe worth supporting it?

There's also an option for selecting which fields you want statistics for. Though this would require changing all the GetBlockStatsResult fields to be an Option<T>, which isn't great.

All of them?

If we want to support getting a specific list of fields from getblockstats, then I'm afraid yes...

Perhaps it could be a separate struct? So we have get_block_stats(height) -> GetBlockStatsResult and get_block_stats_fields(height, fields) -> GetBlockStatsResultPartial. Maintaining this would be annoying though, but perhaps not so bad with some macros?

Okay, I proposed some code for this :-) Please check out this commit eb5cb8b @shesek

Looks good! Commented with two small nits

shesek · 2021-01-25T05:50:19Z

json/src/lib.rs

+    pub total_size: usize,
+    pub total_weight: usize,
+    #[serde(rename = "totalfee")]
+    pub total_fee: u64,


This should be a bitcoin::Amount (will also require with = "bitcoin::util::amount::serde::as_sat" in the derive).

Oh yeah, true, we're using those in this crate.

As far as I understand, bitcoin::Amount should then also be used for: avg_fee, fee_rate_percentiles, max_fee, median_fee and min_fee, right?

When I try too define the fields as Option<Amount>, the compiler throws the follwoing error at me: the trait SerdeAmount is not implemented for std::option::Option<Amount>

Any idea how I should deal with this?

Yes, and also avg_fee_rate, max_fee_rate, min_fee_rate, total_fee, subsidy and total_out.

Note that unlike most other RPCs in bitcoind, the feerates here are represented in sat/vB rather than BTC/KB, while amounts are in satoshis rather than BTC.

shesek · 2021-01-25T05:51:35Z

json/src/lib.rs

+#[derive(Clone, PartialEq, Debug, Deserialize, Serialize)]
+pub struct GetBlockStatsResult {
+    #[serde(rename = "avgfee")]
+    pub avg_fee: u32,


IIUC there's a preference by the folks here to use u64 over u32/usize. Maybe @stevenroose can explain more.

I suppose this can also be an Amount, right?

Oh right, this could. But I was referring to all the u32 fields, not just that one

Would appreciate some more info about 'using u64 over u32/usize for all fields'. @stevenroose ?

Just that we use 64-bit numbers for amounts. But in this crate, we are mostly using the dedicated Amount type. You can find some other usages of that type and the serde-tag you have to use with it.

shesek · 2021-01-27T20:48:40Z

client/src/client.rs

@@ -443,6 +443,14 @@ pub trait RpcApi: Sized {
        self.call("getblockstats", &[height.into()])
    }

+    fn get_block_stats_fields(&self, height: u64, fields: &Vec<bitcoincore_rpc_json::BlockStatsFields>) -> Result<json::GetBlockStatsResultPartial> {


Can use fields: &[bitcoincore_rpc_json::BlockStatsFields] so it also accepts slices. And bitcoincore_rpc_json is also aliased as just json.

gcomte · 2021-02-07T10:40:05Z

With commit 03b7fa9, I have now a version that works (with Amounts), but it has also added a ton of boilerplate code.

Still open is the question, about whether I should use u64 over u32 for the fields avg_tx_size, max_tx_sizemax_tx_size, median_time, median_tx_size, min_tx_size, time, and potentially i64 over i32 for the fields utxo_increase and utxo_size_inc.

0xB10C · 2021-02-12T17:28:32Z

I'd say

avg_tx_size, max_tx_size, median_tx_size and min_tx_size can be a u32 as single block will currently will always be smaller than 4_000_000 bytes (thus limiting the transaction size to something less)
time and median_time should be u64
utxo_increase can be i32 unless I'm missing something, if you really want to be safe then i64
utxo_size_inc maybe i64, also not sure here

gcomte · 2021-02-22T08:52:10Z

Thanks for your input @0xB10C, I'll wait for more comments from @stevenroose and @shesek about your suggestion.

stevenroose · 2021-03-13T22:49:03Z

I agree with @0xB10C 's suggestions :)

stevenroose · 2021-03-13T23:25:51Z

Could you also rebase to make CI happy?

gcomte · 2021-03-14T11:30:28Z

Rebased. Travis seems to have a problem still, concerning the -txindex enabled on Bitcoin Core versions 18.x .

I've update the data type for timestamps in commit 890c211.
Please note that this introduces inconsistencies about what data type is used for timestamps within this library: bbf0532

@0xB10C Here's my rationale for why i32 should do the job for both utxo_increase and utxo_size_inc:

Given a maximum block size of 4_000_000 bytes, there is no way how the UTXO set could increase or decrease beyond that number per-block ( = utxo_size_inc) in any way. Consequently, utxo_increase will also fit into a i32, as it is by definition always closer to 0 than utxo_size_inc.

i32 range: -2_147_483_648 - 2_147_483_647

sgeisler · 2021-03-14T12:42:00Z

integration_test/run.sh

@@ -9,6 +9,7 @@ mkdir -p ${TESTDIR}/1 ${TESTDIR}/2
 killall -9 bitcoind

 bitcoind -regtest \
+    -txindex=1 \


The txindex argument was added to the wrong bitcoind instance imo. There's another one started a few lines below (line 32, idk why), which is used for testing apparently as the RPC_URL env var is set to its port. That's probably why the 0.18.* tests fail, possibly because -blockfilterindex=1 is set for all other versions and works for stats too?

You are right, the one below has the flag -server=1 set, while the one above has it set to 0.
I moved the flag in commit f7d1f3bce957be592b6f90d1f7667e2a008a1022.

gcomte · 2021-03-14T13:07:53Z

Just to get a rough idea about the field utxo_increase I looked up the past 4500 blocks:

Lowest increase:
for i in $(seq 670000 674578); do bitcoin-cli getblockstats $i | jq -r ".utxo_size_inc"; done | sort -n | head -n 1
Result: -732_265

Highest increase:
for i in $(seq 670000 674578); do bitcoin-cli getblockstats $i | jq -r ".utxo_size_inc"; done | sort -n | tail -n 1
Result: 1_086_480

sgeisler · 2021-07-15T20:41:50Z

Is this PR still active? Sorry for the wait. We are not good at keeping track of PRs, so pinging reviewers generally helps.

gcomte · 2021-07-20T06:53:48Z

@sgeisler Yes it is still active. I just rebased it, so it should be ready to be merged.

Okay thank you for the information, will ping you guys every now and then :-)

shesek · 2021-07-20T23:06:45Z

All looks good to me, ACK 6af032a. Thanks for the contribution!

Edit: actually, one small last nit is that the get_rpc_keyword method could instead be an implementation for Display. But it's fine by me either way.

gcomte · 2021-07-21T09:00:46Z

Hey @shesek, good input, thank you!

I tried to address this with the commit e0583c4 and then further simplified it with the commit fdef9f2.
What do you think?

shesek · 2021-07-21T15:09:25Z

@gcomte Looks good! re-ACK fdef9f2

gcomte · 2021-07-22T11:12:45Z

@sgeisler @stevenroose Looks like this PR is ready to be reviewed/merged.

gcomte · 2021-08-31T11:52:25Z

How is the progress on this guys? Is there something else I could provide you with, to assist you?

gcomte · 2021-11-03T17:30:25Z

I rebased this again, so it is once again, ready to be merged.

gcomte · 2021-11-30T09:53:47Z

I rebased this again, so it is once again, ready to be merged.

RCasatta · 2022-02-03T17:33:45Z

ACK d83b8fe
tested with CI at https://github.com/RCasatta/rust-bitcoincore-rpc/actions/runs/1790852210

I am going to wait some days before merging, it would be great this rebase to master and squash some commits

gcomte · 2022-02-04T01:27:22Z

@RCasatta thanks for the review, rebased and squashed some commits.

RCasatta

utACK 60b0dd7

will merge soon if there is no more feedback

gcomte · 2022-02-28T22:36:56Z

Hey @RCasatta

Friendly reminder that 3 weeks have passed. Could you specify soon?

RCasatta · 2022-03-01T09:26:11Z

Sorry for the delay and thanks for the contribution

shesek reviewed Jan 25, 2021

View reviewed changes

gcomte force-pushed the master branch from bde6b64 to eb5cb8b Compare January 27, 2021 19:54

shesek reviewed Jan 27, 2021

View reviewed changes

gcomte force-pushed the master branch from 03b7fa9 to 32f6639 Compare March 14, 2021 09:07

sgeisler reviewed Mar 14, 2021

View reviewed changes

gcomte force-pushed the master branch from d317068 to b2b4ce6 Compare March 15, 2021 12:21

gcomte force-pushed the master branch from b2b4ce6 to 5742ad9 Compare March 27, 2021 12:42

gcomte force-pushed the master branch from 5742ad9 to 6af032a Compare July 20, 2021 06:51

gcomte requested review from stevenroose and sgeisler August 2, 2021 07:02

gcomte force-pushed the master branch from fdef9f2 to 9fc141e Compare November 3, 2021 17:28

gcomte force-pushed the master branch from 9fc141e to d83b8fe Compare November 30, 2021 09:52

gcomte force-pushed the master branch 3 times, most recently from a1d803b to 14a1d36 Compare February 4, 2022 00:55

Gabriel Comte added 6 commits February 4, 2022 02:24

Add rpc call getblockstats

64b7a5e

getblockstats requires txindex

1740440

Add per-field querying for getblockstats

c807b14

Change data types, fix absent values

f2d74ca

Fix code formatting

826e01a

Implement Display and serde_json::Value for BlockStatsFields

60b0dd7

gcomte force-pushed the master branch from 14a1d36 to 60b0dd7 Compare February 4, 2022 01:25

RCasatta approved these changes Feb 7, 2022

View reviewed changes

RCasatta merged commit 9bbca7f into rust-bitcoin:master Mar 1, 2022

Add RPC call 'getblockstats' #157

Add RPC call 'getblockstats' #157

Conversation

gcomte commented Jan 19, 2021

stevenroose commented Jan 25, 2021

stevenroose commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shesek Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

gcomte Jan 27, 2021 • edited Loading

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

gcomte commented Feb 7, 2021

0xB10C commented Feb 12, 2021

gcomte commented Feb 22, 2021

stevenroose commented Mar 13, 2021

stevenroose commented Mar 13, 2021

gcomte commented Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

gcomte Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

gcomte commented Mar 14, 2021

sgeisler commented Jul 15, 2021

gcomte commented Jul 20, 2021

shesek commented Jul 20, 2021 • edited Loading

gcomte commented Jul 21, 2021 • edited Loading

shesek commented Jul 21, 2021

gcomte commented Jul 22, 2021

gcomte commented Aug 31, 2021

gcomte commented Nov 3, 2021

gcomte commented Nov 30, 2021

RCasatta commented Feb 3, 2022

gcomte commented Feb 4, 2022

RCasatta left a comment

Choose a reason for hiding this comment

gcomte commented Feb 28, 2022

RCasatta commented Mar 1, 2022

shesek Jan 25, 2021 •

edited

Loading

gcomte Jan 27, 2021 •

edited

Loading

gcomte commented Mar 14, 2021 •

edited

Loading

gcomte Mar 15, 2021 •

edited

Loading

shesek commented Jul 20, 2021 •

edited

Loading

gcomte commented Jul 21, 2021 •

edited

Loading