Skip to content
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.

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

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #11413

Merged
merged 8 commits into from Jun 25, 2020

Conversation

@kallewoof
Copy link
Member

@kallewoof kallewoof commented Sep 28, 2017

This lets users pick their own fees when using sendtoaddress/sendmany if they prefer this over the estimators.

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 28, 2017

Not opposed to add that. Though IMO the more experience users (which are those who may use explicit fee-rates) should use create/fund/sendrawtx.

Concept ACK

Copy link
Member

@promag promag left a comment

Please add tests.

src/policy/fees.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@esotericnonsense
Copy link
Contributor

@esotericnonsense esotericnonsense commented Sep 28, 2017

Concept ACK. I don't like that this results in sendtoaddress having 8 parameters but I don't see an immediately obvious way to avoid that.

Sendmany could get it too?

@kallewoof
Copy link
Member Author

@kallewoof kallewoof commented Sep 29, 2017

Can't fix people putting too high fees in, but if they put too low, they should be able to bump the fee unless they specifically disable it. As such, RBF is default-on when fees are explicit (3d14d35).

@promag Added tests (1768936).

@esotericnonsense I don't know how frequently this will be used. I suggest it for sendtoaddress because I see a lot of people asking how to fire off a tx with a given fee. There's also settxfee.

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Sep 29, 2017

Re: the 8 parameter issue @esotericnonsense mentioned, it would be cleaner to convert most parameters to a JSON object input like bumpfee, listunspent, etc. use since most are independent of each other and optional

src/rpc/client.cpp Outdated Show resolved Hide resolved
@kallewoof kallewoof force-pushed the kallewoof:explicit-fee branch 4 times, most recently Oct 10, 2017
Copy link
Member

@luke-jr luke-jr left a comment

Concept ACK, but sendmany should get it too.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@kallewoof kallewoof force-pushed the kallewoof:explicit-fee branch Nov 13, 2017
@kallewoof
Copy link
Member Author

@kallewoof kallewoof commented Nov 13, 2017

Addressed @luke-jr requests (double-use conf_target and add feature to sendmany).

@kallewoof kallewoof force-pushed the kallewoof:explicit-fee branch Nov 13, 2017
@kallewoof kallewoof force-pushed the kallewoof:explicit-fee branch 2 times, most recently Dec 8, 2017
@kallewoof kallewoof changed the title [wallet] [rpc] sendtoaddress: Add explicit feerate option to sendtoaddress [wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option Dec 21, 2017
@kallewoof kallewoof force-pushed the kallewoof:explicit-fee branch 3 times, most recently Feb 20, 2018
@kallewoof kallewoof force-pushed the kallewoof:explicit-fee branch 4 times, most recently May 22, 2018
@Sjors
Copy link
Member

@Sjors Sjors commented Jun 25, 2020

re-utACK 25dac9f: rebased, more fancy C++,
Schermafbeelding 2020-06-25 om 18 37 03

Strong preference for merging this and improving the rest in followups. This has been blocking #16378 for a while and rebasing/tweaking a big change can be a lot of work, also on reviewers.

@jonatack
Copy link
Member

@jonatack jonatack commented Jun 25, 2020

Ouch, that image with the flashy white background hitting the eyes in dark mode 😄

Strong preference for merging this and improving the rest in followups. This has been blocking #16378 for a while and rebasing/tweaking a big change can be a lot of work, also on reviewers.

💯... @kallewoof has the patience of a sphinx. We're still early enough in the release cycle to do the follow-ups.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 25, 2020

Sorry to be so late with this review, but it seems that this overloads the conf_target which used to be an integer (number of blocks) to also accept floats or strings. Then, if a float or string is passed, its unit is taken from the estimate mode (which used to be an enum string).

I believe, in the past all amounts (with a few exceptions) were passed as BTC and all feerates were passed as BTC/kB. If we start making feerates more type safe by forcing unit to be passed, then we should come up with a method that works for the whole code base and the whole RPC interface, not only for the few RPCs that happen to have two RPCArgs that can be recycled into passing a new type.
For example, in the future one could imagine an RPC to set the feefilter of a peer or to set the mempool min fee or simply settxfee would be updated to allow passing a type-safe feerate.
Then having multiple ways to pass a feerate would be confusing and added complexity at the call site.

Also, passing the feerate (float) as an argument that used to be an integer is going to ask for mishaps, no?

While this is obviously an improvement, and I don't want to block it, I also think that we should aim to get the RPC interface right the very first time, since it is impossible to change after release and hard to change in future releases.

@laanwj laanwj merged commit f32f7e9 into bitcoin:master Jun 25, 2020
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
x86_64 Linux [GOAL: install] [focal] [no depends, only system libs, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer] Task Summary
Details
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 25, 2020

Oh, looks like this got merged before I left my comment.

Anyway, I'd like to hear what the reviewers think of adding a new feerate argument that simply passed the feerate (with unit) in one string. Then, parsing code could be shared among all places in the RPC code that use the new feerate type.

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Jun 25, 2020

@laanwj
Copy link
Member

@laanwj laanwj commented Jun 25, 2020

Anyway, I'd like to hear what the reviewers think of adding a new feerate argument that simply passed the feerate (with unit) in one string. Then, parsing code could be shared among all places in the RPC code that use the new feerate type.

I do agree that this would be better defined. Overloading an argument on type seems somewhat brittle, and could lead to serious accidents. Sorry for merging I thought there was agreement.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 25, 2020

Yeah, no worries. I was about to merge this myself just now when I realized that this might not play out well long-term.

@jonatack
Copy link
Member

@jonatack jonatack commented Jun 25, 2020

I also prefer not overloading the argument. From reading the years of discussion I thought the discussion had been settled and that ship had sailed. In any case, there's work to do before it is release-ready and I'm happy to help.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 25, 2020

I am happy to review any follow-ups, so please ping me if I don't show up on my own

@kallewoof kallewoof deleted the kallewoof:explicit-fee branch Jun 26, 2020
@kallewoof
Copy link
Member Author

@kallewoof kallewoof commented Jun 26, 2020

@jonatack Are you up for doing a follow-up PR? I'll gladly review it if so.

@jonatack
Copy link
Member

@jonatack jonatack commented Jun 26, 2020

@kallewoof SGTM. Fixups + tests now, then propose a universal feerate arg unless you want to jump on that.

@kallewoof
Copy link
Member Author

@kallewoof kallewoof commented Jun 26, 2020

Go for it! :) Let me know if it becomes overwhelming and I'll see if I can pick up a part of it.

sidhujag added a commit to syscoin/syscoin that referenced this pull request Jul 7, 2020
…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
@meshcollider meshcollider moved this from PRs to Done in Hardware wallet support Jul 17, 2020
meshcollider added a commit that referenced this pull request Sep 15, 2020
92326d8 [rpc] add send method (Sjors Provoost)
2c2a144 [rpc] add snake case aliases for transaction methods (Sjors Provoost)
1bc8d0f [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost)

Pull request description:

  `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have:
  * manual coin selection
  * outputting a PSBT (it was controversial to add this, see #18201)
  * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`)

  At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases.

  This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format.

  Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546).

  For `bitcoin-cli` users, it tries to keep the simplest use case easy to use:

  ```sh
  bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
  ```

  This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address.

  Depends on:
  - [x] #16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`)
  - [x] #11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`)
  - [x] #18244 (`[rpc] have lockUnspents also lock manually selected coins`)

ACKs for top commit:
  meshcollider:
    Light re-utACK 92326d8
  achow101:
    ACK 92326d8 Reviewed code and test, ran tests.
  kallewoof:
    utACK 92326d8

Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f
sidhujag added a commit to syscoin/syscoin that referenced this pull request Sep 15, 2020
92326d8 [rpc] add send method (Sjors Provoost)
2c2a144 [rpc] add snake case aliases for transaction methods (Sjors Provoost)
1bc8d0f [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost)

Pull request description:

  `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have:
  * manual coin selection
  * outputting a PSBT (it was controversial to add this, see bitcoin#18201)
  * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`)

  At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases.

  This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format.

  Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see bitcoin#16546).

  For `bitcoin-cli` users, it tries to keep the simplest use case easy to use:

  ```sh
  bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b
  ```

  This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address.

  Depends on:
  - [x] bitcoin#16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`)
  - [x] bitcoin#11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`)
  - [x] bitcoin#18244 (`[rpc] have lockUnspents also lock manually selected coins`)

ACKs for top commit:
  meshcollider:
    Light re-utACK 92326d8
  achow101:
    ACK 92326d8 Reviewed code and test, ran tests.
  kallewoof:
    utACK 92326d8

Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f
@jonatack
Copy link
Member

@jonatack jonatack commented Oct 22, 2020

First follow-up is #20220.

meshcollider added a commit that referenced this pull request Nov 4, 2020
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 #11413 providing a base to build on for #19543:

  - bugfix for `bumpfee` raising a JSON error with explicit feerates, fixes issue #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 #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
MarcoFalke added a commit that referenced this pull request Nov 17, 2020
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 #11413 and #20220 to address #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 #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.