wallet: Allow users to specify input weights when funding a transaction #23201

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:ext-input-weight changing 9 files +376 −18
  1. achow101 commented at 11:15 PM on October 5, 2021: member

    When funding a transaction with external inputs, instead of providing solving data, a user may want to just provide the maximum signed size of that input. This is particularly useful in cases where the input is nonstandard as our dummy signer is unable to handle those inputs.

    The input weight can be provided to any input regardless of whether it belongs to the wallet and the provided weight will always be used regardless of any calculated input weight. This allows the user to override the calculated input weight which may overestimate in some circumstances due to missing information (e.g. if the private key is not known, a maximum size signature will be used, but the actual signer may be doing additional work which reduces the size of the signature).

    For send and walletcreatefundedpsbt, the input weight is specified in a weight field in an input object. For fundrawtransaction, a new input_weights field is added to the options object. This is an array of objects consisting of a txid, vout, and weight.

    Closes #23187

  2. achow101 force-pushed on Oct 5, 2021
  3. achow101 force-pushed on Oct 5, 2021
  4. achow101 commented at 11:22 PM on October 5, 2021: member

    Note that bumpfee does not take this into account yet, but it's next on my list.

  5. meshcollider commented at 11:23 PM on October 5, 2021: contributor

    Concept ACK

  6. achow101 force-pushed on Oct 5, 2021
  7. DrahtBot added the label RPC/REST/ZMQ on Oct 5, 2021
  8. DrahtBot added the label Wallet on Oct 5, 2021
  9. fanquake requested review from instagibbs on Oct 6, 2021
  10. fanquake commented at 12:26 AM on October 6, 2021: member

    cc @t-bast maybe @apoelstra

  11. t-bast commented at 6:46 AM on October 6, 2021: member

    Concept ACK, I'll test this today with lightning transactions. Should this PR update the release notes?

  12. in src/wallet/rpcwallet.cpp:4772 in 5c2ff25785 outdated 
    4766 | @@ -4707,10 +4767,15 @@ static RPCHelpMan walletcreatefundedpsbt()
4767 |          }, true
4768 |      );
4769 |  
4770 | +    UniValue options = request.params[3];
4771 | +    if (options.exists("input_weights")) {
4772 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify weight in inputs instead");
    MarcoFalke commented at 8:41 AM on October 6, 2021:

    Isn't this already done in SetOptionsInputWeights?

    achow101 commented at 3:31 PM on October 6, 2021:

    Indeed, removed.

  13. t-bast commented at 9:14 AM on October 6, 2021: member

    I tested https://github.com/bitcoin/bitcoin/pull/23201/commits/5c2ff2578512fca8134d68a8a86ae144c9335cca with lightning transactions, and I'm correctly able to specify input weights for non-standard inputs. Thanks @achow101!

  14. DrahtBot commented at 11:23 AM on October 6, 2021: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)
    • #23475 (wallet: add config to prioritize a solution that doesn't create change in coin selection by brunoerg)
    • #23202 (wallet: allow psbtbumpfee to work with txs with external inputs by achow101)
    • #19602 (wallet: Migrate legacy wallets to descriptor wallets 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.

  15. DrahtBot added the label Needs rebase on Oct 6, 2021
  16. achow101 force-pushed on Oct 6, 2021
  17. achow101 force-pushed on Oct 6, 2021
  18. DrahtBot removed the label Needs rebase on Oct 6, 2021
  19. achow101 force-pushed on Oct 6, 2021
  20. achow101 force-pushed on Oct 6, 2021
  21. in src/wallet/rpcwallet.cpp:3364 in c3fbbe7de9 outdated 
    3359 | +            const UniValue& weight_v = find_value(input, "weight");
3360 | +            if (!weight_v.isNum()) {
3361 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, missing weight key");
3362 | +            }
3363 | +            int64_t weight = weight_v.get_int64();
3364 | +            if (weight < 40 * 4) {
    instagibbs commented at 11:00 PM on October 6, 2021:

    nit: use WITNESS_SCALE_FACTOR

    achow101 commented at 11:33 PM on October 6, 2021:

    Done

  22. in test/functional/rpc_fundrawtransaction.py:1061 in 7121427e24 outdated 
    1060 | +        signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx["hex"])
1061 | +        assert self.nodes[0].testmempoolaccept([signed_tx["hex"]])[0]["allowed"]
1062 | +        # Reducing the weight should have a lower fee
1063 | +        funded_tx2 = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": low_input_weight}]})
1064 | +        assert_greater_than(funded_tx["fee"], funded_tx2["fee"])
1065 | +        # Increasing the weight should have a higher lower fee
    instagibbs commented at 11:05 PM on October 6, 2021:

    higher lower fee?

    achow101 commented at 11:33 PM on October 6, 2021:

    Done

  23. achow101 force-pushed on Oct 6, 2021
  24. DrahtBot added the label Needs rebase on Oct 7, 2021
  25. achow101 force-pushed on Oct 7, 2021
  26. DrahtBot removed the label Needs rebase on Oct 7, 2021
  27. in src/wallet/coincontrol.h:125 in d0592296b1 outdated 
     119 | +        m_input_weights[outpoint] = weight;
 120 | +    }
 121 | +
 122 | +    bool HasInputWeight(const COutPoint& outpoint) const
 123 | +    {
 124 | +        return m_input_weights.count(outpoint) > 0;
    yanmaani commented at 3:08 PM on October 8, 2021:

    Isn't "no weight" -1? If so, shouldn't this be >= 0?

    achow101 commented at 3:50 PM on October 8, 2021:

    This is checking whether the outpoint exists in the map. The result of count is always 0 or 1, it does not actually fetch the value stored.

  28. in src/wallet/rpcwallet.cpp:3188 in d0592296b1 outdated 
    3183 | +        return {
3184 | +            "input_weights", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "Inputs and their corresponding weights",
3185 | +             {
3186 | +                 {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
3187 | +                 {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"},
3188 | +                 {"weight", RPCArg::Type::NUM, RPCArg::Optional::NO, "The maximum weight for this input, including the weight of the outpoint and sequence number. Note that signature sizes are not guaranteed to be consistent, so the maximum DER signatures size of 73 bytes should be used when considering ECDSA signatures"},
    yanmaani commented at 3:10 PM on October 8, 2021:

    Would be cleaner to concatenate this across multiple lines so you don't have to scroll out, no?

    achow101 commented at 4:09 PM on October 8, 2021:

    Done

  29. in src/wallet/rpcwallet.cpp:3392 in d0592296b1 outdated 
    3386 | @@ -3367,6 +3387,35 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3387 |          }
3388 |      }
3389 |  
3390 | +    if (options.exists("input_weights")) {
3391 | +        const UniValue& inputs = options["input_weights"].get_array();
3392 | +        for (unsigned int i = 0; i < inputs.size(); ++i) {
    yanmaani commented at 3:12 PM on October 8, 2021:

    Any reason here not to use a C++-style loop? Granted, it's not a big difference.

    achow101 commented at 4:09 PM on October 8, 2021:

    Done

  30. in src/wallet/rpcwallet.cpp:3515 in d0592296b1 outdated 
    3511 | @@ -3446,7 +3512,7 @@ static RPCHelpMan fundrawtransaction()
3512 |                                  },
3513 |                              },
3514 |                          },
3515 | -                        FundTxDoc()),
3516 | +                        FundTxDoc(true)),
    yanmaani commented at 3:14 PM on October 8, 2021:

    Comment here explaining the name of the parameter?

    achow101 commented at 4:09 PM on October 8, 2021:

    Removed the comment, the help can actually be inline.

  31. in src/wallet/rpcwallet.cpp:3436 in d0592296b1 outdated 
    3452 | @@ -3404,6 +3453,23 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3453 |      }
3454 |  }
3455 |  
3456 | +static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options)
3457 | +{
3458 | +    if (options.exists("input_weights")) {
    yanmaani commented at 3:16 PM on October 8, 2021:

    What's going on here? Are there two mutually exclusive ways to give the input weights?

    achow101 commented at 3:58 PM on October 8, 2021:

    Yes.

    For send and walletcreatefundedpsbt, the weights can and should be specified in the inputs array. To avoid conflicts, these are mutually exclusive. input_weights exists for fundrawtransaction because it does not have an inputs array. However it is also useful to be able to pass it as part of the options object, which is what this function does.

  32. in src/wallet/spend.cpp:459 in d0592296b1 outdated 
     462 | @@ -463,15 +463,17 @@ bool SelectCoins(const CWallet& wallet, const std::vector<COutput>& vAvailableCo
 463 |              }
 464 |              input_bytes = GetTxSpendSize(wallet, wtx, outpoint.n, false);
 465 |              txout = wtx.tx->vout.at(outpoint.n);
 466 | -        }
 467 | -        if (input_bytes == -1) {
 468 | -            // The input is external. We either did not find the tx in mapWallet, or we did but couldn't compute the input size with wallet data
 469 | +        } else {
 470 | +            // The input is external. We did not find the tx in mapWallet.
    yanmaani commented at 3:18 PM on October 8, 2021:

    Pardon my ignorance here, but why do we need it in mapWallet if we have its size?

    achow101 commented at 3:59 PM on October 8, 2021:

    We are not guaranteed to have the size.

  33. in test/functional/rpc_fundrawtransaction.py:1022 in d0592296b1 outdated 
    1022 | @@ -1023,6 +1023,11 @@ def test_external_inputs(self):
1023 |          assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["01234567890a0b0c0d0e0f"]}})
1024 |          assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"scripts":["not a script"]}})
1025 |          assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, {"solving_data": {"descriptors":["not a descriptor"]}})
1026 | +        assert_raises_rpc_error(-8, "Invalid parameter, missing vout key", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"]}]})
1027 | +        assert_raises_rpc_error(-8, "Invalid parameter, vout cannot be negative", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": -1}]})
1028 | +        assert_raises_rpc_error(-8, "Invalid parameter, missing weight key", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"]}]})
1029 | +        assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be less than 160", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 159}]})
1030 | +        assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be less than 160", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": -1}]})
1031 |
    yanmaani commented at 3:22 PM on October 8, 2021:

    Are you missing the Specify weight in inputs instead error case?

    achow101 commented at 4:22 PM on October 8, 2021:

    Added to rpc_psbt and wallet_send where it actually can be an error.

  34. yanmaani commented at 3:25 PM on October 8, 2021: none

    utACK d059229. Here's some nits.

  35. in src/wallet/rpcwallet.cpp:3491 in d0592296b1 outdated 
    3511 | @@ -3446,7 +3512,7 @@ static RPCHelpMan fundrawtransaction()
3512 |                                  },
3513 |                              },
    MarcoFalke commented at 3:58 PM on October 8, 2021:

    Sorry for causing the conflict, but it might be cleaner to just inline the doc here if only one RPC method is using it and none of the others.

                                },
                            {"input_weights", RPCArg::.......
    achow101 commented at 4:10 PM on October 8, 2021:

    Done. For some reason I thought it needed to be in that function, but actually it doesn't.

  36. achow101 force-pushed on Oct 8, 2021
  37. achow101 force-pushed on Oct 8, 2021
  38. achow101 force-pushed on Oct 8, 2021
  39. in src/wallet/wallet.cpp:1536 in 0224b9db65 outdated 
    1473 | +    // possible to have a stack element size and combination to exactly equal a target.
1474 | +    // To avoid this possibility, if the weight to add is less than 10 bytes greater than
1475 | +    // a boundary, the size will be split so that 2/3rds will be in one stack element, and
1476 | +    // the remaining 1/3rd in another. Using 3rds allows us to avoid additional boundaries.
1477 | +    // 10 bytes is used because that accounts for the maximum size. This does not need to be super precise.
1478 | +    if ((add_weight >= 253 && add_weight < 263)
    rajarshimaitra commented at 12:14 PM on October 9, 2021:

    Assuming this numbers are denoting 8 bit boundaries, why its not 255 and 265?

    glozow commented at 4:01 PM on January 17, 2022:

    I was confused about this as well since I had never come across compact size uints before. It helped me to read serialize.h and about tx serialization (bip141 and this doc).

    This is just saying that when the stack size goes from 252 to 253, you need more bytes to encode the size in the serialized transaction. It's not the same as a byte boundary since (I assume) you need to reserve a few values for codifying how many bytes it is.

  40. in test/functional/rpc_fundrawtransaction.py:1047 in 0224b9db65 outdated 
    1056 | +
1057 | +        # Funding should also work if the input weight is provided
1058 | +        funded_tx = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": input_weight}]})
1059 | +        signed_tx = wallet.signrawtransactionwithwallet(funded_tx["hex"])
1060 | +        signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx["hex"])
1061 | +        assert self.nodes[0].testmempoolaccept([signed_tx["hex"]])[0]["allowed"]
    rajarshimaitra commented at 2:53 PM on October 9, 2021:

    This can have the complete == true/false assertions too.

    achow101 commented at 6:00 PM on January 17, 2022:

    Done

  41. yanmaani commented at 10:46 AM on October 11, 2021: none

    LGTM

  42. gaarf approved
  43. in src/wallet/coincontrol.h:129 in 0224b9db65 outdated 
     124 | +        return m_input_weights.count(outpoint) > 0;
 125 | +    }
 126 | +
 127 | +    int64_t GetInputWeight(const COutPoint& outpoint) const
 128 | +    {
 129 | +        if (HasInputWeight(outpoint)) {
    promag commented at 10:35 PM on October 18, 2021:

    It seems that all callers already check for this. Instead of returning -1, how about

    int64_t GetInputWeight(const COutPoint& outpoint) const
{
    auto it = m_input_weights.find(output);
    assert(it != m_input_weights.end();
    return it->second;
}
    achow101 commented at 11:57 PM on October 18, 2021:

    Done

  44. in src/wallet/rpcwallet.cpp:3439 in 0224b9db65 outdated 
    3430 | @@ -3404,6 +3431,23 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
3431 |      }
3432 |  }
3433 |  
3434 | +static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options)
3435 | +{
3436 | +    if (options.exists("input_weights")) {
3437 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify weight in inputs instead");
3438 | +    }
3439 | +    if (inputs.size() == 0) {
    promag commented at 10:46 PM on October 18, 2021:

    Instead of this check, check below:

    if (!weights.empty()) {
    options.pushKV("input_weights", weights);
}
    achow101 commented at 11:57 PM on October 18, 2021:

    This doesn't work because inputs.getValues() expects inputs to be non-null.

    promag commented at 7:40 AM on October 19, 2021:

    nit, right 😞 you can keep the check below to avoid "input_weights": [].

  45. promag commented at 10:53 PM on October 18, 2021: member

    Concept ACK.

  46. achow101 force-pushed on Oct 18, 2021
  47. t-bast commented at 1:54 PM on November 10, 2021: member

    Looks like this PR has multiple concept ACKs (and an E2E test with lightning!), is it possible to move forward with it?

  48. t-bast commented at 3:46 PM on November 29, 2021: member

    It would be really great to have this PR integrated for pre-signed transaction protocols (and ideally a follow-up to add these arguments to the bump fee RPCs), it would really make using the bitcoind RPC much smoother!

  49. DrahtBot added the label Needs rebase on Dec 8, 2021
  50. achow101 force-pushed on Dec 8, 2021
  51. DrahtBot removed the label Needs rebase on Dec 8, 2021
  52. DrahtBot added the label Needs rebase on Dec 9, 2021
  53. achow101 force-pushed on Dec 9, 2021
  54. DrahtBot removed the label Needs rebase on Dec 9, 2021
  55. fanquake deleted a comment on Dec 9, 2021
  56. achow101 force-pushed on Dec 14, 2021
  57. in src/wallet/spend.cpp:468 in e150c15f66 outdated 
     466 |              input_bytes = CalculateMaximumSignedInputSize(txout, &coin_control.m_external_provider, /* use_max_sig */ true);
 467 |          }
 468 | +        // If available, override calculated size with coin control specified size
 469 | +        if (coin_control.HasInputWeight(outpoint)) {
 470 | +            input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0);
 471 | +        }
    benthecarman commented at 3:53 PM on January 4, 2022:

    this will be skipped if !coin_control.GetExternalOutput(outpoint, txout), which should happen for some external inputs, no?

    achow101 commented at 7:24 PM on January 10, 2022:

    I don't think that's right. If !coin_control.GetExternalOutput, then we can't do coin selection at all and would have returned at an earlier point.

    glozow commented at 3:10 PM on January 17, 2022:

    I don't think that's right. If !coin_control.GetExternalOutput, then we can't do coin selection at all and would have returned at an earlier point.

    Reviewed code and I agree with this. If it's an external input, we must have not found it in mapWallet. If we also didn't find it in GetExternalOutput(), then we already returned nullopt on line 461.

  58. in src/wallet/rpc/spend.cpp:680 in 386768e460 outdated 
     674 | +                                    {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
 675 | +                                    {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"},
 676 | +                                    {"weight", RPCArg::Type::NUM, RPCArg::Optional::NO, "The maximum weight for this input, "
 677 | +                                        "including the weight of the outpoint and sequence number. "
 678 | +                                        "Note that signature sizes are not guaranteed to be consistent, "
 679 | +                                        "so the maximum DER signatures size of 73 bytes should be used when considering ECDSA signatures"},
    glozow commented at 12:58 PM on January 13, 2022:

    in e519573f81:

    It may be confusing if you expect the input to be in weight units, but mention using 73 bytes in the helpstring

    achow101 commented at 7:42 PM on January 13, 2022:

    I'm not sure how to better word this. Signatures have different weights depending on whether they are witness, but they all have the same max physical bytes size.

    glozow commented at 10:47 AM on January 14, 2022:

    True, that makes sense.

    I guess my actual concern was that people might mix up weight units and virtual bytes, and end up with bugs where they underestimate the size and don't add enough fees. But given that they'd be off by a factor of 4 and you have a sanity check minimum 160, probably they'll discover this mistake really quickly.

    instagibbs commented at 5:59 AM on January 17, 2022:

    maybe just put "serialized size" to make it clear this comment is not suggesting how much weight it takes on any particular context.

    rajarshimaitra commented at 7:51 AM on January 17, 2022:

    Or may be just add one more line saying "weight = total byte size * 4"? In that way it conveys both that these are two different units and how to go from one to another.

    glozow commented at 12:55 PM on January 17, 2022:

    "weight = total byte size * 4" is not true.

    I would add "Remember to use weight units, not serialized size or virtual bytes, when calculating signature size" to the end of the helpstring. Sorry to be nitpicking this, it's not blocking for me.

    achow101 commented at 5:59 PM on January 17, 2022:

    I've added "Remember to convert serialized sizes to weight units when necessary."

  59. in src/wallet/wallet.cpp:1477 in 386768e460 outdated 
    1473 | @@ -1474,6 +1474,34 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut
1474 |      return true;
1475 |  }
1476 |  
1477 | +static void FillInputToWeight(CTxIn& txin, int64_t target_weight)
    glozow commented at 1:07 PM on January 13, 2022:

    Please add some unit tests for this function? Given that there's so much consideration for boundaries, etc.

    achow101 commented at 7:49 PM on January 14, 2022:

    Done

  60. in test/functional/rpc_fundrawtransaction.py:1053 in 386768e460 outdated 
    1055 | +        signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx["hex"])
1056 | +        assert self.nodes[0].testmempoolaccept([signed_tx["hex"]])[0]["allowed"]
1057 | +        # Reducing the weight should have a lower fee
1058 | +        funded_tx2 = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": low_input_weight}]})
1059 | +        assert_greater_than(funded_tx["fee"], funded_tx2["fee"])
1060 | +        # Increasing the weight should have a higher fee
    glozow commented at 1:18 PM on January 13, 2022:

    Perhaps also test providing a feerate and ensure that the fee / total size is enough?

    achow101 commented at 7:49 PM on January 14, 2022:

    Done

  61. glozow commented at 1:37 PM on January 13, 2022: member

    Concept ACK, light review so far

  62. achow101 force-pushed on Jan 14, 2022
  63. achow101 force-pushed on Jan 14, 2022
  64. in src/wallet/coincontrol.h:138 in 52d8d44adb outdated 
     133 | +    }
 134 | +
 135 |  private:
 136 |      std::set<COutPoint> setSelected;
 137 |      std::map<COutPoint, CTxOut> m_external_txouts;
 138 | +    //! Map of COutPoints to the maximum weight for that input
    rajarshimaitra commented at 10:52 AM on January 15, 2022:

    Should this be "maximum weight" or provided "user input weight"?

    glozow commented at 12:57 PM on January 17, 2022:

    imo maximum weight. This is CCoinControl so everything is implicitly user input.

  65. rajarshimaitra commented at 11:53 AM on January 15, 2022: contributor

    Initial Concept ACK, will do a more detail pass later.

    Below are some minor nits (ignorable) and questions I had.

  66. in src/wallet/rpc/spend.cpp:618 in 52d8d44adb outdated 
     611 | @@ -585,6 +612,23 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
 612 |      }
 613 |  }
 614 |  
 615 | +static void SetOptionsInputWeights(const UniValue& inputs, UniValue& options)
 616 | +{
 617 | +    if (options.exists("input_weights")) {
 618 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "Specify weight in inputs instead");
    rajarshimaitra commented at 7:47 AM on January 17, 2022:

    Can this error message be written like "input weights already exists in option, remove them and try again"? It might be a little more clear on the intent of this check? This clearly tells whats wrong in the option values and how to fix it.

    achow101 commented at 6:00 PM on January 17, 2022:

    Changed to "Input weights should be specified in inputs rather than in options."

  67. in src/wallet/rpc/spend.cpp:676 in 52d8d44adb outdated 
     669 | @@ -626,6 +670,16 @@ RPCHelpMan fundrawtransaction()
 670 |                                      {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."},
 671 |                                  },
 672 |                              },
 673 | +                            {"input_weights", RPCArg::Type::ARR, RPCArg::Optional::OMITTED_NAMED_ARG, "Inputs and their corresponding weights",
 674 | +                                {
 675 | +                                    {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
 676 | +                                    {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"},
    rajarshimaitra commented at 7:53 AM on January 17, 2022:

    I think its more standard to call it an index?

                                        {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output index"},
    achow101 commented at 6:00 PM on January 17, 2022:

    Done

  68. in src/wallet/wallet.h:946 in 52d8d44adb outdated 
     938 | @@ -939,6 +939,9 @@ bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
 939 |  bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
 940 |  
 941 |  bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut &txout, bool use_max_sig);
 942 | +
 943 | +void FillInputToWeight(CTxIn& txin, int64_t target_weight);
 944 |  } // namespace wallet
 945 |  
 946 | +
    rajarshimaitra commented at 9:26 AM on January 17, 2022:

    I think this is an extra space?

    achow101 commented at 6:00 PM on January 17, 2022:

    Removed

  69. in test/functional/rpc_fundrawtransaction.py:1009 in 52d8d44adb outdated 
    1004 | @@ -1003,14 +1005,19 @@ def test_external_inputs(self):
1005 |          ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0]
1006 |  
1007 |          # An external input without solving data should result in an error
1008 | -        raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): 15})
1009 | +        raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): ext_utxo["amount"] / 2})
1010 |          assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
    rajarshimaitra commented at 10:43 AM on January 17, 2022:

    I would expect the solving data error should notify here "solving data not provided" or something like that. Not "Insufficient funds". Can this be ensured by putting required funds in input and then omitting solving data?

    achow101 commented at 5:49 PM on January 17, 2022:

    Currently the only error message SelectCoins can return (which is where we find this failure) is "Insufficient funds". I think it is an orthogonal change to change that to return better errors, and can be done in a future PR.

    rajarshimaitra commented at 9:52 AM on January 18, 2022:

    Ah ok.. Ya probably a good idea for a future PR.

  70. in test/functional/rpc_psbt.py:634 in 52d8d44adb outdated 
     630 |          self.generate(self.nodes[0], 6)
 631 |          ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0]
 632 |  
 633 |          # An external input without solving data should result in an error
 634 | -        assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[1].walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 10 + ext_utxo['amount']}, 0, {'add_inputs': True})
 635 | +        assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 15})
    rajarshimaitra commented at 10:44 AM on January 17, 2022:

    Same regarding "Insufficient funds" error here.

  71. in test/functional/wallet_send.py:553 in 52d8d44adb outdated 
     547 | +            inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": input_weight}],
 548 | +            add_inputs=True,
 549 | +            psbt=True,
 550 | +            include_watching=True
 551 | +        )
 552 | +        signed = ext_wallet.walletprocesspsbt(res["psbt"])
    rajarshimaitra commented at 11:00 AM on January 17, 2022:

    We can also send by providing solving data and descriptor. I think those checks could get covered here too like other tests.

    achow101 commented at 5:53 PM on January 17, 2022:

    That is already being tested above.

  72. rajarshimaitra commented at 11:03 AM on January 17, 2022: contributor

    tACK https://github.com/bitcoin/bitcoin/pull/23201/commits/52d8d44adba48c23b6047418eec431fbb39fd8db

    Verified all tests are covering expected behavior.

    Below are few non blocking nits and comments.

  73. MarcoFalke commented at 11:34 AM on January 17, 2022: member

    Is this for 0.23?

  74. in test/functional/rpc_fundrawtransaction.py:1059 in 52d8d44adb outdated 
    1058 | +        # The provided weight should override the calculated weight when solving data is provided
1059 | +        funded_tx3 = wallet.fundrawtransaction(raw_tx, {"solving_data": {"descriptors": [desc]}, "input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}]})
1060 | +        assert_equal(funded_tx2["fee"], funded_tx3["fee"])
1061 | +        # The feerate should be met
1062 | +        funded_tx4 = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}], "fee_rate": 10})
1063 | +        signed_tx4 = wallet.signrawtransactionwithwallet(funded_tx4["hex"])
    glozow commented at 12:52 PM on January 17, 2022:

    linter:

    test/functional/rpc_fundrawtransaction.py:1059:9: F841 local variable 'signed_tx4' is assigned to but never used

    achow101 commented at 6:01 PM on January 17, 2022:

    Oops, removed

  75. in src/wallet/wallet.cpp:1510 in 52d8d44adb outdated 
    1506 | @@ -1507,6 +1507,34 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut
1507 |      return true;
1508 |  }
1509 |  
1510 | +void FillInputToWeight(CTxIn& txin, int64_t target_weight)
    glozow commented at 3:50 PM on January 17, 2022:

    Manually tested this function a little bit. Passing in any target_weight less than 166 causes this to crash. Error is cannot create std::vector larger than max_size() because add_weight is 0 and the compact size of that is 1, so you're calling scriptWitness.stack.emplace() with an argument of -1 bytes.

    Also when I print GetTransactionInputWeight(CTxIn()), I get 165.

    I don't know if 166 would be a better minimum than 160. But my point is, this function has some preconditions, i.e. assert(txin_weight < target_weight) and assert(add_weight >= GetSizeOfCompactSize(add_weight), that aren't enforced by the RPC input sanitization. It's probably not good to crash if the user passes in a legal input? Perhaps they're confused and thought they were putting 162 virtual bytes, for example.

    achow101 commented at 6:03 PM on January 17, 2022:

    Apparently GetTransactionInputWeight includes the empty witness, and I forgot to include the empty scriptSig in my calculation.

    I've changed restriction to be at least 165 bytes. FillInputToWeight will also check the size meets the minimum. If it does not, it will just do nothing rather than crashing. There are also unit tests for this lower bound. add_weight should always be at least 1 now, and that is fine.

  76. in src/wallet/rpc/spend.cpp:578 in 52d8d44adb outdated 
     567 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, missing weight key");
 568 | +            }
 569 | +            int64_t weight = weight_v.get_int64();
 570 | +            if (weight < 40 * WITNESS_SCALE_FACTOR) {
 571 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, weight cannot be less than 160 (40 bytes (size of outpoint + sequence) * 4 (witness scaling factor))");
 572 | +            }
    glozow commented at 4:04 PM on January 17, 2022:

    Did you consider having an upper bound for weights as well? e.g. based on MAX_STANDARD_SCRIPTSIG_SIZE * WITNESS_SCALE_FACTOR or even MAX_STANDARD_TX_WEIGHT?

  77. in src/wallet/wallet.cpp:1510 in 8a1ea6a050 outdated 
    1506 | @@ -1507,7 +1507,7 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut
1507 |      return true;
1508 |  }
1509 |  
1510 | -static void FillInputToWeight(CTxIn& txin, int64_t target_weight)
    glozow commented at 4:09 PM on January 17, 2022:

    nit: Seems like changing FillInputToWeight to not be static and adding its unit tests were squashed into 8a1ea6a050 but should have been squashed into 9191748088 instead?

    achow101 commented at 6:04 PM on January 17, 2022:

    Oops, fixed.

  78. glozow commented at 4:17 PM on January 17, 2022: member

    Is this for 0.23?

    I hope so!

    I've reviewed up until 8a1ea6a050. I have some concerns about the argument sanitization, but otherwise looks good to me. This also needs release notes, yes?

  79. in test/functional/rpc_fundrawtransaction.py:9 in 52d8d44adb outdated 
       3 | @@ -4,6 +4,8 @@
   4 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 |  """Test the fundrawtransaction RPC."""
   6 |  
   7 | +import math
   8 | +
   9 |  from decimal import Decimal
  10 |  from itertools import product
    glozow commented at 4:23 PM on January 17, 2022:

    nit in 52d8d44adb

    from decimal import Decimal
from itertools import product
from math import ceil
    achow101 commented at 6:04 PM on January 17, 2022:

    Done

  80. in test/functional/wallet_send.py:531 in 52d8d44adb outdated 
     526 | +        psbt_in = dec["inputs"][input_idx]
 527 | +        # Calculate the input weight
 528 | +        # (prevout + sequence + length of scriptSig + 2 bytes buffer) * 4 + len of scriptwitness
 529 | +        len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
 530 | +        len_scriptwitness = len(psbt_in["final_scriptwitness"]["hex"]) // 2 if "final_scriptwitness" in psbt_in else 0
 531 | +        input_weight = ((44 + len_scriptsig + 2) * 4) + len_scriptwitness
    glozow commented at 5:08 PM on January 17, 2022:

    typo?

            input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitness
    achow101 commented at 6:04 PM on January 17, 2022:

    Done

  81. in test/functional/wallet_send.py:556 in 52d8d44adb outdated 
     551 | +        )
 552 | +        signed = ext_wallet.walletprocesspsbt(res["psbt"])
 553 | +        signed = ext_fund.walletprocesspsbt(res["psbt"])
 554 | +        assert signed["complete"]
 555 | +        tx = self.nodes[0].finalizepsbt(signed["psbt"])
 556 | +        assert self.nodes[0].testmempoolaccept([tx["hex"]])[0]["allowed"]
    glozow commented at 5:10 PM on January 17, 2022:

    Recommend adding the feerate check here too, it caught the input_weight being off up there^ (also added fee_rate=10 to the self.test_send() call above)

            testres = self.nodes[0].testmempoolaccept([tx["hex"]])[0]
        assert testres["allowed"]
        assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001))
    achow101 commented at 6:04 PM on January 17, 2022:

    Done

  82. glozow commented at 5:29 PM on January 17, 2022: member

    reviewed the tests

  83. achow101 force-pushed on Jan 17, 2022
  84. achow101 force-pushed on Jan 17, 2022
  85. achow101 force-pushed on Jan 17, 2022
  86. glozow commented at 11:05 AM on January 18, 2022: member

    ACK db4efdeb6d

    I'm happy with FillInputToWeight() now, thanks for addressing comments. The unit tests seem comprehensive. I tried every input from -1 to 65536 and no crashes. Minimum input of 165 = 4 * (32 + 4 + 4 + 1) + 1 makes more sense to me than 160. Test coverage for walletcreatefundedpsbt, fundrawtransaction, and send look good to me, error messages and resulting feerates are tested and the weights used are correct now.

  87. in src/wallet/wallet.cpp:1515 in e7a318a8a8 outdated 
    1506 | @@ -1507,6 +1507,41 @@ bool DummySignInput(const SigningProvider& provider, CTxIn &tx_in, const CTxOut
1507 |      return true;
1508 |  }
1509 |  
1510 | +void FillInputToWeight(CTxIn& txin, int64_t target_weight)
1511 | +{
1512 | +    int64_t txin_weight = GetTransactionInputWeight(txin);
1513 | +
1514 | +    // Do nothing if the weight that should be added is less than the weight that already exists
1515 | +    if (target_weight <= txin_weight) {
    instagibbs commented at 2:39 AM on January 20, 2022:

    nit: let's assert that no witness data exists yet?

    achow101 commented at 4:29 PM on January 24, 2022:

    Done

  88. in src/wallet/wallet.cpp:1562 in e7a318a8a8 outdated 
    1549 | @@ -1515,6 +1550,12 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut>
1550 |      for (const auto& txout : txouts)
1551 |      {
1552 |          CTxIn& txin = txNew.vin[nIn];
1553 | +        // If weight was provided, fill the input to that weight
1554 | +        if (coin_control && coin_control->HasInputWeight(txin.prevout)) {
    instagibbs commented at 2:42 AM on January 20, 2022:

    If the given value is "too low" as per https://github.com/bitcoin/bitcoin/pull/23201/commits/e7a318a8a88489477264060ccb332a719260fa67#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8R1515, it looks like logic uses just thinks it signed and returns true.

    There's test coverage from the RPC perspective but ideally we'd catch this issue by returning false?

    achow101 commented at 4:30 PM on January 24, 2022:

    Changed to return a bool.

  89. in src/wallet/rpc/spend.cpp:570 in db4efdeb6d outdated 
     565 | +            const UniValue& weight_v = find_value(input, "weight");
 566 | +            if (!weight_v.isNum()) {
 567 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, missing weight key");
 568 | +            }
 569 | +            int64_t weight = weight_v.get_int64();
 570 | +            if (weight < 41 * WITNESS_SCALE_FACTOR + 1) {
    instagibbs commented at 2:51 AM on January 20, 2022:

    follow-up only: would be nice if this kind of thing wasn't checked in two places in different ways. I prefer the other approach of directly measuring what a blank input size is.

    achow101 commented at 4:30 PM on January 24, 2022:

    Done

  90. instagibbs approved
  91. instagibbs commented at 2:53 AM on January 20, 2022: member

    ACK db4efdeb6d5ffd982777197d72c8c2a0f1ea82f9

  92. wallet: add input weights to CCoinControl
    In order to allow coin selection to take weights from the user,
CCoinControl needs to be able to set and get them.
    4060c50d7e
  93. wallet: Allow user specified input size to override
    If the user specifies an input size, allow it to override any input size
calculations during coin selection.
    808068e90e
  94. rpc, wallet: Allow users to specify input weights
    Coin selection requires knowing the weight of a transaction so that fees
can be estimated. However for external inputs, the weight may not be
avialble, and solving data may not be enough as the input could be one
that we do not support. By allowing users to specify input weights,
those external inputs can be included in the transaction.

Additionally, if the weight for an input is specified, that value will
always be used, regardless of whether the input is in the wallet or
solving data is available. This allows us to account for scenarios where
the wallet may be more conservative and estimate a larger input than may
actually be created.

For example, we assume the maximum DER signature size, but an external
input may be signed by a wallet which does nonce grinding in order to get
a smaller signature. In that case, the user can specify the smaller
input weight to avoid overpaying transaction fees.
    6fa762a372
  95. tests: Test specifying input weights
    Added tests to rpc_fundrawtransaction, wallet_send, and rpc_psbt that
test that external inputs can be spent when input weight is provided.
Also tested that the input weight overrides any calculated weight.

Additionally, rpc_psbt's external inputs test is cleaned up a bit to be
more similar to rpc_fundrawtransaction's and avoid potential pitfalls
due to non-deterministic coin selection behavior.
    3866272c45
  96. achow101 force-pushed on Jan 24, 2022
  98. fanquake requested review from glozow on Jan 25, 2022
  99. fanquake commented at 2:50 AM on January 25, 2022: member

    @t-bast want to take another look here?

  100. t-bast approved
  101. t-bast commented at 9:08 AM on January 25, 2022: member

    ACK https://github.com/bitcoin/bitcoin/pull/23201/commits/3866272c450cc659207fbc2cff3c690ae8593341

    I re-run tests from a lightning node: I'm correctly able to use this new parameter to make bitcoind fund then sign transactions that contain inputs that aren't managed by the bitcoind wallet (in my case, HTLC outputs from a lightning commitment transaction).

    A big thanks to everyone who contributed to this PR!

  102. glozow commented at 10:11 AM on January 25, 2022: member

    reACK 3866272 via range-diff

    Changes since last review:

    • Changed FillInputToWeight() to return a bool for information about whether filling happened or if the input was invalid, changed DummySigns and tests to reflect that.
    • assertions in FillInputToWeight() that scriptSig and witness start out empty
    • changed the 165 input minimum to be calculated using size of empty input instead of arithmetic, added a CHECK_NONFATAL that it matches 165
  103. glozow approved
  104. laanwj merged this on Jan 25, 2022
  105. laanwj closed this on Jan 25, 2022
  106. sidhujag referenced this in commit 4d48ec3d85 on Jan 28, 2022
  107. fanquake referenced this in commit 607d5a46aa on Aug 22, 2022
  108. DrahtBot locked this on Jan 25, 2023
Contributors
achow101meshcolliderfanquaket-bastMarcoFalkeDrahtBotinstagibbsyanmaanirajarshimaitraglozowgaarfpromagbenthecarman
Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#23187 RPC: Allow user to supply weight of external input#23202 wallet: allow psbtbumpfee to work with txs with external inputs
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: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me