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

pull achow101 wants to merge 4 commits into bitcoin:master from achow101:sigdata-partial-sigs changing 7 files +283 −260
  1. achow101 commented at 11:31 pm on June 8, 2018: member

    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.

  2. fanquake requested review from sipa on Jun 9, 2018
  3. sipa commented at 3:59 am on June 9, 2018: member
    Here is an extra simplification we discussed IRL: https://github.com/sipa/bitcoin/commits/achow_sigdata-partial-sigs
  4. in src/script/sign.h:63 in 642a8b8674 outdated 
    55@@ -56,9 +56,12 @@ extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;
56 struct SignatureData {
57     CScript scriptSig;
58     CScriptWitness scriptWitness;
59+    std::map<CPubKey, std::vector<unsigned char>> signatures;
60+    std::map<uint160, CScript> scripts;
    sipa commented at 7:11 pm on June 9, 2018:
    Use CScriptID as key type?
    achow101 commented at 8:49 pm on June 9, 2018:
    Done
  5. in src/script/sign.h:62 in 642a8b8674 outdated 
    55@@ -56,9 +56,12 @@ extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;
56 struct SignatureData {
57     CScript scriptSig;
58     CScriptWitness scriptWitness;
59+    std::map<CPubKey, std::vector<unsigned char>> signatures;
    sipa commented at 7:12 pm on June 9, 2018:
    If you use CKeyID as key type here, you can avoid the iteration to find matches.
    achow101 commented at 7:37 pm on June 9, 2018:
    GetPubKey won’t work if these are CKeyIDs
    sipa commented at 7:42 pm on June 9, 2018:
    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.
    achow101 commented at 9:16 pm on June 9, 2018:
    Done
  6. in src/script/sign.cpp:266 in 642a8b8674 outdated 
    230+    SignatureExtractorChecker checker(&tx, nIn, txout.nValue, &data);
231+    VerifyScript(data.scriptSig, txout.scriptPubKey, &data.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, checker);
232+
233+    // Get scripts
234+    txnouttype script_type;
235+    std::vector<std::vector<unsigned char> > solutions;
    sipa commented at 7:12 pm on June 9, 2018:
    Nit: > >.
    achow101 commented at 8:49 pm on June 9, 2018:
    Fixed
  7. in src/script/sign.h:60 in 642a8b8674 outdated 
    55@@ -56,9 +56,12 @@ extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;
56 struct SignatureData {
57     CScript scriptSig;
    sipa commented at 7:15 pm on June 9, 2018:

    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.
    achow101 commented at 8:49 pm on June 9, 2018:
    Done
  8. in src/script/sign.h:89 in 642a8b8674 outdated 
    83@@ -81,4 +84,14 @@ void UpdateInput(CTxIn& input, const SignatureData& data);
84  * Solvability is unrelated to whether we consider this output to be ours. */
85 bool IsSolvable(const SigningProvider& provider, const CScript& script);
86 
87+class SignatureExtractorChecker : public MutableTransactionSignatureChecker
    sipa commented at 7:18 pm on June 9, 2018:

    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.

    achow101 commented at 8:25 pm on June 9, 2018:
    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.
    sipa commented at 8:30 pm on June 9, 2018:
    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).
    achow101 commented at 9:31 pm on June 9, 2018:
    Ah, right. Done.
  9. in src/bitcoin-tx.cpp:651 in d0f96c4544 outdated 
    647@@ -648,10 +648,8 @@ static void MutateTxSign(CMutableTransaction& tx, const std::string& flagStr)
648         SignatureData sigdata = DataFromTransaction(mergedTx, i, coin.out);
649         // Only sign SIGHASH_SINGLE if there's a corresponding output:
650         if (!fHashSingle || (i < mergedTx.vout.size()))
651-            ProduceSignature(keystore, MutableTransactionSignatureCreator(&mergedTx, i, amount, nHashType), prevPubKey, sigdata);
652+            ProduceSignature(SignatureDataSigningProvider(&sigdata, &keystore), SignatureDataSignatureCreator(&sigdata, &mergedTx, i, amount, nHashType), prevPubKey, sigdata);
    sipa commented at 7:22 pm on June 9, 2018:

    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.

    achow101 commented at 8:50 pm on June 9, 2018:
    Done
  10. in src/test/transaction_tests.cpp:632 in 40d5ce7016 outdated 
    628@@ -629,7 +629,10 @@ BOOST_AUTO_TEST_CASE(test_witness)
629     CreateCreditAndSpend(keystore2, scriptMulti, output2, input2, false);
630     CheckWithFlag(output2, input2, 0, false);
631     BOOST_CHECK(*output1 == *output2);
632-    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])));
633+    sigdata = DataFromTransaction(input1, 0, output1->vout[0]);
    sipa commented at 7:23 pm on June 9, 2018:
    Would it make sense to introduce a wrapper here as well for the pattern that replaces the CombineSignatures calls?
    achow101 commented at 10:51 pm on June 9, 2018:
    Done
  11. achow101 force-pushed on Jun 9, 2018
  12. achow101 commented at 7:42 pm on June 9, 2018: member
    I have included squashed in @sipa’s suggested commits.
  13. in src/script/sign.cpp:192 in 6ce5a289bd outdated 
    179@@ -152,7 +180,7 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
180         // the final scriptSig is the signatures from that
181         // and then the serialized subscript:
182         subscript = CScript(result[0].begin(), result[0].end());
183-        solved = solved && SignStep(provider, creator, subscript, result, whichType, SigVersion::BASE) && whichType != TX_SCRIPTHASH;
184+        solved = solved && SignStep(provider, creator, subscript, result, whichType, SigVersion::BASE, sigdata) && whichType != TX_SCRIPTHASH;
    sipa commented at 7:51 pm on June 9, 2018:
    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.
    achow101 commented at 8:50 pm on June 9, 2018:
    Done
  14. sipa commented at 7:54 pm on June 9, 2018: member
    You’re missing the DUMMY_SIGNING_PROVIDER instance.
  15. achow101 force-pushed on Jun 9, 2018
  16. achow101 commented at 8:50 pm on June 9, 2018: member

    You’re missing the DUMMY_SIGNING_PROVIDER instance.

    Fixed

  17. in src/script/sign.cpp:44 in 340201bfff outdated 
    45+        return true;
46+    }
47+    // Look for scripts in SignatureData
48+    auto mi = sigdata->scripts.find(scriptid);
49+    if (mi != sigdata->scripts.end()) {
50+        script = (*mi).second;
    sipa commented at 9:12 pm on June 9, 2018:
    Nit: mi->second.
    achow101 commented at 9:52 pm on June 9, 2018:
    Done
  18. in src/script/sign.cpp:56 in 340201bfff outdated 
    67+    if (provider != nullptr && provider->GetPubKey(address, vchPubKeyOut)) {
68+        return true;
69     }
70-    return nSigned==nRequired;
71+    // Look for pubkey in all partial sigs
72+    for (auto& it : sigdata->signatures) {
    sipa commented at 9:12 pm on June 9, 2018:
    Can be const auto&.
    achow101 commented at 9:52 pm on June 9, 2018:
    Done
  19. in src/script/sign.cpp:50 in 340201bfff outdated 
    52+    }
53+    return false;
54 }
55 
56-static bool SignN(const SigningProvider& provider, const std::vector<valtype>& multisigdata, const BaseSignatureCreator& creator, const CScript& scriptCode, std::vector<valtype>& ret, SigVersion sigversion)
57+static bool GetPubKey(const SigningProvider* provider, const SignatureData* sigdata, const CKeyID &address, CPubKey& vchPubKeyOut)
    sipa commented at 9:13 pm on June 9, 2018:
    Can probably rename vchPubKeyOut to pubkey or so now.
    achow101 commented at 9:52 pm on June 9, 2018:
    Done
  20. in src/script/sign.cpp:97 in 340201bfff outdated 
    104-            provider.GetPubKey(keyID, vch);
105-            ret.push_back(ToByteVector(vch));
106-        }
107+        if (!creator.CreateSig(provider, sig, keyID, scriptPubKey, sigversion)) return false;
108+        ret.push_back(std::move(sig));
109+        CPubKey vch;
    sipa commented at 9:14 pm on June 9, 2018:
    We should probably rename this to pubkey.
    achow101 commented at 9:52 pm on June 9, 2018:
    Done
  21. in src/script/sign.cpp:277 in 340201bfff outdated 
    323+
324+    if (script_type == TX_SCRIPTHASH && !stack.script.empty() && !stack.script.back().empty()) {
325+        // Get the redeemScript
326+        CScript redeem_script(stack.script.back().begin(), stack.script.back().end());
327+        data.scripts.emplace(CScriptID(redeem_script), redeem_script);
328+        next_script = redeem_script;
    sipa commented at 9:15 pm on June 9, 2018:
    next_script = std::move(redeem_script).
    achow101 commented at 9:52 pm on June 9, 2018:
    Done
  22. in src/script/sign.cpp:287 in 340201bfff outdated 
    333+    }
334+    if (script_type == TX_WITNESS_V0_SCRIPTHASH && !stack.witness.empty() && !stack.witness.back().empty()) {
335+        // Get the witnessScript
336+        CScript witness_script(stack.witness.back().begin(), stack.witness.back().end());
337+        data.scripts.emplace(CScriptID(witness_script), witness_script);
338+        next_script = witness_script;
    sipa commented at 9:16 pm on June 9, 2018:
    next_script = std::move(witness_script);.
    achow101 commented at 9:53 pm on June 9, 2018:
    Done
  23. achow101 force-pushed on Jun 9, 2018
  24. in src/script/sign.cpp:119 in 64f4a710bb outdated 
    129+            CPubKey pubkey = CPubKey(vSolutions[i]);
130+            auto it = sigdata.signatures.find(pubkey.GetID());
131+            if (it != sigdata.signatures.end()) {
132+                sig = it->second.second;
133+            } else {
134+                if (!creator.CreateSig(provider, sig, pubkey.GetID(), scriptPubKey, sigversion)) sig.clear();
    sipa commented at 9:19 pm on June 9, 2018:
    You’re computing pubkey.GetID() twice; perhaps store in a variable and reuse?
    achow101 commented at 9:53 pm on June 9, 2018:
    done
  25. in src/script/sign.cpp:292 in 64f4a710bb outdated 
    337+        next_script = witness_script;
338+
339+        // Get witnessScript type
340+        Solver(next_script, script_type, solutions);
341+        stack.witness.pop_back();
342+        stack.script = stack.witness;
    sipa commented at 9:20 pm on June 9, 2018:
    stack.script = std::move(stack.witness);.
    achow101 commented at 9:53 pm on June 9, 2018:
    Done
  26. in src/script/sign.cpp:303 in 64f4a710bb outdated 
    364-            result.witness = result.script;
365-            result.script.clear();
366-            result.witness.push_back(valtype(pubKey2.begin(), pubKey2.end()));
367-            return result;
368+            for (unsigned int i = 0; i < num_pubkeys; i++)
369+            {
    sipa commented at 9:21 pm on June 9, 2018:
    Style nit: { on the same line as for.
    achow101 commented at 9:53 pm on June 9, 2018:
    Done
  27. in src/script/sign.cpp:306 in 64f4a710bb outdated 
    367-            return result;
368+            for (unsigned int i = 0; i < num_pubkeys; i++)
369+            {
370+                const valtype& pubkey = solutions[i+1];
371+                if (data.signatures.count(CPubKey(pubkey).GetID()))
372+                    continue; // Already got a sig for this pubkey
    sipa commented at 9:21 pm on June 9, 2018:
    Style nit: continue either on the same line as if, or indented while surrounded by braces.
    achow101 commented at 9:53 pm on June 9, 2018:
    Done
  28. in src/script/sign.cpp:309 in 64f4a710bb outdated 
    370+                const valtype& pubkey = solutions[i+1];
371+                if (data.signatures.count(CPubKey(pubkey).GetID()))
372+                    continue; // Already got a sig for this pubkey
373+
374+                if (checker.CheckSig(sig, pubkey, next_script, sigversion))
375+                {
    sipa commented at 9:21 pm on June 9, 2018:
    Style nit: { on the same line as if.
    achow101 commented at 9:53 pm on June 9, 2018:
    Done
  29. in src/script/sign.h:69 in 64f4a710bb outdated 
    66+    std::map<CKeyID, SigPair> signatures; // BIP 174 style partial signatures for the input. May contain complete signatures
67+    std::map<CScriptID, CScript> scripts; // BIP 174 style scripts for the input
68 
69     SignatureData() {}
70     explicit SignatureData(const CScript& script) : scriptSig(script) {}
71+    void UpdateWithSignatureData(SignatureData sigdata);
    sipa commented at 9:23 pm on June 9, 2018:
    Ultra nit: “update with signature data” sounds a bit weird for signature data itself. Perhaps call it Merge or so?
    achow101 commented at 9:53 pm on June 9, 2018:
    Done
  30. sipa commented at 9:24 pm on June 9, 2018: member
    utACK 64f4a710bbddace7ee51ff6cfbc19f039e7739fa, only nits remaining.
  31. achow101 force-pushed on Jun 9, 2018
  32. achow101 force-pushed on Jun 9, 2018
  33. achow101 force-pushed on Jun 9, 2018
  34. sipa commented at 11:56 pm on June 9, 2018: member

    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.

  35. fanquake added this to the "Blockers" column in a project
  36. achow101 force-pushed on Jun 11, 2018
  37. achow101 commented at 7:03 pm on June 11, 2018: member
    I added a helper function for createSig which handles the fetching and placing of signatures from the SignatureData.
  38. achow101 force-pushed on Jun 12, 2018
  39. achow101 commented at 6:03 pm on June 12, 2018: member
    Removed the newsigs map from the first commit. Also moved some changes around to commits where they actually made sense to be in.
  40. in src/script/sign.cpp:308 in 93f4cec9b9 outdated 
    305+    if (script_type == TX_MULTISIG && !stack.script.empty()) {
306+        // Build a map of pubkey -> signature by matching sigs to pubkeys:
307+        assert(solutions.size() > 1);
308+        unsigned int num_pubkeys = solutions.size()-2;
309+        for (const valtype& sig : stack.script) {
310+            for (unsigned int i = 0; i < num_pubkeys; i++) {
    sipa commented at 6:23 pm on June 12, 2018:

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

    achow101 commented at 7:02 pm on June 12, 2018:
    Done
  41. in src/script/sign.cpp:310 in 93f4cec9b9 outdated 
    307+        assert(solutions.size() > 1);
308+        unsigned int num_pubkeys = solutions.size()-2;
309+        for (const valtype& sig : stack.script) {
310+            for (unsigned int i = 0; i < num_pubkeys; i++) {
311+                const valtype& pubkey = solutions[i+1];
312+                if (data.signatures.count(CPubKey(pubkey).GetID())) continue; // Already got a sig for this pubkey
    sipa commented at 6:23 pm on June 12, 2018:
    This can end with a break as well, I think.
    achow101 commented at 7:02 pm on June 12, 2018:
    Done
  42. sipa commented at 6:55 pm on June 12, 2018: member
    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.
  43. achow101 renamed this:
    Moving final scriptSig construction from CombineSignatures to ProduceSignature
    Moving final scriptSig construction from CombineSignatures to ProduceSignature (PSBT signer logic)
    on Jun 12, 2018
  44. achow101 force-pushed on Jun 12, 2018
  45. achow101 commented at 7:03 pm on June 12, 2018: member
    Updated the title to include “PSBT signer logic”.
  46. meshcollider commented at 9:01 pm on June 13, 2018: contributor
    Concept ACK
  47. in src/script/sign.cpp:267 in ef4602f900 outdated 
    264+        for (const valtype& sig : stack.script) {
265+            for (unsigned int i = last_success_key; i < num_pubkeys; i++) {
266+                const valtype& pubkey = solutions[i+1];
267+                // We either have a signature for this pubkey, or we have found a signature and it is valid
268+                if (data.signatures.count(CPubKey(pubkey).GetID()) || checker.CheckSig(sig, pubkey, next_script, sigversion)) {
269+                    last_success_key = i;
    sipa commented at 11:52 pm on June 13, 2018:
    i + 1 even, I think (the same signature can’t be reused for another pubkey).
    achow101 commented at 1:49 am on June 14, 2018:
    Done
  48. in src/script/sign.cpp:290 in ef4602f900 outdated 
    285+    if (complete) return;
286+    if (sigdata.complete) {
287+        *this = std::move(sigdata);
288+        return;
289+    }
290+    scripts.insert(sigdata.scripts.begin(), sigdata.scripts.end());
    sipa commented at 0:53 am on June 14, 2018:
    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.
    achow101 commented at 1:49 am on June 14, 2018:
    Done
  49. sipa commented at 1:06 am on June 14, 2018: member

    utACK 1d5c2bbaba8e3b66195acefe37053d9048c81ecf

    Some tiny nits only, feel free to ignore.

  50. achow101 force-pushed on Jun 14, 2018
  51. laanwj added the label Wallet on Jun 14, 2018
  52. achow101 force-pushed on Jun 14, 2018
  53. DrahtBot commented at 8:25 pm on June 14, 2018: member
    • #13449 ([WIP] support new multisig template in wallet for Solver, signing, and signature combining by instagibbs)
    • #13429 (Return the script type from Solver by Empact)
    • #13360 ([Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012)
    • #13266 (refactoring: Inline DataFromTransaction via new SignatureData constructor by Empact)

    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.

  54. sipa commented at 11:09 pm on June 14, 2018: member
    utACK 3d2cbe67e413e8de0b7144280a19bb348d1a76ca. Only change is addressing my last nits.
  55. achow101 force-pushed on Jun 27, 2018
  56. achow101 commented at 3:03 am on June 27, 2018: member
    I rebased this and have also updated the SignatureData to match the planned changes for BIP 174.
  57. sipa commented at 4:03 am on June 27, 2018: member
    utACK 33b85bb01f71d6da7cde39eb5cf42dd581de14b6.
  58. in src/script/sign.h:64 in d102f08f8f outdated 
    61+    bool complete = false; // Stores whether the scriptSig and scriptWitness are complete
62+    CScript scriptSig; // The scriptSig of an input. Contains complete signatures or the traditional partial signatures format
63+    CScript redeem_script; // The redeemScript (if any) for the input
64+    CScript witness_script; // The witnessScript (if any) for the input
65+    CScriptWitness scriptWitness; // The scriptWitness of an input. Contains complete signatures or the traditional partial signatures format
66+    std::map<CKeyID, SigPair> signatures; // BIP 174 style partial signatures for the input. May contain complete signatures
    instagibbs commented at 1:07 pm on June 27, 2018:
    may want to rephrase or define “complete signatures”. I assume you mean it in the PSBT sense.
    achow101 commented at 8:57 pm on June 27, 2018:
    Done
  59. in src/script/sign.h:64 in d102f08f8f outdated 
    52@@ -53,12 +53,19 @@ class MutableTransactionSignatureCreator : public BaseSignatureCreator {
53 /** A signature creator that just produces 72-byte empty signatures. */
54 extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;
55 
56+typedef std::pair<CPubKey, std::vector<unsigned char>> SigPair;
57+
58 struct SignatureData {
59-    CScript scriptSig;
60-    CScriptWitness scriptWitness;
61+    bool complete = false; // Stores whether the scriptSig and scriptWitness are complete
    instagibbs commented at 1:08 pm on June 27, 2018:
    Since we’re heavily augmenting this struct could we get a description of the struct’s purpose in a comment above it?
    achow101 commented at 8:58 pm on June 27, 2018:
    Done
  60. in src/script/sign.h:83 in d102f08f8f outdated 
    78@@ -72,7 +79,7 @@ bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom,
79 SignatureData CombineSignatures(const CScript& scriptPubKey, const BaseSignatureChecker& checker, const SignatureData& scriptSig1, const SignatureData& scriptSig2);
80 
81 /** Extract signature data from a transaction, and insert it. */
    instagibbs commented at 1:09 pm on June 27, 2018:
    “from a transaction input”?
    achow101 commented at 9:01 pm on June 27, 2018:
    Done
  61. in src/script/sign.h:62 in d102f08f8f outdated 
    59-    CScript scriptSig;
60-    CScriptWitness scriptWitness;
61+    bool complete = false; // Stores whether the scriptSig and scriptWitness are complete
62+    CScript scriptSig; // The scriptSig of an input. Contains complete signatures or the traditional partial signatures format
63+    CScript redeem_script; // The redeemScript (if any) for the input
64+    CScript witness_script; // The witnessScript (if any) for the input
    instagibbs commented at 1:16 pm on June 27, 2018:
    witness script vs script witness…. let’s explain this one as well.
    achow101 commented at 8:58 pm on June 27, 2018:
    Done
  62. in src/script/sign.cpp:210 in d102f08f8f outdated 
    207+        return result;
208+    }
209+};
210+}
211+
212+// Extracts signatures and scripts from scriptSigs. Please do not extend this, use PSBT instead
    instagibbs commented at 1:17 pm on June 27, 2018:
    May want to note it won’t extract scripts(redeem script and witness script) if the signatures are already complete
    instagibbs commented at 1:50 pm on June 27, 2018:
    Also, I presume this function is only dealing with multisigs, since it doesn’t need to know any special scripts to otherwise sign or extract signatures for pubkey-based outputs?
    achow101 commented at 8:58 pm on June 27, 2018:
    Done
  63. in src/script/sign.cpp:221 in d102f08f8f outdated 
    218     data.scriptWitness = tx.vin[nIn].scriptWitness;
219+    Stacks stack(data);
220+
221+    // Get signatures
222+    MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue);
223+    SignatureExtractorChecker checker(&data, &tx_checker);
    instagibbs commented at 1:47 pm on June 27, 2018:
    suggestion: rename this extractor_checker to make later code more clear. Final code block looks like it doesn’t change any state if reader thinks it’s a different checker.
    achow101 commented at 8:58 pm on June 27, 2018:
    Done
  64. instagibbs commented at 1:51 pm on June 27, 2018: member
    Comments on just making it clearer to readers of the code what’s going on.
  65. achow101 force-pushed on Jun 27, 2018
  66. achow101 force-pushed on Jun 27, 2018
  68. achow101 force-pushed on Jun 28, 2018
  69. achow101 commented at 7:12 pm on June 28, 2018: member
    Addressed @promag’s comments from #13557#pullrequestreview-132751570
  70. sipa commented at 1:32 am on June 29, 2018: member
    reutACK a0982864893db999484850282822472cce16913c
  71. in src/script/sign.h:67 in b48348d137 outdated 
    64+    bool complete = false; // Stores whether the scriptSig and scriptWitness are complete
65+    CScript scriptSig; // The scriptSig of an input. Contains complete signatures or the traditional partial signatures format
66+    CScript redeem_script; // The redeemScript (if any) for the input
67+    CScript witness_script; // The witnessScript (if any) for the input. witnessScripts are used in P2WSH outputs.
68+    CScriptWitness scriptWitness; // The scriptWitness of an input. Contains complete signatures or the traditional partial signatures format. scriptWitness is part of a transaction input per BIP 144.
69+    std::map<CKeyID, SigPair> signatures; // BIP 174 style partial signatures for the input. May contain all signatures necessary for producing a final scriptSig or scriptWitness.
    Empact commented at 10:46 pm on June 29, 2018:
    nit: these comments could be doxygenized, e.g. https://www.stack.nl/~dimitri/doxygen/manual/docblocks.html#memberdoc
    achow101 commented at 0:33 am on June 30, 2018:
    Done
  72. in src/script/sign.h:26 in 1b5de926f0 outdated 
    20@@ -21,11 +21,13 @@ class SigningProvider
21 {
22 public:
23     virtual ~SigningProvider() {}
24-    virtual bool GetCScript(const CScriptID &scriptid, CScript& script) const =0;
25-    virtual bool GetPubKey(const CKeyID &address, CPubKey& pubkey) const =0;
26-    virtual bool GetKey(const CKeyID &address, CKey& key) const =0;
27+    virtual bool GetCScript(const CScriptID &scriptid, CScript& script) const { return false; }
28+    virtual bool GetPubKey(const CKeyID &address, CPubKey& pubkey) const { return false; }
29+    virtual bool GetKey(const CKeyID &address, CKey& key) const { return false; }
    Empact commented at 10:52 pm on June 29, 2018:
    If you leave this pure virtual, it will continue to require that all inheritors implement the associated functions. With this change there’s no such forward requirement.
    achow101 commented at 0:23 am on June 30, 2018:
    So?
    Empact commented at 2:47 am on June 30, 2018:
    The theory is that if this stays an abstract base class, any future descendant are more likely to be correct & intentional if the compiler ensures that each of these methods is defined. It’s a more defensive and theoretical concern than a practical one, but I also think it’s cleaner if a dummy class does not influence the base class for all of the other classes.
    sipa commented at 3:24 am on June 30, 2018:
    The question is really whether “return false;” is a reasonable default implementation for someone who doesn’t want to implement one or more of the methods. I believe it is, though it can still be split up into an interface pure virtual class, and a dummy implementation you can inherit from when one or more things are intentionally missing.
  73. in src/script/sign.cpp:66 in fdcbcb3802 outdated 
    64-    case TX_PUBKEYHASH:
65+        if (!creator.CreateSig(provider, sig, CPubKey(vSolutions[0]).GetID(), scriptPubKey, sigversion)) return false;
66+        ret.push_back(std::move(sig));
67+        return true;
68+    case TX_PUBKEYHASH: {
69         keyID = CKeyID(uint160(vSolutions[0]));
    Empact commented at 11:14 pm on June 29, 2018:
    nit: This is the only use of keyID now, so you could declare it in this block.
    achow101 commented at 0:33 am on June 30, 2018:
    Done
  74. Inline Sign1 and SignN 
    Sign1 and SignN are kind of redundant so remove them and inline their
behavior into SignStep
    b6edb4f5e6
  75. achow101 force-pushed on Jun 30, 2018
  76. in src/script/sign.h:69 in a96d2e579b outdated 
    64+    bool complete = false; ///< Stores whether the scriptSig and scriptWitness are complete
65+    CScript scriptSig; ///< The scriptSig of an input. Contains complete signatures or the traditional partial signatures format
66+    CScript redeem_script; ///< The redeemScript (if any) for the input
67+    CScript witness_script; ///< The witnessScript (if any) for the input. witnessScripts are used in P2WSH outputs.
68+    CScriptWitness scriptWitness; ///< The scriptWitness of an input. Contains complete signatures or the traditional partial signatures format. scriptWitness is part of a transaction input per BIP 144.
69+    std::map<CKeyID, SigPair> signatures; ///< BIP 174 style partial signatures for the input. May contain all signatures necessary for producing a final scriptSig or scriptWitness.
    Empact commented at 8:49 pm on July 1, 2018:
    This is the same as CryptedKeyMap in keystore.h, might make sense to integrate the definitions.
    sipa commented at 9:00 pm on July 1, 2018:
    No it’s not. This stores signatures and pubkeys.
    Empact commented at 9:04 pm on July 1, 2018:
    To be clear, the type definitions are the same: https://github.com/bitcoin/bitcoin/blob/686e97a0c7358291d628213447cf33e99cde7ce8/src/keystore.h#L78
    sipa commented at 9:19 pm on July 1, 2018:

    Ah, I see.

    I generally don’t think types should be identical because their construction is the same. The contents has a very different meaning, so having separate names makes sense.

  77. in src/script/sign.h:73 in a96d2e579b outdated 
    68+    CScriptWitness scriptWitness; ///< The scriptWitness of an input. Contains complete signatures or the traditional partial signatures format. scriptWitness is part of a transaction input per BIP 144.
69+    std::map<CKeyID, SigPair> signatures; ///< BIP 174 style partial signatures for the input. May contain all signatures necessary for producing a final scriptSig or scriptWitness.
70 
71     SignatureData() {}
72     explicit SignatureData(const CScript& script) : scriptSig(script) {}
73+    void MergeSignatureData(SignatureData sigdata);
    Empact commented at 9:02 pm on July 1, 2018:
    It’s a bit strange that this commit adds MergeSignatureData but doesn’t use it.
    achow101 commented at 9:12 pm on July 2, 2018:
    Moved to the commit where it is used.
    sipa commented at 11:51 pm on July 3, 2018:
    The implementation is now in the next commit, but the definition is still in “Make SignatureData able to store signatures and scripts”.
    achow101 commented at 0:20 am on July 4, 2018:
    Moved the definition.
  78. achow101 force-pushed on Jul 2, 2018
  79. in src/script/sign.h:94 in 187c348d88 outdated 
    90@@ -81,4 +91,15 @@ void UpdateInput(CTxIn& input, const SignatureData& data);
91  * Solvability is unrelated to whether we consider this output to be ours. */
92 bool IsSolvable(const SigningProvider& provider, const CScript& script);
93 
94+class SignatureExtractorChecker final : public BaseSignatureChecker
    sipa commented at 11:52 pm on July 3, 2018:
    I don’t think this class is used (or intended to be used) outside of sign.cpp, so perhaps move it there, inside the anonymous namespace?
    achow101 commented at 0:20 am on July 4, 2018:
    Moved
  80. sipa commented at 11:55 pm on July 3, 2018: member
    Tiny nits.
  81. Make SignatureData able to store signatures and scripts 
    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.
    0422beb9bd
  82. Replace CombineSignatures with ProduceSignature 
    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.
    ed94c8b556
  83. Remove CombineSignatures and replace tests 
    Removes CombineSignatures and replaces its use in tests with
ProduceSignature to test the same behavior for ProduceSignature.
    b815600295
  84. achow101 force-pushed on Jul 4, 2018
  85. laanwj commented at 3:17 pm on July 5, 2018: member
    utACK b815600295bd5e7b274c8ee32da65f1419a79ab2
  86. laanwj merged this on Jul 5, 2018
  87. laanwj closed this on Jul 5, 2018
  88. laanwj referenced this in commit 028b0d963c on Jul 5, 2018
  89. laanwj removed this from the "Blockers" column in a project
  90. laanwj referenced this in commit b654723461 on Jul 18, 2018
  91. kittywhiskers referenced this in commit c61e4f1307 on Jun 4, 2021
  92. kittywhiskers referenced this in commit 4696244569 on Jun 4, 2021
  93. UdjinM6 referenced this in commit 5baf21b849 on Jun 7, 2021
  94. barrystyle referenced this in commit acb122e6cf on Jun 8, 2021
  95. barrystyle referenced this in commit 9166796714 on Jun 8, 2021
  96. kittywhiskers referenced this in commit 549a48f819 on Jun 9, 2021
  97. UdjinM6 referenced this in commit edabb0259c on Jun 24, 2021
  98. UdjinM6 referenced this in commit feeef686e6 on Jun 26, 2021
  99. UdjinM6 referenced this in commit d77a0f3075 on Jun 26, 2021
  100. UdjinM6 referenced this in commit c91e67d79a on Jun 26, 2021
  101. UdjinM6 referenced this in commit 0ef8fae2c0 on Jun 28, 2021
  102. kittywhiskers referenced this in commit 50012e4bd6 on Jul 9, 2021
  103. kittywhiskers referenced this in commit a19aa6d822 on Jul 13, 2021
  104. kittywhiskers referenced this in commit b60872b89e on Jul 13, 2021
  105. PastaPastaPasta referenced this in commit e98241da5d on Jul 13, 2021
  106. DrahtBot locked this on Sep 8, 2021


achow101 sipa meshcollider DrahtBot instagibbs Empact laanwj

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me