[DRAFT] New fee estimator #15

willcl-ark · 2024-05-21T19:37:10Z

.

willcl-ark · 2024-05-21T19:37:27Z

willcl-ark

Left some comments :)

willcl-ark · 2024-05-21T19:39:49Z

src/policy/estimator.cpp

+    }
+
+    if (!forecast.empty()) {
+        LogInfo("Fee Estimator: %s, low priority fee rate estimate %s %s/kvB, high priority fee rate estimate %s %s/kvB.\n", forecast.m_forecaster,


In [policy, fees]: add fee estimator module

We'll likely want to use BCLOG::ESTIMATEFEE for this and other logs

willcl-ark · 2024-05-21T20:05:48Z

src/policy/fees_util.cpp

+    const unsigned int p5_Size = static_cast<unsigned int>(0.05 * DEFAULT_BLOCK_MAX_WEIGHT);
+    const unsigned int p25_Size = static_cast<unsigned int>(0.25 * DEFAULT_BLOCK_MAX_WEIGHT);
+    const unsigned int p50_Size = static_cast<unsigned int>(0.5 * DEFAULT_BLOCK_MAX_WEIGHT);
+    const unsigned int p75_Size = static_cast<unsigned int>(0.75 * DEFAULT_BLOCK_MAX_WEIGHT);


In [policy, fees]: create a mempool based fee rate forecaster:

Perhaps we should call these p*_weight?

willcl-ark · 2024-05-21T20:10:43Z

src/policy/mempool_fees.cpp

+
+ForecastResult MemPoolPolicyEstimator::EstimateFeeWithMemPool(unsigned int targetBlocks)
+{
+    if (targetBlocks > MEMPOOL_FORECAST_MAX_TARGET) {


In [policy, fees]: create a mempool based fee rate forecaster:

We don't check that it's >0 here either, so currently 0 is valid.

Added an early check for this in the RPC...

Also each forecaster should check for this, because we may call estimatefee via interfaces

willcl-ark · 2024-05-21T20:13:25Z

src/test/mempoolestimator_tests.cpp

+    }
+
+    {
+        // Test ehrn mempool is not loaded


In [test]: add MemPoolPolicyEstimator unit test:

ehrn

willcl-ark · 2024-05-21T20:14:02Z

src/test/mempoolestimator_tests.cpp

+        // Test ehrn mempool is not loaded
+        const auto fee_estimate = mempool_fee_estimator->EstimateFee(MEMPOOL_FORECAST_MAX_TARGET);
+        BOOST_CHECK(fee_estimate.empty() == true);
+        BOOST_CHECK(fee_estimate.m_err_message == strprintf("%s: Mempool did not finished loading, can't get accurate fee rate forecast", MEMPOOL_FORECAST_NAME_STR));


In [test]: add MemPoolPolicyEstimator unit test:

Will need to update these if you take the suggestions in previous commit " [policy, fees]: create a mempool based fee rate forecaster "

willcl-ark · 2024-05-21T20:29:28Z

src/node/mini_miner.h

-    // number = included earlier).  Transactions included in an ancestor set together have the same
-    // sequence number.
-    std::map<Txid, uint32_t> m_inclusion_order;
+    LinearizationResult m_linearize_result;


In [mini miner]: Linearize should also return packages fees and sizes:

Would be nice to keep a comment about this member I think.

It's preserved in the new struct, all members of m_linearize_result were described.

willcl-ark · 2024-05-21T20:36:11Z

src/policy/block_fees.cpp

+    BlockPercentiles percentiles_average;
+
+    if (targetBlocks > BLOCK_FORECAST_MAX_TARGET) {
+        return ForecastResult(strprintf("%s: Confirmation target %u is above the maximum limit of %u. Mempool conditions might change, and estimates above %u are unreliable",


In [policy, fees]: add last 6 blocks fee estimate forecaster:

I feel like this makes less sense with a block_fees estimator, but not sure what to suggest to change it to...

Yes, updated

willcl-ark · 2024-05-21T20:38:25Z

src/rpc/fees.cpp

+                {RPCResult::Type::NUM, "Low priority fee rate estimate (" + CURRENCY_UNIT + "/kvB)", /*optional=*/true, "fee rate estimate in " + CURRENCY_UNIT + "/kvB for economical users"},
+                {RPCResult::Type::NUM, "High priority fee rate estimate (" + CURRENCY_UNIT + "/kvB)", /*optional=*/true, "fee rate estimate in " + CURRENCY_UNIT + "/kvB for conservative users"},
+                {RPCResult::Type::STR, "Forecaster", /*optional=*/true, "the forecaster that provide the fee rate estimate"},
+                {RPCResult::Type::ARR, "errors", /*optional=*/true, "Errors encountered during processing (if there are any)", {


In [rpc]: create an rpc estimatefeewithforecasters :

suggest changing to e.g.

Suggested change

{RPCResult::Type::NUM, "Low priority fee rate estimate (" + CURRENCY_UNIT + "/kvB)", /*optional=*/true, "fee rate estimate in " + CURRENCY_UNIT + "/kvB for economical users"},

{RPCResult::Type::NUM, "High priority fee rate estimate (" + CURRENCY_UNIT + "/kvB)", /*optional=*/true, "fee rate estimate in " + CURRENCY_UNIT + "/kvB for conservative users"},

{RPCResult::Type::STR, "Forecaster", /*optional=*/true, "the forecaster that provide the fee rate estimate"},

{RPCResult::Type::ARR, "errors", /*optional=*/true, "Errors encountered during processing (if there are any)", {

{RPCResult::Type::NUM, "low", /*optional=*/true, "fee rate estimate in " + CURRENCY_UNIT + "/kvB for economical users"},

{RPCResult::Type::NUM, "high", /*optional=*/true, "fee rate estimate in " + CURRENCY_UNIT + "/kvB for conservative users"},

as per Signal convo

Taken, but I left out the forecaster and errors from the outputs for now.

Also, per our conversation, I looked into why the block value is returned in estimatesmartfee. It's because sometimes you may want to estimate the fee for a 5-confirmation target, but there may be no valid estimate for that target. In such cases, it might return an estimate for a different block target like 6 where a valid estimate is available.

This does not apply to our forecasters.

willcl-ark · 2024-05-21T20:39:37Z

src/rpc/fees.cpp

+            // The estimator currently support only Fee estimators that provide estimate for transaction to confirm
+            // as soon as possible in one or two blocks.
+            // If the confirmation target is more than 2 fail early.
+            // This should change whenever we have a forecaster that can support higher target.
+            if (targetBlocks > 2) {
+                errors.push_back("estimatefeewithforecasters currently provide estimates for confirmation target 1 and 2 only");
+                result.pushKV("errors", errors);
+                return result;
+            }


In [rpc]: create an rpc estimatefeewithforecasters :

I wonder whether ideally this error should be surfaced from the fee_estimator itself, and not here...?

willcl-ark · 2024-05-21T20:40:25Z

src/rpc/fees.cpp

+                result.pushKV("errors", errors);
+                return result;
+            }
+            std::pair<ForecastResult, std::vector<std::string>> forecast_result = fee_estimator.GetFeeEstimateFromForecasters(/*targetBlocks=*/1);


In [rpc]: create an rpc estimatefeewithforecasters :

This seems to be erroneouly hardcoded to 1?

def6dd0 depends: sqlite 3.46.1 (fanquake) Pull request description: Update sqlite in depends from [3.38.5](https://sqlite.org/releaselog/3_38_5.html) to [3.46.1](https://sqlite.org/releaselog/3_46_1.html). ACKs for top commit: TheCharlatan: ACK def6dd0 theuni: Not opposed utACK def6dd0 Tree-SHA512: 1f12c8ed8d05600b8240bcdbad5cf7d073ea5ab0bbd4a0f49a39ccfe1a93c043ee855b6eb0c67028edec57d8c21588dc33246e64d0b94feafad1a6ec38839893

…compile-time fa69a5f util: Treat Assume as Assert when evaluating at compile-time (MarcoFalke) Pull request description: There is no downside or cost of treating an `Assume` at compile-time as an `Assert` and it may even help to find bugs while compiling without `ABORT_ON_FAILED_ASSUME`. This is also required for bitcoin#31093 ACKs for top commit: dergoegge: ACK fa69a5f brunoerg: ACK fa69a5f marcofleon: ACK fa69a5f Tree-SHA512: 17604403f841343a6d5b6e5d777e1760d38e0c27dc1fd4479e3741894fba40cdb1fb659cf24519a51d051bd5884a75992d1227ec9fa40fbf53bc619fbfb304ad

by passing an additional argument of "outonly" or "o". This has been requested in order to keep the output within screen limits when running -netinfo as a live dashboard, i.e. with `watch`. Also allow passing "h" in addition to "help" to see the help documentation.

Current test coverage doesn't ensure that mempool trimming doesn't appear prior to the entire package, and not just the subpackage, is finished being submitted. Add a scenario that covers this case, where package ancestors can make it in individually, but would be immadiately evicted if not for the package CPFP.

…ased on nNodes 66082ca Preallocate addresses in GetAddr based on nNodes (Lőrinc) Pull request description: The reserve method optimizes memory allocation by preallocating space for the expected number of elements (nNodes), reducing reallocations and improving performance. The upper bound ensures efficient memory usage based on the input constraints. before: ``` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 76,852.79 | 13,011.89 | 0.4% | 1.07 | `AddrManGetAddr` | 76,598.21 | 13,055.14 | 0.2% | 1.07 | `AddrManGetAddr` | 76,296.32 | 13,106.79 | 0.1% | 1.07 | `AddrManGetAddr` ``` after: ``` | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 65,966.97 | 15,159.10 | 0.3% | 1.07 | `AddrManGetAddr` | 66,075.40 | 15,134.23 | 0.2% | 1.06 | `AddrManGetAddr` | 66,306.34 | 15,081.51 | 0.3% | 1.06 | `AddrManGetAddr` ``` ACKs for top commit: stickies-v: ACK 66082ca vasild: ACK 66082ca Tree-SHA512: 1175cff250d9c52ed042e8807ddc2afd64a806e6f2195b5c648752869ff3beec0be8a8cbd7ab6ba35cd7077d79b88a380da6c6e244f5549f98cdd472808b6d8f

552cae2 fuzz: cover `ASMapHealthCheck` in connman target (brunoerg) 33b0f3a fuzz: use `ConsumeNetGroupManager` in connman target (brunoerg) 18c8a09 fuzz: move `ConsumeNetGroupManager` to util (brunoerg) fe62463 fuzz: fuzz `connman` with a non-empty addrman (brunoerg) 0a12cff fuzz: move `AddrManDeterministic` to util (brunoerg) Pull request description: ### Motivation Currently, we fuzz connman with an addrman from `NodeContext`. However, fuzzing connman with only empty addrman might not be effective, especially for functions like `GetAddresses` and other ones that plays with addrman. Also, we do not fuzz connman with ASMap, what would be good for functions that need `GetGroup`, or even for addrman. Without it, I do not see how effective would be fuzzing `ASMapHealthCheck`, for example. ### Changes - Move `AddrManDeterministic` and `ConsumeNetGroupManager` to util. - Use `ConsumeNetGroupManager` in connman target to construct a netgroupmanager and use it for `ConnmanTestMsg`. - Use `AddrManDeterministic` in connman target to create an addrman. It does not slow down as "filling" the addrman (e.g. with `FillAddrman`). - Add coverage for `ASMapHealthCheck`. ACKs for top commit: maflcko: review ACK 552cae2 🏀 dergoegge: Code review ACK 552cae2 marcofleon: Code review ACK 552cae2. Changes match the PR description. Tree-SHA512: ba861c839602054077e4bf3649763eeb48357cda83ca3ddd32b02a1b61f4e44a0c5070182f001f9bf531d0d64717876279a7de3ddb9de028b343533b89233851

As pkg-config is now just redirected to the later, but changing this likely depends on the GHA image being updated first.

fe3457c ci: note that we should install pkgconf in future (fanquake) 8d20348 doc: migrate from pkg-config to pkgconf in macOS build docs (fanquake) Pull request description: Migrate the macOS build docs and CI from `pkg-config` to `pkgconf`. As the former now just redirects to the later. Upstream is currently mass-migrating its formula. i.e Homebrew/homebrew-core#198317. Fixes bitcoin#31334. ACKs for top commit: maflcko: ACK fe3457c 🍭 hebasto: re-ACK fe3457c. Tree-SHA512: 6e337acb6767d163491149b6ae7181d7d7042bc11cdc745eb6f52d4df6d7a19c4f6daa000b314acd9178f97e670aba145f829e48b1b3033117d7e39cdd3af177

92d3d69 fuzz: Implement G_TEST_GET_FULL_NAME (Hodlinator) Pull request description: Catching up to bench & unit tests. Makes for more orderly paths for fuzz tests using `BasicTestingSetup`. ### Before ``` /tmp/test_common bitcoin/0748ae43ef8fa80703bc/regtest/blocks/xor.dat ``` ### After ``` /tmp/test_common bitcoin/tx_pool_standard/f18b3744625e0600eb0c/regtest/blocks/xor.dat ``` ACKs for top commit: kevkevinpal: ACK [92d3d69](bitcoin@92d3d69) furszy: utACK 92d3d69 tdb3: ACK 92d3d69 dergoegge: utACK 92d3d69 brunoerg: code review ACK 92d3d69 Tree-SHA512: 5e83970b111232adece10f79e3a43d0c3c49ab635763e2a4b420f1336cbb8fee94aab751264ddec01ac8363166636e3b29cfe3b2969fc28c8dd6b31bda351950

a0eafc1 functional test: Deduplicate assert_mempool_contents() (Hodlinator) Pull request description: Recently added `mempool_util` implementation probably evolved in parallel with the package RBF one before being submitted as part of ephemeral dust in e2e30e8 (related comments: bitcoin#30239 (comment), bitcoin#31279 (review)). ACKs for top commit: instagibbs: ACK a0eafc1 achow101: ACK a0eafc1 l0rinc: ACK a0eafc1 theStack: ACK a0eafc1 Tree-SHA512: 25ea807d7c041c18be0e4f424131419365d7c1e0fc6c4fb7ac7289c2f8196fd341ff2a2a3ea88df2c3a389edb4571a5fb889efc1b0204c65f7e09ef8f608d0d3

…ed packages - The commit also tests this new behaviour. Co-authored-by: willcl-ark <will@256k1.dev>

- This commit implements `Forecaster` abstract class as the base class of fee rate forecasters. - Derived classes must provide concrete implementation of the virtual methods. Co-authored-by: willcl-ark <will@256k1.dev>

- ForecastType will be used to identify forecasters. - Each time a new forecaster is added, a corresponding enum value should be added to ForecastType. - This allows users to identify which fee estimation strategy was used to make a fee rate estimate.

- Its a module for managing and utilising multiple fee rate forecasters to provide fee estimates. - The FeeEstimator class allows for the registration of multiple fee rate forecasters. Co-authored-by: willcl-ark <will@256k1.dev>

- This commit made CBlockPolicyEstimator member of FeeEstimator class the FeeEstimator will own a pointer to CBlockPolicyEstimator - These does not change any behaviour of CBlockPolicyEstimator. Co-authored-by: willcl-ark <will@256k1.dev>

- This method converts a ForecastType enum to its string representation.

- The CalculatePercentiles function, given a vector of feerates in the order they were added to the block, will return the 25th, 50th, 75th, and 95th percentile feerates. - This function maintains monotonicity by taking the of package with feerates. - Also add a unit test for this function.

- The mempool based fee rate forecaster generate a predicted fee rate estimate for a given confirmation target using the mempool unconfirmed transactions. Co-authored-by: willcl-ark <will@256k1.dev>

- Provide new estimates only when the time delta from previous forecast is older than 30 seconds. - This caching helps avoid the high cost of frequently generating block templates, preventing users from inadvertently calling `estimateFee` repeatedly. Co-authored-by: willcl-ark <will@256k1.dev>

- This commit Added a new method to the fee estimator module. - This method, GetPolicyEstimatorEstimate, returns the fee rate estimate for a given confirmation target. - It provides both conservative and economical mode estimates from CBlockPolicyEstimator, outputting them as low and high fee rate estimates.

- Fallback to Block policy estimator estimates whenever mempool forecaster estimates are higher than block policy estimator.

- Given a confirmation target, we use fee estimator module that call all available fee estimator forcasters and return the lowest fee rate that if a transaction use will likely confirm in a given confirmation target. Co-authored-by: willcl-ark <will@256k1.dev>

Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and: ```bash export CC=clang export CXX=clang++ cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address cmake --build build export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan" ctest --test-dir build ``` ```bash Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms ********* Finished testing of AddressBookTests ********* ================================================================= ==21869==ERROR: LeakSanitizer: detected memory leaks Direct leak of 88 byte(s) in 1 object(s) allocated from: #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872) #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa) #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa) #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18 #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5 #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75 #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13 #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5 bitcoin#8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5 #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7) #10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7) #11 0xffff8cf57c54 (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #12 0xffff8cf5fa18 (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #13 0xffff8cf6067c (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #14 0xffff8cf610a4 (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30 #18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) #19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) #20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872) ``` This happens when building using depends: ```bash Indirect leak of 24 byte(s) in 1 object(s) allocated from: #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #1 0xfbff97f8c164 (<unknown module>) #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) bitcoin#8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o #15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s). ```

5be31b2 lsan: add more Qt suppressions (fanquake) Pull request description: Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and: ```bash export CC=clang export CXX=clang++ cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address cmake --build build export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan" ctest --test-dir build ``` ```bash Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms ********* Finished testing of AddressBookTests ********* ================================================================= ==21869==ERROR: LeakSanitizer: detected memory leaks Direct leak of 88 byte(s) in 1 object(s) allocated from: #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872) #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa) #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa) #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18 #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5 #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75 #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13 #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5 bitcoin#8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5 #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7) #10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7) #11 0xffff8cf57c54 (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #12 0xffff8cf5fa18 (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #13 0xffff8cf6067c (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #14 0xffff8cf610a4 (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd) #17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30 #18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) #19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) #20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872) ``` This happens when building using depends: ```bash Indirect leak of 24 byte(s) in 1 object(s) allocated from: #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #1 0xfbff97f8c164 (<unknown module>) #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) bitcoin#8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5) #14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o #15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7) SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s). ``` ACKs for top commit: maflcko: lgtm ACK 5be31b2 Tree-SHA512: 0c33661c7ec83ea9b874c1ee4ee2de513131690287363e216a88560dfb31a59ef563a50af756c86a991583aa64a600a74e20fd5d6a104cf4c0a27532de8d2211

…xec in RunCommandJSON" faa1c3e Revert "Merge bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke) Pull request description: After a fork() in a multithreaded program, the child can safely call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html)) until such time as it calls execv. The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't. There was an alternative implementation using `readdir` (bitcoin#32529), but that isn't async-signal-safe either, and that implementation was still using `throw`. So temporarily revert this feature. A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach. Fixes bitcoin#32524 Fixes bitcoin#33015 Fixes bitcoin#32855 For reference, a failure can manifest in the GCC debug mode: * While `fork`ing, a debug mode mutex is held (by any other thread). * The `fork`ed child tries to use the stdard libary before `execv` and deadlocks. This may look like the following: ``` (gdb) thread apply all bt Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"): #0 0xf7f4f589 in __kernel_vsyscall () #1 0xf79e467e in ?? () from /lib32/libc.so.6 #2 0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6 #3 0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6 #4 0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6 #5 0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91 #6 0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162 #7 0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539 bitcoin#8 0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687 #9 0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300 #10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372 #11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231 #12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964 #13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27 #14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28 #15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51 ... (truncated, only one thread exists) ``` ACKs for top commit: fanquake: ACK faa1c3e darosior: ACK faa1c3e Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a

willcl-ark commented May 21, 2024

View reviewed changes

ismaelsadeeq force-pushed the new-fee-estimator branch 5 times, most recently from f6748c9 to 6fb2e37 Compare May 29, 2024 11:56

ismaelsadeeq force-pushed the new-fee-estimator branch 4 times, most recently from df7c856 to 94462a4 Compare July 8, 2024 17:54

ismaelsadeeq force-pushed the new-fee-estimator branch 3 times, most recently from 790760e to 84bd850 Compare July 24, 2024 15:56

ismaelsadeeq force-pushed the new-fee-estimator branch 2 times, most recently from c23003e to d9d228d Compare August 13, 2024 15:09

ismaelsadeeq force-pushed the new-fee-estimator branch from d9d228d to 84da9de Compare September 3, 2024 09:23

ismaelsadeeq force-pushed the new-fee-estimator branch from 84da9de to dc92d0c Compare October 14, 2024 15:34

fanquake and others added 12 commits October 25, 2024 11:43

Introduce g_fuzzing global for fuzzing checks

9f243cd

ci: remove UPnP options

94ad614

doc: remove mentions of UPnP

953533d

depends: drop miniupnpc

1b6dec9

doc: add release note for bitcoin#31130

b7b2435

cmake: Add FindZeroMQ module

6b8a744

fanquake and others added 4 commits November 21, 2024 11:08

ci: note that we should install pkgconf in future

fe3457c

As pkg-config is now just redirected to the later, but changing this likely depends on the GHA image being updated first.

ismaelsadeeq force-pushed the new-fee-estimator branch from b26fa5a to 7625166 Compare November 25, 2024 17:07

ismaelsadeeq and others added 20 commits November 25, 2024 13:50

miner: CreateNewBlock will now track fee and virtual size of select…

8541e46

…ed packages - The commit also tests this new behaviour. Co-authored-by: willcl-ark <will@256k1.dev>

===== End of miner commits =====

4603729

fees: add forecast utility structure

7ab86b2

fees: add Forecaster abstract class

6e8ce68

- This commit implements `Forecaster` abstract class as the base class of fee rate forecasters. - Derived classes must provide concrete implementation of the virtual methods. Co-authored-by: willcl-ark <will@256k1.dev>

fees: add ForecastType enum

8ac8e94

- ForecastType will be used to identify forecasters. - Each time a new forecaster is added, a corresponding enum value should be added to ForecastType. - This allows users to identify which fee estimation strategy was used to make a fee rate estimate.

fees: add FeeEstimator class

9d52a2d

- Its a module for managing and utilising multiple fee rate forecasters to provide fee estimates. - The FeeEstimator class allows for the registration of multiple fee rate forecasters. Co-authored-by: willcl-ark <will@256k1.dev>

fees: add forecastTypeToString method

560c02a

- This method converts a ForecastType enum to its string representation.

fees: add MemPoolForecaster class

1a4ace3

- The mempool based fee rate forecaster generate a predicted fee rate estimate for a given confirmation target using the mempool unconfirmed transactions. Co-authored-by: willcl-ark <will@256k1.dev>

test: add MemPoolPolicyEstimator unit test

b5b8146

fees: return current block height in estimateSmartFee

5180964

fees: add GetFeeEstimateFromForecasters method

bb617c9

- Fallback to Block policy estimator estimates whenever mempool forecaster estimates are higher than block policy estimator.

===== End of FeeEstimator commits =====

df49b17

test: test estimatefee rpc

73a28cb

test: add estimatefee functionality test

b6470d6

===== End of FeeEstimator RPC commits =====

4ba028b

ismaelsadeeq force-pushed the new-fee-estimator branch from 7625166 to 4ba028b Compare November 25, 2024 19:35

willcl-ark closed this Nov 26, 2024

[DRAFT] New fee estimator #15

[DRAFT] New fee estimator #15

Uh oh!

Conversation

willcl-ark commented May 21, 2024

Uh oh!

willcl-ark commented May 21, 2024

Uh oh!

willcl-ark 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!

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!

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!

Uh oh!