fundrawtransaction #6088

pull TheBlueMatt wants to merge 5 commits into bitcoin:master from TheBlueMatt:frt2 changing 12 files +787 −14

TheBlueMatt commented at 8:54 PM on April 30, 2015: member

This superceedes #5503 and #5524. I rewrote chunks of the first and largely rewrote the second, stealing test cases almost verbatim from both.
laanwj added the label Wallet on May 1, 2015
laanwj added the label RPC on May 1, 2015
laanwj added this to the milestone 0.11.0 on May 1, 2015
sipa commented at 12:45 PM on May 1, 2015: member

Could we have a DummySignatureCreator, instead of passing a dummy boolean to TransactionSignatureCreator?
sipa commented at 12:50 PM on May 1, 2015: member

I had to read the source code to guess what 'fAllowOtherInputs' means. Can you add a comment?
jonasschnelli commented at 12:52 PM on May 1, 2015: contributor

@sipa: I tried this ("DummySignatorCreator"). But somehow i stopped it because it was getting a inheritance mess. I can't actually remember why exactly.
sipa commented at 12:55 PM on May 1, 2015: member

@jonasschnelli Let me hack something up.
sipa commented at 1:24 PM on May 1, 2015: member

See https://github.com/sipa/bitcoin/commits/dummysig
jonasschnelli commented at 2:11 PM on May 1, 2015: contributor

@sipa: are you sure this would work also for P2SH Multisig inputs?
sipa commented at 2:19 PM on May 1, 2015: member

@jonasschnelli pretty sure, it should.

BaseSignatureCreator is about creating individual (DER+nHashType) signatures, and is called by ProduceSignature wherever necessary. For multisig/P2SH, it will be called multiple times as necessary.
jonasschnelli commented at 5:05 PM on May 1, 2015: contributor

@sipa: Right. I would do so. To make use of https://github.com/sipa/bitcoin/commit/134090be5a0aaad27981ed4d4fe52a843fce337b it would need some adaptation and some changes within SignSignature(). I just tried but had some compiling/casting issues with the DummySignatureChecker class.

But the current solution (as it is in this PR) without a DummySignatorCreator class works well and basically adds only 8 lines of code. But indeed its not that elegant as sipas proposal.
sipa commented at 5:18 PM on May 1, 2015: member

I'm fine with fixing it up afterwards.
jgarzik commented at 4:02 PM on May 2, 2015: contributor

Concept ACK - the dummy sig stuff is ugly and poops all over several function/method sigs
laanwj commented at 8:07 AM on May 6, 2015: member

The dummy sig business may be ugly, but it was introduced to avoid even uglier solutions to compute signature sizes: either having parallel byte accounting functioning (lots of duplicate hard-to-crosscheck code), or doing real signing then throwing away the result (requires wallet to be unlocked and is just wrong).
laanwj removed this from the milestone 0.11.0 on May 18, 2015
jonasschnelli commented at 1:45 PM on May 18, 2015: contributor

Needs rebase.
luke-jr commented at 3:15 AM on June 2, 2015: member

Dummy sign may be ugly, but it also would enable prompting the user with fee etc prior to passphrase being entered...

in src/wallet/rpcdump.cpp:None in 0b05c94ff6 outdated

 157 |              "\nAdds an address or script (in hex) that can be watched as if it were in your wallet but cannot be used to spend.\n"
 158 |              "\nArguments:\n"
 159 |              "1. \"address\"          (string, required) The address\n"
 160 |              "2. \"label\"            (string, optional, default=\"\") An optional label\n"
 161 |              "3. rescan               (boolean, optional, default=true) Rescan the wallet for transactions\n"
 162 | +            "4. p2sh                 (boolean, optional, default=false) Add the P2SH version of the script as well\n"

luke-jr commented at 3:16 AM on June 2, 2015:

This seems nonsensical. What is the use case?

TheBlueMatt commented at 6:58 AM on June 11, 2015: member

So it turns out the watch-only signing never worked anyway (it used the constant "0" for the public key when calculating size of pay-to-pubkey-hash txn), so I walked that back and watchonly-supporting fundrawtransaction will be a separate pull.
TheBlueMatt force-pushed on Jun 11, 2015
TheBlueMatt force-pushed on Jun 11, 2015
TheBlueMatt force-pushed on Jun 11, 2015
Add DummySignatureCreator which just creates zeroed sigs 9b4e7d9a5e
Small tweaks to CCoinControl for fundrawtransaction 2d84e22703

Add FundTransaction method to wallet

Some code stolen from Jonas Schnelli <jonas.schnelli@include7.ch>

1e0d1a2ff0

Add fundrawtransaction RPC method 21bbd920e5
fundrawtransaction tests 208589514c
TheBlueMatt force-pushed on Jun 11, 2015
laanwj merged this on Jun 23, 2015
laanwj closed this on Jun 23, 2015
laanwj referenced this in commit 91389e51c7 on Jun 23, 2015
btcdrak commented at 10:57 PM on June 23, 2015: contributor

ACK
laanwj commented at 5:14 AM on June 24, 2015: member

Tested ACK (which I forgot to post)
jonasschnelli commented at 6:39 AM on June 24, 2015: contributor

Post merge ACK

in src/wallet/wallet.cpp:None in 208589514c

1753 | +    CReserveKey reservekey(this);
1754 | +    CWalletTx wtx;
1755 | +    if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosRet, strFailReason, &coinControl, false))
1756 | +        return false;
1757 | +
1758 | +    if (nChangePosRet != -1)

sipa commented at 8:34 PM on June 24, 2015:

Any reason for trying to guess the changes made by CreateTransaction, and applying those on the original, rather than just using the constructed result?

sipa commented at 9:03 PM on June 24, 2015: member

Posthumous untested ACK.
zkbot referenced this in commit 9af55822fb on Feb 15, 2017
zkbot referenced this in commit a7cf698873 on Mar 4, 2017
furszy referenced this in commit 0724bbbad2 on Jun 28, 2020
random-zebra referenced this in commit 5a092159f6 on Aug 5, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#5503 [RPC] fundrawtransaction #5524 Implement watchonly for fundrawtransaction