[wallet] couple of walletcreatefundedpsbt fixes #13968

instagibbs · 2018-08-14T18:42:58Z

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.
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.

achow101

Note that the first commit also effects createrawtransaction.

achow101 · 2018-08-14T18:50:16Z

src/wallet/rpcwallet.cpp

@@ -4715,10 +4714,16 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
        }, true
    );

+


Nit: miscellaneous whitespace change

achow101 · 2018-08-14T18:51:53Z

src/wallet/rpcwallet.cpp

    CAmount fee;
    int change_position;
    CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]);
-    FundTransaction(pwallet, rawTx, fee, change_position, request.params[4]);
+    // Make a copy and edit replaceability in-place
+    UniValue params_copy(request.params);


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.

achow101 · 2018-08-14T19:32:32Z

src/wallet/rpcwallet.cpp

+    if (!request.params[3].isNull()){
+        params_copy.pushKV("replaceable", request.params[3].get_bool());
+    }
+    FundTransaction(pwallet, rawTx, fee, change_position, params_copy[4]);


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

achow101 · 2018-08-14T19:33:15Z

src/wallet/rpcwallet.cpp

+    // Make a copy and edit replaceability in-place
+    UniValue params_copy(request.params[4]);
+    if (!request.params[3].isNull()){
+        params_copy.pushKV("replaceable", request.params[3].get_bool());


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

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

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

instagibbs · 2018-08-15T01:34:06Z

reworked to preseve the options replaceable instead

instagibbs · 2018-08-15T01:53:09Z

pushed basic functional test

instagibbs · 2018-08-15T01:59:47Z

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.

instagibbs · 2018-08-16T19:06:49Z

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

instagibbs · 2018-08-19T22:17:45Z

? Isn't 0.17 the first release with PSBT?

…

On Sun, Aug 19, 2018, 6:05 PM João Barbosa ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/wallet/rpcwallet.cpp <#13968 (comment)>: > @@ -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 <#13968 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC00pHDttEoIq9rL8lAL8PnsPDrj0hks5uSeEPgaJpZM4V8-M0> .

promag · 2018-08-19T22:22:41Z

? Isn't 0.17 the first release with PSBT?

Yes, does that matter?

sipa · 2018-08-19T22:26:36Z

So it's not in a release yet?

promag · 2018-08-19T22:28:19Z

🤦‍♂️ it's not released..

promag

Should update L4646:

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

And vRPCConvertParams in src/rpc/client.cpp.

instagibbs · 2018-08-20T16:59:10Z

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

DrahtBot · 2018-08-20T18:23:03Z

Note to reviewers: This pull request conflicts with the following ones:

Refactoring CRPCCommand with enum category #13945 (Refactoring CRPCCommand with enum category by isghe)
[wallet] Kill accounts #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.

achow101 · 2018-08-21T02:48:01Z

utACK faaac5c

laanwj · 2018-08-21T07:44:15Z

utACK faaac5c

faaac5c RPCTypeCheck bip32derivs arg in walletcreatefunded (Gregory Sanders) 1f0c428 QA: add basic walletcreatefunded optional arg test (Gregory Sanders) 1f18d7b walletcreatefundedpsbt: remove duplicate replaceable arg (Gregory Sanders) 2252ec5 Allow ConstructTransaction to not throw error with 0-input txn (Gregory Sanders) Pull request description: 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`. Tree-SHA512: 26eb0c9e2d38ea51d11f741d61100223253271a084adadeb7e78c6d4e9004636f089e4273c5bf64a41bd7e9ff795317acf30531cb36aeb0d8db9304b3c8270c3

Github-Pull: #13968 Rebased-From: 2252ec5 Tree-SHA512: 1f9978ee25fbe9bb338af19d81b401fa531d314ac8288cdb21c1bf10459edea50b43e2d5e97c9bb5fe24c8db89363f8233c0a3d96066ed85f7bd6d2eb234aac0

Github-Pull: #13968 Rebased-From: 1f18d7b Tree-SHA512: 90391703181db6880a135c60aca792a9e92c4abcad26907cd6cb0a0378593fe45cf995a22ae142ea7de2767c72a9df444e918ff15e460ce19c0435163917d812

Github-Pull: #13968 Rebased-From: 1f0c428 Tree-SHA512: 1f8b10629a314f623d589801ef2e62724de2cd82a0e523e0ab25a285f92b76a3b31304c1c0418b1b611ec3ca0016016d1e6af410ac81a78449b875c4f89a46d7

Github-Pull: #13968 Rebased-From: faaac5c Tree-SHA512: 758c0c3e4435897d1a9b03ea93f1b2a1a1b64071eda9450f968acf537c172ee61acf9d962bc22ddb6de26e0ad39d9165cdee6f260bb5a95bf97b4003853f0874

61fe653 fix walletcreatefundedpsbt deriv paths, add test (Gregory Sanders) Pull request description: Added the regression in #13968 Tree-SHA512: a31290b57ed80a8486925e562ca5412500d4215a238de7e448f48edfa671c87aebd79ee179a8340b289d9811ae6fa30ef75eefd5f5890fb6285174c5db72ff65

Github-Pull: bitcoin#13968 Rebased-From: 2252ec5 Tree-SHA512: 1f9978ee25fbe9bb338af19d81b401fa531d314ac8288cdb21c1bf10459edea50b43e2d5e97c9bb5fe24c8db89363f8233c0a3d96066ed85f7bd6d2eb234aac0

Github-Pull: bitcoin#13968 Rebased-From: 1f18d7b Tree-SHA512: 90391703181db6880a135c60aca792a9e92c4abcad26907cd6cb0a0378593fe45cf995a22ae142ea7de2767c72a9df444e918ff15e460ce19c0435163917d812

Github-Pull: bitcoin#13968 Rebased-From: 1f0c428 Tree-SHA512: 1f8b10629a314f623d589801ef2e62724de2cd82a0e523e0ab25a285f92b76a3b31304c1c0418b1b611ec3ca0016016d1e6af410ac81a78449b875c4f89a46d7

Github-Pull: bitcoin#13968 Rebased-From: faaac5c Tree-SHA512: 758c0c3e4435897d1a9b03ea93f1b2a1a1b64071eda9450f968acf537c172ee61acf9d962bc22ddb6de26e0ad39d9165cdee6f260bb5a95bf97b4003853f0874

faaac5c RPCTypeCheck bip32derivs arg in walletcreatefunded (Gregory Sanders) 1f0c428 QA: add basic walletcreatefunded optional arg test (Gregory Sanders) 1f18d7b walletcreatefundedpsbt: remove duplicate replaceable arg (Gregory Sanders) 2252ec5 Allow ConstructTransaction to not throw error with 0-input txn (Gregory Sanders) Pull request description: 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`. Tree-SHA512: 26eb0c9e2d38ea51d11f741d61100223253271a084adadeb7e78c6d4e9004636f089e4273c5bf64a41bd7e9ff795317acf30531cb36aeb0d8db9304b3c8270c3

Allow ConstructTransaction to not throw error with 0-input txn

2252ec5

achow101 reviewed Aug 14, 2018

View reviewed changes

instagibbs force-pushed the wcpsbtreplace branch from 6a5822d to ac0778b Compare August 14, 2018 19:16

achow101 reviewed Aug 14, 2018

View reviewed changes

fanquake added the Wallet label Aug 14, 2018

instagibbs force-pushed the wcpsbtreplace branch from ac0778b to 2020fe1 Compare August 15, 2018 01:33

fanquake added the Needs backport label Aug 17, 2018

fanquake added this to the 0.17.0 milestone Aug 17, 2018

promag reviewed Aug 19, 2018

View reviewed changes

instagibbs force-pushed the wcpsbtreplace branch from 3d3f08a to 547bc0f Compare August 20, 2018 16:58

instagibbs added 3 commits August 20, 2018 13:02

walletcreatefundedpsbt: remove duplicate replaceable arg

1f18d7b

QA: add basic walletcreatefunded optional arg test

1f0c428

RPCTypeCheck bip32derivs arg in walletcreatefunded

faaac5c

instagibbs force-pushed the wcpsbtreplace branch from 547bc0f to faaac5c Compare August 20, 2018 17:02

This was referenced Aug 20, 2018

Refactoring CRPCCommand with enum category #13945

Closed

[wallet] Kill accounts #13825

Closed

laanwj merged commit faaac5c into bitcoin:master Aug 21, 2018

laanwj removed the Needs backport label Aug 21, 2018

instagibbs mentioned this pull request Aug 24, 2018

fix walletcreatefundedpsbt deriv paths, add test #14055

Merged

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

HashUnlimited mentioned this pull request Sep 14, 2018

Allow ConstructTransaction to not throw error with 0-input txn chaincoin/chaincoin#661

Merged

HashUnlimited mentioned this pull request Sep 14, 2018

walletcreatefundedpsbt: remove duplicate replaceable arg chaincoin/chaincoin#662

Merged

HashUnlimited mentioned this pull request Sep 14, 2018

QA: add basic walletcreatefunded optional arg test chaincoin/chaincoin#663

Merged

HashUnlimited mentioned this pull request Sep 14, 2018

RPCTypeCheck bip32derivs arg in walletcreatefunded chaincoin/chaincoin#664

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -4715,10 +4714,16 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
		}, true
		);

[wallet] couple of walletcreatefundedpsbt fixes #13968

[wallet] couple of walletcreatefundedpsbt fixes #13968

Uh oh!

Conversation

instagibbs commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Aug 15, 2018

Uh oh!

instagibbs commented Aug 15, 2018

Uh oh!

instagibbs commented Aug 15, 2018

Uh oh!

instagibbs commented Aug 16, 2018

Uh oh!

instagibbs commented Aug 19, 2018 via email

Uh oh!

promag commented Aug 19, 2018

Uh oh!

sipa commented Aug 19, 2018

Uh oh!

promag commented Aug 19, 2018

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Aug 20, 2018

Uh oh!

DrahtBot commented Aug 20, 2018

Uh oh!

achow101 commented Aug 21, 2018

Uh oh!

laanwj commented Aug 21, 2018

Uh oh!

Uh oh!

instagibbs commented Aug 14, 2018 •

edited

Loading

promag left a comment •

edited

Loading