Skip to content

rpc: Don't allow to 'estimatesmartfee' in blocksonly mode #16890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

rpc: Don't allow to 'estimatesmartfee' in blocksonly mode #16890

wants to merge 1 commit into from

Conversation

darosior
Copy link
Member

@darosior darosior commented Sep 16, 2019 •
edited
Loading

Fixes #16840.

EDIT: some context from the issue: estimatesmartfee will return bad (outdated) fee estimation if ran in blocksonly mode. Seems better for it to fail instead of succeeding with potentially harmful (in the case of a lightning penalty tx for example) values.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 17, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag
promag reviewed Sep 17, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding a small release note?

src/rpc/mining.cpp
@@ -830,6 +830,9 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request)
if (fee_mode == FeeEstimateMode::ECONOMICAL) conservative = false;
}

if (!g_relay_txes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, add braces or single line.

@sdaftuar
Copy link
Member

I'm not sure I understand the thinking here -- should we also disallow transacting if in blocksonly mode? It's not clear to me that hiding the fee estimation results but allowing the wallet to transact is a useful user experience.

I see the original issue has to do with a lightning wallet that is using our fee estimates under the hood; perhaps we should instead expose the blocksonly setting via an rpc call (if we're not doing it already) and the lightning wallet can decide how to handle the configuration?

@darosior
Copy link
Member Author

I'm not sure I understand the thinking here -- should we also disallow transacting if in blocksonly mode? It's not clear to me that hiding the fee estimation results but allowing the wallet to transact is a useful user experience.

Since the estimatesmartfee command returns an estimation based on relayed transaction, if we don't relay (or rather stop relaying) the estimation will be wrong (based on the last relayed transactions). This change assumes that it's wise to return an error instead of a wrong information.

I see the original issue has to do with a lightning wallet that is using our fee estimates under the hood;

Yes (C-lightning) and it assumes that the estimation is accurate (or rather not very inaccurate).

perhaps we should instead expose the blocksonly setting via an rpc call (if we're not doing it already) and the lightning wallet can decide how to handle the configuration?

I think this should be handled on our side as we expose an inaccurate information.

@darosior darosior force-pushed the estimatesmartfee_blockonly branch from 33cb347 to f53a815 Compare September 17, 2019 16:29
@sdaftuar
Copy link
Member

I think this should be handled on our side as we expose an inaccurate information.

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

@darosior
Copy link
Member Author

Yes that's preferable and also what I wanted to do instead of this PR.
However I don't see how we could give an accurate estimation without an updated mempool ? If you have an idea I'd be happy to work on this.

I thought about median fees in the last n blocks but in the case that a lot of transactions are mined with unnecessary high fees that would give inaccurate estimation and contribute to a global fee increase. Or maybe I miss something ?

@fanquake fanquake changed the title JSONRPC: Don't allow to 'estimatesmartfee' in blocksonly mode rpc: Don't allow to 'estimatesmartfee' in blocksonly mode Sep 17, 2019
@promag
Copy link
Contributor

promag commented Sep 18, 2019

However I don't see how we could give an accurate estimation without an updated mempool

Could just fail? There's already the error "Insufficient data or no feerate found" so looks like we could add "Expired data" or something like that - this would affect wallets too.

@darosior
Copy link
Member Author

However I don't see how we could give an accurate estimation without an updated mempool

Could just fail? There's already the error "Insufficient data or no feerate found" so looks like we could add "Expired data" or something like that - this would affect wallets too.

Yes, that's why I ended up submitting this :-)

@DrahtBot DrahtBot mentioned this pull request Apr 5, 2020
Merged
@darosior darosior mentioned this pull request Apr 25, 2020
Merged
@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@jnewbery
Copy link
Contributor

Should this be closed? I think it's no longer needed now that #18766 is open.

@darosior
Copy link
Member Author

Absolutely, i thought i added a magical "closes xxx" link in #18766 .

Anyways, thanks for the reminder!

@darosior darosior closed this Sep 29, 2020
maflcko pushed a commit that referenced this pull request Dec 7, 2020
Merge #18766: Disable fee estimation in blocksonly mode (by removing …
03b1db6 
…the fee estimates global)

4e28753 feestimator: encapsulate estimation file logic (Antoine Poinsot)
e8ea6ad init: don't create a CBlockPolicyEstimator if we don't relay transactions (Antoine Poinsot)
86ff2cf Remove the remaining fee estimation globals (Antoine Poinsot)
03bfeee interface: remove unused estimateSmartFee method from node (Antoine Poinsot)

Pull request description:

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

ACKs for top commit:
  MarcoFalke:
    re-ACK 4e28753 👋
  jnewbery:
    utACK 4e28753

Tree-SHA512: c869cf03b86d8194002970bbc84662dae76874967949b9be0d9a4511a1eabcb1627c38aca3154da9dcece1a4c49ec02bd4f9fcca2ec310986e07904559e63ba8
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs Conceptual Review Needs rebase RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fee estimate should be disabled with blocksonly mode
7 participants
@darosior @DrahtBot @sdaftuar @promag @jnewbery @laanwj @fanquake