rpc: Note in fundrawtransaction doc, fee rate is for package

benthecarman commented at 10:30 pm on May 23, 2025: contributor

Accidentally made some transactions with a much higher fee rate than I wanted because I did not know this would do it for the package rather than the individual tx.

DrahtBot commented at 10:30 pm on May 23, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32607.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, danielabrozzoni, achow101
Stale ACK	i-am-yuvi, kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32618 (wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs by achow101)
#32523 (wallet: Remove watchonly behavior and isminetypes by achow101)

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.

DrahtBot added the label RPC/REST/ZMQ on May 23, 2025

in src/wallet/rpc/spend.cpp:764 in fc6d17f4d9 outdated

757@@ -758,6 +758,9 @@ RPCHelpMan fundrawtransaction()
758                 "in the wallet using importdescriptors (to calculate fees).\n"
759                 "You can see whether this is the case by checking the \"solvable\" field in the listunspent output.\n"
760                 "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n",
761+                "Note that if specifying an exact fee rate, the resulting transaction may have a higher fee rate\n"
762+                "if the transaction has unconfirmed inputs, this is because the wallet will attempt to make the\n"
763+                "entire package have the given fee rate, not the resulting transaction."
764                 {

davidgumberg commented at 10:34 pm on May 23, 2025:

0                "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n"
1                "Note that if specifying an exact fee rate, the resulting transaction may have a higher fee rate\n"
2                "if the transaction has unconfirmed inputs, this is because the wallet will attempt to make the\n"
3                "entire package have the given fee rate, not the resulting transaction.",
4                {

benthecarman commented at 10:35 pm on May 23, 2025:

woops

benthecarman force-pushed on May 23, 2025

DrahtBot added the label CI failed on May 23, 2025

DrahtBot commented at 10:35 pm on May 23, 2025: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross, gui, no tests: https://github.com/bitcoin/bitcoin/runs/42814612722 LLM reason (✨ experimental): The CI failure is due to a build error in src/wallet/rpc/spend.cpp related to a missing closing curly brace.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

benthecarman force-pushed on May 23, 2025

in src/wallet/rpc/spend.cpp:760 in fe0432b1c4 outdated

756@@ -757,7 +757,10 @@ RPCHelpMan fundrawtransaction()
757                 "Note that all inputs selected must be of standard form and P2SH scripts must be\n"
758                 "in the wallet using importdescriptors (to calculate fees).\n"
759                 "You can see whether this is the case by checking the \"solvable\" field in the listunspent output.\n"
760-                "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n",
761+                "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n"

rkrux commented at 8:35 am on May 24, 2025:

I see it needs a full stop after watch-only.

achow101 commented at 10:17 pm on May 28, 2025:

0                "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only.\n"

rkrux commented at 8:39 am on May 24, 2025: contributor

ACK fe0432b1c4a10b74844c2dedefccfe340c0d3b10

This is a good note, helpful. I verified in the code that the outputs selected in the transaction have a corresponding ancestor_bump_fees param that is used in calculating the TotalBumpFees inside CreateTransactionInternal function.

https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet/coinselection.h#L75-L76

https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet/spend.cpp#L1251-L1257

Interesting that the feature_fee_estimation.py test fails in the CI coincidentally but seems unrelated to me as it passed in my system. Maybe a rerun/push would fix it?

maflcko commented at 10:54 am on May 24, 2025: member

Interesting that the feature_fee_estimation.py test fails in the CI coincidentally but seems unrelated to me as it passed in my system. Maybe a rerun/push would fix it?

See https://github.com/bitcoin/bitcoin/issues/32461

yuvicc commented at 3:52 pm on May 24, 2025: contributor

ACK fe0432b1c4a10b74844c2dedefccfe340c0d3b10

This will be a helpful note for someone using the rpc.

luke-jr commented at 4:24 pm on May 27, 2025: member

Is this actually intended behaviour?

achow101 commented at 6:05 pm on May 27, 2025: member

Is this actually intended behaviour?

Yes, see #26152

kevkevinpal commented at 7:57 pm on May 28, 2025: contributor

ACK fe0432b

in src/wallet/rpc/spend.cpp:762 in fe0432b1c4 outdated

756@@ -757,7 +757,10 @@ RPCHelpMan fundrawtransaction()
757                 "Note that all inputs selected must be of standard form and P2SH scripts must be\n"
758                 "in the wallet using importdescriptors (to calculate fees).\n"
759                 "You can see whether this is the case by checking the \"solvable\" field in the listunspent output.\n"
760-                "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n",
761+                "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n"
762+                "Note that if specifying an exact fee rate, the resulting transaction may have a higher fee rate\n"
763+                "if the transaction has unconfirmed inputs, this is because the wallet will attempt to make the\n"

achow101 commented at 10:16 pm on May 28, 2025:

Comma should be a period.

0                "if the transaction has unconfirmed inputs. This is because the wallet will attempt to make the\n"

DrahtBot removed the label CI failed on May 29, 2025

rpc: Note in fundrawtransaction doc, fee rate is for package f98e1aaf34

benthecarman force-pushed on Jun 2, 2025

benthecarman commented at 4:18 pm on June 2, 2025: contributor

impl’d @achow101’s suggestions, also added missing \n at end of the doc

rkrux commented at 9:34 am on June 3, 2025: contributor

re-ACK f98e1aaf34e347088caa54403521e3b5cb55dd40

danielabrozzoni commented at 1:15 pm on June 4, 2025: contributor

ACK f98e1aaf34e347088caa54403521e3b5cb55dd40

achow101 commented at 9:49 pm on June 4, 2025: member

ACK f98e1aaf34e347088caa54403521e3b5cb55dd40

achow101 merged this on Jun 4, 2025

achow101 closed this on Jun 4, 2025

benthecarman deleted the branch on Jun 4, 2025

glozow added the label Needs backport (29.x) on Jun 9, 2025

fanquake referenced this in commit c899334e36 on Jun 9, 2025

fanquake removed the label Needs backport (29.x) on Jun 9, 2025

fanquake commented at 3:15 pm on June 9, 2025: member

Backported to 29.x in #32589.

rpc: Note in fundrawtransaction doc, fee rate is for package #32607

Code Coverage & Benchmarks

Reviews

Conflicts