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

darosior · 2020-04-25T17:13:47Z

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 lightningnetwork/lnd#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 · 2020-04-25T17:36:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

src/init.cpp

darosior · 2020-04-27T10:28:51Z

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

luke-jr · 2020-04-30T17:37:15Z

Concept ACK / Sounds reasonable.

brakmic · 2020-05-01T10:48:18Z

Concept ACK.

laanwj · 2020-05-04T14:37:53Z

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 · 2020-05-04T14:48:16Z

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

Same opinion as above.

maflcko · 2020-05-04T15:03:14Z

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.

maflcko · 2020-05-04T15:04:25Z

Concept ACK 33ca359

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). Signed-off-by: Antoine Poinsot <darosior@protonmail.com> Github-Pull: bitcoin#18766 Rebased-From: 33ca359

darosior · 2020-06-12T09:14:42Z

@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 · 2020-07-29T16:14:47Z

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).

maflcko

Concept ACK cc21167 🚟

Show signature and timestamp

Signature:

Timestamp of file with hash b08551e34b42f9bb8f8e283ece496e30ae7d418c171ecf636076b60c63a83df6 -

src/init.cpp

src/test/util/setup_common.cpp

src/init.cpp

src/interfaces/node.cpp

jnewbery · 2020-12-01T12:07:51Z

utACK 01e3998

maflcko

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

review ACK 01e3998 🐪

Show signature and timestamp

Signature:

Timestamp of file with hash 532e63b9bee4dd7e9da189eb2890bcc77d9c1f37ba300a2a322c9d027b0c9158 -

test/functional/feature_fee_estimation.py

src/policy/fees.cpp

darosior · 2020-12-03T11:17:06Z

Addressed all @MarcoFalke 's remaining nits.

practicalswift · 2020-12-03T11:32:43Z

Concept ACK

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

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>

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

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>

maflcko · 2020-12-07T07:55:10Z

re-ACK 4e28753 👋
only changes:

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 4e28753f60 👋
only changes:
* Small test fixup (restart_node)
* Small log fixups
* Remove dead code (empty path check)
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiivwwAvP/ngY1HjLgcIUIL6koFzy3aWPRMQhVCyB4gj6n7rHQoETxZZKMrgk4Z
soRCvS8dCX1B9OMgJ7B4iBcyX2ywfw7nAA4PwUSPMJLbNitqzPkpxuG1RNLtLfwt
4loau6a5ZZJlKYhLoR8Q6NEdYwCC/OfpZYRIjlQd2+waGdaki1/G86Q6LKGe5xAZ
TM4VW89RpnHofXm5zxJZq3CiUExZMMSGVMwp+4eGpk0h9/eeYoKr1IGFh7qGzTSe
X5U/t5z6EmygD3gL1QaVWSb73TTeaU2Iux3U7JR9pYJgK4dNjZr3WKTCY7NJORqv
kCcW3ZHL4E3w8Oz9hnU6uSHsj+xOONTRxffHzsP7e1nkYTRSqEjqAohO1u2jL0Ho
2FSyXyDtp2uC0mn64JW0duVZI88FT6fRqyt/HiUHvnFzLSdTg90mDMtWMF/qQcb2
jXpEvjaKGI129CgdlHRxVpUFmAK6bOdHxrx9fgcwD1L6hHo1PQX8N59ulLP2uano
g2unQ65X
=AMhQ
-----END PGP SIGNATURE-----

Timestamp of file with hash 3d9e3dc48bc4d73872a862f7eccfe39d2af942484475d2f8af25b52833aa2031 -

jnewbery · 2020-12-07T11:22:42Z

utACK 4e28753

…moving …

Summary: Remove unused method and move variable initialization in init.cpp This is a partial backport of [[bitcoin/bitcoin#18766 | core#18766]] The pull request is mostly not applicable to Bitcoin ABC. The variable moved in init.cpp are just to keep close to the source material and make future backports easier to review. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11169

DrahtBot added the Tests label Apr 25, 2020

This was referenced Apr 25, 2020

Test some transaction creation with non-empty fee estimator #18245

Closed

Allow fee estimation to work with lower fees #13990

Closed

Add fee_est tool for debugging fee estimation code #10443

Closed

promag reviewed Apr 27, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

darosior force-pushed the disable_feeest_blocksonly branch from 9cf5c47 to 33ca359 Compare April 27, 2020 10:18

maflcko added TX fees and policy and removed Tests labels Apr 27, 2020

maflcko mentioned this pull request Jul 19, 2020

Remove mempool global #19556

Merged

darosior force-pushed the disable_feeest_blocksonly branch from 33ca359 to d6c1115 Compare July 29, 2020 16:14

darosior force-pushed the disable_feeest_blocksonly branch 2 times, most recently from 2ddaf65 to 300bf14 Compare July 29, 2020 19:09

This was referenced Nov 26, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

Multiprocess bitcoin #10102

Draft

maflcko suggested changes Dec 1, 2020

View reviewed changes

src/init.cpp Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

src/interfaces/node.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Dec 1, 2020

darosior force-pushed the disable_feeest_blocksonly branch from 4aaad74 to 01e3998 Compare December 1, 2020 10:57

DrahtBot removed the Needs rebase label Dec 1, 2020

maflcko approved these changes Dec 2, 2020

View reviewed changes

test/functional/feature_fee_estimation.py Outdated Show resolved Hide resolved

src/policy/fees.cpp Outdated Show resolved Hide resolved

src/policy/fees.cpp Outdated Show resolved Hide resolved

src/policy/fees.cpp Outdated Show resolved Hide resolved

darosior force-pushed the disable_feeest_blocksonly branch from 01e3998 to 34250a1 Compare December 3, 2020 11:03

darosior force-pushed the disable_feeest_blocksonly branch 2 times, most recently from 9775dd7 to 8547afd Compare December 3, 2020 11:41

darosior and others added 4 commits December 3, 2020 12:56

interface: remove unused estimateSmartFee method from node

03bfeee

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

Remove the remaining fee estimation globals

86ff2cf

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>

init: don't create a CBlockPolicyEstimator if we don't relay transact…

e8ea6ad

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

feestimator: encapsulate estimation file logic

4e28753

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>

darosior force-pushed the disable_feeest_blocksonly branch from 8547afd to 4e28753 Compare December 3, 2020 12:07

This was referenced Dec 5, 2020

tests: Create or use existing properly initialized NodeContexts #20323

Merged

wallet: check for non-representable CFeeRates #20546

Closed

maflcko merged commit 03b1db6 into bitcoin:master Dec 7, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2020

Merge bitcoin#18766: Disable fee estimation in blocksonly mode (by re…

ef40c74

…moving …

darosior deleted the disable_feeest_blocksonly branch January 12, 2021 21:41

chris-belcher mentioned this pull request Oct 22, 2021

"Bitcoin node isnt reachable" after updating to Core 22.0 chris-belcher/electrum-personal-server#251

Closed

Stadicus mentioned this pull request Dec 11, 2021

Verification of tesnet config file raspibolt/raspibolt#808

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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

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

Uh oh!

Conversation

darosior commented Apr 25, 2020 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

darosior commented Apr 27, 2020

Uh oh!

luke-jr commented Apr 30, 2020

Uh oh!

brakmic commented May 1, 2020

Uh oh!

laanwj commented May 4, 2020

Uh oh!

promag commented May 4, 2020

Uh oh!

maflcko commented May 4, 2020

Uh oh!

maflcko commented May 4, 2020

Uh oh!

darosior commented Jun 12, 2020

Uh oh!

darosior commented Jul 29, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Dec 1, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darosior commented Dec 3, 2020

Uh oh!

practicalswift commented Dec 3, 2020

Uh oh!

maflcko commented Dec 7, 2020

Uh oh!

jnewbery commented Dec 7, 2020

Uh oh!

Uh oh!

darosior commented Apr 25, 2020 •

edited by fanquake

Loading

DrahtBot commented Apr 25, 2020 •

edited

Loading