rpc: bugfix, ‘add_inputs’ default value is true unless ‘inputs’ are provided #26053

pull furszy wants to merge 2 commits into bitcoin:master from furszy:2022_rpc_wallet_fix_help_add_inputs changing 2 files +118 −2
  1. furszy commented at 3:53 pm on September 9, 2022: member

    This bugfix was meant to be in #25685, but decoupled it to try to make it part of 24.0 release. It’s a truly misleading functionality.

    This PR doesn’t change behavior in any way. Just fixes two invalid RPC help messages and adds test coverage for the current behavior.

    Description

    In both RPC commands send() and walletcreatefundedpsbt the help message says that add_inputs default value is false when it’s actually dynamically set by the following statement:

    0coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;

    Which means that, by default, add_inputs is true unless there is any pre-set input, in which case, the default is false.

  2. DrahtBot added the label RPC/REST/ZMQ on Sep 9, 2022
  3. amovfx commented at 6:53 pm on September 9, 2022: none
    I appreciated how well your test was commented. That really helps someone new like me read whats going on.
  4. DrahtBot commented at 10:33 pm on September 9, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25685 (wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection by furszy)

    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.

  5. maflcko added this to the milestone 24.0 on Sep 10, 2022
  6. in src/wallet/rpc/spend.cpp:1141 in 2e37f0a084 outdated 
    1136@@ -1137,7 +1137,8 @@ RPCHelpMan send()
1137             {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
1138                 Cat<std::vector<RPCArg>>(
1139                 {
1140-                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."},
1141+                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Automatically include coins from the wallet to cover the target amount (sum of the outputs amounts).\n"
1142+                                                                              "Default value is true unless \"inputs\" are specified, in such case, add_inputs=false."},
    maflcko commented at 6:25 am on September 10, 2022:
    Shouldn’t this be a DefaultHint?
    furszy commented at 12:38 pm on September 10, 2022:
    yeah, better.
  7. maflcko added the label Docs on Sep 10, 2022
  8. furszy force-pushed on Sep 10, 2022
  9. furszy commented at 12:39 pm on September 10, 2022: member

    Thanks MarkoFalke, feedback tackled.

    Changed RPCArg::Default to RPCArg::DefaultHint.

  10. aureleoules commented at 8:18 am on September 12, 2022: member

    ACK 1350349f5951a4e1fdc7d92018cb5be6c014e99a. Verified that m_allow_other_inputs is overridden when inputs are passed.

    nit if you retouch: use .empty() instead of .size() == 0. https://github.com/bitcoin/bitcoin/blob/d33871288636d9e4cdde2e1430de1f4223e2dd3f/src/wallet/rpc/spend.cpp#L1228

  11. w0xlt approved
  13. S3RK commented at 6:39 am on September 13, 2022: contributor

    Code Review ACK 1350349f5951a4e1fdc7d92018cb5be6c014e99a

    Verified that the new help text is consistent with the behaviour. I agree that the new help text is less confusing.

  14. fanquake requested review from achow101 on Sep 13, 2022
  15. in src/wallet/rpc/spend.cpp:1140 in 8d74ea7bc4 outdated 
    1136@@ -1137,7 +1137,7 @@ RPCHelpMan send()
1137             {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
1138                 Cat<std::vector<RPCArg>>(
1139                 {
1140-                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."},
1141+                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true unless \"inputs\" are specified, in such case, add_inputs=false"},"Automatically include coins from the wallet to cover the target amount.\n"},
    achow101 commented at 4:57 pm on September 13, 2022:

    In 8d74ea7bc4c6e484279d860bb0ec533320cc2145 “RPC: bugfix, ‘add_inputs’ default value is true unless ‘inputs’ are provided”

    0                    {"add_inputs", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false when \"inputs\" are specified, true otherwise"},"Automatically include coins from the wallet to cover the target amount.\n"},
    furszy commented at 7:15 pm on September 13, 2022:
    done
  16. in test/functional/rpc_fundrawtransaction.py:1097 in 1350349f59 outdated 
    1088+        # 1. Default add_inputs value with no pre-set inputs (add_inputs=true):
1089+        #       Expect: automatically add coins from the wallet to the tx.
1090+        # 2. Default add_inputs value with preset inputs (add_inputs=false):
1091+        #       Expect: disallow automatic coin selection.
1092+        # 3. Explicit add_inputs=true:
1093+        #       Expect: include inputs from the wallet.
    achow101 commented at 5:01 pm on September 13, 2022:

    In 1350349f5951a4e1fdc7d92018cb5be6c014e99a “test: add coverage for ‘add_inputs’ dynamic default value”

    I think it would be useful to have a case for having both add_inputs=true and preset inputs.

    furszy commented at 7:17 pm on September 13, 2022:
    done
  17. RPC: bugfix, 'add_inputs' default value is true unless 'inputs' are provided 
    In both RPC commands `send()` and `walletcreatefundedpsbt` the RPC help was saying
that `add_inputs` default value was false when it's actually dynamically set
by the following statement:

`coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;`

Which means that, by default, `add_inputs` is true unless there
was any pre-set input, in which case, the default is false.
    ddbcfdf3d0
  18. furszy force-pushed on Sep 13, 2022
  19. furszy commented at 7:20 pm on September 13, 2022: member

    Thanks achow101, feedback tackled (and a bit more).

    Added two new test scenarios:

    1. Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount).        Expect: include inputs from the wallet.

    2. Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount).        Expect: only preset inputs are used.

  20. achow101 commented at 7:57 pm on September 13, 2022: member
    ACK 40b20102371d9fa6ae3dcdcb32a678a940e1130a
  21. test: add coverage for 'add_inputs' dynamic default value 
    Covered cases for send() and walletcreatefundedpsbt() RPC commands:

1. Default add_inputs value with no preset inputs (add_inputs=true):
       Expect: automatically add coins from the wallet to the tx.

2. Default add_inputs value with preset inputs (add_inputs=false):
       Expect: disallow automatic coin selection.

3. Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount).
       Expect: include inputs from the wallet.

4. Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount).
       Expect: only preset inputs are used.

5. Explicit add_inputs=true, no preset inputs (same as (1) but with an explicit set):
       Expect: include inputs from the wallet.
    b00fc44ca5
  22. in test/functional/rpc_fundrawtransaction.py:1107 in 40b2010237 outdated 
    1102+        tx = wallet.send(outputs=[{addr1: 3}])
1103+        assert tx["complete"]
1104+
1105+        # Case (2), 'send' command
1106+        # Select an input manually, which doesn't cover the entire output amount and
1107+        # verify that the dynamically set 'add_inputs=true' value works.
    S3RK commented at 7:15 am on September 14, 2022:
    It seems this comment is now inconsistent with the code
    furszy commented at 2:15 pm on September 14, 2022:
    pushed
  23. furszy force-pushed on Sep 14, 2022
  24. furszy commented at 2:20 pm on September 14, 2022: member
    Thanks @S3RK, review tackled. Only made a small modification to one of the test comments: diff
  25. achow101 commented at 5:54 pm on September 14, 2022: member
    ACK b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f
  26. S3RK commented at 7:06 pm on September 14, 2022: contributor
    ACK b00fc44ca5cb938f18d31cde7feb4e1c968dcc2f
  27. achow101 merged this on Sep 14, 2022
  28. achow101 closed this on Sep 14, 2022
  29. sidhujag referenced this in commit 0f91c685b0 on Sep 14, 2022
  30. furszy deleted the branch on May 27, 2023
  31. bitcoin locked this on May 26, 2024


furszy amovfx DrahtBot maflcko aureleoules w0xlt S3RK achow101

Labels
Docs RPC/REST/ZMQ

Milestone
24.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me