Disable fee estimation in blocksonly mode (by removing the fee estimates global)

darosior commented at 5:13 pm on April 25, 2020: member

If the blocksonly mode is turned on after running with transaction relay enabled for a while, the fee estimation will serve outdated data to both the internal wallet and to external applications that might be feerate-sensitive and make use of estimatesmartfee (for example a Lightning Network node).

This has already caused issues (for example #16840 (C-lightning), or https://github.com/lightningnetwork/lnd/issues/2562 (LND)) and it seems prudent to fail rather than to give inaccurate values.

This fixes #16840, and closes #16890 which tried to fix the symptoms (RPC) but not the cause as mentioned by sdaftuar :

If this is a substantial problem, then I would think we should take action to protect our own wallet users as well (rather than hide the results of what our fee estimation would do!).

DrahtBot added the label Tests on Apr 25, 2020

DrahtBot commented at 5:36 pm on April 25, 2020: 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:

#20546 (policy, wallet, refactor: check for non-representable CFeeRates by jonatack)
#20323 (tests: Create or use existing properly initialized NodeContexts by dongcarl)
#20228 (addrman: Make addrman a top-level component by jnewbery)
#20217 (net: Remove g_relay_txes by jnewbery)
#19806 (validation: UTXO snapshot activation by jamesob)
#19652 (Avoid locking CTxMemPool::cs recursively in Mempool{Info}ToJSON() by hebasto)
#13990 (Allow fee estimation to work with lower fees by ajtowns)

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.

in src/init.cpp:1756 in 9cf5c47e35 outdated

1749@@ -1750,8 +1750,10 @@ bool AppInitMain(NodeContext& node)
1750 
1751     fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
1752     CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION);
1753-    // Allowed to fail as this file IS missing on first startup.
1754-    if (!est_filein.IsNull())
1755+    // Allowed to fail as this file IS missing on first startup. Also, don't initialize
1756+    // fee estimation with old data if we don't relay transactions, as they would never be
1757+    // updated.
1758+    if (!est_filein.IsNull() && g_relay_txes)

promag commented at 9:20 am on April 27, 2020:

Why read the file then? You could early check like:

0// Don't initialize fee estimation if we don't relay transactions.
1if (!g_relay_txes) {
2    fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
3    CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION);
4    if (!est_filein.IsNull()) ::feeEstimator.Read(est_filein);
5    fFeeEstimatesInitialized = true;
6}

This also avoids writing to estimates file in Shutdown.

darosior commented at 9:45 am on April 27, 2020:

I think

0fFeeEstimatesInitialized = true;

should be set anyway ?

What do you think about this conceptually ?

promag commented at 9:51 am on April 27, 2020:

I think fFeeEstimatesInitialized exists to handle early shutdowns. But if you don’t read estimates then writing will clear it right?

darosior commented at 10:00 am on April 27, 2020:

Hmm right

darosior force-pushed on Apr 27, 2020

darosior commented at 10:28 am on April 27, 2020: member

@promag : addressed your feedback. Would be interested about what do you think about disabling the fee estimation in blocksonly conceptually though.

MarcoFalke removed the label Tests on Apr 27, 2020

MarcoFalke added the label TX fees and policy on Apr 27, 2020

luke-jr commented at 5:37 pm on April 30, 2020: member

Concept ACK / Sounds reasonable.

brakmic commented at 10:48 am on May 1, 2020: contributor

Concept ACK.

laanwj commented at 2:37 pm on May 4, 2020: member

Concept ACK, but I don’t know enough about the underlying mechanism to be sure if this is a reasonable way to accomplish it.

promag commented at 2:48 pm on May 4, 2020: member

Would be interested about what do you think about disabling the fee estimation in blocksonly conceptually though.

Same opinion as above.

MarcoFalke commented at 3:03 pm on May 4, 2020: member

Concept ACK, while the fee estimation should work for some limited time after the mempool has been turned off (see #16840 (comment)), I can see the point to fail early on startup instead of later at runtime.

MarcoFalke commented at 3:04 pm on May 4, 2020: member

Concept ACK 33ca35902435a162c526bf6c8bd82145585dfbd9

luke-jr referenced this in commit 897faedafb on Jun 9, 2020

darosior commented at 9:14 am on June 12, 2020: member

@sdaftuar (ping because you reviewed the previous PR) : what do you think of this approach compared to the previous one ? @morcos (ping because you implemented the fee estimation) : what do you think of it implementation-wise ?

darosior force-pushed on Jul 29, 2020

darosior commented at 4:14 pm on July 29, 2020: member

This was intended to be a “quick fix”, but the approach seems to be too much of a workaround.

I’ve reworked this to opt for a more involved approach based on #19556. This now:

Removes the fee estimator related globals.
Conditionally creates the fee estimator: if we don’t relay transactions we don’t even bother to allocate the CBlockPolicyEstimator.

This is rebased on top of #19556 (which i itself rebased on top of #19604).

darosior force-pushed on Jul 29, 2020

DrahtBot added the label Needs rebase on Jul 30, 2020

darosior force-pushed on Sep 6, 2020

darosior renamed this:
~~Disable fee estimation in blocksonly mode~~
Disable fee estimation in blocksonly mode (by removing the fee estimates global)
on Sep 6, 2020

darosior commented at 8:07 pm on September 6, 2020: member

Rebased on #19556 now that the remove-mempool-global series is almost over.

jonatack commented at 8:16 pm on September 6, 2020: member

Concept ACK, will review after 19556 goes in.

jnewbery commented at 8:31 pm on September 6, 2020: member

Concept ACK

DrahtBot removed the label Needs rebase on Sep 6, 2020

jnewbery commented at 8:52 pm on September 6, 2020: member

Intuitively, I don’t think the mempool should ‘own’ the fee estimator, but rather I see the fee estimator as a client of the mempool. I’d suggest constructing the fee estimator in AppInitMain() and owning the memory in NodeContext, like the other components.

MarcoFalke referenced this in commit 07087051af on Sep 7, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

darosior force-pushed on Sep 7, 2020

DrahtBot removed the label Needs rebase on Sep 7, 2020

darosior commented at 10:04 am on September 8, 2020: member

Intuitively, I don’t think the mempool should ‘own’ the fee estimator, but rather I see the fee estimator as a client of the mempool. I’d suggest constructing the fee estimator in AppInitMain() and owning the memory in NodeContext, like the other components.

It seems more intuitive to me for the mempool to own the fee estimator as the latter can not exist without the former, and it would otherwise be a potentially-uninitialized globally-accessible “second class” object ? I think of this as “the mempool may provide fee estimation”.

jnewbery commented at 10:30 am on September 8, 2020: member

it would otherwise be a potentially-uninitialized globally-accessible “second class” object

That depends on how you write the code. NodeContext holds (mostly) unique ptrs which may or may not be initialized. It’s up to the components to make sure that they’re using a component that is enabled (see, for example, the usage of banman in net_processing).

Fee estimator is currently a separate component from the mempool. Let’s not add unnecessary coupling between the components by moving it inside the mempool. Decoupled components with well defined interfaces make it easier to reason about and test the code.

darosior force-pushed on Sep 8, 2020

darosior commented at 5:07 pm on September 8, 2020: member

Refactored to keep the feeEstimator as a separate component as per @jnewbery request (spoiler: it’s cleaner now, i could even squash the first two commits).

darosior force-pushed on Sep 8, 2020

in src/txmempool.h:582 in 3ac3a2f3c8 outdated

577@@ -577,6 +578,9 @@ class CTxMemPool
578     typedef std::set<txiter, CompareIteratorByHash> setEntries;
579 
580     uint64_t CalculateDescendantMaximum(txiter entry) const EXCLUSIVE_LOCKS_REQUIRED(cs);
581+
582+    void initFeeEstimator(CAutoFile& est_file);

jnewbery commented at 9:02 am on September 9, 2020:

This is unused.

darosior commented at 9:23 am on September 9, 2020:

Argh, leftover from the refactor..

in src/txmempool.h:23 in 3ac3a2f3c8 outdated

19@@ -20,6 +20,7 @@
20 #include <optional.h>
21 #include <policy/feerate.h>
22 #include <primitives/transaction.h>
23+#include <streams.h>

jnewbery commented at 9:04 am on September 9, 2020:

Why is this added?

darosior commented at 9:24 am on September 9, 2020:

Same, was for CAutoFile

in src/init.cpp:238 in 3ac3a2f3c8 outdated

233@@ -235,18 +234,18 @@ void Shutdown(NodeContext& node)
234         DumpMempool(*node.mempool);
235     }
236 
237-    if (fFeeEstimatesInitialized)
238+    if (node.feeEstimator)
239     {

jnewbery commented at 9:07 am on September 9, 2020:

Join lines to fix up with current code style:

if (node.feeEstimator) {

(see https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c)

in src/init.cpp:242 in 3ac3a2f3c8 outdated

241+        node.feeEstimator->FlushUnconfirmed();
242         fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
243         CAutoFile est_fileout(fsbridge::fopen(est_path, "wb"), SER_DISK, CLIENT_VERSION);
244         if (!est_fileout.IsNull())
245-            ::feeEstimator.Write(est_fileout);
246+            node.feeEstimator->Write(est_fileout);

jnewbery commented at 9:07 am on September 9, 2020:

Again, fix up braces when touching adjacent lines.

in src/init.cpp:1376 in 3ac3a2f3c8 outdated

1354@@ -1356,15 +1355,27 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1355     // is not yet setup and may end up being set up twice if we
1356     // need to reindex later.
1357 
1358+    // see Step 2: parameter interactions for more information about these

jnewbery commented at 9:09 am on September 9, 2020:

I think only g_relay_txes needs to be moved up here.

darosior commented at 9:27 am on September 9, 2020:

Yeah but it made sense to keep them all grouped imho

in src/init.cpp:1777 in 3ac3a2f3c8 outdated

1773@@ -1768,9 +1774,8 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1774     fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
1775     CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION);
1776     // Allowed to fail as this file IS missing on first startup.
1777-    if (!est_filein.IsNull())
1778-        ::feeEstimator.Read(est_filein);
1779-    fFeeEstimatesInitialized = true;
1780+    if (node.feeEstimator && !est_filein.IsNull())

jnewbery commented at 9:09 am on September 9, 2020:

use braces for if block.

in src/interfaces/chain.cpp:330 in 3ac3a2f3c8 outdated

326@@ -327,11 +327,13 @@ class ChainImpl : public Chain
327     }
328     CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override
329     {
330-        return ::feeEstimator.estimateSmartFee(num_blocks, calc, conservative);
331+        if (!m_node.feeEstimator) return CFeeRate(0);

jnewbery commented at 9:10 am on September 9, 2020:

I think you can just use default initialization of CFeeRate:

0        if (!m_node.feeEstimator) return {};

in src/node/context.h:40 in 3ac3a2f3c8 outdated

36@@ -36,6 +37,7 @@ class WalletClient;
37 struct NodeContext {
38     std::unique_ptr<CConnman> connman;
39     std::unique_ptr<CTxMemPool> mempool;
40+    std::shared_ptr<CBlockPolicyEstimator> feeEstimator;

jnewbery commented at 9:10 am on September 9, 2020:

Why does this need to be shared? NodeContext should have unique ownership of the feeEstimator object.

darosior commented at 10:04 am on September 9, 2020:

Right, fixed

in src/rpc/mining.cpp:1071 in 3ac3a2f3c8 outdated

1063@@ -1064,7 +1064,13 @@ static RPCHelpMan estimatesmartfee()
1064 {
1065     RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
1066     RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
1067-    unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
1068+
1069+    NodeContext& node = EnsureNodeContext(request.context);
1070+    if (!node.feeEstimator) {
1071+        throw JSONRPCError(RPC_INTERNAL_ERROR, "Fee estimation disabled");
1072+    }

jnewbery commented at 9:12 am on September 9, 2020:

How about extracting this to an EnsureFeeEstimator() function? (see EnsureMemPool and EnsureChainman)

darosior commented at 9:29 am on September 9, 2020:

I thought EnsureXX() were used for components that should not be absent, while the fee estimator may be ?

jnewbery commented at 9:37 am on September 9, 2020:

I think it’s fine to add EnsureXX() for components that may not be enabled. Does that sound right to you @MarcoFalke ?

MarcoFalke commented at 10:02 am on September 9, 2020:

It doesn’t make sense to call estimatesmartfee without the fee estimator. So yes, EnsureFeeEstimator is the way to go right now. (In the future the fee estimator can register the RPC itself, so when it is missing, the RPC won’t even be there)

darosior commented at 10:15 am on September 9, 2020:

In the future the fee estimator can register the RPC itself, so when it is missing, the RPC won’t even be there

Hmm that would be particularly nice, as for example applications up the stack relying on fee estimation would be able to just error at startup rather than “check and see if it errors”. Might propose a follow-up implementing this.

in src/txmempool.cpp:334 in 3ac3a2f3c8 outdated

330@@ -331,7 +331,7 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee,
331     assert(int(nSigOpCostWithAncestors) >= 0);
332 }
333 
334-CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator)
335+CTxMemPool::CTxMemPool(std::shared_ptr<CBlockPolicyEstimator> estimator)

jnewbery commented at 9:15 am on September 9, 2020:

You don’t need to make this change. The fee estimator’s memory/lifetime is owned by NodeContext. This can continue being a raw pointer to that object (in the same way that PeerManager holds pointers and references to other components).

Not making this change should reduce the diff of this PR considerably.

in src/init.cpp:1362 in 3ac3a2f3c8 outdated

1366     node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
1367 
1368+    // Don't initialize fee estimation with old data if we don't relay transactions,
1369+    // as they would never get updated.
1370+    assert(!node.feeEstimator);
1371+    if (g_relay_txes) {

jnewbery commented at 9:19 am on September 9, 2020:

Consider joining into one line for simple one-statement if blocks:

0    if (g_relay_txes) node.feeEstimator = std::make_shared<CBlockPolicyEstimator>();

in src/init.cpp:1372 in 3ac3a2f3c8 outdated

1367 
1368+    // Don't initialize fee estimation with old data if we don't relay transactions,
1369+    // as they would never get updated.
1370+    assert(!node.feeEstimator);
1371+    if (g_relay_txes) {
1372+        node.feeEstimator = std::make_shared<CBlockPolicyEstimator>();

jnewbery commented at 9:20 am on September 9, 2020:

You’re constructing the fee estimator here. You should destruct it on shutdown. At line 304 (after the node.mempool unique ptr is reset), you should reset this ptr too.

darosior commented at 9:31 am on September 9, 2020:

Dropped this somehow.. Fixing now.

in test/functional/feature_fee_estimation.py:270 in 3ac3a2f3c8 outdated

263@@ -263,6 +264,13 @@ def run_test(self):
264         self.log.info("Final estimates after emptying mempools")
265         check_estimates(self.nodes[1], self.fees_per_kb)
266 
267+        self.log.info("Testing that fee estimation is disabled in blocksonly.")
268+        self.stop_nodes()
269+        self.start_nodes(extra_args=[["-blocksonly"]] * len(self.nodes))
270+        for node in self.nodes:

jnewbery commented at 9:21 am on September 9, 2020:

You only need to test this on one node.

darosior commented at 10:20 am on September 9, 2020:

Thanks, amended to only start and test on one node.

darosior force-pushed on Sep 9, 2020

in src/init.cpp:1793 in 6e33130190 outdated

1773@@ -1768,10 +1774,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1774     fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
1775     CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION);
1776     // Allowed to fail as this file IS missing on first startup.
1777-    if (!est_filein.IsNull())
1778-        ::feeEstimator.Read(est_filein);
1779-    fFeeEstimatesInitialized = true;

jnewbery commented at 11:39 am on September 9, 2020:

I think this flag might protect us from overwriting the estimates.dat file in case we abort start-up for some reason. Rather than removing it entirely, I think it should be moved to CBlockPolicyEstimator, and then checked on destruction.

If you really wanted to encaptulate this better, you could move this file read logic to the feeEstimator constructor, and the flush logic to a public flush method in CBlockPolicyEstimator, and hide all the construction/destruction logic from init.cpp.

darosior commented at 1:49 pm on September 9, 2020:

Good catch. Went for the encapsulation way, will push a commit.

jnewbery commented at 3:54 pm on September 9, 2020:

Moving this code to the destructor may be a behaviour change, since there will have been changes to the mempool in the intervening period. I suggest adding a public flush method and calling it here to avoid the behaviour change.

darosior commented at 4:16 pm on September 9, 2020:

Yeah, i initially swapped the two calls to reset(), but as you prefer.

in src/node/context.h:40 in 6e33130190 outdated

36@@ -36,6 +37,7 @@ class WalletClient;
37 struct NodeContext {
38     std::unique_ptr<CConnman> connman;
39     std::unique_ptr<CTxMemPool> mempool;
40+    std::unique_ptr<CBlockPolicyEstimator> feeEstimator;

MarcoFalke commented at 12:56 pm on September 9, 2020:

0    std::unique_ptr<CBlockPolicyEstimator> fee_estimator;

for new code

in src/rpc/mining.cpp:1068 in 6e33130190 outdated

1063@@ -1064,7 +1064,10 @@ static RPCHelpMan estimatesmartfee()
1064 {
1065     RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR});
1066     RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
1067-    unsigned int max_target = ::feeEstimator.HighestTargetTracked(FeeEstimateHorizon::LONG_HALFLIFE);
1068+
1069+    CBlockPolicyEstimator& feeEstimator = EnsureFeeEstimator(request.context);

MarcoFalke commented at 12:58 pm on September 9, 2020:

0    CBlockPolicyEstimator& fee_estimator = EnsureFeeEstimator(request.context);

snake case for new code ;)

darosior commented at 2:26 pm on September 9, 2020:

Thanks! Amended

MarcoFalke commented at 12:58 pm on September 9, 2020: member

Concept ACK

darosior force-pushed on Sep 9, 2020

darosior commented at 3:35 pm on September 9, 2020: member

Amended to use snake case, and added a commit to encapsulate the management of the fee estimation file in CBlockPolicyEstimator.

in src/policy/fees.cpp:905 in 941806deef outdated

901@@ -886,8 +902,14 @@ bool CBlockPolicyEstimator::Write(CAutoFile& fileout) const
902     return true;
903 }
904 
905-bool CBlockPolicyEstimator::Read(CAutoFile& filein)
906-{
907+bool CBlockPolicyEstimator::ReadEstFile() {

jnewbery commented at 3:53 pm on September 9, 2020:

You don’t need a separate function for this since it’s only called in one place. You can just insert this code into the constructor.

in src/init.cpp:1363 in 941806deef outdated

1355     node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
1356 
1357+    // Don't initialize fee estimation with old data if we don't relay transactions,
1358+    // as they would never get updated.
1359+    if (g_relay_txes) {
1360+        fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;

jnewbery commented at 3:55 pm on September 9, 2020:

Can you move this line inside the constructor and avoid changing the signature of the constructor?

darosior commented at 4:09 pm on September 9, 2020:

Thought it would be better to keep the filename constants in init ?..

jnewbery commented at 4:25 pm on September 9, 2020:

No need. If it’s only used in policy/fees.cpp you can move the constant there.

darosior commented at 1:05 pm on September 10, 2020:

Ok, done.

darosior force-pushed on Sep 9, 2020

sidhujag referenced this in commit 5cab891acb on Sep 9, 2020

jnewbery commented at 9:47 am on September 10, 2020: member

This is very confusing. In your latest branch you’re now calling FlushUnconfirmed() twice, which is definitely a behaviour change. I don’t think you need to change the dtor at all, or the interface for the ctor.

See https://github.com/jnewbery/bitcoin/tree/pr18766.1. The only thing that has to change is a new flush() function is added and some logic is moved inside the CBlockPolicyEstimator class.

darosior commented at 10:42 am on September 10, 2020: member

This is very confusing. In your latest branch you’re now calling FlushUnconfirmed() twice, which is definitely a behaviour change. I don’t think you need to change the dtor at all, or the interface for the ctor.

I pushed in a hurry yesterday, and noticed this afterwards. Will fix this and the rest as soon as possible, but want to take some time for that to avoid wasting your time (and potentially others’ too) again by pushing half-baked patches. Coming back to it soon (:tm:)

darosior force-pushed on Sep 10, 2020

darosior commented at 1:10 pm on September 10, 2020: member

Ok, it should be in a reviewable state now. This:

Moves the fee estimator from a global to the node context.
Encapsulates the fee_estimates.dat logic in CBlockPolicyEstimator.
Avoid creating the fee estimator at all if we don’t relay transactions.

in src/policy/fees.cpp:528 in d84b4b9d66 outdated

523+    // If the fee estimation file is present, record current estimations
524+    fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME;
525+    if (est_filepath.empty()) return;
526+
527+    CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION);
528+    if (est_file.IsNull() || !Write(est_file)) {

jnewbery commented at 1:27 pm on September 10, 2020:

Why are you moving this to the destructor? Are we sure that nothing happens to the fee estimator’s state between https://github.com/bitcoin/bitcoin/pull/18766/files#diff-c865a8939105e6350a50af02766291b7R236 and https://github.com/bitcoin/bitcoin/pull/18766/files#diff-c865a8939105e6350a50af02766291b7R294?

darosior commented at 3:01 pm on September 10, 2020:

Right. What do you think of instead exposing a RecordEstimation() method to the CBlockPolicyEstimator ?

jnewbery commented at 4:18 pm on September 10, 2020:

Was there a reason you didn’t like the flush() method in https://github.com/jnewbery/bitcoin/tree/pr18766.1?

darosior commented at 4:32 pm on September 10, 2020:

Ok didn’t see this. Had a version with both the FlushUnconfirmed() and RecordEstimation() separated locally. Grouping them and pushing in a minute !

darosior force-pushed on Sep 10, 2020

darosior force-pushed on Sep 11, 2020

jnewbery commented at 12:55 pm on September 11, 2020: member

utACK 4105c63a67a40fe109b2f2c020c9fbf25d1fc250

If you retouch this, you can remove the policy/fees.h #include in validation.cpp:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 86d18a9d7f..7d60dd6e20 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -22,7 +22,6 @@
 5 #include <logging/timer.h>
 6 #include <node/ui_interface.h>
 7 #include <optional.h>
 8-#include <policy/fees.h>
 9 #include <policy/policy.h>
10 #include <policy/settings.h>
11 #include <pow.h>

fjahr commented at 12:28 pm on September 12, 2020: member

Code review ACK 4105c63a67a40fe109b2f2c020c9fbf25d1fc250

If you retouch you could also check the description of 4105c63a67a40fe109b2f2c020c9fbf25d1fc250, which saying logic has move to the destructor while it is actually in the Flush() method now.

jnewbery commented at 9:15 am on October 21, 2020: member

~Oh, one more thing - you could make the git commit logs just a little more verbose. No need to write an essay, but just a short justification for the changes.~

EDIT: oops. I left this comment on the wrong PR. The commit logs look fine. Sorry for the noise.

darosior commented at 9:19 am on October 21, 2020: member

Yeah, did not want to invalidate the ACKs but now it didn’t make it in before the feature freeze, i’ll address both your comments.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ Le mercredi, octobre 21, 2020 11:16 AM, John Newbery notifications@github.com a écrit :

Oh, one more thing - you could make the git commit logs just a little more verbose. No need to write an essay, but just a short justification for the changes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

DrahtBot added the label Needs rebase on Oct 21, 2020

darosior force-pushed on Oct 22, 2020

DrahtBot removed the label Needs rebase on Oct 22, 2020

in src/init.cpp:1392 in c3c145b72b outdated

1392     assert(!node.banman);
1393     node.banman = MakeUnique<BanMan>(GetDataDir() / "banlist.dat", &uiInterface, args.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME));
1394     assert(!node.connman);
1395     node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
1396 
1397-    assert(!node.fee_estimator);

jnewbery commented at 11:28 am on October 22, 2020:

I think you should keep this assert.

darosior commented at 11:36 am on October 22, 2020:

Good catch, will re-add it if i have to retouch.

jnewbery commented at 12:03 pm on October 22, 2020:

Just re-add it now. I’m the only person who’s ACKed the latest head and I’m happy to reACK.

darosior commented at 12:05 pm on October 22, 2020:

Ok, done.

jnewbery commented at 12:23 pm on October 22, 2020:

Thanks!

darosior commented at 11:30 am on October 22, 2020: member

Rebased. Addressed @jnewbery and @fjahr nits.

jnewbery commented at 11:30 am on October 22, 2020: member

utACK 4105c63a67a40fe109b2f2c020c9fbf25d1fc250

darosior force-pushed on Oct 22, 2020

jnewbery commented at 12:23 pm on October 22, 2020: member

utACK 4aaad74c4c8a24c4b67af4e92c29805b923f2225

fjahr commented at 11:06 am on October 25, 2020: member

Code review ACK 4aaad74c4c8a24c4b67af4e92c29805b923f2225

ellemouton commented at 10:03 am on October 31, 2020: contributor

Hi @darosior :) quick question: Will serving outdated fee data not also be a problem if the node has been off for a while and the fee estimation files are just stale in general? i.e would it not perhaps be useful to look at ’last-updated’ time on the data and then starting fresh if the data is too old?

darosior commented at 11:42 am on October 31, 2020: member

Hi @ellemouton, fortunately we discard these estimations already. If the node has been off for a while, we’ll exponentially decay the estimation as we connect new blocks :).

EDIT: to give slightly more context, if however the node is turned back on after a short period of time the estimation will instead be relatively quickly updated as we are sent new unconfirmed transaction. This PR fixes the edge case introduced by blocksonly (which was turned into a bug by fee sensitive applications such as the Lightning Network): the estimation isn’t yet decayed and we told our peer not to send us unconfirmed transactions, so we are stuck we the stalled estimation!

ellemouton commented at 1:50 pm on October 31, 2020: contributor

Aha! That makes sense. Thanks for the context @darosior 😄

luke-jr referenced this in commit 700493d8ae on Nov 25, 2020

in src/init.cpp:1793 in cc21167415 outdated

1797@@ -1795,10 +1798,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1798     fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
1799     CAutoFile est_filein(fsbridge::fopen(est_path, "rb"), SER_DISK, CLIENT_VERSION);
1800     // Allowed to fail as this file IS missing on first startup.
1801-    if (!est_filein.IsNull())
1802-        ::feeEstimator.Read(est_filein);
1803-    fFeeEstimatesInitialized = true;

MarcoFalke commented at 7:53 am on December 1, 2020:

cc21167415aa01db862035cd07b6b9b35fb53f35

This is not a refactor. Shutting down before this line will wipe the estimates on disk

darosior commented at 10:57 am on December 1, 2020:

Right, removed the “refactor” prefix from the commit.

in src/init.cpp:1393 in cc21167415 outdated

1388@@ -1389,10 +1389,13 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1389     assert(!node.connman);
1390     node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
1391 
1392+    assert(!node.fee_estimator);
1393+    node.fee_estimator = MakeUnique<CBlockPolicyEstimator>();

MarcoFalke commented at 8:24 am on December 1, 2020:

cc21167415aa01db862035cd07b6b9b35fb53f35

would be nice if new code used std::make_unique instead of the deprecated wrapper

(needs rebase for that)

darosior commented at 10:57 am on December 1, 2020:

Included, had to rebase anyway. Used it for CTxMemPool too as it was free diff-wise.

in src/test/util/setup_common.cpp:145 in cc21167415 outdated

141@@ -141,7 +142,9 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
142 
143     pblocktree.reset(new CBlockTreeDB(1 << 20, true));
144 
145-    m_node.mempool = MakeUnique<CTxMemPool>(&::feeEstimator);
146+    m_node.fee_estimator = MakeUnique<CBlockPolicyEstimator>();

MarcoFalke commented at 8:25 am on December 1, 2020:

same

in src/init.cpp:238 in cc21167415 outdated

234@@ -236,18 +235,18 @@ void Shutdown(NodeContext& node)
235         DumpMempool(*node.mempool);
236     }
237 
238-    if (fFeeEstimatesInitialized)
239-    {
240-        ::feeEstimator.FlushUnconfirmed();
241+    if (node.feeEstimator) {

MarcoFalke commented at 8:26 am on December 1, 2020:

cc21167415aa01db862035cd07b6b9b35fb53f35

 0  CXX      libbitcoin_server_a-init.o
 1init.cpp:238:14: error: no member named 'feeEstimator' in 'NodeContext'; did you mean 'fee_estimator'?
 2    if (node.feeEstimator) {
 3             ^~~~~~~~~~~~
 4             fee_estimator
 5./node/context.h:40:44: note: 'fee_estimator' declared here
 6    std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
 7                                           ^
 8init.cpp:239:14: error: no member named 'feeEstimator' in 'NodeContext'; did you mean 'fee_estimator'?
 9        node.feeEstimator->FlushUnconfirmed();
10             ^~~~~~~~~~~~
11             fee_estimator
12./node/context.h:40:44: note: 'fee_estimator' declared here
13    std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
14                                           ^
15init.cpp:243:18: error: no member named 'feeEstimator' in 'NodeContext'; did you mean 'fee_estimator'?
16            node.feeEstimator->Write(est_fileout);
17                 ^~~~~~~~~~~~
18                 fee_estimator
19./node/context.h:40:44: note: 'fee_estimator' declared here
20    std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
21                                           ^
22init.cpp:1801:14: error: no member named 'feeEstimator' in 'NodeContext'; did you mean 'fee_estimator'?
23    if (node.feeEstimator && !est_filein.IsNull()) {
24             ^~~~~~~~~~~~
25             fee_estimator
26./node/context.h:40:44: note: 'fee_estimator' declared here
27    std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
28                                           ^
29init.cpp:1802:14: error: no member named 'feeEstimator' in 'NodeContext'; did you mean 'fee_estimator'?
30        node.feeEstimator->Read(est_filein);
31             ^~~~~~~~~~~~
32             fee_estimator
33./node/context.h:40:44: note: 'fee_estimator' declared here
34    std::unique_ptr<CBlockPolicyEstimator> fee_estimator;
35                                           ^
365 errors generated.

darosior commented at 10:57 am on December 1, 2020:

Fixed..

in src/interfaces/node.cpp:212 in cc21167415 outdated

206@@ -207,8 +207,9 @@ class NodeImpl : public Node
207     bool getNetworkActive() override { return m_context->connman && m_context->connman->GetNetworkActive(); }
208     CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override
209     {
210+        if (!m_context->fee_estimator) return {};
211         FeeCalculation fee_calc;
212-        CFeeRate result = ::feeEstimator.estimateSmartFee(num_blocks, &fee_calc, conservative);
213+        CFeeRate result = m_context->fee_estimator->estimateSmartFee(num_blocks, &fee_calc, conservative);

MarcoFalke commented at 8:27 am on December 1, 2020:

cc21167415aa01db862035cd07b6b9b35fb53f35

this is unused. Instead of shuffling code around, you may add a commit that removes it.

 0diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
 1index 2c5f8627e6..a9ca4f9cf8 100644
 2--- a/src/interfaces/node.cpp
 3+++ b/src/interfaces/node.cpp
 4@@ -205,15 +205,6 @@ public:
 5         }
 6     }
 7     bool getNetworkActive() override { return m_context->connman && m_context->connman->GetNetworkActive(); }
 8-    CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override
 9-    {
10-        FeeCalculation fee_calc;
11-        CFeeRate result = ::feeEstimator.estimateSmartFee(num_blocks, &fee_calc, conservative);
12-        if (returned_target) {
13-            *returned_target = fee_calc.returnedTarget;
14-        }
15-        return result;
16-    }
17     CFeeRate getDustRelayFee() override { return ::dustRelayFee; }
18     UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override
19     {
20diff --git a/src/interfaces/node.h b/src/interfaces/node.h
21index 5079be038e..36f76aeb4f 100644
22--- a/src/interfaces/node.h
23+++ b/src/interfaces/node.h
24@@ -151,9 +151,6 @@ public:
25     //! Get network active.
26     virtual bool getNetworkActive() = 0;
27 
28-    //! Estimate smart fee.
29-    virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) = 0;
30-
31     //! Get dust relay fee.
32     virtual CFeeRate getDustRelayFee() = 0;
33

darosior commented at 10:57 am on December 1, 2020:

Right, i didn’t notice.. Cherry-picked your patch and adapted it to the new interface path post-rebase.

 0Author: Antoine Poinsot <darosior@protonmail.com>
 1Date:   Tue Dec 1 11:24:33 2020 +0100
 2
 3    interface: remove unused estimateSmartFee method from node
 4    
 5    Co-Authored-by: MarcoFalke <falke.marco@gmail.com>
 6    Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
 7
 8diff --git a/src/interfaces/node.h b/src/interfaces/node.h
 9index 5079be038..36f76aeb4 100644
10--- a/src/interfaces/node.h
11+++ b/src/interfaces/node.h
12@@ -151,9 +151,6 @@ public:
13     //! Get network active.
14     virtual bool getNetworkActive() = 0;
15 
16-    //! Estimate smart fee.
17-    virtual CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) = 0;
18-
19     //! Get dust relay fee.
20     virtual CFeeRate getDustRelayFee() = 0;
21 
22diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
23index 77a5957a5..3cd9c6fda 100644
24--- a/src/node/interfaces.cpp
25+++ b/src/node/interfaces.cpp
26@@ -221,15 +221,6 @@ public:
27         }
28     }
29     bool getNetworkActive() override { return m_context->connman && m_context->connman->GetNetworkActive(); }
30-    CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override
31-    {
32-        FeeCalculation fee_calc;
33-        CFeeRate result = ::feeEstimator.estimateSmartFee(num_blocks, &fee_calc, conservative);
34-        if (returned_target) {
35-            *returned_target = fee_calc.returnedTarget;
36-        }
37-        return result;
38-    }
39     CFeeRate getDustRelayFee() override { return ::dustRelayFee; }
40     UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) override
41     {

MarcoFalke changes_requested

MarcoFalke commented at 8:28 am on December 1, 2020: member

Concept ACK cc21167415aa01db862035cd07b6b9b35fb53f35 🚟

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Concept ACK cc21167415aa01db862035cd07b6b9b35fb53f35  🚟
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUilvAv/d0DZXa+aEjrm/fPzFOmqe/7bGZZTctNR8VQxeQ8DE+Ft7Q7zWo4bvHYl
 8YeB42SZeTofhU5+ICOGsmxytQ52FA0bPvOJXGH/8zQ6wIvrdTxAhjt5MP3YunOy9
 9mt8qOOMtLi/RybMV4VgEPDqYZRz27s6LuUxgbgK8YJ5q981pp15LhQ5e0Au4duU0
10MQfqwZIqV7L7H5DEcLo/VeN4hVurII5vLg3PK1a56APtW2MKi4lPx5kS17TuAZY5
1112kew9JuoaGVrAJKHHzXme0Q+yCwOgptZ1rVEhEYsB11aRG9kkThXvO+oo+oPtc6
12SdcGtilVdnfE6x/VNEiD7VTxWgbQ5BUUAPoAoWIrOMDsWB15rcw/dtlAXoHF8JFR
13pOJUCK4racs+TPjfwGtAVpjBmqiebzZFNIRPjizMDCKmxEnMCLNNX2l3jR3U3x8M
14ObOSwoIIF8W384rupBCjF2+aVnmOsJ2ktx8AFdoNX1NdAs81f0NIfOyoW4hfOl4N
157l5DRdLQ
16=6uOX
17-----END PGP SIGNATURE-----

Timestamp of file with hash b08551e34b42f9bb8f8e283ece496e30ae7d418c171ecf636076b60c63a83df6 -

DrahtBot added the label Needs rebase on Dec 1, 2020

darosior force-pushed on Dec 1, 2020

DrahtBot removed the label Needs rebase on Dec 1, 2020

jnewbery commented at 12:07 pm on December 1, 2020: member

utACK 01e39989f23b26a23cea26e5ec4d8dfc2010bc97

in test/functional/feature_fee_estimation.py:269 in 8527183fde outdated

262@@ -262,6 +263,13 @@ def run_test(self):
263         self.log.info("Final estimates after emptying mempools")
264         check_estimates(self.nodes[1], self.fees_per_kb)
265 
266+        self.log.info("Testing that fee estimation is disabled in blocksonly.")
267+        self.stop_nodes()
268+        self.nodes[0].start(extra_args=["-blocksonly"])
269+        self.nodes[0].wait_for_rpc_connection()

MarcoFalke commented at 12:05 pm on December 2, 2020:

can be written shorter with self.restart_node(1, ["-bla"])

darosior commented at 11:03 am on December 3, 2020:

Done, thanks

in src/policy/fees.cpp:509 in 01e39989f2 outdated

502@@ -500,6 +503,15 @@ CBlockPolicyEstimator::CBlockPolicyEstimator()
503     feeStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE));
504     shortStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE));
505     longStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
506+
507+    // If the fee estimation file is present, read recorded estimations
508+    fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME;
509+    if (est_filepath.empty()) return;

MarcoFalke commented at 12:09 pm on December 2, 2020:

can you explain what this line of code is doing and why it is needed?

darosior commented at 11:03 am on December 3, 2020:

Well, there is a comment just above.. If the file isn’t present (likely first start), don’t report a failure to read fee estimates: it’s expected. Kind of the echo of the // Allowed to fail as this file IS missing on first startup. comment in the red part of the diff.

Thanks, you however ringed a bell in my head: this check should not be present in Flush or the file would never be created!

MarcoFalke commented at 11:17 am on December 3, 2020:

what I mean is that a path representation that is (not) empty (https://en.cppreference.com/w/cpp/filesystem/path/empty) doesn’t imply anything about the presence of a file.

You can think of fs::path as a “fancy string”. empty means the string is empty. non-empty means the string is non-empty, but it doesn’t imply the file exists (or even any of the parent folders)

darosior commented at 11:41 am on December 3, 2020:

Thanks.. That was clearly stupid.. Removed as i don’t think i can use C++17’s fs::exists() instead?

in src/policy/fees.cpp:513 in 01e39989f2 outdated

508+    fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME;
509+    if (est_filepath.empty()) return;
510+
511+    CAutoFile est_file(fsbridge::fopen(est_filepath, "rb"), SER_DISK, CLIENT_VERSION);
512+    if (est_file.IsNull() || !Read(est_file)) {
513+        LogPrintf("%s: Failed to read fee estimates from %s\n", __func__, est_filepath.string());

MarcoFalke commented at 12:13 pm on December 2, 2020:

Why is __func__ needed here? It should already be possible to find the function name with git grep -W "Failed to read fee est".

See also #19809 which makes this redundant.

Also the log message could be suffixed with ". Continue anyway.\n" to clarify that this failure does not hinder program execution and is normally hit on first start.

darosior commented at 11:03 am on December 3, 2020:

is normally hit on first start.

~~It’s not, on first start we would have returned just above.~~ EDIT: i’m wrong, it’s not properly checked!

Removed the __func__.

MarcoFalke commented at 11:51 am on December 3, 2020:

Also the log message could be suffixed with “. Continue anyway.\n” to clarify that this failure does not hinder program execution and is normally hit on first start.

Has this been addressed or responded to?

darosior commented at 12:07 pm on December 3, 2020:

No, this was missed. Done now.

in src/policy/fees.cpp:879 in 01e39989f2 outdated

874+    fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME;
875+    if (est_filepath.empty()) return;
876+
877+    CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION);
878+    if (est_file.IsNull() || !Write(est_file)) {
879+        LogPrintf("%s: Failed to write fee estimates to %s\n", __func__, est_filepath.string());

MarcoFalke commented at 3:45 pm on December 2, 2020:

same

darosior commented at 11:03 am on December 3, 2020:

Removed

MarcoFalke approved

MarcoFalke commented at 3:47 pm on December 2, 2020: member

left some style nits. Let me know if you want to fix them or get this merged instead.

review ACK 01e39989f23b26a23cea26e5ec4d8dfc2010bc97 🐪

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 01e39989f23b26a23cea26e5ec4d8dfc2010bc97 🐪
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhJMgwAwnGF/r7ezo/CimEvIuOfeD6LzJ7NDb6SHs5vwyofUN2w8UojfZ7MxuQV
 89kGtWH4oHtzNU5732r2HYPfAGWZI6iJe8HAnZY5fjzC0KoN8PLY2DJ97lS9/rWnb
 95Dco0RD4r+g0ZVFnN+mfMrQ47nmDHCiULNP0nbav40iXlWS8IQiZiNNJVKInW0q7
10w5M3e3I1Np2PQ3lEMqMJ4An2Dt+WAtHLeA9OYB4b0yoXIc4JtYiiQ/mrogEqkAv/
11tWal6S7ElJGmuL+/ceXNg6xXqvKpPT6U4yNxcKR9s/ceyhEngaOt7EmDWWRVVXt7
12PkV29eLM4TYmPbasK6dIUei2q+xtDWjzKFgAuMe83+kWDyb8+oqNxZi140xHVG2P
13AYlbfKD4JnoGB32tEfzgh/Z9dQj4xG+mWhLYL8I7oUDU/SkC3blBmpwPiSHxmvA8
14NWabgQshHcDeKWiw3GpZs6BbMjI6xJnq9Dq8kOmYX5Hz6uT3mIJ76KN31WRHFUfC
156kbMbc1m
16=5+en
17-----END PGP SIGNATURE-----

Timestamp of file with hash 532e63b9bee4dd7e9da189eb2890bcc77d9c1f37ba300a2a322c9d027b0c9158 -

darosior force-pushed on Dec 3, 2020

darosior commented at 11:17 am on December 3, 2020: member

Addressed all @MarcoFalke ’s remaining nits.

practicalswift commented at 11:32 am on December 3, 2020: contributor

Concept ACK

darosior force-pushed on Dec 3, 2020

interface: remove unused estimateSmartFee method from node

Co-Authored-by: MarcoFalke <falke.marco@gmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

03bfeee957

Remove the remaining fee estimation globals

This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

86ff2cf202

init: don't create a CBlockPolicyEstimator if we don't relay transactions

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

e8ea6ad9c1

feestimator: encapsulate estimation file logic

This moves the fee_estimates file management to the CBlockPolicyEstimator
Flush() method.

Co-authored-by: John Newbery <john@johnnewbery.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

4e28753f60

darosior force-pushed on Dec 3, 2020

MarcoFalke commented at 7:55 am on December 7, 2020: member

re-ACK 4e28753f60 👋 only changes:

Small test fixup (restart_node)
Small log fixups
Remove dead code (empty path check)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 4e28753f60 👋
 4only changes:
 5* Small test fixup (restart_node)
 6* Small log fixups
 7* Remove dead code (empty path check)
 8-----BEGIN PGP SIGNATURE-----
 9
10iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
11pUiivwwAvP/ngY1HjLgcIUIL6koFzy3aWPRMQhVCyB4gj6n7rHQoETxZZKMrgk4Z
12soRCvS8dCX1B9OMgJ7B4iBcyX2ywfw7nAA4PwUSPMJLbNitqzPkpxuG1RNLtLfwt
134loau6a5ZZJlKYhLoR8Q6NEdYwCC/OfpZYRIjlQd2+waGdaki1/G86Q6LKGe5xAZ
14TM4VW89RpnHofXm5zxJZq3CiUExZMMSGVMwp+4eGpk0h9/eeYoKr1IGFh7qGzTSe
15X5U/t5z6EmygD3gL1QaVWSb73TTeaU2Iux3U7JR9pYJgK4dNjZr3WKTCY7NJORqv
16kCcW3ZHL4E3w8Oz9hnU6uSHsj+xOONTRxffHzsP7e1nkYTRSqEjqAohO1u2jL0Ho
172FSyXyDtp2uC0mn64JW0duVZI88FT6fRqyt/HiUHvnFzLSdTg90mDMtWMF/qQcb2
18jXpEvjaKGI129CgdlHRxVpUFmAK6bOdHxrx9fgcwD1L6hHo1PQX8N59ulLP2uano
19g2unQ65X
20=AMhQ
21-----END PGP SIGNATURE-----

Timestamp of file with hash 3d9e3dc48bc4d73872a862f7eccfe39d2af942484475d2f8af25b52833aa2031 -

jnewbery commented at 11:22 am on December 7, 2020: member

utACK 4e28753f60613ecd35cdef87bef5f99c302c3fbd

MarcoFalke merged this on Dec 7, 2020

MarcoFalke closed this on Dec 7, 2020

sidhujag referenced this in commit ef40c74069 on Dec 7, 2020

darosior deleted the branch on Jan 12, 2021

Fabcien referenced this in commit 23b6ca0b1e on Mar 15, 2022

DrahtBot locked this on Aug 16, 2022

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Conflicts