backport: bitcoin#18275, #20220, #20305, #20410, #20426, #20573, #21083, #21201, #21786, #21787 (fee backports) #6245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
UdjinM6
requested changes
Sep 3, 2024
addressed comments from PR. Still draft so far as CJ issue is not addressed yet. |
knst
force-pushed
the
bp-v21-p28
branch
2 times, most recently
from
0ab1e8c
to
044e140
Compare
knst
changed the title
knst
force-pushed
the
bp-v21-p28
branch
5 times, most recently
from
49b9bb6
to
b3d540b
Compare
knst
changed the title
…but the needed fee rate differed 44cc75f wallet: error if an explicit fee rate was given but the needed fee rate differed (Karl-Johan Alm) Pull request description: This ensures that the code doesn't silently ignore too low fee reates. It will now trigger an error in the QT client, if the user provides a fee rate below the minimum, and becomes a necessary check for bitcoin#11413. ACKs for top commit: Sjors: utACK 44cc75f (rebased) fjahr: re-ACK 44cc75f Tree-SHA512: cd5a60ee496e64f7ab37aaa53f7748a7393357b1629ccd9660839d366c6191b6413b871ce3aa7293fce1539336222c300ef6f86304f30a1ae8fe361b02310483
…for 0.21 0be2900 rpc: update conf_target helps for correctness/consistency (Jon Atack) 778b9be wallet, rpc: fix send subtract_fee_from_outputs help (Jon Atack) 603c005 wallet: add rpc send explicit fee rate coverage (Jon Atack) dd341e6 wallet: add sendtoaddress/sendmany explicit fee rate coverage (Jon Atack) 44e7bfa wallet: add walletcreatefundedpsbt explicit fee rate coverage (Jon Atack) 6e1ea42 test: refactor for walletcreatefundedpsbt fee rate coverage (Jon Atack) 3ac7b0c wallet: fundrawtx fee rate coverage, fixup ParseConfirmTarget() (Jon Atack) 2d8eba8 wallet: combine redundant bumpfee invalid params and args tests (Jon Atack) 1697a40 wallet: improve bumpfee error/help, add explicit fee rate coverage (Jon Atack) fc57217 wallet: fix SetFeeEstimateMode() error message (Jon Atack) 052427e wallet, bugfix: fix bumpfee with explicit fee rate modes (Jon Atack) Pull request description: Follow-up to bitcoin#11413 providing a base to build on for bitcoin#19543: - bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue bitcoin#20219 - adds explicit feerate test coverage for `bumpfee`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send`, `sendtoaddress`, and `sendmany` - improves a few related RPC error messages and `ParseConfirmTarget()` / error message - fixes/improves the explicit fee rate information in the 6 RPC helps, of which 2 were also missing `conf_target` sat/B units This provides a spec and regression coverage for the potential next step of a universal `sat/vB` feerate argument (see bitcoin#19543), as well as immediate coverage and minimum fixes for 0.21. ACKs for top commit: kallewoof: Concept/Tested ACK 0be2900 meshcollider: Code review + functional test run ACK 0be2900 Tree-SHA512: efd965003e991cba51d4504e2940f06ab3d742e34022e96a673606b44fad85596aa03a8c1809f06df7ebcf21a38e18a891e54392fe3d6fb4d120bbe4ea0cf5e0
05e82d8 wallet: override minfee checks (fOverrideFeeRate) for fee_rate (Jon Atack) 9a670b4 wallet: update sendtoaddress, send RPC examples with fee_rate (Jon Atack) be481b7 wallet: use MIN_RELAY_TX_FEE in bumpfee help (Jon Atack) 449b730 wallet: provide valid values if invalid estimate mode passed (Jon Atack) 6da3afb wallet: update remaining rpcwallet fee rate units to BTC/kvB (Jon Atack) 173b5b5 wallet: update fee rate units, use sat/vB for fee_rate error messages (Jon Atack) 7f9835a wallet: remove fee rates from conf_target helps (Jon Atack) b7994c0 wallet: add fee_rate unit warnings to bumpfee (Jon Atack) 410e471 wallet: remove redundant bumpfee fee_rate checks (Jon Atack) a0d4957 wallet: introduce fee_rate (sat/vB) param/option (Jon Atack) e21212f wallet: remove unneeded WALLET_BTC_KB_TO_SAT_B constant (Jon Atack) 6112cf2 wallet: add CFeeRate ctor doxygen documentation (Jon Atack) 3f72791 wallet: fix bug in RPC send options (Jon Atack) Pull request description: This PR builds on bitcoin#11413 and bitcoin#20220 to address bitcoin#19543. - replace overloading the conf_target and estimate_mode params with `fee_rate` in sat/vB in the sendtoaddress, sendmany, send, fundrawtransaction, walletcreatefundedpsbt, and bumpfee RPCs - allow non-actionable conf_target value of `0` and estimate_mode value of `""` to be passed to use `fee_rate` as a positional argument, in addition to as a named argument - fix a bug in the experimental send RPC described in bitcoin#20220 (comment) where args were not being passed correctly into the options values - update the feerate error message units for these RPCs from BTC/kB to sat/vB - update the test coverage, help docs, doxygen docs, and some of the RPC examples - other changes to address the excellent review feedback See this wallet meeting log for more context: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-06.html#l-309 ACKs for top commit: achow101: re-ACK 05e82d8 MarcoFalke: review ACK 05e82d8 did not test and found a few style nits, which can be fixed later 🍯 Xekyo: tACK 05e82d8 Sjors: utACK 05e82d8 Tree-SHA512: a4ee5f184ada53f1840b2923d25873bda88c5a2ae48e67eeea2417a0b35154798cfdb3c147b05dd56bd6608a784e1b91623bb985ee2ab9ef2baaec22206d0a9c
…createfundedpsbt and other fixes 9f08780 Use the correct incremental fee constant in bumpfee help (Jon Atack) 3f1e10b Update feeRate (BTC/kvB) to fee_rate (sat/vB) in wallet_bumpfee (Jon Atack) 1b3d700 Allow zero-fee fundrawtxn and walletcreatefundedpsbt calls (Jon Atack) Pull request description: - Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525406176. A check to raise an error on zero-fee txns was mistakenly extended in a0d4957 from the bumpfee and send{toaddress, many} RPCs to also include fundrawtransaction and walletcreatefundedpsbt. This commit re-overrides zero fee rate checking for these two RPCs, not only for the feeRate (BTC/kvB) arg to return to previous behavior, but also for the new fee_rate (sat/vB) arg. Negative fee rates will still raise "amount out of range" by the MoneyRange check in src/bitcoin-tx.cpp::AmountFromValue. - Updates a wallet bumpfee test from feeRate (BTC/kvB) to fee_rate (sat/vB) - Fixes https://github.com/bitcoin/bitcoin/pull/20305/files#r525405363 to use the correct incremental fee rate constant in the bumpfee help (thanks Marco Falke for the catch) and rectifies "1.000 sat/vB sat/vB" in the help to "1.000 sat/vB" ACKs for top commit: MarcoFalke: review ACK 9f08780 🐾 promag: Code review ACK 9f08780. Xekyo: Code review reACK 9f08780. Tree-SHA512: 413dfb4f23ebaf3d2ef210dd04610a843272e64eabba428699f5de4d646a86ac4911dab66b5e2f5ebea53b76e4be8347ef40824c1592c750d5eaa12579d3cdf6
…s None-type fa69c2c wallet: Do not treat default constructed types as None-type (MarcoFalke) fac4e13 refactor: Change pointer to reference because it can not be null (MarcoFalke) Pull request description: Equating `0==None` and `""==None` is confusing, unneeded and undocumented ACKs for top commit: jonatack: ACK fa69c2c achow101: ACK fa69c2c Sjors: tACK fa69c2c modulo `unset` Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac
…amounts 6fa72ce test: add coverage for passing fee rate as a string (Jon Atack) ce207d6 wallet, bugfix: allow send to take string fee rate values (Jon Atack) Pull request description: RPC send currently only accepts fee rates as numbers, which is a user-facing bug. It should accept fee rates as an amount, e.g. a string or a number, as documented in its help and like sendtoaddress, sendmany, fundrawtransaction, walletcreatefundedpsbt, and bumpfee. Provide a fix and regression test coverage. ACKs for top commit: MarcoFalke: review ACK 6fa72ce achow101: Code review ACK 6fa72ce promag: Code review ACK 6fa72ce. Tree-SHA512: 735f9269cb1b81953764b5283449c0b154bd62de034225be5bcedc515c84faf767fe8fe0741008679fe412922c847b00d116cb11aab775236b779c847ba87167
…ivate keys disabled 6bfbc97 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack) 0997019 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow) Pull request description: Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made. Fixes bitcoin#21104 ACKs for top commit: jonatack: ACK 6bfbc97 meshcollider: utACK 6bfbc97 kristapsk: ACK 6bfbc97. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API. Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
…s during coin selection f9cd2bf Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow) bdd0c29 wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow) 448d04b wallet: Move long term feerate setting to CreateTransaction (Andrew Chow) e2f429e wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow) 1a6a0b0 wallet: Use existing feerate instead of getting a new one (Andrew Chow) Pull request description: During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail. Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed. While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same. Fixes bitcoin#19229 ACKs for top commit: glozow: reACK bitcoin@f9cd2bf fjahr: Code review re-ACK f9cd2bf Xekyo: tACK bitcoin@f9cd2bf meshcollider: Code review + test run ACK f9cd2bf Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
…assertions f09e6b2 test: fix off-by-ones in rpc_fundrawtransaction (Jon Atack) Pull request description: The variables in these assertions should be the same within each line. ACKs for top commit: laanwj: ACK f09e6b2 theStack: ACK f09e6b2 Tree-SHA512: 7ac754eaadd8cb00a725afa55bccbb8de7547dedac9350d79a9a470918245617e075c56a91adc36fb653bbe8a0a325d59b00443155a7e1a81ebf22e4e4cf56d9
…tissa of 3) 847288d test: fee rate values that cannot be represented as sat/vB (Jon Atack) 06a90fa rpc: for sat/vB fee rates, limit ParseFixedPoint decimals to 3 (Jon Atack) 0742c78 rpc: enable passing decimals to AmountFromValue, add doxygen (Jon Atack) 8ce3ef5 test: ParseFixedPoint with 3 decimals for sat/vB fee rates (Jon Atack) b503327 test: type error and out of range fee rates where missing (Jon Atack) c5fd434 test: explicit fee rates with invalid amounts (Jon Atack) ea6f76b test: improve zero-value explicit fee rate coverage (Jon Atack) Pull request description: - Improve/close gaps in existing test coverage before making the change - Enable passing `decimals` to `ParseFixedPoint()` when calling `AmountFromValue()` - Limit explicit fee rates in sat/vB passed in by users to 3 decimals, and raise otherwise - Add regression test coverage Closes bitcoin#20534. ACKs for top commit: MarcoFalke: review ACK 847288d 🔷 Tree-SHA512: c539d07ae9b21c0d6c8ea460beb9c8dad5559445518aace560abc3c05c588907bae189b6fd7602b3b397de4a42356136c3ec6f960d3dcf2d5d16377aef4ab5a2
UdjinM6
approved these changes
Sep 23, 2024
knst
added
RPC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Just regular backports from bitcoin v0.21, v22; mostly wallet+fee related
What was done?
How Has This Been Tested?
Run unit and functional tests
Breaking Changes
Some wallet rpc (sendtoaddress, sendmany, send) have a new argument
fee_ratewhich is inserted beforeverbose.Release notes will be provided in a new PR once scope of backports and fixes in this PR is finalized by merging it to develop/
Checklist: