fundrawtransaction #6088

Merged
merged 5 commits into from Jun 23, 2015

Conversation

Projects
None yet
7 participants
@TheBlueMatt
Contributor

TheBlueMatt commented Apr 30, 2015

This superceedes #5503 and #5524. I rewrote chunks of the first and largely rewrote the second, stealing test cases almost verbatim from both.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 1, 2015

Member

Could we have a DummySignatureCreator, instead of passing a dummy boolean to TransactionSignatureCreator?

Member

sipa commented May 1, 2015

Could we have a DummySignatureCreator, instead of passing a dummy boolean to TransactionSignatureCreator?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 1, 2015

Member

I had to read the source code to guess what 'fAllowOtherInputs' means. Can you add a comment?

Member

sipa commented May 1, 2015

I had to read the source code to guess what 'fAllowOtherInputs' means. Can you add a comment?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 1, 2015

Member

@sipa: I tried this ("DummySignatorCreator"). But somehow i stopped it because it was getting a inheritance mess. I can't actually remember why exactly.

Member

jonasschnelli commented May 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 1, 2015

Member

@jonasschnelli Let me hack something up.

Member

sipa commented May 1, 2015

@jonasschnelli Let me hack something up.

@sipa

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 1, 2015

Member

@sipa: are you sure this would work also for P2SH Multisig inputs?

Member

jonasschnelli commented May 1, 2015

@sipa: are you sure this would work also for P2SH Multisig inputs?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

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

Member

sipa commented May 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 1, 2015

Member

@sipa: Right. I would do so. To make use of sipa@134090b 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.

Member

jonasschnelli commented May 1, 2015

@sipa: Right. I would do so. To make use of sipa@134090b 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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 1, 2015

Member
Member

sipa commented May 1, 2015

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik May 2, 2015

Contributor

Concept ACK - the dummy sig stuff is ugly and poops all over several function/method sigs

Contributor

jgarzik commented May 2, 2015

Concept ACK - the dummy sig stuff is ugly and poops all over several function/method sigs

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented May 6, 2015

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 laanwj removed this from the 0.11.0 milestone May 18, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 18, 2015

Member

Needs rebase.

Member

jonasschnelli commented May 18, 2015

Needs rebase.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 2, 2015

Member

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

Member

luke-jr commented Jun 2, 2015

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

src/wallet/rpcdump.cpp
"\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"
"\nArguments:\n"
"1. \"address\" (string, required) The address\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
+ "4. p2sh (boolean, optional, default=false) Add the P2SH version of the script as well\n"

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

This seems nonsensical. What is the use case?

@luke-jr

luke-jr Jun 2, 2015

Member

This seems nonsensical. What is the use case?

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jun 11, 2015

Contributor

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.

Contributor

TheBlueMatt commented Jun 11, 2015

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.

@laanwj laanwj merged commit 2085895 into bitcoin:master Jun 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 23, 2015

Merge pull request #6088
2085895 fundrawtransaction tests (Jonas Schnelli)
21bbd92 Add fundrawtransaction RPC method (Matt Corallo)
1e0d1a2 Add FundTransaction method to wallet (Matt Corallo)
2d84e22 Small tweaks to CCoinControl for fundrawtransaction (Matt Corallo)
9b4e7d9 Add DummySignatureCreator which just creates zeroed sigs (Pieter Wuille)
@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jun 23, 2015

Member

ACK

Member

btcdrak commented Jun 23, 2015

ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 24, 2015

Member

Tested ACK (which I forgot to post)

Member

laanwj commented Jun 24, 2015

Tested ACK (which I forgot to post)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 24, 2015

Member

Post merge ACK

Member

jonasschnelli commented Jun 24, 2015

Post merge ACK

+ if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosRet, strFailReason, &coinControl, false))
+ return false;
+
+ if (nChangePosRet != -1)

This comment has been minimized.

@sipa

sipa Jun 24, 2015

Member

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

sipa Jun 24, 2015

Member

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 24, 2015

Member

Posthumous untested ACK.

Member

sipa commented Jun 24, 2015

Posthumous untested ACK.

@laanwj laanwj referenced this pull request Feb 10, 2016

Closed

Add createtransaction #7498

@str4d str4d referenced this pull request in zcash/zcash Feb 14, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Dec 27, 2017

Merged

Add fundrawtransaction #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment