txscript: add taproot script type #1768

buck54321 · 2021-11-01T19:40:18Z

Add the WitnessV1TaprootTy script class and return it from GetScriptClass / typeOfScript.

Fix ComputePkScript, which was returning incorrect results for taproot (an probably other) scripts. ComputePkScript is now essentially useless for both v0 and v1 output witnesses.

Bump the btcutil dep to leverage new taproot address type.

ellemouton · 2021-11-02T08:59:29Z

txscript/pkscript.go

+// IMPORTANT: With the addition of taproot, we can no longer say for certain
+// what kind of script the witness is in most cases. The only case in which we
+// can say for sure is when the witness data has an annex as the last push. In
+// that case, we can identify the script type, but we lack the ability to
+// reconstruct the script itself.


i would say that the only case we know for certain is when the number of witness elements is exactly 1. In that case we know it is p2tr keypath (for now at least. not future proof).

I dont know if we can say that that it is taproot just cause there is an annex starting with 0x50 cause surely there can be other p2w scripts (pre-taproot) that just happen to have a final witness element starting with 0x50?

let me know what you think

I dont know if we can say that that it is taproot just cause there is an annex starting with 0x50 cause surely there can be other p2w scripts (pre-taproot) that just happen to have a final witness element starting with 0x50?

I guess that's true. Mkay.

i would say that the only case we know for certain is when the number of witness elements is exactly 1. In that case we know it is p2tr keypath (for now at least. not future proof).

I'm unable to verify this. Is it impossible to construct a p2wsh script spent by a length 1 witness?

I'm unable to verify this. Is it impossible to construct a p2wsh script spent by a length 1 witness?

yeah indeed, you are correct. It would be a really weird anyone-can-spend p2wsh though. Since the element would have to be the redeemscript and then there is no spending script that follows...

I guess that's true. Mkay.

Ok, I was wrong about this. I was not super familiar with how scripts are parsed. So yeah, if it starts with 0x50 then it is the annex

Sorry to interject as a third party, but while I found lightninglabs/neutrino#234 (comment) very enlightening, I think it would all fall apart when witness version 2 rolls around, like what happened with version 1 showed up and looked like p2wsh.
So while it sounds like much can be surmised from the witness stack, a hard fail based on the result of computeWitnessPkScript seems risky.
I think @Roasbeef has previously suggested somewhere (Slack maybe?) that multiple candidate types could be identified where there are ambiguities like the cases sipa and others have described, and I wonder if that approach is safest.

a hard fail based on the result of computeWitnessPkScript seems risky

yeah, especially when considering future witness versions like you said

coveralls · 2021-11-02T21:16:58Z

Pull Request Test Coverage Report for Build 1493217358

53 of 73 (72.6%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.02%) to 52.925%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
txscript/pkscript.go	2	5	40.0%
txscript/standard.go	47	64	73.44%

Files with Coverage Reduction	New Missed Lines	%
connmgr/connmanager.go	2	86.07%

Totals
Change from base Build 1469863602:	0.02%
Covered Lines:	21178
Relevant Lines:	40015

💛 - Coveralls

philipglazman · 2021-11-03T03:28:41Z

These new test vectors for BIP341 might be applicable. bitcoin/bips#1225

Roasbeef · 2021-11-04T00:25:54Z

Thanks for the PR! Want to hold off on a bit of this till we get #1684 in since it modifies a lot w.r.t how we parse scripts in general (main goal is GC optimization).

Add the WitnessV1TaprootTy script class and return it from GetScriptClass / typeOfScript. Fix ComputePkScript, which was returning incorrect results for taproot (an probably other) scripts. ComputePkScript is now essentially useless for both v0 and v1 output witnesses. Bump the btcutil dep to leverage new taproot address type.

buck54321 · 2021-11-21T12:16:18Z

txscript/script.go

+	case 1:
+		// https://github.com/bitcoin/bitcoin/blob/368831371d97a642beb54b5c4eb6eb0fedaa16b4/src/script/interpreter.cpp#L2090
+		return 0
 	}


I'm not certain about this.

buck54321 · 2021-11-21T12:19:21Z

txscript/standard.go

+	case 1:
+		switch {
+		case isWitnessTaprootScript(script):
+			return WitnessV1TaprootTy
+		}
 	}


I guess this isn't really exercised since typeOfScript is only used in CalcScriptInfo, which is now clearly defined only for v0 scripts. Should I leave?

benma · 2021-11-22T09:09:09Z

Thanks for the PR! Want to hold off on a bit of this till we get #1684 in since it modifies a lot w.r.t how we parse scripts in general (main goal is GC optimization).

The linked PR #1769 is merged. @buck54321 @Roasbeef would be great to get this merged soon, as it is blocking taproot support in the BitBoxApp 🙏

benma · 2021-11-22T09:17:48Z

@buck54321 PayToAddrScript in standard.go needs to be extended by a *btcutil.AddressTaproot case.

We use functions from the btcd library to convert addresses to pubKeyScripts and vice versa. While the address type has support for Taproot addresses, the pubkey script functions in btcd don't handle them yet. See also: btcsuite/btcd#1768 To speed up send-to-taproot support, refactor the pubkey script encoding/decoding into separate unit-tested functions, to which we can easily add taproot support in the next commit.

Add taproot address handling in PayToAddrScript. Adds a test and also some missing tests for p2wsh and p2wpkh addresses.

gcsfred2 · 2021-12-06T18:46:19Z

script.go has CalcWitnessSigHash with const scriptVersion = 0 hardcoded.

gcsfred2 · 2021-12-06T19:42:14Z

script.go has still

const (
	SigHashOld          SigHashType = 0x0

unchanged, but BIP341 redefines 0x00: "We define a new hashtype SIGHASH_DEFAULT (value 0x00)".

buck54321 · 2021-12-06T22:06:35Z

script.go has CalcWitnessSigHash with const scriptVersion = 0 hardcoded.

That gets trickier since it gets into the ScriptTokenizer and signature hash calculations. Can I call that work out of scope for this PR so that someone with more time or know-how can handle it in a separate effort? I don't want to slow you guys down.

gcsfred2 · 2021-12-06T22:46:17Z

Understood. I agree. No problem with me.

script.go has CalcWitnessSigHash with const scriptVersion = 0 hardcoded.

That gets trickier since it gets into the ScriptTokenizer and signature hash calculations. Can I call that work out of scope for this PR so that someone with more time or know-how can handle it in a separate effort? I don't want to slow you guys down.

gdsoumya · 2021-12-16T15:58:20Z

Hi, just checking on the status of the PR. We are using btcd in our project and this PR is necessary for us to activate support for taproot. Any update would be appreciated, thanks!

We use functions from the btcd library to convert addresses to pubKeyScripts and vice versa. While the address type has support for Taproot addresses, the pubkey script functions in btcd don't handle them yet. See also: btcsuite/btcd#1768 To speed up send-to-taproot support, refactor the pubkey script encoding/decoding into separate unit-tested functions, to which we can easily add taproot support in the next commit.

Roasbeef · 2021-12-25T00:54:30Z

Can I call that work out of scope for this PR so that someone with more time or know-how can handle it in a separate effort? I don't want to slow you guys down.

np @buck54321, I'm building off this PR currently as a follow up to #1773 where I finish up the rest of the taproot validation logic.

Roasbeef · 2022-01-07T02:59:00Z

Rolled these commits (with some modifications) into #1787, thanks for starting this @buck54321!

ellemouton reviewed Nov 2, 2021

View reviewed changes

buck54321 added 2 commits November 21, 2021 05:46

add some txscript support for parsing taproot scripts

3316b84

buck54321 force-pushed the a-little-taproot branch from 93b8570 to 3316b84 Compare November 21, 2021 12:11

buck54321 commented Nov 21, 2021

View reviewed changes

return accidentally deleted comment

6708834

benma mentioned this pull request Nov 22, 2021

update btcutil to 9cdf59f60c51 #1771

Closed

PayToAddrScript too

4fafad4

Add taproot address handling in PayToAddrScript. Adds a test and also some missing tests for p2wsh and p2wpkh addresses.

Roasbeef mentioned this pull request Jan 7, 2022

multi: implement BIP 341 and 342 a.k.a complete taproot and tapscript consensus verification logic #1787

Merged

5 tasks

Roasbeef closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txscript: add taproot script type #1768

txscript: add taproot script type #1768

buck54321 commented Nov 1, 2021

ellemouton Nov 2, 2021

ellemouton Nov 2, 2021

buck54321 Nov 3, 2021

ellemouton Dec 2, 2021 •

edited

ellemouton Dec 2, 2021

chappjc Dec 2, 2021 •

edited

ellemouton Dec 2, 2021

coveralls commented Nov 2, 2021 •

edited

philipglazman commented Nov 3, 2021

Roasbeef commented Nov 4, 2021

buck54321 Nov 21, 2021

buck54321 Nov 21, 2021 •

edited

benma commented Nov 22, 2021

benma commented Nov 22, 2021

gcsfred2 commented Dec 6, 2021

gcsfred2 commented Dec 6, 2021

buck54321 commented Dec 6, 2021

gcsfred2 commented Dec 6, 2021

gdsoumya commented Dec 16, 2021

Roasbeef commented Dec 25, 2021

Roasbeef commented Jan 7, 2022

txscript: add taproot script type #1768

txscript: add taproot script type #1768

Conversation

buck54321 commented Nov 1, 2021

ellemouton Nov 2, 2021

Choose a reason for hiding this comment

ellemouton Nov 2, 2021

Choose a reason for hiding this comment

buck54321 Nov 3, 2021

Choose a reason for hiding this comment

ellemouton Dec 2, 2021 • edited

Choose a reason for hiding this comment

ellemouton Dec 2, 2021

Choose a reason for hiding this comment

chappjc Dec 2, 2021 • edited

Choose a reason for hiding this comment

ellemouton Dec 2, 2021

Choose a reason for hiding this comment

coveralls commented Nov 2, 2021 • edited

Pull Request Test Coverage Report for Build 1493217358

💛 - Coveralls

philipglazman commented Nov 3, 2021

Roasbeef commented Nov 4, 2021

buck54321 Nov 21, 2021

Choose a reason for hiding this comment

buck54321 Nov 21, 2021 • edited

Choose a reason for hiding this comment

benma commented Nov 22, 2021

benma commented Nov 22, 2021

gcsfred2 commented Dec 6, 2021

gcsfred2 commented Dec 6, 2021

buck54321 commented Dec 6, 2021

gcsfred2 commented Dec 6, 2021

gdsoumya commented Dec 16, 2021

Roasbeef commented Dec 25, 2021

Roasbeef commented Jan 7, 2022

ellemouton Dec 2, 2021 •

edited

chappjc Dec 2, 2021 •

edited

coveralls commented Nov 2, 2021 •

edited

buck54321 Nov 21, 2021 •

edited