A few follow-ups for taproot signing #22275

sipa commented at 12:47 AM on June 18, 2021: member

This addresses a few review comments from #21365 that were left at the time of merge (as well as some from #22166 applying to the commit it shared with #21365).

I do not think any are blockers for a 22.0 release.

sipa force-pushed on Jun 18, 2021

DrahtBot added the label Consensus on Jun 18, 2021

theStack commented at 1:58 PM on June 18, 2021: member

Concept ACK

achow101 commented at 7:26 PM on June 18, 2021: member

aCK 38844bc18492a847a6a3aff000eb0e85571070dd

sipa force-pushed on Jun 18, 2021

sipa commented at 8:28 PM on June 18, 2021: member

Improved the comment in CKey::SignSchnorr to elaborate on the 3 variants of tweaking (pointed out here).

Sjors commented at 3:46 PM on June 21, 2021: member

utACK 9336670

theStack approved

theStack commented at 4:53 PM on June 22, 2021: member

ACK 9336670df5e0b8baa4169f13ba1d242ecae85d26

Checked that the introduced comments match the logic, verified by review that the code-changes are refactor-only (+ an added sanity check in TaprootBuilder::GetSpendData), built locally, ran unit tests and the functional test feature_taproot.py.

in src/script/sign.cpp:648 in 9336670df5 outdated

 642 |          CTxIn& txin = mtx.vin[i];
 643 |          auto coin = coins.find(txin.prevout);
 644 |          if (coin == coins.end() || coin->second.IsSpent()) {
 645 | -            have_all_spent_outputs = false;
 646 | +            txdata.Init(txConst, {}, true);
 647 | +            break;

jonatack commented at 10:34 AM on June 27, 2021:

2b19345 suggestion

@@ -636,11 +636,11 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
 
     PrecomputedTransactionData txdata;
     std::vector<CTxOut> spent_outputs;
-    for (unsigned int i = 0; i < mtx.vin.size(); i++) {
+    for (unsigned int i = 0; i < mtx.vin.size(); ++i) {
         CTxIn& txin = mtx.vin[i];
         auto coin = coins.find(txin.prevout);
         if (coin == coins.end() || coin->second.IsSpent()) {
-            txdata.Init(txConst, {}, true);
+            txdata.Init(txConst, /* spent_outputs */ {}, /* force */ true);
             break;
         } else {
             spent_outputs.emplace_back(coin->second.out.nValue, coin->second.out.scriptPubKey);
@@ -651,7 +651,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
     }
 
     // Sign what we can:
-    for (unsigned int i = 0; i < mtx.vin.size(); i++) {
+    for (unsigned int i = 0; i < mtx.vin.size(); ++i) {
         CTxIn& txin = mtx.vin[i];

sipa commented at 11:42 PM on July 2, 2021:

Done.

in src/script/interpreter.h:176 in 9336670df5 outdated

 171 | +     *
 172 | +     * @param[in]   tx             The transaction for which data is being precomputed.
 173 | +     * @param[in]   spent_outputs  The CTxOuts being spent, one for each tx.vin, in order
 174 | +     * @param[in]   force          Precompute data for all optional features, regardless of
 175 | +     *                             what is in the inputs (used at signing time, when the inputs
 176 | +     *                             aren't filled in yet). */

jonatack commented at 10:36 AM on June 27, 2021:

8b1d3db suggestion

@@ -170,10 +170,10 @@ struct PrecomputedTransactionData
     /** Initialize this PrecomputedTransactionData with transaction data.
      *
      * [@param](/bitcoin-bitcoin/contributor/param/)[in]   tx             The transaction for which data is being precomputed.
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   spent_outputs  The CTxOuts being spent, one for each tx.vin, in order
-     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   force          Precompute data for all optional features, regardless of
-     *                             what is in the inputs (used at signing time, when the inputs
-     *                             aren't filled in yet). */
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   spent_outputs  The CTxOuts being spent, one for each tx.vin, in order.
+     * [@param](/bitcoin-bitcoin/contributor/param/)[in]   force          Whether to precompute data for all optional features,
+     *                             regardless of what is in the inputs (used at signing
+     *                             time, when the inputs aren't filled in yet). */

sipa commented at 11:42 PM on July 2, 2021:

Done.

jonatack commented at 10:41 AM on June 27, 2021: member

ACK 9336670df5e0b8baa4169f13ba1d242ecae85d26

DrahtBot commented at 12:23 PM on June 29, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22558 (psbt: Taproot fields for PSBT by achow101)
#22512 (Consolidate XOnlyPubKey lookup hack by achow101)
#22440 (Eliminate Signature Checker/Creator Ambiguity w/ LIFETIMEBOUND References by JeremyRubin)
#22364 (wallet: Make a tr() descriptor by default by achow101)

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.

sipa force-pushed on Jul 2, 2021

theStack commented at 9:38 AM on July 3, 2021: member

It seems like after rebasing, the CKey::SignSchnorr comment improvement (previously done in commit https://github.com/bitcoin/bitcoin/commit/8b1d3dbfac899c19ab92e3591157d9f4a8c3ca49) is missing now?

jonatack commented at 9:47 PM on July 4, 2021: member

ACK per git range-diff 7a49fdc 9336670 f0fb5f8 modulo the missing CKey::SignSchnorr() doxygen improvement as mentioned in #22275#pullrequestreview-698581320

Improve comments in taproot signing logic addb9b5a71

Simplify SignTransaction precomputation loop c7048aae95

Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator

These were unused except in tests, and were also overlooked when changing
SIGHASH_ALL -> SIGHASH_DEFAULT.

d8f4b976d5

Assert that IsComplete() in GetSpendData() 08f57a0057

sipa force-pushed on Aug 20, 2021

sipa commented at 6:30 PM on August 20, 2021: member

@theStack @jonatack Restored the accidentally deleted comment improvement.

theStack approved

theStack commented at 9:17 PM on August 21, 2021: member

re-ACK 08f57a0057c58f19cd8ae3de89548d014980478a 🌴

Checked via git range-diff 9336670d...08f57a00 that since my last ACK, the only changes were jonatack's suggestions w.r.t. to doxygen comments wording and code style (https://github.com/bitcoin/bitcoin/pull/22275#discussion_r659299734, #22275 (review)) plus the rebase on master.

Zero-1729 commented at 2:02 AM on August 22, 2021: contributor

crACK 08f57a0 🧉

LGTM, changes check out and taproot tests passed locally.

jonatack commented at 3:39 PM on August 22, 2021: member

Code review ACK 08f57a0057c58f19cd8ae3de89548d014980478a per git range-diff e9d6eb1 9336670 08f57a0 followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check

fanquake merged this on Aug 23, 2021

fanquake closed this on Aug 23, 2021

sidhujag referenced this in commit a77514a7e9 on Aug 23, 2021

uvhw commented at 3:44 PM on October 31, 2021: none

#14529

DrahtBot locked this on Oct 31, 2022