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:
This is interesting but still no comment.
Concept ACK
I’ll play around with this a bit.
0Wshadow statistics:
1 1 policy/fees_input.cpp:127:23: warning: declaration shadows a local variable [-Wshadow]
2 1 policy/fees_input.cpp:197:38: warning: declaration shadows a local variable [-Wshadow]
3 1 policy/fees_input.cpp:229:22: warning: declaration shadows a local variable [-Wshadow]
4 1 policy/fees_input.cpp:233:27: warning: declaration shadows a local variable [-Wshadow]
5 1 policy/fees_input.cpp:27:61: warning: declaration shadows a field of 'CBlockPolicyInput' [-Wshadow]
6 1 test/fee_est/fee_est.cpp:301:34: warning: declaration shadows a local variable [-Wshadow]
7 1 test/fee_est/fee_est.cpp:79:20: warning: declaration shadows a field of 'TxData' [-Wshadow]
8 1 test/fee_est/fee_est.cpp:79:29: warning: declaration shadows a field of 'TxData' [-Wshadow]
9 1 test/fee_est/fee_est.cpp:79:39: warning: declaration shadows a field of 'TxData' [-Wshadow]
fee_est is generating empty data in the HTML file. Emit some warning in such cases?
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?
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.
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.
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.
0@@ -0,0 +1,37 @@
1+src/test/fee_est -- Fee estimation offline testing tool
2+=======================================================
3+
4+The `fee_est` tool is intended to help debug and test changes in bitcoin fee
5+estimation code using transaction data gathered from live bitcoin nodes.
6+
7+Transaction data can be collected by running bitcoind with `-estlog` parameter
8+which will produce newline-delimited json file (http://ndjson.org/,
02018-09-11 16:31:38 mdl(pr=10443): src/test/fee_est/README.md:8: MD034 Bare URL used
12018-09-11 16:31:38 mdl(pr=10443): src/test/fee_est/README.md:9: MD034 Bare URL used
172+ if (entry.second.actualBlocks > int(maxTarget)) {
173+ continue;
174+ } else if (sample.size() < SAMPLE_TXS) {
175+ sample.emplace_back(&entry);
176+ } else {
177+ int j = rand() % i;
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:177:21: warning: rand() has limited randomness; use C++11 random library instead [cert-msc30-c]
196- std::vector<const CTxMemPoolEntry*>& entries);
197+ const std::function<void(Block&)> process_txs);
198
199 /** Process a transaction accepted to the mempool*/
200- void processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate);
201+ void processTx(const uint256& hash, unsigned int height, CAmount fee, size_t size, Block* block, bool valid);
02018-09-20 04:48:56 clang-tidy(pr=10443): src/policy/fees.h:199:10: warning: function 'CBlockPolicyEstimator::processTx' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
13 #include <util.h>
14
15 static constexpr double INF_FEERATE = 1e99;
16
17+struct CBlockPolicyEstimator::Block {
18+ Block(int height) : height(height) {}
02018-09-20 04:48:56 clang-tidy(pr=10443): src/policy/fees.cpp:17:5: warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [google-explicit-constructor]
02018-09-20 05:13:44 cppcheck(pr=10443): [src/policy/fees.cpp:17]: (style) Struct 'Block' has a constructor with 1 argument that is not explicit.
02018-09-22 22:51:38 cpplint(pr=10443): src/policy/fees.cpp:17: Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
144+
145+void WriteGraph(const std::string& filename,
146+ const TxMap& txMap,
147+ const std::vector<unsigned int>& targets,
148+ CBlockPolicyEstimator& estimator,
149+ CTxMemPool& pool)
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:149:17: warning: parameter 'pool' is unused [misc-unused-parameters]
158+ UniValue fee(UniValue::VOBJ);
159+ fee.pushKV("mode", conservative ? "conservative" : "default");
160+ fee.pushKV("blocks", int(target));
161+ int64_t feeRate = estimator.estimateSmartFee(target, nullptr /* confTarget */, conservative).GetFeePerK();
162+ fee.pushKV("feeRate", feeRate);
163+ fees.push_back(std::move(fee));
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:163:28: warning: passing result of std::move() as a const reference argument; no move will actually happen [hicpp-move-const-arg]
180+ }
181+ }
182+ ++i;
183+ }
184+
185+ if (sample.size() > 0) {
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:185:9: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
224+ }
225+ return true;
226+}
227+
228+void PrintCross(const TxMap& txMap,
229+ const std::vector<unsigned int>& targets,
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:229:38: warning: parameter 'targets' is unused [misc-unused-parameters]
225+ return true;
226+}
227+
228+void PrintCross(const TxMap& txMap,
229+ const std::vector<unsigned int>& targets,
230+ CBlockPolicyEstimator& estimator,
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:230:28: warning: parameter 'estimator' is unused [misc-unused-parameters]
226+}
227+
228+void PrintCross(const TxMap& txMap,
229+ const std::vector<unsigned int>& targets,
230+ CBlockPolicyEstimator& estimator,
231+ CTxMemPool& pool)
02018-09-20 04:48:56 clang-tidy(pr=10443): src/test/fee_est/fee_est.cpp:231:17: warning: parameter 'pool' is unused [misc-unused-parameters]
121+ }
122+
123+ if (filename.empty()) {
124+ log.reset();
125+ } else {
126+ log.reset(new std::ofstream(filename, std::ofstream::out | std::ofstream::app));
02018-09-20 04:48:56 clang-tidy(pr=10443): src/policy/fees_input.cpp:126:13: warning: use std::make_unique instead [modernize-make-unique]
MakeUnique in our case :-)
138+ }
139+
140+ return true;
141+}
142+
143+bool FeeEstInput::readLog(const std::string& filename, std::function<bool(UniValue&)> filter)
02018-09-20 04:48:56 clang-tidy(pr=10443): src/policy/fees_input.cpp:143:87: warning: the parameter 'filter' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
193+ }
194+
195+ const UniValue& read = value["read"];
196+ if (read.isStr()) {
197+ std::vector<unsigned char> data = ParseHex(read.get_str());
198+ unsigned short randv = 0;
02018-09-20 04:48:56 clang-tidy(pr=10443): src/policy/fees_input.cpp:198:13: warning: consider replacing 'unsigned short' with 'uint16' [google-runtime-int]
02018-09-22 22:51:38 cpplint(pr=10443): src/policy/fees_input.cpp:198: Use int16/int64/etc, rather than the C type short [runtime/int] [4]
18+class uint256;
19+
20+class FeeEstInput
21+{
22+public:
23+ FeeEstInput(CBlockPolicyEstimator& estimator);
02018-09-20 05:13:44 cppcheck(pr=10443): [src/policy/fees_input.h:23]: (style) Class 'FeeEstInput' has a constructor with 1 argument that is not explicit.
02018-09-22 22:51:38 cpplint(pr=10443): src/policy/fees_input.h:23: Single-parameter constructors should be marked explicit. [runtime/explicit] [5]
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | paveljanik, jnewbery, sipa, jonatack, aureleoules |
| Approach NACK | josibake |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
22+public:
23+ explicit FeeEstInput(CBlockPolicyEstimator& estimator);
24+
25+ /** Process all the transactions that have been included in a block */
26+ struct Block;
27+ void processBlock(int height, const std::function<void(Block&)>& process_txs);
height guaranteed to be non-negative? Should be unsigned int?
104+ if (file.IsNull() || !estimator.Read(file)) {
105+ return false;
106+ }
107+ if (log) {
108+ UniValue value(UniValue::VOBJ);
109+ std::ifstream file(filename.string(), std::ifstream::binary);
file shadows an already existing local variable. Please choose another name :-)
173+ const UniValue& block = value["block"];
174+ if (block.isObject()) {
175+ int height = block["height"].get_int();
176+ estimator.processBlock(height, [&](CBlockPolicyEstimator::Block& est_block) {
177+ const auto& txs = value["txs"].getValues();
178+ for (const UniValue& tx : txs) {
tx shadows a local variable. Please choose another name :-)
199+ const UniValue& read = value["read"];
200+ if (read.isStr()) {
201+ std::vector<unsigned char> data = ParseHex(read.get_str());
202+ uint16_t randv = 0;
203+ GetRandBytes((unsigned char*)&randv, sizeof(randv));
204+ fs::path filename = strprintf("fee_estimates.tmp.%04x", randv);
filename shadows another variable. Please use another name :-)
203+ GetRandBytes((unsigned char*)&randv, sizeof(randv));
204+ fs::path filename = strprintf("fee_estimates.tmp.%04x", randv);
205+ CAutoFile(fsbridge::fopen(filename, "wb"), SER_DISK, CLIENT_VERSION)
206+ .write((const char*)data.data(), data.size());
207+ {
208+ CAutoFile file(fsbridge::fopen(filename, "rb"), SER_DISK, CLIENT_VERSION);
159+ for (unsigned int target : targets) {
160+ if (target > maxTarget) continue;
161+ UniValue fee(UniValue::VOBJ);
162+ fee.pushKV("mode", conservative ? "conservative" : "default");
163+ fee.pushKV("blocks", int(target));
164+ int64_t feeRate = estimator.estimateSmartFee(target, nullptr /* confTarget */, conservative).GetFeePerK();
176+ if (entry.second.actualBlocks > int(maxTarget)) {
177+ continue;
178+ } else if (sample.size() < SAMPLE_TXS) {
179+ sample.emplace_back(&entry);
180+ } else {
181+ int j = randint() % i;
198+ }
199+
200+ UniValue txs(UniValue::VARR);
201+ for (const auto& entry : sample) {
202+ if (entry->second.actualBlocks > 0) {
203+ int64_t feeRate = CFeeRate(entry->second.fee, entry->second.size).GetFeePerK();
217+
218+bool UpdateCross(TxMap::value_type& tx, const std::vector<unsigned int>& targets, CBlockPolicyEstimator& estimator)
219+{
220+ if (*tx.first.begin() == 0) {
221+ auto it = std::lower_bound(targets.begin(), targets.end(), nullptr, [&](unsigned int target, std::nullptr_t) {
222+ CAmount estFee = estimator.estimateSmartFee(target, nullptr /* feeCalc */, false /* conservative */)
221+ auto it = std::lower_bound(targets.begin(), targets.end(), nullptr, [&](unsigned int target, std::nullptr_t) {
222+ CAmount estFee = estimator.estimateSmartFee(target, nullptr /* feeCalc */, false /* conservative */)
223+ .GetFee(tx.second.size);
224+ return estFee <= 1 || tx.second.fee < estFee;
225+ });
226+ tx.second.expectedBlocks = it == targets.end() ? std::numeric_limits<int>::max() : *it;
228+ }
229+ return true;
230+}
231+
232+void PrintCross(const TxMap& txMap,
233+ CBlockPolicyEstimator& estimator)
estimator can be removed?
297+
298+ const UniValue& block = value["block"];
299+ if (block.isObject()) {
300+ int blockHeight = block["height"].get_int();
301+ const auto& txs = value["txs"].getValues();
302+ for (const UniValue& tx : txs) {
tx here shadows an already existing local variable. Please choose another name :-)
37+ tx.pushKV("size", uint64_t(size));
38+ if (block) {
39+ block->json_txs.push_back(std::move(tx));
40+ } else {
41+ UniValue value(UniValue::VOBJ);
42+ value.pushKV("tx", std::move(tx));
move not needed?
34+ tx.pushKV("hash", hash.ToString());
35+ tx.pushKV("height", int(height));
36+ tx.pushKV("fee", fee);
37+ tx.pushKV("size", uint64_t(size));
38+ if (block) {
39+ block->json_txs.push_back(std::move(tx));
move not needed?
G_TRANSLATION_FUN link error after #13961
Rebased 8cd1bb684bfec22fc54e091dc96c782a26847393 -> a60d26ac2bceea2c67ef34c8641705b16f656d07 (pr/fee.23 -> pr/fee.24) due to conflict with #13311
Updated a60d26ac2bceea2c67ef34c8641705b16f656d07 -> 1e7ec287c927437b5a183bc807c49eba4a16d18a (pr/fee.24 -> pr/fee.25) with suggested lint fixes
Updated 1e7ec287c927437b5a183bc807c49eba4a16d18a -> 56446378f5d0c2bdeb7a45fa7382ccd58c9b994c (pr/fee.25 -> pr/fee.26) with more lint fixes
Updated 56446378f5d0c2bdeb7a45fa7382ccd58c9b994c -> 8d99281859540689b55285bd0acdf8c1529c4b37 (pr/fee.26 -> pr/fee.27) with more lint fixes
Updated 8d99281859540689b55285bd0acdf8c1529c4b37 -> 68944a5120c651772fd7f3c028c446e38f9a3f57 (pr/fee.27 -> pr/fee.28) with more lint fixes
Rebased 68944a5120c651772fd7f3c028c446e38f9a3f57 -> 50cd9b1cb7d03a81140d8a59698eba8d06b89612 (pr/fee.28 -> pr/fee.29) due to conflict with #14555
Rebased 50cd9b1cb7d03a81140d8a59698eba8d06b89612 -> a0148157c57e91b299e6b450c7ee758f606488e6 (pr/fee.29 -> pr/fee.30) due to conflict with #13128
Updated a0148157c57e91b299e6b450c7ee758f606488e6 -> 6f9c07f0a72ff0d95bfee9f37e4ae3dc9ed9c0e4 (pr/fee.30 -> pr/fee.31) restructuring to work around clang lock annotation errors from #13128
Rebased 6f9c07f0a72ff0d95bfee9f37e4ae3dc9ed9c0e4 -> ca993015e0e115540f0ef2710a8f2a81f5fbf0c1 (pr/fee.31 -> pr/fee.32) due to conflict with #14963
Rebased ca993015e0e115540f0ef2710a8f2a81f5fbf0c1 -> b197b85a6a1c682070c445c0314d6a645936ba0f (pr/fee.32 -> pr/fee.33) due to conflict with #15043
Rebased b197b85a6a1c682070c445c0314d6a645936ba0f -> b5df05ab46d25052c95344e91fc0631b29635f1d (pr/fee.33 -> pr/fee.34) due to conflict with #15295
Updated b5df05ab46d25052c95344e91fc0631b29635f1d -> 9588e9e81d29227213babe45d37e9bfcd10ab05c (pr/fee.34 -> pr/fee.35, compare) to fix travis link error with #15295
Rebased 9588e9e81d29227213babe45d37e9bfcd10ab05c -> e2330472f0ed7466e951d99f1c707d1852289cd1 (pr/fee.35 -> pr/fee.36) due to conflict with #15534
Rebased e2330472f0ed7466e951d99f1c707d1852289cd1 -> 28ab62aaaeac6d8f7ce5f3297504b549ca4cea5a (pr/fee.36 -> pr/fee.37) due to conflict with #15638
Rebased 28ab62aaaeac6d8f7ce5f3297504b549ca4cea5a -> ccc44e115237b5337869ddb738ecb1af04c52f61 (pr/fee.37 -> pr/fee.38) due to conflict with #15323
544@@ -546,11 +545,13 @@ CBlockPolicyEstimator::~CBlockPolicyEstimator()
545 {
546 }
547
548-void CBlockPolicyEstimator::processTransaction(const CTxMemPoolEntry& entry, bool validFeeEstimate)
88+ int actualBlocks = -1;
89+};
90+
91+using TxMap = std::map<uint256, TxData>;
92+
93+const char* const GRAPH_HTML = R"(<!DOCTYPE html>
This PR has some overlap with @kallewoof’s mff utility. One difference is that this creates logs in json-lines format and only logs mempool data needed to run fee estimation code reproducibly, while mff uses a custom format and logs additional information.
Rebased ccc44e115237b5337869ddb738ecb1af04c52f61 -> 6251342fc77cb49e9913aeeb89bf6f3e3234f617 (pr/fee.38 -> pr/fee.39 due to various conflicts
Rebased 6251342fc77cb49e9913aeeb89bf6f3e3234f617 -> 23a1edcfaa469022c9e8296559fb0d00417225ec (pr/fee.39 -> pr/fee.40) due to conflict with #16171
Rebased 6251342fc77cb49e9913aeeb89bf6f3e3234f617 -> 1b322b073e0ce93f55b9f5678bf50ee1af069c58 (pr/fee.39 -> pr/fee.40) due to conflict with #16171
Rebased 1b322b073e0ce93f55b9f5678bf50ee1af069c58 -> 207cf9f8356ab4ceb72b1fd0bbbb66f6e726006b (pr/fee.40 -> pr/fee.41) due to conflicts with #14193 and #16291
Rebased 207cf9f8356ab4ceb72b1fd0bbbb66f6e726006b -> a1c3573115b25a6b702a1b56e5813a80281f9d7d (pr/fee.41 -> pr/fee.42) due to conflict with #16362
Rebased a1c3573115b25a6b702a1b56e5813a80281f9d7d -> 73b1eaca4edecae3dcde6fcf57b3832fb84a8d10 (pr/fee.42 -> pr/fee.43)
Rebased 73b1eaca4edecae3dcde6fcf57b3832fb84a8d10 -> aa41b10397895687dcdafad40fd9ac1934796425 (pr/fee.43 -> pr/fee.44) due to conflict with #15921
Rebased aa41b10397895687dcdafad40fd9ac1934796425 -> f00110e8b6836ef384c0dbbd3ec693d5b97c21bb (pr/fee.44 -> pr/fee.45) due to minor conflict with #17050
Rebased f00110e8b6836ef384c0dbbd3ec693d5b97c21bb -> 03bcb1190e4e6673414a3ae9dfca1112ab14b305 (pr/fee.45 -> pr/fee.46) due to conflicts with #17851 and #16688
Rebased 03bcb1190e4e6673414a3ae9dfca1112ab14b305 -> 35af64c93097ca65e5d0e8ef76b0326fdc1561cf (pr/fee.46 -> pr/fee.47) due to conflict with #17891
Rebased 35af64c93097ca65e5d0e8ef76b0326fdc1561cf -> 6597699b2215d5aef8633c973b0e4f4fddcf230b (pr/fee.47 -> pr/fee.48) due to conflict with #17261
Rebased 6597699b2215d5aef8633c973b0e4f4fddcf230b -> 8392f77f047c1bc11e0475ef5fbf624f7f3f9488 (pr/fee.48 -> pr/fee.49) due to conflict with #17925
Rebased 8392f77f047c1bc11e0475ef5fbf624f7f3f9488 -> b5adc1b2819d88f5e2ed67dd98c9f5c091e99101 (pr/fee.49 -> pr/fee.50, compare) due to conflict with #18126
Rebased b5adc1b2819d88f5e2ed67dd98c9f5c091e99101 -> b698b5dbaa87cba45f867141a97b010813d86160 (pr/fee.50 -> pr/fee.51, compare) due to conflict with #18134
Rebased b698b5dbaa87cba45f867141a97b010813d86160 -> 47164d0ef2d2625c3c47e49659e6f1833c472700 (pr/fee.51 -> pr/fee.52, compare) due to conflict with #18673
Rebased 47164d0ef2d2625c3c47e49659e6f1833c472700 -> 5123ff1f22b38162e42529b2f1484bb980c7834c (pr/fee.52 -> pr/fee.53, compare) to due conflicts with #18698 and #18922
Updated 5123ff1f22b38162e42529b2f1484bb980c7834c -> 854a21ddb8007cdea471befd25cb401f7f4f96e4 (pr/fee.53 -> pr/fee.54, compare) fixing travis fuzzer build
Updated 854a21ddb8007cdea471befd25cb401f7f4f96e4 -> e635c970bd0a6b8bfe7f923e96fd17186d623843 (pr/fee.54 -> pr/fee.55, compare) fixing travis fuzzer assert https://travis-ci.org/github/bitcoin/bitcoin/jobs/691844334#L3561
Updated e635c970bd0a6b8bfe7f923e96fd17186d623843 -> 009c76da19c271a552ec88ba17009a30e1a3141b (pr/fee.55 -> pr/fee.56, compare) fixing travis fuzzer assert https://travis-ci.org/github/bitcoin/bitcoin/jobs/694860650#L3536
Rebased 009c76da19c271a552ec88ba17009a30e1a3141b -> 3f92adfecc9dc3b11e620b421ea4084ff9fcd280 (pr/fee.56 -> pr/fee.57, compare) to work around travis arm timeouts “sudo: unable to resolve host travis-job-bitcoin-bitcoin-694879056” https://travis-ci.org/github/bitcoin/bitcoin/jobs/694879056
Rebased 3f92adfecc9dc3b11e620b421ea4084ff9fcd280 -> 110245a8dbd11d31d9e52c7c17dc8795743617f8 (pr/fee.57 -> pr/fee.58, compare) due to conflict with #19561
Rebased 110245a8dbd11d31d9e52c7c17dc8795743617f8 -> 00d514a60820d7c3be4fa0e5433aefabd594d237 (pr/fee.58 -> pr/fee.59, compare) due to conflict with #19556
Rebased 00d514a60820d7c3be4fa0e5433aefabd594d237 -> 2c512ff0729f89aa47019009534d3958ec18f42b (pr/fee.59 -> pr/fee.60, compare) due to conflict with #20003
Discovered this PR thanks to DrahtBot’s “needs rebase” notifications.
Concept ACK, interesting idea for dev tooling in the tests or contrib.
125+ }
126+
127+ if (filename.empty()) {
128+ log.reset();
129+ } else {
130+ log = MakeUnique<std::ofstream>(filename, std::ofstream::out | std::ofstream::app);
std::make_unique in new code.
pr/fee.60 -> pr/fee.61, compare) due to conflicts with #18766
Updated de8a2d80f74d33067a3b532368cca877fc65e7d2 -> 4b1c4957a59f68f1c501e1fc5e0fea4a58b14525 (pr/fee.61 -> pr/fee.62, compare) due to std::filesystem compile error https://cirrus-ci.com/task/4871270773817344?command=ci#L8556 and https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38354674
Rebased 4b1c4957a59f68f1c501e1fc5e0fea4a58b14525 -> e793f63c336be2eb0c8c82e31c7ddc05600f9a3d (pr/fee.62 -> pr/fee.63, compare) due to conflict with #21575
Rebased e793f63c336be2eb0c8c82e31c7ddc05600f9a3d -> 941fabf8da5031d7405e5d69a607552ae3273fdf (pr/fee.63 -> pr/fee.64, compare) due to conflicts with #21732, #22003, #21850
Rebased 941fabf8da5031d7405e5d69a607552ae3273fdf -> de097a24fa9364aa2230dd3cb5ceac3677cb4d2a (pr/fee.64 -> pr/fee.65, compare) due to conflict #21966 with attempted fixes for fuzz UndefinedBehaviorSanitizer implicit-unsigned-integer-truncation error https://cirrus-ci.com/task/5690421675294720
Rebased de097a24fa9364aa2230dd3cb5ceac3677cb4d2a -> 50ce90cedb6480446e8d1970b2c138d95b2410af (pr/fee.65 -> pr/fee.66, compare) due to conflict with #20833
Rebased 50ce90cedb6480446e8d1970b2c138d95b2410af -> f51bb257abe46226a84a9a6dd01a362f06187f9e (pr/fee.66 -> pr/fee.67, compare) due to conflict with #22084
Rebased f51bb257abe46226a84a9a6dd01a362f06187f9e -> 233c1b2776e33d0c0657f06aca33b39880a20f4e (pr/fee.67 -> pr/fee.68, compare) due to conflict with #22796
Rebased 233c1b2776e33d0c0657f06aca33b39880a20f4e -> 3c5c30fcb1bac408f34604c81bf392ae7efcf787 (pr/fee.68 -> pr/fee.69, compare) due to conflicts with #23044, #22976, #22951
Rebased 3c5c30fcb1bac408f34604c81bf392ae7efcf787 -> 84f5df64005de6b6c0f651f2f6b0e470737fed4e (pr/fee.69 -> pr/fee.70, compare) due to conflicts with #22937 and #22766
various others
Rebased 84f5df64005de6b6c0f651f2f6b0e470737fed4e -> 1e8d0d3852d6cb83ce60c50d94f565dc9093eae7 (pr/fee.70 -> pr/fee.71, compare) due to conflicts with #25290 and various others
Rebased 1e8d0d3852d6cb83ce60c50d94f565dc9093eae7 -> 950f76289a1344c12fd3d3460a0688d0ead44607 (pr/fee.71 -> pr/fee.72, compare) due to conflict with #25667
Updated 950f76289a1344c12fd3d3460a0688d0ead44607 -> 538c780d078ecf400eb7b509f9fbcd176cdd290c (pr/fee.72 -> pr/fee.73, compare) to fix lint error https://cirrus-ci.com/task/6497917761486848?logs=lint#L1474
Updated 538c780d078ecf400eb7b509f9fbcd176cdd290c -> d1f1faa851d25685fb264c4a83669161632719c6 (pr/fee.73 -> pr/fee.74, compare) to fix test failure https://cirrus-ci.com/task/4838035710803968
Rebased d1f1faa851d25685fb264c4a83669161632719c6 -> 1288ad00a5215cfa0e303e7ce765c049d603adc8 (pr/fee.74 -> pr/fee.75, compare) due to conflict with #26286
Rebased 1288ad00a5215cfa0e303e7ce765c049d603adc8 -> 6e28e72462a9ebcb4ca9daa3412d15b451cba018 (pr/fee.75 -> pr/fee.76, compare) due to conflicts with #26705 and #27036
146+const int DROP_FRAC = 20;
147+
148+void WriteGraph(const std::string& filename,
149+ const TxMap& txMap,
150+ const std::vector<unsigned int>& targets,
151+ CBlockPolicyEstimator& estimator)
538c780d078ecf400eb7b509f9fbcd176cdd290c
0 const CBlockPolicyEstimator& estimator)
525@@ -527,8 +526,8 @@ bool CBlockPolicyEstimator::_removeTx(const uint256& hash, bool inBlock)
526 }
527 }
528
529-CBlockPolicyEstimator::CBlockPolicyEstimator(const fs::path& estimation_filepath)
530- : m_estimation_filepath{estimation_filepath}, nBestSeenHeight{0}, firstRecordedHeight{0}, historicalFirst{0}, historicalBest{0}, trackedTxs{0}, untrackedTxs{0}
531+CBlockPolicyEstimator::CBlockPolicyEstimator()
532+ : nBestSeenHeight{0}, firstRecordedHeight{0}, historicalFirst{0}, historicalBest{0}, trackedTxs{0}, untrackedTxs{0}
226+ return false;
227+ }
228+ return true;
229+}
230+
231+void PrintCross(const TxMap& txMap, CBlockPolicyEstimator&)
538c780d078ecf400eb7b509f9fbcd176cdd290c
0void PrintCross(const TxMap& txMap)
212+
213+ std::ofstream file(filename);
214+ file << strprintf(GRAPH_HTML, txs.write(), fees.write());
215+}
216+
217+bool UpdateCross(TxMap::value_type& tx, const std::vector<unsigned int>& targets, CBlockPolicyEstimator& estimator)
538c780d078ecf400eb7b509f9fbcd176cdd290c
0bool UpdateCross(TxMap::value_type& tx, const std::vector<unsigned int>& targets, const CBlockPolicyEstimator& estimator)
Haven’t dug into the code, but I’m leaning toward Concept ACK, Approach NACK
As someone who is planning to do fee estimation research in the near future, I don’t think I’d use something like this the way it’s currently written. I would much prefer something that logs Bitcoin Core’s live fee rate estimations, or some option that would allow me to tell Bitcoin Core it’s at a certain chain height, feed it some mempool data, and then ask it for fee estimations.
I also agree that the tool for analyzing the data shouldn’t be in this repo. Much better if we can figure out what data is missing for people who want to do fee estimation analysis and provide an option for logging that data.
Perhaps a good next step would be to talk to people who are currently doing fee estimation work and see what data would help with their projects/PRs? @ClaraShk @darosior @Xekyo ?
@josibake thanks for looking at this. I don’t think I understand what the idea behind an approach NACK would be based on what you are saying, though.
It sounds more like you’re asking for an additional feature which would be easy to implement on top of the first commit, not pointing to a drawback of the approach. If you want to gather live fee estimates, the first commit outputs a structured jsonl log of fee estimation events, so it would be easy to add the actual fee estimates to this log, or add internal metrics used to produce the fee estimates, or add any other data points that might be interesting. It is also possible to take the existing log files this PR already generates, and feed them to the fee estimation algorithm by modifying the tool in the second commit to produce live fee estimates or a history of estimates over time.
If you just think I should drop the second commit, or trim it down, I think that would be reasonable. The point of the second commit is mainly to provide example code that demonstrates how to read the JSONL log file and how to call the fee estimator API. The idea is to avoid needing reinvent the wheel each time you want to monitor fee estimation behavior, or reproduce a bug, or determine the effects of a change.
I would much prefer […] some option that would allow me to tell Bitcoin Core it’s at a certain chain height, feed it some mempool data, and then ask it for fee estimations.
This is actually what the second commit provides. The .jsonl log contains chain and mempool data. You can feed it to the test program in the second commit, and it calls fee estimation code to produce fee estimates. It can produce a graph of the fee estimates, and calculate how accurate they were by looking at all the transactions that got confirmed and outputting the mean squared error of how many blocks it actually took the transactions to be confirmed vs how many blocks the fee estimation algorithm predicted it would take to get them confirmed based on their feerate.
This PR is a few years old, so I would probably implement the second commit a little differently today. Instead of adding a standalone c++ executable that calls fee estimator code and produces graphs and metrics, I might want to just add a python module that exposes the CBlockPolicyEstimator class, and generate the graphs and metrics from a python notebook.
Thanks for the thorough response @ryanofsky. I realize I didn’t explain myself very well in my initial comment.
It sounds more like you’re asking for an additional feature which would be easy to implement on top of the first commit, not pointing to a drawback of the approach
Since we already record mempool events in the debug log file (this is what @jamesob and I are using to log mempool events in bmon), it feels a bit strange to have special code for logging transactions again here for fee estimation. It’s possible I’m not understanding something here, though: is there some special information in the log you are writing that we can’t get from the debug log file?
This is actually what the second commit provides. The .jsonl log contains chain and mempool data. You can feed it to the test program in the second commit, and it calls fee estimation code to produce fee estimates
I think what I was envisioning was even simpler: have a flag to make Bitcoin Core log fee_rate estimates per block and then have some way to walk the node back to some point in the past, have it log fee_rate estimates (perhaps with a code change) and then leave it up to the individual on how to analyze the diff between those logs. Perhaps this is easier said than done since the fee estimation code in Bitcoin Core today seems to work on a per transaction basis?
is there some special information in the log you are writing that we can’t get from the debug log file?
I think so. The json log captures all inputs to fee estimator to used update its internal state. It includes not just mempool events but also information about which transactions are included in blocks, flush events, and contents of fee_estimates.dat blobs that are loaded. It makes fee estimation code reproducible, so if you make a change to the fee estimation code or parameters, you can evaluate the effects of the change with real world inputs offline. 1
I think what I was envisioning was even simpler: have a flag to make Bitcoin Core log fee_rate estimates per block and then have some way to walk the node back to some point in the past, have it log fee_rate estimates (perhaps with a code change) and then leave it up to the individual on how to analyze the diff between those logs. Perhaps this is easier said than done since the fee estimation code in Bitcoin Core today seems to work on a per transaction basis?
I think I don’t actually understand the proposal or don’t understand the intended use-case. Use-cases for PR this are to:
Is your proposal basically to save a fee_estimates.dat file each time a block is connected and then use those files for something? I’m guess I’m not sure how you would use those files to improve fee estimates. This PR does capture a superset of that information, too. For example, it would be possible to generate those files offline from the json log file produced in this PR by adding a few lines to the filter callback:
0const UniValue& block = value["block"];
1if (block.isObject()) {
2 AutoFile file{fsbridge::fopen(strprintf("fee_estimates-%07d.dat", block["height"].getInt<int>());, "wb")};
3 estimator.Write(file);
4}
Another advantage of this change is that it decouples CBlockPolicyEstimator from the mempool, so it’s possible to call it offline. The external API changes from:
to:
There are probably lots of changes that could be made to this PR to improve it, or trim it down if it is doing too much, but in general I think takes a good first step of pulling things out of CBlockPolicyEstimator that don’t belong there (mempool access, filesystem access), and then builds some useful features on top of that.
🐙 This pull request conflicts with the target branch and needs rebase.
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Going to close this because I think @josibake is working on a different approach which will try replace current fee estimator, instead of just instrumenting it like this PR.
There’s also another PR #28368 which decouples the fee estimator from the mempool, and might make it easier to replay fee estimation events like the changes in this PR, even though it only provides an API, not a CLI tool.
