Wallet: Disable -fallbackfee by default #16524
Conversation
|
Fine with me, but I doubt the tests are passing with this change. |
Oh, right, the tests that make use of fallbackfee will need to set fallbackfee=20000 explicitly now, I'm happy to fix that, but please tell me if I miss any of the tests, specially feature_pruning,feature_dbcrash,feature_fee_estimation which seem to be the tests I forget about the most lately for some reason. Extra thanks for the fast feedback. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
d754933 to
e29978d
Compare
|
Added 1 commit to document current failures and one failed attempt to fix all of them at once. |
|
Concept ACK. For now fallback fee could be set for all tests IMO, unless setting explicitly in each tests results in a short change. |
6e26e20 to
3cc1a7c
Compare
|
Alright, setting it up for all tests is easier, I guess, not a strong opinion. |
3cc1a7c to
4505674
Compare
4505674 to
47c0246
Compare
|
Concept ACK
The release notes are only put on the wiki momentarily before a major release. Between releases, please include the release note change in your PR, preferably as fragment to avoid merge conflicts. |
|
ACK 47c0246 |
|
Can you add a file Low-level changes
=================
Tests
---
- `-fallbackfee` was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before. (#16524) |
Before it was 0 by default for main and 20000 for test and regtest. Now it is 0 by default for all chains, thus there's no need to call Params(). Also now the default for main is properly documented
47c0246 to
ea4cc3a
Compare
|
Fixed nit, added the suggested release notes file. As a reminder, this is incompatible with #16771 and we don't want to merge both of them. |
|
ACK ea4cc3a Show signature and timestampSignature: Timestamp of file with hash |
ea4cc3a Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón) Pull request description: Before it was 0 by default for main and 20000 for test and regtest. Now it is 0 by default for all chains, thus there's no need to call Params(). Also now the default for main is properly documented. Suggestion for release notes: -fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before. Should I propose them to the wiki for the release notes or only after merge? For more context, see #16402 (comment) ACKs for top commit: MarcoFalke: ACK ea4cc3a Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
|
Oops, actually the release notes are wrong. People have to set fallbackfee=0.0002 rather than fallbackfee=20000 because it is in BTC, not satoshis. |
ad70a7f Set the fallbackfee in bitcoind params (Andrew Chow) Pull request description: bitcoin/bitcoin#16524 Broke our tests by disabling `-fallbackfee`. This PR sets `-fallbackfee=20000` in our test bitcoind startup args. ACKs for top commit: instagibbs: utACK ad70a7f Tree-SHA512: e0f44607e350ed6a0782e22427114cdfa165af354cabfd77779fcc97b84f44301185dc114f221d6535786a2c183c907af6499658a3fcfcc511f60536248d268d
Summary: Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón) Pull request description: Before it was 0 by default for main and 20000 for test and regtest. Now it is 0 by default for all chains, thus there's no need to call Params(). Also now the default for main is properly documented. Suggestion for release notes: -fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before. Should I propose them to the wiki for the release notes or only after merge? For more context, see bitcoin/bitcoin#16402 (comment) bitcoin/bitcoin@ea4cc3a --- Depends on D7061 Backport of Core [[bitcoin/bitcoin#16524 | PR16524]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7062
This commit adds a -fallbackfee=0.0002 parameter to the start 'bitcoind'
command to keep bitcoin-core v0.20.1 working as v0.19.1 does -- with the
'fallbackfee' enabled.
Bitcoin v0.20.0 contains a fix for inconsistent behaviour related
to this fallbackfee configuration. Prior to v0.20, fallbackfee
was disabled (0) by default for the mainnet chain, but enabled
(0.0002) for the testnet and regtest chains.
A test case with bitcoin-core v0.20.1 was breaking on
the bitcoin-cli 'sendtoaddress' command, which was returning an
error message instead of a tx-id:
error code: -4
error message:
Fee estimation failed. Fallbackfee is disabled. \
Wait a few blocks or enable -fallbackfee.
Bitcoin-core v0.20.0 release notes contain info about this change:
https://bitcoin.org/en/release/v0.20.0#updated-rpcs-1
The Bitcoin-core PR is bitcoin/bitcoin#16524
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see #16402 (comment)