[RPC] fundrawtransaction #5503

jonasschnelli commented at 9:22 PM on December 17, 2014: contributor

Basic implementation of a new rpc call fundrawtransaction. This call will fill a rawtransaction containing only vouts with unspent coins from the wallet.

Currently there is no way to return the CReserveKey to the keypool.

jonasschnelli force-pushed on Dec 17, 2014

jonasschnelli commented at 9:26 PM on December 17, 2014: contributor

As createrawtransactions with potential unspent vins, this call does not lock the coins somehow.

in src/rpcrawtransaction.cpp:None in 88de6ee955 outdated

 792 | +    CTransaction tx;
 793 | +    if (!DecodeHexTx(tx, params[0].get_str()))
 794 | +        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
 795 | +    
 796 | +    if (tx.vin.size() > 0)
 797 | +        throw JSONRPCError(RPC_INVALID_PARAMETER, "fundrawtransaction only supports transactions with zero exiting vins");

aalness commented at 9:53 AM on December 18, 2014:

s/exiting/existing

aalness commented at 6:31 PM on December 18, 2014: contributor

ACK the idea. It seems useful.

gmaxwell commented at 7:25 PM on December 18, 2014: contributor

Sweet. This should have a mechenism to also include watchonly coins.

gmaxwell commented at 7:33 PM on December 18, 2014: contributor

It should support existing VINs, which are useful if you want to specifically spend a particular coin, but need more inputs to complete the txn. One reason you may want to do this is to achieve mutual exclusion with a prior transaction. If the existing vins are adequate it shouldn't add any more.

kanzure commented at 9:39 PM on December 19, 2014: contributor

Also, not to pile on too much, but a "fundmany" would be neat as well (in addition to watchonly support in fundrawtransaction). This would be useful for batch coin selection, offline signing, and coinjoin. Plus, fundmany is not a huge jump from sendmany.

Also, gmaxwell proposed on IRC a limit=N parameter on fundrawtransaction?

kanzure commented at 8:35 PM on December 20, 2014: contributor

Nevermind about fundmany: http://bitcoinstats.com/irc/bitcoin-dev/logs/2014/12/19#l1419025192

jonasschnelli force-pushed on Dec 20, 2014

jonasschnelli commented at 9:07 PM on December 20, 2014: contributor

Added support for existing VINs. Now comes with an extensive test-script. @gmaxwell the proposed limit parameter needs probably some brainwork first. See (http://bitcoinstats.com/irc/bitcoin-dev/logs/2014/12/20#l1419085131). Maybe another pull.

jonasschnelli force-pushed on Dec 20, 2014

kanzure referenced this in commit 034b8544c6 on Dec 21, 2014

jonasschnelli force-pushed on Dec 22, 2014

jonasschnelli commented at 8:25 AM on December 22, 2014: contributor

Just fixed an issue in CreateTransaction which produced endless recursive loops. Travis should now be happy.

kanzure referenced this in commit 25e83c2b43 on Dec 22, 2014

in src/wallet.cpp:None in 20e1d72881 outdated

1372 | +                if (!out.fSpendable)
1373 | +                    continue;
1374 | +                
1375 | +                nValueTroughVINs    += out.tx->vout[out.i].nValue;
1376 | +                
1377 | +                // temporary keep the coin to add them later after SelectCoinsMinConf has added some

fanquake commented at 1:10 PM on December 27, 2014:

s/temporary/temporarily

jonasschnelli commented at 7:53 PM on December 27, 2014:

Thanks. Fixed.

jonasschnelli force-pushed on Dec 27, 2014

laanwj added the label Docs and Output on Jan 8, 2015

laanwj removed the label Docs and Output on Jan 8, 2015

laanwj added the label RPC on Jan 8, 2015

kanzure referenced this in commit 59d167d49f on Jan 16, 2015

kanzure referenced this in commit cd51e3e123 on Jan 16, 2015

kanzure referenced this in commit 3ce454fe04 on Jan 31, 2015

laanwj added the label Wallet on Feb 27, 2015

laanwj added this to the milestone 0.11.0 on Feb 27, 2015

laanwj commented at 1:37 PM on February 27, 2015: member

needs rebase (assigned 0.11 milestone, would be nice to have this)

jonasschnelli force-pushed on Mar 5, 2015

jonasschnelli commented at 8:04 AM on March 5, 2015: contributor

rebased.

jonasschnelli commented at 8:02 AM on March 14, 2015: contributor

should solve #3794

jonasschnelli commented at 8:06 AM on March 14, 2015: contributor

binaries to test: https://builds.jonasschnelli.ch/pulls/5503/

kanzure referenced this in commit a3af4c1c11 on Mar 15, 2015

jonasschnelli force-pushed on Mar 21, 2015

jonasschnelli commented at 8:28 PM on March 21, 2015: contributor

Rebased (hard rebase after "Subtract fee from amount [#5831]"), added some code comments.

jonasschnelli force-pushed on Mar 22, 2015

in src/rpcserver.cpp:None in e5819b005c outdated

 320 | @@ -321,7 +321,9 @@ static const CRPCCommand vRPCCommands[] =
 321 |      { "rawtransactions",    "getrawtransaction",      &getrawtransaction,      true,      false },
 322 |      { "rawtransactions",    "sendrawtransaction",     &sendrawtransaction,     false,     false },
 323 |      { "rawtransactions",    "signrawtransaction",     &signrawtransaction,     false,     false }, /* uses wallet if enabled */
 324 | -
 325 | +#ifdef ENABLE_WALLET
 326 | +    { "rawtransactions",    "fundrawtransaction",     &fundrawtransaction,     false,     false },

jgarzik commented at 1:51 PM on March 23, 2015:

Bug: should be 'true' to indicate wallet requirement. Otherwise, the 'pwalletMain' reference will oops when NULL (wallet support built, but disabled at runtime).

jonasschnelli commented at 7:18 PM on March 23, 2015:

Thanks for catching this one! Fixed and pushed.

jgarzik commented at 2:08 PM on March 23, 2015: contributor

ut ACK + plan to test this this week (modulo bug fix mentioned above of course)

jonasschnelli force-pushed on Mar 23, 2015

in src/wallet/wallet.cpp:None in 0f1fe50bc9 outdated

1892 | @@ -1823,6 +1893,14 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend,
1893 |                      strFailReason = _("Transaction too large");
1894 |                      return false;
1895 |                  }
1896 | +                
1897 | +                //remove signature if we used the signing only for the fee calculation

laanwj commented at 8:18 AM on March 27, 2015:

Why sign at all if it's only for the fee calculation? I ask because we don't want to unnecessarily require the signing keys.

jonasschnelli commented at 7:45 PM on April 2, 2015:

We still need to remove the signature, but now, during fundrawtransaction, the signature is dummy/void.

laanwj commented at 9:32 AM on April 10, 2015:

Yes, removing a dummy sigature is fine.

jonasschnelli commented at 8:02 AM on March 31, 2015: contributor

Currently i'm working on this to allow fundrawtransaction to calculate serialized transaction length for getting the right fee and priority without actually signing the tx. Tests won't make sense at the moment.

jonasschnelli force-pushed on Apr 2, 2015

jonasschnelli commented at 7:41 PM on April 2, 2015: contributor

Updated! fundrawtransaction now works with locked wallets. Fee / priority calculation now goes over a new dummy-signing way in sign.cpp. RPC tests are now even more extensive and includes multisig test, locked wallet tests as well as comparison of calculated fee and correct fee tests.

The only question is, if it would be worth to add a optional correctFee=1|0 argument for the fundrawtransaction rpc call. With settings this flag, users could enforce correct signing of the tx (would require unlocked wallet) which would end up in getting a more precise/correct fee.

jonasschnelli force-pushed on Apr 16, 2015

jonasschnelli commented at 7:55 AM on April 16, 2015: contributor

rebased.

Moved RPC function "fundrawtransaction()" from rpcrawtransaction.cpp to rpcwallet.cpp (fundrawtransaction is a wallet function). Added wallet-is-presence check because we recently removed "reqWallet" check over the RPC dispatch table.

jonasschnelli force-pushed on Apr 20, 2015

[RPC] fundrawtransaction basics 8369b93f9e

[RPC] add support for existing vins for `fundrawtransaction` 111b248648

[RPC] add simple unittest for `fundrawtransaction` (increase test coverage) d0ea91a99d

[RPC] fundrawtransaction overhaul

- fix typo
- fix RPCTypeCheck for fundrawtransaction
- some comments
- merged with subtractFeeFromAmout patch (non trivial)

7863a700a0

[RPC] fundrawtransaction dummy signing / ser. length calculation

- serialize transaction size calculation (aka dummy-signing) without the need of signing the transaction (wallet can be locked to use fundrawtransaction)
- rename VINS to vInputs

5ef0b3306a

[moveonly] move fundrawtransaction() rpc call to rpcwallet.cpp

fundrawtransaction requires a wallet and therefore it belongs to rpcwallet.cpp

50fc9e810c

add wallet-is-present check for fundrawtransaction

rpc dispatch tables reqWallet has been removed.

4109df978a

[QA] adapt generate instead of setgenerate for fundrawtransaction tests 79858cd7c8

jonasschnelli force-pushed on Apr 21, 2015

jonasschnelli commented at 6:25 PM on April 21, 2015: contributor

rebased.

TheBlueMatt commented at 12:50 AM on April 24, 2015: member

I needed to use this for something and rewrote/tweaked a few chunks. See changes at https://github.com/TheBlueMatt/bitcoin/tree/frt

Fix whitespace errors

# Conflicts:
#	src/rpcserver.cpp

3e791407ab

Tweak fundrawtransaction help text and return the fee as a float 7393177f3b

jonasschnelli commented at 9:12 AM on April 24, 2015: contributor

Pulled in some commits from @TheBlueMatt. Only pulled in clear beneficial and easy reviewable commits. Other things (CCoinControl usage, watch-only support) may be better handled in a 2nd pull request to avoid large changeset.

Test inputs are not modified in fundrawtransaction 3f66c2e66f

Also return changepos from fundrawtransaction

# Conflicts:
#	src/wallet/rpcwallet.cpp

840b89bd40

jonasschnelli force-pushed on Apr 24, 2015

TheBlueMatt commented at 9:30 AM on April 24, 2015: member

@jonasschnelli Actually, the changes I made were mostly to limit the size of this pull request. Some of them look like it would become a big pull request, but once squashed, the total size is actually smaller than the original :)

TheBlueMatt commented at 9:31 AM on April 24, 2015: member

Also, not sure when you pulled in, but I force pushe'd a few times, you may want to check that the ones you merged are the latest versions (I'm done now, I think). I also merged in a tweaked-up version of fundrawtransaction there.

jonasschnelli commented at 9:35 AM on April 24, 2015: contributor

@TheBlueMatt Thank you for your tweaks! Will try to go through the optimizing commits but need a clear head for that. Will do it soon.

TheBlueMatt commented at 1:33 AM on April 25, 2015: member

I added two more commits to that tree and made a nice rebased version at https://github.com/TheBlueMatt/bitcoin/commits/frt2

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

Rebased frt2. @jonasschnelli do you want to review my changes and merge them here, or should I just open a new pull request? I'd like to keep this moving.

jonasschnelli commented at 8:56 PM on April 30, 2015: contributor

closing in favor of #6088

jonasschnelli closed this on Apr 30, 2015

TheBlueMatt referenced this in commit aaf439bfd6 on Oct 20, 2015

DrahtBot locked this on Sep 8, 2021