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 fee_est tool for debugging fee estimation code #10443

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@ryanofsky
Copy link
Contributor

ryanofsky commented May 22, 2017

This PR adds an -estlog option for saving live fee estimation data from a bitcoin node, and a fee_est command line tool for processing the data and testing fee estimation code.

The idea is to make it easier to test improvements to fee estimation like #10199 in a more systematic and reproducible way.

Some documentation is in src/test/fee_est/README.md

Sample log file: est.log.xz (65M)

Sample fee_est.cpp graph output:

graph

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from ae719a1 to fbe0961 May 23, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch 3 times, most recently from 43fa494 to ec5f6ec Jun 2, 2017

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Jun 13, 2017

This is interesting but still no comment.

Concept ACK

I'll play around with this a bit.

Wshadow statistics: 
   1 policy/fees_input.cpp:127:23: warning: declaration shadows a local variable [-Wshadow]
   1 policy/fees_input.cpp:197:38: warning: declaration shadows a local variable [-Wshadow]
   1 policy/fees_input.cpp:229:22: warning: declaration shadows a local variable [-Wshadow]
   1 policy/fees_input.cpp:233:27: warning: declaration shadows a local variable [-Wshadow]
   1 policy/fees_input.cpp:27:61: warning: declaration shadows a field of 'CBlockPolicyInput' [-Wshadow]
   1 test/fee_est/fee_est.cpp:301:34: warning: declaration shadows a local variable [-Wshadow]
   1 test/fee_est/fee_est.cpp:79:20: warning: declaration shadows a field of 'TxData' [-Wshadow]
   1 test/fee_est/fee_est.cpp:79:29: warning: declaration shadows a field of 'TxData' [-Wshadow]
   1 test/fee_est/fee_est.cpp:79:39: warning: declaration shadows a field of 'TxData' [-Wshadow]
@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Jun 14, 2017

When estimate log doesn't have enough data, fee_est is generating empty data in the HTML file. Emit some warning in such cases?

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Jun 14, 2017

For perfect output, there should be 3rd dimension - time or current block height when the tx was first seen :-)

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jun 28, 2017

Concept ACK. I think this could be a useful tool.

Could this be split into two PRs to aid reviewers? The first PR would cover the -estlog option and writing the fee estimation data to disk. The second PR would be for a tool to read and graph the logs.

Parsing and graphing json files seems like a problem that has probably been solved many times before. If I was approaching this, I'd look at implementing this as a script in /contrib or a separate repository. Was there a particular reason you chose to implement this as a new C++ program?

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch 2 times, most recently from da0e959 to 88e26a1 Jul 18, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from 88e26a1 to 242fea6 Aug 25, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch 2 times, most recently from 87e104b to f49e21d Nov 29, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from f49e21d to d9cc337 Dec 12, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from d9cc337 to 1b18551 Feb 9, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from 1b18551 to 12e3eed Mar 2, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from 12e3eed to b8c2ce5 Mar 30, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from b8c2ce5 to 925281a Apr 10, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 10, 2018

Could this be split into two PRs to aid reviewers? The first PR would cover the -estlog option and writing the fee estimation data to disk. The second PR would be for a tool to read and graph the logs.

I think this is useful.

However I'm not sure the analysis tool belongs in this repository. As it's specific to developers debugging the fee estimation code it's not something we want to ship with the distribution, or install by default. One place it could be is e.g. https://github.com/bitcoin-core/bitcoin-maintainer-tools . You could still document or refer to it from here.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Apr 10, 2018

Agree that fee_est tool shouldn't be installed, since it's a tool specifically for made for modifying and debugging fee estimation code.

But it would be awkward to use and maintain from a separate repository because it links and calls into the fee estimation code. (The tool works by piping historical data into the fee estimator so it's possible to make experimental changes to fee estimation and see how those changes affect its output and internal state.)

As far as build / distribution is concerned I think it makes sense to think of it more like a unit test or benchmark than like a maintainer tool.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 14, 2018

But it would be awkward to use and maintain from a separate repository because it links and calls into the fee estimation code.

Yes, I agree, that's unfortunately true. That part needs to stay in the repository with the rest of the code.

Parsing and graphing json files seems like a problem that has probably been solved many times before. If I was approaching this, I'd look at implementing this as a script in /contrib or a separate repository.

I agree - especially with the references to cloudflare CDN and such. It feels a bit ugly to have that in the C++ code. Better to spit out e.g. JSON or CSV or whatever is convenient, then use a separate script for pretty formatting. This would also keep open the option of using different (non-web) visualization tools.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 29, 2018

The last travis run for this pull request was 54 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
errsq += err * err;
}
}
printf("Non-test txs: %i\n", nonTestTxs);

This comment has been minimized.

@practicalswift

practicalswift Nov 13, 2018

Member

printf(…) is it is not used in the rest of the project:

$ git grep "[^frns]printf" -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/tinyformat.h" ":(exclude)src/univalue/"
$

@bitcoin bitcoin deleted a comment from ryanofsky Nov 13, 2018

tx.pushKV("feeRate", feeRate);
tx.pushKV("blocks", entry->second.actualBlocks);
tx.pushKV("height", entry->second.height);
txs.push_back(std::move(tx));

This comment has been minimized.

@practicalswift

practicalswift Nov 13, 2018

Member

This move is not needed?

@bitcoin bitcoin deleted a comment from ryanofsky Nov 13, 2018

block->json_txs.push_back(std::move(tx));
} else {
UniValue value(UniValue::VOBJ);
value.pushKV("tx", std::move(tx));

This comment has been minimized.

@practicalswift

practicalswift Nov 13, 2018

Member

move not needed?

@bitcoin bitcoin deleted a comment from ryanofsky Nov 13, 2018

tx.pushKV("fee", fee);
tx.pushKV("size", uint64_t(size));
if (block) {
block->json_txs.push_back(std::move(tx));

This comment has been minimized.

@practicalswift

practicalswift Nov 13, 2018

Member

move not needed?

CAutoFile(fsbridge::fopen(data_filename, "wb"), SER_DISK, CLIENT_VERSION)
.write((const char*)data.data(), data.size());
{
CAutoFile data(fsbridge::fopen(data_filename, "rb"), SER_DISK, CLIENT_VERSION);

This comment has been minimized.

@practicalswift

practicalswift Nov 21, 2018

Member

This data shadows the data on line 206 above :-)

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch 2 times, most recently from a014815 to 6f9c07f Jan 4, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 5, 2019

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Jan 7, 2019

Rebased 51163ce -> 8b9c3da (pr/fee.16 -> pr/fee.17) due to conflict with #13341
Rebased 8b9c3da -> 4bcecdb (pr/fee.17 -> pr/fee.18) due to conflict with #13786
Updated 4bcecdb -> aaa4b68 (pr/fee.18 -> pr/fee.19) fixing travis size_t univalue errors
Rebased aaa4b68 -> a5faeb1 (pr/fee.19 -> pr/fee.20) to fix broken lint error caused by #13705
Updated a5faeb1 -> 61ab4c1 (pr/fee.20 -> pr/fee.21) adding missing lock annotation.
Rebased 61ab4c1 -> a9eb5cc (pr/fee.21 -> pr/fee.22) due to conflict with #14100
Updated a9eb5cc -> 8cd1bb6 (pr/fee.22 -> pr/fee.23) to fix G_TRANSLATION_FUN link error after #13961
Rebased 8cd1bb6 -> a60d26a (pr/fee.23 -> pr/fee.24) due to conflict with #13311
Updated a60d26a -> 1e7ec28 (pr/fee.24 -> pr/fee.25) with suggested lint fixes
Updated 1e7ec28 -> 5644637 (pr/fee.25 -> pr/fee.26) with more lint fixes
Updated 5644637 -> 8d99281 (pr/fee.26 -> pr/fee.27) with more lint fixes
Updated 8d99281 -> 68944a5 (pr/fee.27 -> pr/fee.28) with more lint fixes
Rebased 68944a5 -> 50cd9b1 (pr/fee.28 -> pr/fee.29) due to conflict with #14555
Rebased 50cd9b1 -> a014815 (pr/fee.29 -> pr/fee.30) due to conflict with #13128
Updated a014815 -> 6f9c07f (pr/fee.30 -> pr/fee.31) restructuring to work around clang lock annotation errors from #13128
Rebased 6f9c07f -> ca99301 (pr/fee.31 -> pr/fee.32) due to conflict with #14963
Rebased ca99301 -> b197b85 (pr/fee.32 -> pr/fee.33) due to conflict with #15043
Rebased b197b85 -> b5df05a (pr/fee.33 -> pr/fee.34) due to conflict with #15295
Updated b5df05a -> 9588e9e (pr/fee.34 -> pr/fee.35, compare) to fix travis link error with #15295
Rebased 9588e9e -> e233047 (pr/fee.35 -> pr/fee.36) due to conflict with #15534

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from 6f9c07f to ca99301 Jan 15, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 15, 2019

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from ca99301 to b197b85 Jan 30, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 31, 2019

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from b197b85 to b5df05a Feb 22, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 22, 2019

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from b5df05a to 9588e9e Feb 22, 2019

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/fee branch from 9588e9e to e233047 Mar 5, 2019

@DrahtBot DrahtBot removed the Needs rebase label Mar 5, 2019

@@ -546,11 +545,13 @@ CBlockPolicyEstimator::~CBlockPolicyEstimator()
{
}

void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate)

This comment has been minimized.

@Empact

Empact Mar 10, 2019

Member

nit: this renaming is succinct, yes, but also less communicative.


using TxMap = std::map<uint256, TxData>;

const char* const GRAPH_HTML = R"(<!DOCTYPE html>

This comment has been minimized.

@Empact

Empact Mar 10, 2019

Member

nit: maybe load this from a separate file?

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.