wallet: fix ignored subtract_fee_from_outputs option #35317

The shared wallet funding validation accepts subtract_fee_from_outputs, but fundrawtransaction and walletcreatefundedpsbt previously only read subtractFeeFromOutputs when building recipients. This caused the accepted snake_case option to be silently ignored.

This PR reads either accepted spelling before interpreting subtract-fee outputs, and adds functional coverage for both affected RPCs.

Tested:

build/test/functional/test_runner.py wallet_fundrawtransaction.py

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #35302 (Silent Payments: Sending (take 2) by Eunovo)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

This caused the accepted snake_case option to be silently ignored.

I don't think this is entirely correct. The docs of walletcreatefundedpsbt and fundrawtransaction don't mention the snake_case at all. So I think the problem here is that undocumented options are silently ignored.

So I don't think this is the correct fix, because at a minimum this is missing a doc update? Also, I wonder if undocumented options should be rejected in all RPCs? Note that undocumented named args are rejected:

bitcoin-cli -named walletcreatefundedpsbt  unknown_foo=2
error code: -8
error message:
Unknown named parameter unknown_foo

So I don't think this is the correct fix, because at a minimum this is missing a doc update?

I agree, this should include a doc update, i can add that, along with the other undocumented snake case aliases that are already accepted.

For fundrawtransaction and walletcreatefundedpsbt, the missing valid functional options are:

change_address, change_position, lock_unspents, subtract_fee_from_outputs

Also, I wonder if undocumented options should be rejected in all RPCs?

Happy to look into making a separate PR to address this, but it might break someones app if they are using the undocumented options already?

Happy to look into making a separate PR to address this, but it might break someones app if they are using the undocumented options already?

Yes, but their app is already broken, because it silently ignores those undocumented options. It is generally better to error or warn than to silently ignore.

Yes, but their app is already broken, because it silently ignores those undocumented options. It is generally better to error or warn than to silently ignore.

True but only subtract_fee_from_outputs was silently ignored. change_address, change_position, and lock_unspents already work when passed through the options object to fundrawtransaction and walletcreatefundedpsbt, because FundTransaction checks both the camelCase and snake_case forms.

Updated the docs with this commit: e58bdfc.

I wasn't exactly sure how to update the docs for both the camelCase and snake_case options. I noticed the only other place with double aliases was verbosity|verbose in getblock(), so I followed the same pattern here.

However, it only shows the first name in the help docs, so I thought it might be better to show the updated snake_case option first.

Alternatively, I could list them as separate help entries.

rpc_help.py was failing, added the snake case aliases to vRPCConvertParams in 1b56f36.

Since there are so many RPCs that have (almost) the same set of these arguments, with different mixes of snake case and camel case, I think it would be better for everyone if the argument handling was all unified and that the docs only document the snake case names. For a few releases, all of the RPCs could accept both camel and snake case so that people have ample time to change over, then we could deprecate and remove the camel case args.