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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
829@@ -830,6 +830,9 @@ static UniValue estimatesmartfee(const JSONRPCRequest& request)
830 if (fee_mode == FeeEstimateMode::ECONOMICAL) conservative = false;
831 }
832
833+ if (!g_relay_txes)
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?
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.
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!).
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 ?
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.
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 :-)
🐙 This pull request conflicts with the target branch and needs rebase.
