Moving final scriptSig construction from CombineSignatures to ProduceSignature (PSBT signer logic) #13425

achow101 · 2018-06-08T23:31:28Z

Currently CombineSignatures is used to create the final scriptSig or an input. However ProduceSignature is capable of doing this itself. Using both CombineSignatures and ProduceSignature results in code duplication which is unnecessary.

To move the scriptSig construction to ProduceSignatures, the SignatureData class contains two maps to hold pubkeys mapped to signatures, and script ids mapped to scripts. DataFromTransaction is extended to be able to extract signatures, their public keys, and scripts from existing ScriptSigs.

The SignaureData are then passed down to SignStep which can use the aforementioned maps to get the signatures, pubkeys, and scripts that it needs, falling back to the actual SigningProvider and SignatureCreator if the data are not available in the SignatureData.

Additionally, Sign1 and SignN have been removed and their functionality inlined into SignStep since Sign1 is really just a wrapper around CreateSig.

Since ProduceSignature can produce the final scriptSig or scriptWitness by using SignatureData which has extracted data from the transaction, CombineSignatures is unnecessary as ProduceSignature is able to replicate all of CombineSignatures' functionality.

This also furthers BIP 174 support and begins moving towards a BIP 174 style backend.

The tests have also been updated to use the new combining methodology.

sipa · 2018-06-09T03:59:31Z

Here is an extra simplification we discussed IRL: https://github.com/sipa/bitcoin/commits/achow_sigdata-partial-sigs

sipa · 2018-06-09T19:11:54Z

src/script/sign.h

    CScript scriptSig;
    CScriptWitness scriptWitness;
+    std::map<CPubKey, std::vector<unsigned char>> signatures;
+    std::map<uint160, CScript> scripts;


Use CScriptID as key type?

sipa · 2018-06-09T19:12:15Z

src/script/sign.h

 struct SignatureData {
    CScript scriptSig;
    CScriptWitness scriptWitness;
+    std::map<CPubKey, std::vector<unsigned char>> signatures;


If you use CKeyID as key type here, you can avoid the iteration to find matches.

GetPubKey won't work if these are CKeyIDs

Ah, good point. You could have an std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>> though, which stores both full public key and signature.

sipa · 2018-06-09T19:12:48Z

src/script/sign.cpp

+
+    // Get scripts
+    txnouttype script_type;
+    std::vector<std::vector<unsigned char> > solutions;


Nit: > >.

sipa · 2018-06-09T19:15:17Z

src/script/sign.h

 extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;

 struct SignatureData {
    CScript scriptSig;


Perhaps add a comment that explains the overlap in relevance of these fields:

scriptSig and scriptWitness are used for both complete signatures, and for old-style partial signatures.

signatures and scripts are only used for new-style partial signatures, but may be present for complete signatures too.

sipa · 2018-06-09T19:18:21Z

src/script/sign.h

 * Solvability is unrelated to whether we consider this output to be ours. */
 bool IsSolvable(const SigningProvider& provider, const CScript& script);

+class SignatureExtractorChecker : public MutableTransactionSignatureChecker


It feels a bit ugly to hardcode the dependency on MutableTransactionSignatureChecker, as there isn't technically anything in this class that requires it (you could pass in a BaseSignatureChecker reference instead).

This is just a nit, as it's really only used in DataFromTransaction and probably won't ever be used elsewhere.

I'm not sure that that is possible since I need to use MutableTransactionSignatureChecker::CheckSig as I do not want to re-implement a signature checker when one already exists.

Oh of course; I just mean that you pass in a BaseSignatureChecker* which is stored inside. You can then call its CheckSig member function (which is public).

Ah, right. Done.

sipa · 2018-06-09T19:22:58Z

src/bitcoin-tx.cpp

        // Only sign SIGHASH_SINGLE if there's a corresponding output:
        if (!fHashSingle || (i < mergedTx.vout.size()))
-            ProduceSignature(keystore, MutableTransactionSignatureCreator(&mergedTx, i, amount, nHashType), prevPubKey, sigdata);
+            ProduceSignature(SignatureDataSigningProvider(&sigdata, &keystore), SignatureDataSignatureCreator(&sigdata, &mergedTx, i, amount, nHashType), prevPubKey, sigdata);


In "Replace CombineSignatures with ProduceSignature"

See my suggested commits in https://github.com/sipa/bitcoin/tree/achow_sigdata-partial-sigs to avoid introducing the SignatureDataSigningProvider and SignatureDataSignatureCreator classes here.

sipa · 2018-06-09T19:23:53Z

src/test/transaction_tests.cpp

    CheckWithFlag(output2, input2, 0, false);
    BOOST_CHECK(*output1 == *output2);
-    UpdateInput(input1.vin[0], CombineSignatures(output1->vout[0].scriptPubKey, MutableTransactionSignatureChecker(&input1, 0, output1->vout[0].nValue), DataFromTransaction(input1, 0, output1->vout[0]), DataFromTransaction(input2, 0, output1->vout[0])));
+    sigdata = DataFromTransaction(input1, 0, output1->vout[0]);


Would it make sense to introduce a wrapper here as well for the pattern that replaces the CombineSignatures calls?

achow101 · 2018-06-09T19:42:43Z

I have included squashed in @sipa's suggested commits.

sipa · 2018-06-09T19:51:27Z

src/script/sign.cpp

If you'd introduce a sigdata.scripts.emplace(CScriptID(subscript), subscript); here (and similarly below for the TX_WITNESS_V0_SCRIPTHASH case), you get (I think) all functionality you need for operating on SignatureData objects without the need for the legacy partial signatures.

sipa · 2018-06-09T19:54:47Z

You're missing the DUMMY_SIGNING_PROVIDER instance.

achow101 · 2018-06-09T20:50:38Z

You're missing the DUMMY_SIGNING_PROVIDER instance.

Fixed

sipa

utACK 64f4a710bbddace7ee51ff6cfbc19f039e7739fa, only nits remaining.

sipa · 2018-06-09T21:12:11Z

src/script/sign.cpp

Nit: mi->second.

sipa · 2018-06-09T21:12:32Z

src/script/sign.cpp

Can be const auto&.

sipa · 2018-06-09T21:13:37Z

src/script/sign.cpp

Can probably rename vchPubKeyOut to pubkey or so now.

sipa · 2018-06-09T21:14:00Z

src/script/sign.cpp

We should probably rename this to pubkey.

sipa · 2018-06-09T21:15:22Z

src/script/sign.cpp

next_script = std::move(redeem_script).

sipa · 2018-06-09T21:20:49Z

src/script/sign.cpp

stack.script = std::move(stack.witness);.

sipa · 2018-06-09T21:21:13Z

src/script/sign.cpp

Style nit: { on the same line as for.

sipa · 2018-06-09T21:21:39Z

src/script/sign.cpp

Style nit: continue either on the same line as if, or indented while surrounded by braces.

sipa · 2018-06-09T21:21:50Z

src/script/sign.cpp

Style nit: { on the same line as if.

sipa · 2018-06-09T21:23:44Z

src/script/sign.h

Ultra nit: "update with signature data" sounds a bit weird for signature data itself. Perhaps call it Merge or so?

sipa · 2018-06-09T23:56:24Z

utACK 862c0d1749e28c46fd7c5fb951ac41f6d5dfad06

Perhaps something to add to the rationale: by having all processing be done by ProduceSignature (which now can take existing partial signatures as input), the model much more closely matches BIP 174 procedures. The changes here should make it possible for the BIP174 implementation to largely reuse the existing signing code.

achow101 · 2018-06-11T19:03:36Z

I added a helper function for createSig which handles the fetching and placing of signatures from the SignatureData.

achow101 · 2018-06-12T18:03:25Z

Removed the newsigs map from the first commit. Also moved some changes around to commits where they actually made sense to be in.

sipa · 2018-06-12T18:23:29Z

src/script/sign.cpp

You could start this loop after the last successful key in previous iterations, as keys and signature need to have a corresponding order.

Otherwise you risk O(n^2) behaviour if they're in opposite order (perhaps we want to support that, but I don't think we currently do).

sipa · 2018-06-12T18:23:47Z

src/script/sign.cpp

This can end with a break as well, I think.

sipa · 2018-06-12T18:55:33Z

Perhaps this PR can be renamed to "PSBT signer logic"; it may not be clear right now that in addition to being a code simplification, this also effectively reduces the PSBT PR to serialization + RPC implementations.

achow101 · 2018-06-12T19:03:16Z

Updated the title to include "PSBT signer logic".

meshcollider · 2018-06-13T21:01:05Z

Concept ACK

sipa

utACK 1d5c2bbaba8e3b66195acefe37053d9048c81ecf

Some tiny nits only, feel free to ignore.

sipa · 2018-06-13T23:52:59Z

src/script/sign.cpp

i + 1 even, I think (the same signature can't be reused for another pubkey).

sipa · 2018-06-14T00:53:53Z

src/script/sign.cpp

Very nitty: this line and the next will perform an unnecessary copy. You can avoid it with scripts.insert(std::make_move_iterator(sigdata.scripts.begin()), std::make_move_iterator(sigdata.scripts.end()); etc.

In addition to having the scriptSig and scriptWitness, have SignatureData also be able to store just the signatures (pubkeys mapped to sigs) and scripts (script ids mapped to scripts). Also have DataFromTransaction be able to extract signatures and scripts from the scriptSig and scriptWitness of an input to put them in SignatureData. Adds a new SignatureChecker which takes a SignatureData and puts pubkeys and signatures into it when it successfully verifies a signature. Adds a new field in SignatureData which stores whether the SignatureData was complete. This allows us to also update the scriptSig and scriptWitness to the final one when updating a SignatureData with another one.

Instead of using CombineSignatures to create the final scriptSig or scriptWitness of an input, use ProduceSignature itself. To allow for ProduceSignature to place signatures, pubkeys, and scripts that it does not know about, we pass down the SignatureData to SignStep which pulls out the information that it needs from the SignatureData.

Removes CombineSignatures and replaces its use in tests with ProduceSignature to test the same behavior for ProduceSignature.

laanwj · 2018-07-05T15:17:55Z

utACK b815600

…res to ProduceSignature (PSBT signer logic) b815600 Remove CombineSignatures and replace tests (Andrew Chow) ed94c8b Replace CombineSignatures with ProduceSignature (Andrew Chow) 0422beb Make SignatureData able to store signatures and scripts (Andrew Chow) b6edb4f Inline Sign1 and SignN (Andrew Chow) Pull request description: Currently CombineSignatures is used to create the final scriptSig or an input. However ProduceSignature is capable of doing this itself. Using both CombineSignatures and ProduceSignature results in code duplication which is unnecessary. To move the scriptSig construction to ProduceSignatures, the SignatureData class contains two maps to hold pubkeys mapped to signatures, and script ids mapped to scripts. DataFromTransaction is extended to be able to extract signatures, their public keys, and scripts from existing ScriptSigs. The SignaureData are then passed down to SignStep which can use the aforementioned maps to get the signatures, pubkeys, and scripts that it needs, falling back to the actual SigningProvider and SignatureCreator if the data are not available in the SignatureData. Additionally, Sign1 and SignN have been removed and their functionality inlined into SignStep since Sign1 is really just a wrapper around CreateSig. Since ProduceSignature can produce the final scriptSig or scriptWitness by using SignatureData which has extracted data from the transaction, CombineSignatures is unnecessary as ProduceSignature is able to replicate all of CombineSignatures' functionality. This also furthers BIP 174 support and begins moving towards a BIP 174 style backend. The tests have also been updated to use the new combining methodology. Tree-SHA512: 78cd58a4ebe37f79229bd5eee2958a0bb45cd7f36d0e993eee13ff685b3665dd76ef2dfd5f47d34678995bb587f5594100ee5f6c09b1c69ee96d3684d470d01e

020628e Tests for PSBT (Andrew Chow) a4b06fb Create wallet RPCs for PSBT (Andrew Chow) c27fe41 Create utility RPCs for PSBT (Andrew Chow) 8b5ef27 SignPSBTInput wrapper function (Andrew Chow) 58a8e28 Refactor transaction creation and transaction funding logic (Andrew Chow) e9d86a4 Methods for interacting with PSBT structs (Andrew Chow) 12bcc64 Add pubkeys and whether input was witness to SignatureData (Andrew Chow) 41c607f Implement PSBT Structures and un/serialization methods per BIP 174 (Andrew Chow) Pull request description: This Pull Request fully implements the [updated](bitcoin/bips#694) BIP 174 specification. It is based upon #13425 which implements the majority of the signing logic. BIP 174 specifies a binary transaction format which contains the information necessary for a signer to produce signatures for the transaction and holds the signatures for an input while the input does not have a complete set of signatures. This PR contains structs for PSBT, serialization, and deserialzation code. Some changes to `SignatureData` have been made to support detection of UTXO type and storing public keys. *** Many RPCs have been added to handle PSBTs. `walletprocesspsbt` takes a PSBT format transaction, updates the PSBT with any inputs related to this wallet, signs, and finalizes the transaction. There is also an option to not sign and just update. `walletcreatefundedpsbt` creates a PSBT from user provided data in the same form as createrawtransaction. It also funds the transaction and takes an options argument in the same form as `fundrawtransaction`. The resulting PSBT is blank with no input or output data filled in. It is analogous to a combination of `createrawtransaction` and `fundrawtransaction` `decodepsbt` takes a PSBT and decodes it to JSON. It is analogous to `decoderawtransaction` `combinepsbt` takes multiple PSBTs for the same tx and combines them. It is analogous to `combinerawtransaction` `finalizepsbt` takes a PSBT and finalizes the inputs. If all inputs are final, it extracts the network serialized transaction and returns that instead of a PSBT unless instructed otherwise. `createpsbt` is like `createrawtransaction` but for PSBTs instead of raw transactions. `convertpsbt` takes a network serialized transaction and converts it into a psbt. The resulting psbt will lose all signature data and an explicit flag must be set to allow transactions with signature data to be converted. *** This supersedes #12136 Tree-SHA512: 1ac7a79e5bc669933f0a6fcc93ded55263fdde9e8c144a30266b13ef9f62aacf43edd4cbca1ffbe003090b067e9643c9298c79be69d7c1b10231b32acafb6338

…ures to ProduceSignature

…Signatures to ProduceSignature (PSBT signer logic) b815600 Remove CombineSignatures and replace tests (Andrew Chow) ed94c8b Replace CombineSignatures with ProduceSignature (Andrew Chow) 0422beb Make SignatureData able to store signatures and scripts (Andrew Chow) b6edb4f Inline Sign1 and SignN (Andrew Chow) Pull request description: Currently CombineSignatures is used to create the final scriptSig or an input. However ProduceSignature is capable of doing this itself. Using both CombineSignatures and ProduceSignature results in code duplication which is unnecessary. To move the scriptSig construction to ProduceSignatures, the SignatureData class contains two maps to hold pubkeys mapped to signatures, and script ids mapped to scripts. DataFromTransaction is extended to be able to extract signatures, their public keys, and scripts from existing ScriptSigs. The SignaureData are then passed down to SignStep which can use the aforementioned maps to get the signatures, pubkeys, and scripts that it needs, falling back to the actual SigningProvider and SignatureCreator if the data are not available in the SignatureData. Additionally, Sign1 and SignN have been removed and their functionality inlined into SignStep since Sign1 is really just a wrapper around CreateSig. Since ProduceSignature can produce the final scriptSig or scriptWitness by using SignatureData which has extracted data from the transaction, CombineSignatures is unnecessary as ProduceSignature is able to replicate all of CombineSignatures' functionality. This also furthers BIP 174 support and begins moving towards a BIP 174 style backend. The tests have also been updated to use the new combining methodology. Tree-SHA512: 78cd58a4ebe37f79229bd5eee2958a0bb45cd7f36d0e993eee13ff685b3665dd76ef2dfd5f47d34678995bb587f5594100ee5f6c09b1c69ee96d3684d470d01e

merge bitcoin#13269, bitcoin#13425, bitcoin#13557, bitcoin#13721, bitcoin#13666, bitcoin#13723: BIP 174 PSBT Serializations and RPCs

fanquake requested a review from sipa June 9, 2018 01:04

sipa reviewed Jun 9, 2018

View reviewed changes

achow101 force-pushed the sigdata-partial-sigs branch from 40d5ce7 to 6ce5a28 Compare June 9, 2018 19:36

sipa reviewed Jun 9, 2018

View reviewed changes

achow101 force-pushed the sigdata-partial-sigs branch from 6ce5a28 to 340201b Compare June 9, 2018 20:48

achow101 force-pushed the sigdata-partial-sigs branch from 340201b to 64f4a71 Compare June 9, 2018 21:16

sipa reviewed Jun 9, 2018

View reviewed changes

achow101 force-pushed the sigdata-partial-sigs branch 3 times, most recently from 2099402 to 862c0d1 Compare June 9, 2018 22:51

achow101 mentioned this pull request Jun 10, 2018

Implement BIP 174 Partially Signed Bitcoin Transactions serialization and RPCs #12136

Closed

achow101 force-pushed the sigdata-partial-sigs branch from 862c0d1 to f55cfa3 Compare June 11, 2018 19:00

achow101 force-pushed the sigdata-partial-sigs branch from f55cfa3 to 93f4cec Compare June 12, 2018 18:03

sipa reviewed Jun 12, 2018

View reviewed changes

sipa mentioned this pull request Jun 12, 2018

[WIP] support new multisig template in wallet for Solver, signing, and signature combining #13449

Closed

achow101 changed the title ~~Moving final scriptSig construction from CombineSignatures to ProduceSignature~~ Moving final scriptSig construction from CombineSignatures to ProduceSignature (PSBT signer logic) Jun 12, 2018

achow101 force-pushed the sigdata-partial-sigs branch from 93f4cec to 1d5c2bb Compare June 12, 2018 19:02

sipa reviewed Jun 14, 2018

View reviewed changes

achow101 force-pushed the sigdata-partial-sigs branch from 1d5c2bb to 9e5fdc7 Compare June 14, 2018 01:49

laanwj added the Wallet label Jun 14, 2018

achow101 added 3 commits July 3, 2018 17:18

Remove CombineSignatures and replace tests

b815600

Removes CombineSignatures and replaces its use in tests with ProduceSignature to test the same behavior for ProduceSignature.

achow101 force-pushed the sigdata-partial-sigs branch from ef0c9fb to b815600 Compare July 4, 2018 00:19

laanwj merged commit b815600 into bitcoin:master Jul 5, 2018

This was referenced Jul 6, 2018

Enforce that amount is provided for signrawtransaction prevtxs #12458

Closed

Make signrawtransaction* give an error when amount is needed but missing #13547

Merged

Empact mentioned this pull request Jul 6, 2018

refactor: privatize SignatureExtractorChecker [moveonly] #13266

Merged

Bushstar mentioned this pull request Jul 9, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#353

Merged

HashUnlimited mentioned this pull request Sep 11, 2018

BIP 174 PSCT Serializations and RPCs chaincoin/chaincoin#543

Merged

kwvg mentioned this pull request Jun 4, 2021

merge #13269, #13425, #13557, #13721, #13666, #13723: BIP 174 PSBT Serializations and RPCs dashpay/dash#4186

Merged

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jun 7, 2021

Merge bitcoin#13425: Moving scriptSig construction from CombineSignat…

5baf21b

…ures to ProduceSignature

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Jul 13, 2021

Merge pull request #4186 from kittywhiskers/psbt

e98241d

merge bitcoin#13269, bitcoin#13425, bitcoin#13557, bitcoin#13721, bitcoin#13666, bitcoin#13723: BIP 174 PSBT Serializations and RPCs

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

Moving final scriptSig construction from CombineSignatures to ProduceSignature (PSBT signer logic) #13425

Moving final scriptSig construction from CombineSignatures to ProduceSignature (PSBT signer logic) #13425

Uh oh!

Conversation

achow101 commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 9, 2018

Uh oh!

achow101 commented Jun 9, 2018

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 8, 2018 •

edited

Loading