Add fee rate distribution in -getinfo

ghost commented at 1:59 am on September 5, 2021: none

This PR is on top of #21422

Context: #17314 (comment)

What?

Add fee rate distribution information in -getinfo

Why?

-getinfo has almost everything except a section for mempool and some information which can be helpful while doing transactions or analysis.
All PRs trying to add related things in GUI are dead: https://github.com/bitcoin-core/gui/pull/320#issuecomment-912821023. We can try adding basic things in -getinfo and see if it helps users. It can be be optional and only work if some argument is used.
Fee rate distribution in mempool helps more than any fee estimation because there is nothing to predict. You just need to look at fee rates used by others and bid for block space accordingly.

How?

Approach similar to progress bar which was added in #22547

Use results from bitcoin-cli getmempoolinfo "[1,10,100,200,1000]"
Use dynamic increment for bar in GetFeerateBar() so that it works for different mempool size and all chains.
Magenta color for mempool section

Testing

Run bitcoind (testnet, signet and mainnet)
Run command: watch -c -t -n 1 bitcoin-cli -getinfo -color=always
Check if everything look okay. Press Ctrl+C. Check different edge cases.

Introduce fee histogram in getmempoolinfo RPC command

Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch>
Co-authored-by: Jon Atack <jon@atack.com>

dc67e67fd1

test: Add mempool fee histogram test coverage

Original commit: https://github.com/bitcoin/bitcoin/commit/0b6ba66238c377116bc6c21e19cffbf1b6dfc788

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
Co-authored-by: Jon Atack <jon@atack.com>

103e880ae7

DrahtBot added the label RPC/REST/ZMQ on Sep 5, 2021

DrahtBot added the label Utils/log/libs on Sep 5, 2021

in src/bitcoin-cli.cpp:1075 in 9e266a955a outdated

1071+    {    
1072+    case 0: fee_rate = "1-9"; 
1073+        break;
1074+    case 1: fee_rate = "10-19";
1075+        break;
1076+    case 2: fee_rate = "20-29";

jonatack commented at 12:45 pm on September 5, 2021:

These fee rate ranges seem arbitrary. Relevant real fee rates may evolve to be much higher or lower.

unknown commented at 3:38 pm on September 5, 2021:

I used them to test. Was planning to add more once results are as expected. We can do one of these to improve:

Add more groups: https://i.imgur.com/ffyusBA.png
Define a MIN and MAX, MIN will be 1 and MAX can be highest fee rate used by transactions in mempool. And then divide this in two few groups.

unknown commented at 3:38 am on September 7, 2021:

Used different groups in last commit: 1-9, 10-99, 100-199 and Above 200

jonatack commented at 12:49 pm on September 5, 2021: member

Various initial thoughts:

is #21422 enough; what is the added value of an additional CLI?
if a CLI command would add additional value relative to an RPC, -getinfo is already a bit slow due to calling several RPCs; maybe a separate CLI -feerateinfo or -mempoolinfo command

ghost commented at 3:38 pm on September 5, 2021: none

@jonatack Thanks for review

is Add feerate histogram to getmempoolinfo #21422 enough; what is the added value of an additional CLI?

I consider -getinfo as a dashboard for users who do not like GUI but still prefer to see a summary open in a terminal while using Bitcoin Core for several things. If used with watch it can also update regularly.

if a CLI command would add additional value relative to an RPC, -getinfo is already a bit slow due to calling several RPCs; maybe a separate CLI -feerateinfo or -mempoolinfo command

Agree this will add one more RPC in it and things can be slow sometimes. So we can do one of these:

Add some option which is false by default and -getinfo will not return fee rate distribution until user mentions bitcoin-cli -getinfo mempool=true
Create a new CLI option as you suggested only for this information (-feerateinfo or -mempoolinfo)
Add it in RPC getmempoolinfo with an argument feerate_dist which is false by default

in src/bitcoin-cli.cpp:319 in 9e266a955a outdated

315@@ -315,6 +316,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
316         result.push_back(JSONRPCRequestObj("getblockchaininfo", NullUniValue, ID_BLOCKCHAININFO));
317         result.push_back(JSONRPCRequestObj("getwalletinfo", NullUniValue, ID_WALLETINFO));
318         result.push_back(JSONRPCRequestObj("getbalances", NullUniValue, ID_BALANCES));
319+        result.push_back(JSONRPCRequestObj("getmempoolinfo", "[1,10,20,30]", ID_MEMPOOLINFO));

klementtan commented at 7:56 pm on September 6, 2021:

Updating the code to this solved the error for me.

0        UniValue params{RPCConvertValues("getmempoolinfo", {"[1,10,20,30]"})};
1        result.push_back(JSONRPCRequestObj("getmempoolinfo", params, ID_MEMPOOLINFO));

in src/bitcoin-cli.cpp:1057 in 9e266a955a outdated

1052@@ -1027,7 +1053,31 @@ static void ParseGetInfoResult(UniValue& result)
1053         }
1054         result_string += "\n";
1055     }
1056-
1057+    
1058+    result_string += strprintf("Feerate Distribution: \n");

klementtan commented at 7:56 pm on September 6, 2021:

Maybe add color to standardize with the other headers?

unknown commented at 3:37 am on September 7, 2021:

Changed ‘Feerate Distribution’ to ‘Mempool’ and added RED color

klementtan commented at 7:59 pm on September 6, 2021: contributor

Concept ACK

ghost commented at 3:35 am on September 7, 2021: none

Result for bitcoin-cli -getinfo after last commit:

Changes:

Change feerate groups
Increment by 1000
Use L_BAR for vbytes less than 250k
Add RED color and use it for MEMPOOL
Fix fee_size in for loop
Only show fee distribution for a group if size>0 @klementtan Thanks for review and help in fixing JSON error

lsilva01 commented at 6:43 pm on September 10, 2021: contributor

Concept ACK. It’s a really nice feature. It would be great if the fee rate ranges were automatically calculated based on the minimum and maximum fees.

ghost commented at 11:24 pm on September 10, 2021: none

@lsilva01 Thanks for review. Made few changes in last commit. Let me know if this looks better. Will try few more things in next few days.

Result for bitcoin-cli -getinfo after last commit:

Changes:

Add one more group for fee rates above 200
Change colors: Mempool: RED -> MAGENTA Wallet: MAGENTA -> CYAN (Wallet and Balance are related so can use the same color. Red color looks weird for anything except errors.)
Do not show Mempool section if mempool size is zero

TO DO:

Dynamic INCREMENT. Use INCREMENT based on size.
~~mempool=true argument~~ Not required
Other things based on reviews

unknown marked this as ready for review on Sep 11, 2021

ghost commented at 7:53 pm on September 11, 2021: none

I have made few changes and squashed commits. Also updated things in PR description. Please test and let me know if there are any issues and scope for improvement. @klementtan I tried adding you as co-author because PR is inspired by your work to add progress bar for sync, used lot of code from it and you helped in fixing JSON error initially. Maybe didn’t add it correctly. Will have to commit again anyway, so will fix it. Please share your email address that I should use.

Cc: @jonatack @lsilva01

klementtan commented at 5:22 am on September 12, 2021: contributor

@prayank23 My email is klementtan@gmail.com. Thanks!

Will review the PR soon 👍

in src/bitcoin-cli.cpp:370 in 58fd0a4d44 outdated

361@@ -359,6 +362,13 @@ class GetinfoRequestHandler: public BaseRequestHandler
362             result.pushKV("balance", batch[ID_BALANCES]["result"]["mine"]["trusted"]);
363         }
364         result.pushKV("relayfee", batch[ID_NETWORKINFO]["result"]["relayfee"]);
365+
366+        result.pushKV("feerate_dist1", batch[ID_MEMPOOLINFO]["result"]["fee_histogram"]["fee_rate_groups"]["1"]["size"]);
367+        result.pushKV("feerate_dist2", batch[ID_MEMPOOLINFO]["result"]["fee_histogram"]["fee_rate_groups"]["10"]["size"]);
368+        result.pushKV("feerate_dist3", batch[ID_MEMPOOLINFO]["result"]["fee_histogram"]["fee_rate_groups"]["100"]["size"]);
369+        result.pushKV("feerate_dist4", batch[ID_MEMPOOLINFO]["result"]["fee_histogram"]["fee_rate_groups"]["200"]["size"]);
370+        result.pushKV("mempool_bytes", batch[ID_MEMPOOLINFO]["result"]["bytes"]);

klementtan commented at 3:38 am on September 13, 2021:

Just checking, did you intentionally leave out batch[ID_MEMPOOLINFO]["result"]["fee_histogram"]["fee_rate_groups"]["1000"]["size"]?

unknown commented at 5:42 am on September 13, 2021:

Yes. Because fee rate groups are:

1 (from 1 to 9) 10 (from 10 to 99) 100 (from 100 to 199) 200 (from 200 to 1000)

You can see them in the results for bitcoin-cli getmempoolinfo "[1,10,100,200,1000]"

klementtan commented at 5:58 am on September 13, 2021:

Ah got it! Tx with fee >= 1000 sat/vB will not be displayed.

Would it be better to update s/Above 200 |/200-1000 |/? IIUC batch[ID_MEMPOOLINFO]["result"]["fee_histogram"]["fee_rate_groups"]["200"]["size"] only considers fees from 200-1000

0./src/bitcoin-cli -signet getmempoolinfo "[1,10,100,200,1000]"
1...
2      "200": {
3        "size": 0,
4        "count": 0,
5        "fees": 0,
6        "from": 200,
7        "to": 999
8      },
9...

unknown commented at 6:03 am on September 13, 2021:

Possible. Or we can change 1000 to a higher number. Will see what other reviewers think and decide accordingly.

unknown commented at 7:18 pm on November 3, 2021:

Will keep this unchanged as nobody reviewed and had issues with it. Fee rates are normally less than 1000 sat/vB and we can ignore if someone uses higher in few transactions because this is just the fee rate distribution summary to help users and complete information.

in src/bitcoin-cli.cpp:1083 in 58fd0a4d44 outdated

1072@@ -1028,6 +1073,64 @@ static void ParseGetInfoResult(UniValue& result)
1073         result_string += "\n";
1074     }
1075 
1076+    const double mempool_bytes{result["mempool_bytes"].get_real()};

klementtan commented at 3:48 am on September 13, 2021:

nit: Possible to refactor this into a separate method?

unknown commented at 5:43 am on September 13, 2021:

A function for whole thing? Yes.

klementtan commented at 6:02 am on September 13, 2021:

Yup L1076-L1130 looks like it could be cleanly refactored into another function 👍

unknown commented at 7:16 pm on November 3, 2021:

Sorry Its been more than a month. I was waiting for some progress in PR 21422 before making any changes here as it depends on it. Anyways looked at the code today and this looks alright because everything is inside ParseGetInfoResult()

in src/bitcoin-cli.cpp:949 in 58fd0a4d44 outdated

937@@ -928,6 +938,39 @@ static void GetProgressBar(double progress, std::string& progress_bar)
938     }
939 }
940 
941+
942+static void GetFeerateBar(double size, std::string& size_bar, double inc_size, double max_size)

klementtan commented at 3:51 am on September 13, 2021:

Could add Doxygen documentation for this new function.

unknown commented at 5:44 am on September 13, 2021:

Sure

unknown commented at 7:14 pm on November 3, 2021:

Done

kiminuo commented at 8:52 pm on November 9, 2021:

I would consider naming the function GetFeeRateBar as I can see that we have CFeeRate class. But I may be wrong on this.

klementtan commented at 4:04 am on September 13, 2021: contributor

tested ACK 58fd0a4d4431f084f7f8260ed453be0c6d984517

DrahtBot commented at 1:57 pm on November 2, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)
#21422 (Add feerate histogram to getmempoolinfo by kiminuo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

add fee_rate_distribution in getinfo

+ 4 fee rate groups: 1-9,10-99,100-199,Above 200
+ Use dynamic increment based on size
+ MAGENTA color for mempool section, CYAN for wallet
+ Do not show mempool section and fee rate group if size is zero
+ Add doxygen documentation for `GetFeerateBar`

Co-authored-by: Klement Tan <klementtan@gmail.com>

797eac624a

in src/bitcoin-cli.cpp:1085 in 797eac624a outdated

1079@@ -1028,6 +1080,64 @@ static void ParseGetInfoResult(UniValue& result)
1080         result_string += "\n";
1081     }
1082 
1083+    const double mempool_bytes{result["mempool_bytes"].get_real()};
1084+
1085+    if (mempool_bytes >0){

kiminuo commented at 8:50 pm on November 9, 2021:

0    if (mempool_bytes > 0) {

in src/bitcoin-cli.cpp:1084 in 797eac624a outdated

1079@@ -1028,6 +1080,64 @@ static void ParseGetInfoResult(UniValue& result)
1080         result_string += "\n";
1081     }
1082 
1083+    const double mempool_bytes{result["mempool_bytes"].get_real()};
1084+

kiminuo commented at 8:50 pm on November 9, 2021:

I can see mixing of tabs and spaces in the source code:

(This is actually interesting because if you use an editor that supports .editorconfig, it should be quite hard to get to this situation)

unknown commented at 0:44 am on November 11, 2021:

I think I used different editor while working on this and my VS Code is behaving weird since few days. Will try to fix this.

in src/bitcoin-cli.cpp:1098 in 797eac624a outdated

1093+    	double size1 = result["feerate_dist1"].get_real();
1094+    	double size2 = result["feerate_dist2"].get_real();
1095+    	double size3 = result["feerate_dist3"].get_real();
1096+    	double size4 = result["feerate_dist4"].get_real();
1097+
1098+    	//Removing zero from numbers used in std::min to avoid division by zero in GetFeerateBar()

kiminuo commented at 8:56 pm on November 9, 2021:

0    	// Removing zero from numbers used in std::min to avoid division by zero in GetFeeRateBar()

in src/bitcoin-cli.cpp:1131 in 797eac624a outdated

1126+    		}
1127+
1128+    		GetFeerateBar(fee_size, size_bar, inc_size, max_size);
1129+    		size_bar += " ";
1130+
1131+    		if (fee_size > 0){

kiminuo commented at 8:57 pm on November 9, 2021:

0    		if (fee_size > 0) {

in src/bitcoin-cli.cpp:944 in 797eac624a outdated

937@@ -928,6 +938,46 @@ static void GetProgressBar(double progress, std::string& progress_bar)
938     }
939 }
940 
941+/**
942+ * GetFeerateBar constructs fee rate distribution bars using dynamic increment based on size.
943+ *
944+ * @param[in]  size      Size of fee rate group in bytes.

kiminuo commented at 9:02 pm on November 9, 2021:

Maybe the arguments can be re-ordered so that in params are first and the out param is last.

unknown commented at 0:53 am on November 11, 2021:

Done

in src/bitcoin-cli.cpp:956 in 797eac624a outdated

951+    if (size == 0) return;
952+
953+    static double INCREMENT{inc_size};
954+    static const std::string BAR{"\u2591"};
955+
956+    if (max_size < 10000){

kiminuo commented at 9:06 pm on November 9, 2021:

Given that there so many very similar numbers differing in the number of zero digits, one may consider using 10'000 syntax to represent 10000 (https://en.cppreference.com/w/cpp/language/integer_literal). (I’m not sure if it is allowed in this repo though)

unknown commented at 0:58 am on November 11, 2021:

Not sure about this as I have not seen such format for any constants in this repository.

kiminuo commented at 7:21 am on November 11, 2021:

IDK, maybe @jonatack would know. 🙏

in src/bitcoin-cli.cpp:972 in 797eac624a outdated

967+        }
968+    } else if (max_size > 1000000 && max_size < 10000000){
969+    	for (int i = 0; i < size / (INCREMENT*1000); ++i) {
970+            size_bar += BAR;
971+        }
972+    } else if (max_size > 10000000 && max_size < 100000000){

kiminuo commented at 9:07 pm on November 9, 2021:

0    } else if (max_size > 10000000 && max_size < 100000000) {

in src/bitcoin-cli.cpp:960 in 797eac624a outdated

955+
956+    if (max_size < 10000){
957+        for (int i = 0; i < size / INCREMENT; ++i) {
958+            size_bar += BAR;
959+        }
960+    } else if (max_size > 10000 && max_size < 100000){

kiminuo commented at 9:10 pm on November 9, 2021:

What will happen for value 100000?

unknown commented at 1:03 am on November 11, 2021:

Good catch. Will have to make some changes.

in src/bitcoin-cli.cpp:967 in 797eac624a outdated

962+            size_bar += BAR;
963+        }
964+    } else if (max_size > 100000 && max_size < 1000000){
965+    	for (int i = 0; i < size / (INCREMENT*100); ++i) {
966+            size_bar += BAR;
967+        }

kiminuo commented at 9:11 pm on November 9, 2021:

All the for-cycles are the same basically, so maybe you can have one for-cycle after this set of conditions. That may actually be easier to review.

unknown commented at 1:07 am on November 11, 2021:

Right this can be done. Will do later today.

in src/bitcoin-cli.cpp:1100 in 797eac624a outdated

1095+    	double size3 = result["feerate_dist3"].get_real();
1096+    	double size4 = result["feerate_dist4"].get_real();
1097+
1098+    	//Removing zero from numbers used in std::min to avoid division by zero in GetFeerateBar()
1099+    	auto const ignore_zero = [](auto const& a, auto const& b) -> bool {
1100+        	if(0 == a || 0 == b)

kiminuo commented at 9:12 pm on November 9, 2021:

0        	if (0 == a || 0 == b)

in src/bitcoin-cli.cpp:1108 in 797eac624a outdated

1103+        	}
1104+        	return a < b;
1105+    	};
1106+
1107+    	double max_size = std::max({size1, size2, size3, size4});
1108+    	double inc_size = std::min({size1, size2, size3, size4}, ignore_zero);

kiminuo commented at 9:19 pm on November 9, 2021:

What should happen when all values are zero?

unknown commented at 1:09 am on November 11, 2021:

mempool_bytes will be zero in that case and mempool section will not be printed L1085

in src/bitcoin-cli.cpp:1123 in 797eac624a outdated

1118+    		        fee_size = size2;
1119+        		break;
1120+    		case 3: fee_rate = "100-199   |";
1121+    		        fee_size = size3;
1122+        		break;
1123+        	case 4: fee_rate = "Above 200 |";

kiminuo commented at 9:20 pm on November 9, 2021:

This line makes me think that value 200 is not included neither in case 3 nor in case 4.

unknown commented at 1:11 am on November 11, 2021:

200 is included in case 4. I wrote Above 200 in short for 200 and Above 200

kiminuo commented at 7:20 am on November 11, 2021:

Yeah. Maybe one can use ≥ 200 (https://www.fileformat.info/info/unicode/char/2265/index.htm) instead of “Above 200” to make the meaning absolutely clear.

kiminuo commented at 9:20 pm on November 9, 2021: contributor

Just a few comments based on reading the code

nopara73 commented at 10:05 am on November 10, 2021: none

Feature ACK. Useful.

in src/bitcoin-cli.cpp:319 in 797eac624a outdated

315@@ -315,6 +316,8 @@ class GetinfoRequestHandler: public BaseRequestHandler
316         result.push_back(JSONRPCRequestObj("getblockchaininfo", NullUniValue, ID_BLOCKCHAININFO));
317         result.push_back(JSONRPCRequestObj("getwalletinfo", NullUniValue, ID_WALLETINFO));
318         result.push_back(JSONRPCRequestObj("getbalances", NullUniValue, ID_BALANCES));
319+        UniValue params{RPCConvertValues("getmempoolinfo", {"[1,10,100,200,1000]"})};

kiminuo commented at 10:28 am on November 10, 2021:

Playing devil’s advocate: Why to use these values and not different ones?

I think it’s very hard to argue that these values are the best possible. Without more information in PR’s description, this feels arbitrary. And even if you can persuade people for Bitcoin mainnet chain, I would be surprised if these values were good for Bitcoin testnet chain.

I don’t really know how to approach this but it seems important. This is also the reason why #21422 lets user to decide. Original #15836 mentioned log scale which seems in principle much better to me.

unknown commented at 1:19 am on November 11, 2021:

Playing devil’s advocate: Why to use these values and not different ones?

Different can be either random values which is difficult to support in this feature as its just a summary of mempool. Or use more values in between which will use more space hence used all multiples of 10 until 100 and then trying to include everything else in ‘Above 200’ category.

And even if you can persuade people for Bitcoin mainnet chain, I would be surprised if these values were good for Bitcoin testnet chain.

I tried them on testnet and mainnet. Works fine because it wont print a category if it has zero size.

This is also the reason why #21422 lets user to decide.

This is not replacement for 21422 but a usecase for it.

Original #15836 mentioned log scale which seems in principle much better to me.

It was closed by one of the maintainers with comment: “Closing this, given it’s been taken over in #21422”

One example of fee rate distribution from https://btc.bitaps.com/:

kiminuo commented at 7:18 am on November 11, 2021:

And even if you can persuade people for Bitcoin mainnet chain, I would be surprised if these values were good for Bitcoin testnet chain.

I tried them on testnet and mainnet. Works fine because it wont print a category if it has zero size.

I’m not sure we understand each other. Anyway, I just wanted to point out that any fixed intervals may sooner or later become obsolete. Suppose that your PR is merged and for whatever reasons nobody will mine fee rates less than 200. Then you will just show one bar (right?) and that’s not useful really. That is my main concern and that’s why I mentioned #15836 and and the log scale (a nice curve with interesting mathematical properties) - again, I’m not saying “use this”, just trying to say that some math analysis might be needed here.

This is also the reason why #21422 lets user to decide.

This is not replacement for 21422 but a usecase for it.

Yes, I know.

One example of fee rate distribution from https://btc.bitaps.com/:

I like it but it’s a website and they can change the output whenever they like. I don’t consider this to be true for Bitcoin Core.

Update src/bitcoin-cli.cpp