Add new statistics to getblockstats RPC by marcinja · Pull Request #1 · bitcoinops/bitcoin

marcinja · 2018-07-16T15:35:11Z

Extends getblockstats RPC introduced in bitcoin#10757.

getblockstats now tracks spends of specific SegWit output types: P2SH-nested P2WPKH, P2SH-nested P2WSH, native P2WPKH, and native P2WSH. New CTxIn and CScript methods added to check that the scriptSig and scriptPubKey of a input and the ouput it spends match the format of the above output types as specified in BIP 141. (Note: there is probably a bug in this part of the PR!) Also tracks creation of new native SegWit outputs using the same new methods.

This PR also introduces a count of transactions that signal RBF. It also adds batching, consolidation, and dust-creation metrics.

A batching transaction is defined as having at least 3 outputs. Separately, this RPC now counts transactions that have fit into different ranges of their number of outputs. // Batch ranges = [(1), (2), (3-4), (5-9), (10-49), (50-99), (100+)] These ranges are predetermined but arbitrary .

A consolidating transaction is defined as having at least 3 inputs.

Dust creation is calculated by using the IsDust function on each output at different fee-rates. The outputs are then put into bins of different ranges (e.g. dust at 100 sat/vbyte or higher. These ranges are also predetermined but arbitrary .

jnewbery · 2018-07-16T18:14:00Z

I'll try to review this this week if I get a chance.

@jamesob - can you also review?

jnewbery

Looks generally good. I can't immediately see a problem with your segwit input/output counting.

I've left a bunch of nits. A few other pointers:

try adding a sanity check to your btcd branch that checks that swtcs <= txs_spending_native + txs_spending_nested
even better, you could add an assert to the getblockstats() code here that asserts that if tx->HasWitness() is true, then there is at least one native segwit input or nested segwit input (you might only want that in a debug build to avoid degrading performance).
consider adding test cases to /test/functional/rpc_getblockstats.py to cover the new functionality
see if there are any unit tests in /src/test that you could modify to test your segwit counting.
I'd split the new segwit counting code into its own commit, and then have 3 commits for the new functionality in getblockstats
commit messages should have a summary line (<80 chars) and then (optionally) a more detailed log, separated by a blank line.

jnewbery · 2018-07-23T14:48:27Z

nit: please remove unrelated newline addition

jnewbery · 2018-07-23T14:51:17Z

nit: Looks like this function is declared but not defined or used. Remove

jnewbery · 2018-07-23T14:54:16Z

Documentation doesn't match the field name (name refers to txs, doc refers to outputs).

Same for next field.

Sign in to view

jnewbery · 2018-07-23T14:56:07Z

nit: one newline is enough!

jnewbery · 2018-07-23T14:58:50Z

nit: place these at the top of the file as constexprs

Sign in to view

jnewbery · 2018-07-23T16:15:02Z

Have you profiled how performant this is? Seems like it could be slow to run IsDust() many times on each output in a block.

Yup. Profiling just showed that almost all time spent in getblockstats is spent in the GetTransaction call used to get the transaction with prevoutput. But I agree that running IsDust() so many times here isn't the best thing to do.

jnewbery · 2018-07-23T16:20:04Z

nit: Bitcoin Core code style is camel_case for variables. Prefer tx_outputs.

jnewbery · 2018-07-23T17:55:51Z

I don't think you need the this-> pointer derefences here. Just scriptWitness.IsNull() should be fine.

(same for this-> repetitions below)

jamesob · 2018-07-24T15:56:42Z

This looks like it may be problematic; should the ! be inside the parens? Otherwise it looks like null scriptWitnesses don't cause an immediate return.

Ah wait, nevermind - misbalanced parens when reading :)

jamesob · 2018-07-24T20:08:50Z

Not sure if noted elsewhere, but "signaling" only has one l.

jamesob · 2018-07-24T20:10:06Z

Might be worth returning a list of txids instead of a raw count, and maybe consider same for some of the other added keys. This'd bloat the size of the returned JSON by a bit, but gives much more information to the user.

jamesob

In general looks good, all I've got for you are nits.

I went to go find the segwit spend discrepancy on the existing dashboard (i.e. total_segwit_spend_percentage > sum(segwit_input_percentage)) but I wasn't able to find it. Are we still seeing that discrepancy? Is it unique to the per-block display?

jamesob · 2018-07-24T20:30:12Z

jamesob · 2018-07-24T20:45:55Z

Are these mutually exclusive conditions? Should this be absorbed into the block above?

No, the block above is for checking what kind of output each individual input spends and this block is for the entire transaction. And a transaction can spend all of the different kinds of outputs

marcinja · 2018-07-24T21:38:58Z

Thanks for the review. At this point, I'm pretty sure the discrepancy was either caused by a bug in the Go code that has been fixed, or a mistake in the Grafana setup. (I remade the graph at the top of that dashboard to convince myself)

Still, I'll try to add some simple tests to confirm it when I fix the remaining nits.

marcinja · 2018-07-25T21:42:59Z

Added a couple of tests that verify the native SegWit functions

Adds member functions to CTxIn and CScript for determining if a transaction spends any of the SegWit V0 output types.

Count creation of new native P2WSH and P2WPKH outputs, and the number of transactions creating each type of output. Count the number of native P2WPKH, native P2WSH, nested P2WSH, and nested P2WPKH outputs spent. Count the number of transactions that spend each type of SegWit output.

For each new output, checks whether it would be considered dust at several different fee-rates.

Adds batching transaction counter. Tracks the number of outputs created by each transaction. Adds consolidating transaction counter. Counts the number of transactions signalling opt-in RBF.

getblockstats now counts the number of transactions with at least 3 inputs and exactly one output as "many-to-one" (mto) consolidations. It also counts the total number of outputs consolidated by mto transactions per block.

6f53edb Acquire cs_main before ATMP call in block_assemble bench (James O'Beirne) Pull request description: Calling `bench_bitcoin` currently fails due to calling ATMP without acquiring cs_main first in the recently added block_assemble bench (bitcoin#13219). ``` $ cat <(uname -a) <(gcc --version) Linux james 4.4.0-119-generic bitcoin#143+jamesob SMP Mon Apr 16 21:47:24 EDT 2018 x86_64 x86_64 x86_64 GNU/Linux gcc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609 $ ./src/bench/bench_bitcoin WARNING: This is a debug build - may result in slower benchmarks. # Benchmark, evals, iterations, total, min, max, median Assertion failed: lock cs_main not held in validation.cpp:566; locks held: [1] 19323 abort (core dumped) ./src/bench/bench_bitcoin ``` ``` (gdb) bt #0 0x00007fbdc9cf5428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54 #1 0x00007fbdc9cf702a in __GI_abort () at abort.c:89 #2 0x0000555a19580dc5 in AssertLockHeldInternal (pszName=pszName@entry=0x555a19834549 "cs_main", pszFile=pszFile@entry=0x555a1988a001 "validation.cpp", nLine=nLine@entry=566, cs=cs@entry=0x555a19ba55c0 <cs_main>) at sync.cpp:157 #3 0x0000555a194b395f in AcceptToMemoryPoolWorker (chainparams=..., pool=..., state=..., ptx=std::shared_ptr (count 1, weak 0) 0x555a1bb819b0, pfMissingInputs=pfMissingInputs@entry=0x0, nAcceptTime=1532964079, plTxnReplaced=0x0, bypass_limits=false, nAbsurdFee=@0x7ffcbc1719d8: 0, coins_to_uncache=std::vector of length 0, capacity 0, test_accept=false) at validation.cpp:566 #4 0x0000555a194ba661 in AcceptToMemoryPoolWithTime (chainparams=..., pool=..., state=..., tx=std::shared_ptr (count 1, weak 0) 0x555a1bb819b0, pfMissingInputs=pfMissingInputs@entry=0x0, nAcceptTime=<optimized out>, plTxnReplaced=0x0, bypass_limits=false, nAbsurdFee=0, test_accept=false) at validation.cpp:998 #5 0x0000555a194ba7ce in AcceptToMemoryPool (pool=..., state=..., tx=std::shared_ptr (count 1, weak 0) 0x555a1bb819b0, pfMissingInputs=pfMissingInputs@entry=0x0, plTxnReplaced=plTxnReplaced@entry=0x0, bypass_limits=bypass_limits@entry=false, nAbsurdFee=0, test_accept=false) at validation.cpp:1014 #6 0x0000555a19363fbe in AssembleBlock (state=...) at bench/block_assemble.cpp:102 #7 0x0000555a193654d3 in std::_Function_handler<void (benchmark::State&), void (*)(benchmark::State&)>::_M_invoke(std::_Any_data const&, benchmark::State&) (__functor=..., __args#0=...) at /usr/include/c++/5/functional:1871 #8 0x0000555a193501d7 in std::function<void (benchmark::State&)>::operator()(benchmark::State&) const (this=this@entry=0x555a1ba2cda0, __args#0=...) at /usr/include/c++/5/functional:2267 #9 0x0000555a1934ec4c in benchmark::BenchRunner::RunAll (printer=..., num_evals=5, scaling=<optimized out>, filter=..., is_list_only=false) at bench/bench.cpp:121 #10 0x0000555a1934ade9 in main (argc=<optimized out>, argv=<optimized out>) at bench/bench_bitcoin.cpp:92 ``` Tree-SHA512: fdd7b28ff123ccea7a4f334d53f735d0c0f94aa9cc52520c2dd34dca45d78c691af64efcd32366fc472fedffbd79591d2be2bb3bfc4a5186e8712b6b452d64e3

Tracks total value of many_to_one outputs. Tracks total value of different kinds of SegWit outputs spent. Tracks total value of native SegWit outputs created.

marcinja · 2018-08-14T14:56:55Z

Rebased onto master with new feerate_percentiles and added stats for value of spent/created SegWit outputs and for total value of many-to-one consolidations.

jnewbery · 2019-06-24T17:54:20Z

This PR was for review only. We're using the expand-getblockstats branch and don't need to merge into master.

…ter-return checking 8d22ab0 ci: Enable address sanitizer (ASan) stack-use-after-return checking (practicalswift) Pull request description: Enable address sanitizer (ASan) stack-use-after-return checking (`detect_stack_use_after_return=1`). Example: ``` #include <iostream> #include <string> const std::string& get_string(int i) { return std::to_string(i); } int main() { std::cout << get_string(41) << "\n"; } ``` Without address sanitizer (ASan) stack-use-after-return checking: ``` $ ./stack-use-after-return $ ``` With address sanitizer (ASan) stack-use-after-return checking: ``` $ ASAN_OPTIONS="detect_stack_use_after_return=1" ./stack-use-after-return ================================================================= ==10400==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f7fa0400030 at pc 0x00000049d2cc bp 0x7ffcbd617070 sp 0x7ffcbd616820 READ of size 2 at 0x7f7abbecd030 thread T0 #0 0x439781 in fwrite #1 0x7f7ac0504cb3 in std::basic_ostream<char, std::char_traits<char> >& std::__ostream_insert<char, std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*, long) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0x113cb3) #2 0x4f9b5f in main stack-use-after-return.cpp:9:15 #3 0x7f7abf440b96 in __libc_start_main #4 0x41bbc9 in _start … $ ``` Top commit has no ACKs. Tree-SHA512: 6557a9ff184023380fd9aa433cdf413e01a928ea99dbc59ec138e5d69cb9e13592e8bb5951612f231ff17a37a895bec5c0940c8db5f328a5c840a5771bdeeba5

1828c6f refactor: Styling w/ clang-format, comment update (Hennadii Stepanov) 88a94f7 qt: Fix missing qRegisterMetaType for size_t (Hennadii Stepanov) Pull request description: On master (a7aec7a) this connection https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/rpcconsole.cpp#L587 fails due to `ClientModel::mempoolSizeChanged()` signal has unregistered parameter type `size_t`: https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/clientmodel.h#L102 More: ``` $ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- -debug=qt ... (lldb) bt * thread bitcoin#17, name = 'QThread', stop reason = signal SIGABRT * frame #0: 0x00007ffff35fce97 libc.so.6`__GI_raise(sig=2) at raise.c:51 frame #1: 0x00007ffff35fe801 libc.so.6`__GI_abort at abort.c:79 frame #2: 0x00007ffff5901352 libQt5Core.so.5`QMessageLogger::warning(char const*, ...) const + 354 frame #3: 0x00007ffff5b216fe libQt5Core.so.5`___lldb_unnamed_symbol2329$$libQt5Core.so.5 + 334 frame #4: 0x00007ffff5b2456d libQt5Core.so.5`QMetaObject::activate(QObject*, int, int, void**) + 1933 frame #5: 0x000055555566872e bitcoin-qt`ClientModel::mempoolSizeChanged(this=<unavailable>, _t1=<unavailable>, _t2=<unavailable>) at moc_clientmodel.cpp:260 ... ``` `debug.log`: ``` [] GUI: QObject::connect: Cannot queue arguments of type 'size_t' (Make sure 'size_t' is registered using qRegisterMetaType().) ``` This PR fixes it. Refs: - [Qt docs: qRegisterMetaType](https://doc.qt.io/qt-5/qmetatype.html#qRegisterMetaType) - bitcoin#16348 --- Side NOTE: Also I believe this line https://github.com/bitcoin/bitcoin/blob/a7aec7ad97949a82f870c033d8fd8b65d772eacb/src/qt/bitcoin.cpp#L63 is redundant since long `CAmount` is a `typedef`. ACKs for top commit: laanwj: Tested ACK 1828c6f Tree-SHA512: 2c7f9fe6a5ae70f2e1dd86b07f95d4b00c85c5706a9d722f063f80beb71880d012ec46556963fb1544c2af53d006936c2f7612eae60d9193f67db62ba3d86129

marcinja force-pushed the expand-getblockstats branch from 852fa02 to 8b6a316 Compare July 16, 2018 15:49

jnewbery reviewed Jul 23, 2018

View reviewed changes

marcinja force-pushed the expand-getblockstats branch 2 times, most recently from bcec257 to ce797bf Compare July 23, 2018 18:56

jamesob reviewed Jul 24, 2018

View reviewed changes

Comment thread src/rpc/blockchain.cpp Outdated

Copy link
Copy Markdown

jamesob Jul 24, 2018

Choose a reason for hiding this comment

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

Not sure if noted elsewhere, but "signaling" only has one l.

jamesob reviewed Jul 24, 2018

View reviewed changes

marcinja force-pushed the expand-getblockstats branch 3 times, most recently from 7a69f9d to 3866bdd Compare July 25, 2018 21:33

marcinja changed the base branch from master to 0.12 July 25, 2018 21:40

marcinja changed the base branch from 0.12 to master July 25, 2018 21:40

marcinja commented Jul 25, 2018

View reviewed changes

Marcin Jachymiak added 5 commits August 13, 2018 13:40

Add functions to check for spent SegWit outputs

d20b133

Adds member functions to CTxIn and CScript for determining if a transaction spends any of the SegWit V0 output types.

Add stats for creation of dust outputs

2cb9c67

For each new output, checks whether it would be considered dust at several different fee-rates.

Add more stats to getblockstats

ecb3ea0

Adds batching transaction counter. Tracks the number of outputs created by each transaction. Adds consolidating transaction counter. Counts the number of transactions signalling opt-in RBF.

Track many-to-one consolidations

7a2aa5f

getblockstats now counts the number of transactions with at least 3 inputs and exactly one output as "many-to-one" (mto) consolidations. It also counts the total number of outputs consolidated by mto transactions per block.

marcinja force-pushed the expand-getblockstats branch from 0f708b1 to 7a2aa5f Compare August 13, 2018 17:45

marcinja changed the base branch from master to 0.12 August 13, 2018 17:48

marcinja changed the base branch from 0.12 to master August 13, 2018 17:48

Add value tracking stats

8968f34

Tracks total value of many_to_one outputs. Tracks total value of different kinds of SegWit outputs spent. Tracks total value of native SegWit outputs created.

jnewbery closed this Jun 24, 2019

jnewbery deleted the expand-getblockstats branch August 27, 2019 19:18

Conversation

marcinja commented Jul 16, 2018

Uh oh!

jnewbery commented Jul 16, 2018

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcinja commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcinja commented Aug 14, 2018

Uh oh!

jnewbery commented Jun 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants