Nested witness signing #1304

afk11 · 2018-09-22T12:35:02Z

This PR adds SignTxWitness which handles signing normal outputs + any that use segwit. It will return a signature script and witness structure after signing whatever it can and merging signatures. The function is a superset of what SignTxOutput supports. I left SignTxOutput's intact to preserve BC, though SignTxWitness also works with old outputs and should probably be used going forward for easy segwit support.

I went with using [][]byte to pass around input script data because it's easily converted back to a pushonly scriptSig, or can be used as a witness if called for.

Also added test cases comparable to SignTxOutput:

P2WPKH: bare and P2SH
P2PKH: P2SH, P2WSH and P2SH|P2WSH
P2PK: P2SH, P2WSH and P2SH|P2WSH
Multisig 2of2 (basic, step by step, & with key duplication): P2SH, P2WSH and P2SH|P2WSH

jakesylvestre · 2020-03-04T14:06:49Z

@jcvernaleo (as per #1530)

Low priority
Enhancement
Outdated

siansong · 2020-10-26T11:19:47Z

any progress on this?

onyb · 2020-10-26T11:35:00Z

@sunsiansong It seems the PR was lost in the backlog. I've added this to our internal review board to prioritize this.

In the meantime, if you can help with reviewing and testing out this PR, that'd be very helpful!

onyb · 2020-10-26T11:37:08Z

@afk11 If you could rebase your branch on the current master to run the CI checks and fix the outdated Glide stuff, that'd really get the ball rolling.

afk11 · 2020-10-27T14:30:45Z

Hi all

Thanks for the ping. I know most apps got by without this feature, but did think it was worthwhile to bring everything up to date. I'll try rebase this in the next day or so.

Re tests, I covered most nested forms of P2PK/P2PKH/multisig in P2SH, P2WSH, and nested P2WSH in P2SH with the results of each verified by the script interpreter. So I'm pretty confident it was working :) https://github.com/btcsuite/btcd/pull/1304/files#diff-c4fcde2c18ebe144b7b71b53efb1021c7bb089a8c02602134402999ca9fa1417R1436

I'll check in more depth later and report back

coveralls · 2020-10-27T19:55:01Z

Pull Request Test Coverage Report for Build 343822214

184 of 195 (94.36%) changed or added relevant lines in 1 file are covered.
12 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.2%) to 53.581%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
txscript/sign.go	184	195	94.36%

Files with Coverage Reduction	New Missed Lines	%
txscript/sign.go	1	90.6%
peer/peer.go	2	75.55%
connmgr/connmanager.go	3	86.07%
btcec/signature.go	6	91.64%

Totals
Change from base Build 329751959:	0.2%
Covered Lines:	20910
Relevant Lines:	39025

💛 - Coveralls

jcvernaleo

This looks good to me but could you squash it to one commit please? That way you can give one coherent commit message (rather than if I squash from the github interface). But otherwise, great work.

afk11 · 2020-11-03T15:10:25Z

Squashed @jcvernaleo

…wit scripts

afk11 · 2020-10-27T20:09:32Z

txscript/sign.go

-	ScriptClass, []btcutil.Address, int, error) {
+// sign supports returning the scripts for script-hash types, as well
+// as returning a signature stack if the script was P2PK|P2PKH|multisig
+// P2WPKH is not handled here, but instead hints by the


todo: finish this comment

updated comment when squashing

afk11 · 2020-10-27T20:53:14Z

txscript/sign.go

-		builder.AddData(script)
-		finalScript, _ := builder.Script()
-		return finalScript
+		return nil, errors.New("cannot merge " + ScriptHashTy.String() + " type")


should review the history behind this and see why so much was taken out

OK - mergeScripts is now used differently which explains it. It doesn't handle P2SH, or P2WSH because the caller adjusts their parameters based on the case.

I think a follow up PR could remove mergeScripts and just call mergeMultiSig directly if the default case isn't taken - it might not need to be there

afk11 · 2020-11-06T16:00:50Z

txscript/sign_test.go

+	if err != nil {
+		return fmt.Errorf("failed to verify output %s: %v", msg, err)
+	}
+


Adding this section tests SignTxWitness in all the old tests (non-segwit cases)

afk11 · 2020-11-09T17:53:48Z

@jcvernaleo @onyb anything else you'd like done on this?

onyb

Really good work on this!

I am submitting my first set of review comments. I haven't reviewed the unit-tests yet and would like to do another pass on SignTxWitness.

In addition to my review comments, I have a general remark regarding your choice of using raw stacks instead of the script builder that we already have. I realize that it does give you some convenience in a few places, but it reduces readability and a bit unsafe (due to lack of validation, prone to programmer errors, etc.).

To understand the tradeoffs better, can you please add some GitHub review comments to SignTxWitness function indicating the places where you felt that using the script builder was too inconvenient? Perhaps we can brainstorm together to come up with some helpers that make using script-builder less cumbersome?

onyb · 2020-10-28T23:15:11Z

txscript/sign.go

+	subscript []byte, hashType SigHashType,
+	privKey *btcec.PrivateKey, compress bool) (wire.TxWitness, error) {


nit: The wrapping is a bit weird. Line 50 is over 80 characters, so amt int64 could be moved to the next line.

onyb · 2020-10-28T23:35:44Z

txscript/sign.go

+// a nil pointer dereference. We require compress here to ensure segwit public
+// key encoding policy is followed
+func makeSignature(tx *wire.MsgTx, sigHashes *TxSigHashes, idx int,
+	amt int64, subscript []byte, sigVersion int, hashType SigHashType,


Can we introduce an enum for sigVersion? Here is the reference from Bitcoin Core.

onyb · 2020-11-09T23:44:52Z

txscript/sign.go

 	// slices here, rather than a single byte slice.
-	return wire.TxWitness{sig, pkData}, nil
+	return wire.TxWitness(stack), nil


Simply doing this should work too:

Suggested change

return wire.TxWitness(stack), nil

return stack, nil

onyb · 2020-11-09T23:49:37Z

txscript/sign.go

+		}
+		return RawTxInWitnessSignature(tx, sigHashes, idx, amt, subscript, hashType, privKey)
+	}
+	return RawTxInSignature(tx, idx, subscript, hashType, privKey)


What do you think about wrapping this call around an if sigVersion == 0 {...}, and returning an error for other values of sigVersion.

onyb · 2020-11-10T00:00:20Z

txscript/sign.go

+	stack := make([][]byte, 1)
+	stack[0] = sig


Nit

Suggested change

stack := make([][]byte, 1)

stack[0] = sig

stack := [][]byte{sig}

onyb · 2020-11-10T00:48:04Z

txscript/sign.go

+	for i := 0; i < len(scriptStack); i++ {
+		builder.AddData(scriptStack[i])
+	}
+	if redeemScript != nil {


style nit: do not cuddle with the previous block

onyb · 2020-11-10T00:48:18Z

txscript/sign.go

+	}
+
+	// Build final scriptSig and witness
+	builder := NewScriptBuilder()


style nit: add new line after this statement

onyb · 2020-11-10T00:48:33Z

txscript/sign.go

+	if redeemScript != nil {
+		builder.AddData(redeemScript)
+	}
+	scriptSig, err := builder.Script()


style nit: do not cuddle with the previous block

onyb · 2020-11-10T00:48:39Z

txscript/sign.go

+	if err != nil {
+		return nil, nil, err
+	}
+	if witnessScript != nil {


style nit: do not cuddle with the previous block

onyb · 2020-11-10T00:51:13Z

txscript/sign.go

+		sigVersion = 1
+		witnessScript = stack[0]
+		pkScript = stack[0]
+		stack, class, addresses, nrequired, err = sign(chainParams, tx, sigHashes,


Please use a different variable instead of class. Perhaps innerClass?

afk11 force-pushed the nested-witness-sign branch 5 times, most recently from fbef308 to 96b8929 Compare September 22, 2018 16:10

jcvernaleo added enhancement low priority outdated labels Mar 4, 2020

afk11 force-pushed the nested-witness-sign branch from 7e89ed2 to b6bcef2 Compare October 27, 2020 19:50

onyb added this to the 0.22.0 milestone Oct 27, 2020

jcvernaleo requested changes Nov 3, 2020

View reviewed changes

afk11 force-pushed the nested-witness-sign branch from b6bcef2 to 3aef94d Compare November 3, 2020 15:09

TxWitnessSign: supports signing transaction inputs with legacy or seg…

fb7974d

…wit scripts

afk11 force-pushed the nested-witness-sign branch from 3aef94d to fb7974d Compare November 3, 2020 15:13

afk11 commented Nov 3, 2020

View reviewed changes

afk11 commented Nov 6, 2020

View reviewed changes

onyb requested changes Nov 10, 2020

View reviewed changes

onyb removed low priority outdated labels Nov 10, 2020

onyb mentioned this pull request Feb 4, 2021

txscript: backport tokenizer from dcrd #1684

Closed

1 task

jcvernaleo removed this from the 0.22.0 milestone May 25, 2021

jcvernaleo added this to the 0.22.1 milestone May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested witness signing #1304

Nested witness signing #1304

afk11 commented Sep 22, 2018 •

edited

Loading

jakesylvestre commented Mar 4, 2020

siansong commented Oct 26, 2020

onyb commented Oct 26, 2020

onyb commented Oct 26, 2020

afk11 commented Oct 27, 2020 •

edited

Loading

coveralls commented Oct 27, 2020 •

edited

Loading

jcvernaleo left a comment

afk11 commented Nov 3, 2020 •

edited

Loading

afk11 Oct 27, 2020

afk11 Nov 3, 2020

afk11 Oct 27, 2020 •

edited

Loading

afk11 Nov 6, 2020 •

edited

Loading

afk11 commented Nov 9, 2020

onyb left a comment

onyb Oct 28, 2020

onyb Oct 28, 2020

onyb Nov 9, 2020

onyb Nov 9, 2020

onyb Nov 10, 2020

onyb Nov 10, 2020

onyb Nov 10, 2020

onyb Nov 10, 2020

onyb Nov 10, 2020

onyb Nov 10, 2020

		subscript []byte, hashType SigHashType,
		privKey *btcec.PrivateKey, compress bool) (wire.TxWitness, error) {

	stack := make([][]byte, 1)
	stack[0] = sig
	stack := [][]byte{sig}

Nested witness signing #1304

Are you sure you want to change the base?

Nested witness signing #1304

Conversation

afk11 commented Sep 22, 2018 • edited Loading

jakesylvestre commented Mar 4, 2020

siansong commented Oct 26, 2020

onyb commented Oct 26, 2020

onyb commented Oct 26, 2020

afk11 commented Oct 27, 2020 • edited Loading

coveralls commented Oct 27, 2020 • edited Loading

Pull Request Test Coverage Report for Build 343822214

💛 - Coveralls

jcvernaleo left a comment

Choose a reason for hiding this comment

afk11 commented Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afk11 Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

afk11 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

afk11 commented Nov 9, 2020

onyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afk11 commented Sep 22, 2018 •

edited

Loading

afk11 commented Oct 27, 2020 •

edited

Loading

coveralls commented Oct 27, 2020 •

edited

Loading

afk11 commented Nov 3, 2020 •

edited

Loading

afk11 Oct 27, 2020 •

edited

Loading

afk11 Nov 6, 2020 •

edited

Loading