[wallet] couple of walletcreatefundedpsbt fixes #13968

pull instagibbs wants to merge 4 commits into bitcoin:master from instagibbs:wcpsbtreplace changing 4 files +41 −14
  1. instagibbs commented at 6:42 PM on August 14, 2018: member

    1. Previously an empty input argument transaction that is marked for replaceability fails to pass the SignalsOptInRBF check right before funding it. Explicitly check for that condition before throwing an error.

    2. The rpc call had two separate replaceable arguments, each of which being used in mutually exclusive places. I preserved the options version to retain compatability with fundtransaction.

  2. Allow ConstructTransaction to not throw error with 0-input txn 2252ec5008
  3. in src/wallet/rpcwallet.cpp:4717 in 6a5822d267 outdated 
    4713 | @@ -4715,10 +4714,16 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
4714 |          }, true
4715 |      );
4716 |  
4717 | +
    achow101 commented at 6:50 PM on August 14, 2018:

    Nit: miscellaneous whitespace change

  4. in src/wallet/rpcwallet.cpp:4722 in 6a5822d267 outdated 
    4718 |      CAmount fee;
4719 |      int change_position;
4720 |      CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]);
4721 | -    FundTransaction(pwallet, rawTx, fee, change_position, request.params[4]);
4722 | +    // Make a copy and edit replaceability in-place
4723 | +    UniValue params_copy(request.params);
    achow101 commented at 6:51 PM on August 14, 2018:

    I don't think this is correct. The parameters for these options are in request.params[4] but what you are doing here is adding it to request.params so when replaceable is looked up in params_copy[4], it won't be there.

    instagibbs commented at 7:16 PM on August 14, 2018:

    derp

  5. achow101 commented at 6:52 PM on August 14, 2018: member

    Note that the first commit also effects createrawtransaction.

  6. instagibbs force-pushed on Aug 14, 2018
  7. in src/wallet/rpcwallet.cpp:4725 in ac0778b113 outdated 
    4721 | +    // Make a copy and edit replaceability in-place
4722 | +    UniValue params_copy(request.params[4]);
4723 | +    if (!request.params[3].isNull()){
4724 | +        params_copy.pushKV("replaceable", request.params[3].get_bool());
4725 | +    }
4726 | +    FundTransaction(pwallet, rawTx, fee, change_position, params_copy[4]);
    achow101 commented at 7:32 PM on August 14, 2018:

    This should just be params_copy, not params_copy[4].

  8. in src/wallet/rpcwallet.cpp:4723 in ac0778b113 outdated 
    4716 | @@ -4718,7 +4717,12 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
4717 |      CAmount fee;
4718 |      int change_position;
4719 |      CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]);
4720 | -    FundTransaction(pwallet, rawTx, fee, change_position, request.params[4]);
4721 | +    // Make a copy and edit replaceability in-place
4722 | +    UniValue params_copy(request.params[4]);
4723 | +    if (!request.params[3].isNull()){
4724 | +        params_copy.pushKV("replaceable", request.params[3].get_bool());
    achow101 commented at 7:33 PM on August 14, 2018:

    Suppose someone already has replaceable set in their options (because they copied it from fundrawtransaction or something). How does this interact with that?

    instagibbs commented at 8:02 PM on August 14, 2018:

    I'm not sure what's best in this case.

    instagibbs commented at 8:11 PM on August 14, 2018:

    Perhaps I can reverse my decision and only use the option's version.

  9. fanquake added the label Wallet on Aug 14, 2018
  10. instagibbs force-pushed on Aug 15, 2018
  11. instagibbs commented at 1:34 AM on August 15, 2018: member

    reworked to preseve the options replaceable instead

  12. instagibbs commented at 1:53 AM on August 15, 2018: member

    pushed basic functional test

  13. instagibbs commented at 1:59 AM on August 15, 2018: member

    I'll note that when locktime == 0, ConstructTransaction adding inputs and FundTransaction adding inputs acts differently: The former does CTxIn::SEQUENCE_FINAL while the latter does CTxIn::SEQUENCE_FINAL-1. This is kind of weird and probably a fingerprint.

  14. instagibbs commented at 7:06 PM on August 16, 2018: member

    Can I get 0.17 backport tag? This is a bugfix.

  15. laanwj added this to the "Blockers" column in a project
  16. fanquake added the label Needs backport on Aug 17, 2018
  17. fanquake added this to the milestone 0.17.0 on Aug 17, 2018
  18. instagibbs commented at 10:17 PM on August 19, 2018: member

    ? Isn't 0.17 the first release with PSBT?

    On Sun, Aug 19, 2018, 6:05 PM João Barbosa notifications@github.com wrote:

    @promag commented on this pull request.

    In src/wallet/rpcwallet.cpp https://github.com/bitcoin/bitcoin/pull/13968#discussion_r211116677:

    @@ -4670,9 +4670,8 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) " accepted as second parameter.\n" " ]\n" "3. locktime (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"

    •                        "4. replaceable               (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n"

    Can this be done? This was already released. At least add a release note?

    — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/13968#pullrequestreview-147472262, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC00pHDttEoIq9rL8lAL8PnsPDrj0hks5uSeEPgaJpZM4V8-M0 .

  19. promag commented at 10:22 PM on August 19, 2018: member

    ? Isn't 0.17 the first release with PSBT?

    Yes, does that matter?

  20. sipa commented at 10:26 PM on August 19, 2018: member

    So it's not in a release yet?

  21. promag commented at 10:28 PM on August 19, 2018: member

    🤦‍♂️ it's not released..

  22. promag commented at 10:38 PM on August 19, 2018: member

    Should update L4646:

    if (request.fHelp || request.params.size() < 2 || request.params.size() > 5)

    And vRPCConvertParams in src/rpc/client.cpp.

  23. instagibbs force-pushed on Aug 20, 2018
  24. instagibbs commented at 4:59 PM on August 20, 2018: member

    Looks like we're also missing the arg type check for the bip32derivs arg, pushed a fix for that.

  25. walletcreatefundedpsbt: remove duplicate replaceable arg 1f18d7b591
  26. QA: add basic walletcreatefunded optional arg test 1f0c4282e9
  27. RPCTypeCheck bip32derivs arg in walletcreatefunded faaac5caaa
  28. instagibbs force-pushed on Aug 20, 2018
  29. DrahtBot commented at 6:23 PM on August 20, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #13945 (Refactoring CRPCCommand with enum category by isghe)
    • #13825 ([wallet] Kill accounts by jnewbery)

    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.

  30. achow101 commented at 2:48 AM on August 21, 2018: member

    utACK faaac5caaab4d5131040292f4ef2404074ad268b

  31. laanwj commented at 7:44 AM on August 21, 2018: member

    utACK faaac5caaab4d5131040292f4ef2404074ad268b

  32. laanwj merged this on Aug 21, 2018
  33. laanwj closed this on Aug 21, 2018
  34. laanwj referenced this in commit 8aa9badf5e on Aug 21, 2018
  35. laanwj removed the label Needs backport on Aug 21, 2018
  36. laanwj referenced this in commit c6d905746b on Aug 21, 2018
  37. laanwj referenced this in commit 65e7a8b97f on Aug 21, 2018
  38. laanwj referenced this in commit 82e2b9cb25 on Aug 21, 2018
  39. laanwj referenced this in commit 9833545d18 on Aug 21, 2018
  40. laanwj removed this from the "Blockers" column in a project
  41. laanwj referenced this in commit 5de338ec72 on Aug 28, 2018
  42. uhliksk referenced this in commit 1c072bde00 on Aug 29, 2018
  43. uhliksk referenced this in commit 32dea40b9a on Aug 29, 2018
  44. uhliksk referenced this in commit eb68bad8f3 on Aug 29, 2018
  45. uhliksk referenced this in commit 5357c29def on Aug 29, 2018
  46. HashUnlimited referenced this in commit f677a23db2 on Sep 14, 2018
  47. HashUnlimited referenced this in commit 8e0e6301bd on Sep 14, 2018
  48. HashUnlimited referenced this in commit 4008e1681d on Sep 14, 2018
  49. HashUnlimited referenced this in commit 7ef55826b3 on Sep 14, 2018
  50. Munkybooty referenced this in commit 8dd7f857c8 on Sep 8, 2021
  51. MarcoFalke locked this on Sep 8, 2021
Contributors
instagibbsachow101promagsipaDrahtBotlaanwj
Labels
Wallet

Milestone
0.17.0

Linked (view graph)
#14055 fix walletcreatefundedpsbt deriv paths, add test
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-21 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me