Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feerate histogram to getmempoolinfo #15836

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jonasschnelli
Copy link
Member

commented Apr 17, 2019

This follows the approach of adding statistical information to Bitcoin Core that would otherwise be inefficient to calculate outside of the codebase.

Adds an optional feerate histogram to getmempoolinfo.

The concept and code is heavily inspired by the stats @jhoenicke runs (https://github.com/jhoenicke/mempool).

If someone has a good idea how to make the feerate-groups dynamic but also semi-constant for similar fee environments, please comment.

If this is feature we'd like to have in master (concept ACKs), I'd continue this with writing tests.

A simple plot of the data is here.
RPC output sample is here.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2019/04/feeinfo branch from 2f27f15 to 80fbf80 Apr 17, 2019

@promag
Copy link
Member

left a comment

Concept ACK.

I think that out-of-the-box we can expose some stats like this.

I think it may be useful to include the current timestamp in the response - some client can just run a cron script to call getmempoolinfo and store it (without changing the JSON response).

throw std::runtime_error(
RPCHelpMan{"getmempoolinfo",
"\nReturns details on the active state of the TX memory pool.\n",
{},
{
{"with_fee_histogram", RPCArg::Type::BOOL, /* default */ "false", "True for including the fee histogram in the response"},

This comment has been minimized.

Copy link
@promag

promag Apr 17, 2019

Member

Instead of this parameter, it could have fee_histogram_bins (that defaults to [] which means no histogram is included in the response). This would replace the above feelimits and also avoids breaking clients implementation.

std::vector<uint64_t> count(feelimits.size(), 0);
std::vector<uint64_t> fees(feelimits.size(), 0);

LOCK(pool.cs);

This comment has been minimized.

Copy link
@promag

promag Apr 17, 2019

Member

I believe we should move this up (done in #15474).

This comment has been minimized.

Copy link
@promag

promag Apr 17, 2019

Member

That pull was merged, please rebase and remove this lock.

@@ -629,7 +639,8 @@ static const struct {
{"/rest/block/notxdetails/", rest_block_notxdetails},
{"/rest/block/", rest_block_extended},
{"/rest/chaininfo", rest_chaininfo},
{"/rest/mempool/info", rest_mempool_info},
{"/rest/mempool/info", rest_mempool_info_basic},
{"/rest/mempool/info/with_fee_histogram", rest_mempool_info_with_fee_histogram},

This comment has been minimized.

Copy link
@promag

promag Apr 17, 2019

Member

Can't we just start to use query parameters?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Apr 17, 2019

Author Member

Would eventually be better but not scope of this PR (following the current scheme).


// distribute feerates into feelimits
for (size_t i = 0; i < feelimits.size(); i++) {
if (feeperbyte >= feelimits[i] && (i == feelimits.size() - 1 || feeperbyte < feelimits[i + 1])) {

This comment has been minimized.

Copy link
@promag

promag Apr 17, 2019

Member

Correct me if I'm wrong but if feelimits is sorted then && (i == feelimits.size() - 1 || feeperbyte < feelimits[i + 1]) is not necessary.

This comment has been minimized.

Copy link
@promag

promag Apr 17, 2019

Member

Beside, it could avoid linear search by using std::find.

@jonatack
Copy link
Contributor

left a comment

Concept ACK. Useful addition 👍 . Tested RPC output and help man output. Agree with @promag on the current timestamp. Perhaps go with a name-based argument e.g. fee_histogram=true from the start?


// distribute feerates into feelimits
for (size_t i = 0; i < feelimits.size(); i++) {
if (feeperbyte >= feelimits[i] && (i == feelimits.size() - 1 || feeperbyte < feelimits[i + 1])) {

This comment has been minimized.

Copy link
@jonatack

jonatack Apr 17, 2019

Contributor

Would it be efficient to memoize feelimits.size() - 1 ? (if the compiler doesn't optimize it automatically, my C++ is rusty)

This comment has been minimized.

Copy link
@jonatack

jonatack Apr 17, 2019

Contributor

If && (i == feelimits.size() - 1 || feeperbyte < feelimits[i + 1]) can be removed, the dependency on feelimits being sorted would need a regression test.

This comment has been minimized.

Copy link
@promag

promag Apr 18, 2019

Member

Would it be efficient to memoize feelimits.size() - 1 ? (if the compiler doesn't optimize it automatically, my C++ is rusty)

It shouldn't impact performance either way.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

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

Conflicts

No conflicts as of last run.

@@ -1496,16 +1496,76 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK()));
ret.pushKV("minrelaytxfee", ValueFromAmount(::minRelayTxFee.GetFeePerK()));

if (with_fee_histogram) {

This comment has been minimized.

Copy link
@luke-jr

luke-jr Apr 18, 2019

Member

Maybe move this directly into getmempoolinfo? Or another helper?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Apr 24, 2019

Author Member

I thought about another call, but extending mempoolinfo with an option for "more data" seems to be most allied with other calls where one can get more extended infos on option.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Apr 24, 2019

Member

I mean just have the code outside this function. The RPC would then call both MempoolInfoToJSON and also JSONMempoolInfoAddHistogram (or whatever this code gets called)

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2019/04/feeinfo branch from 80fbf80 to c97a9dd Apr 24, 2019

jonasschnelli added some commits Apr 16, 2019

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2019/04/feeinfo branch from c97a9dd to 5d47656 May 9, 2019

@kristapsk

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Concept ACK / tACK 9ef9325

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.