swap: HTLCV3 added #473

arshbot · 2022-03-28T20:26:52Z

In this commit we add the version 3 htlc, which is implemented with
taproot script spending the two payment paths: the claim path case, and
the timeout case.

Pull Request Checklist

Update release_notes.md if your PR contains major features, breaking changes or bugfixes

arshbot · 2022-03-28T20:26:59Z

The internalPubKey bytes are set to a dummy variable for the time being, however we should have a dynamic way of generating keys which neither the client nor the server are aware of.

Used something like the below to attempt to accomplish this but some feedback would be appreciated.

	increment := func(seedPhrase []byte, i int) []byte {
		// Increment the origin of the hash using the mapping candidateSed :=
		// h(i || seedPhrase).
		shaStream := sha256.New()

		var iterationBytes [8]byte
		binary.BigEndian.PutUint64(iterationBytes[:], uint64(i))
		shaStream.Write(iterationBytes[:])

		shaStream.Write(seedPhrase)

		seedHash := shaStream.Sum(nil)

		return seedHash[:]
	}

	var i int
	for {
		// At the start of the iteration, we'll try with a counter
		// offset of 0, each time we fail to find a proper point, we'll
		// increment this by one.
		candidateSeed := increment(randomPub, i)

		fmt.Printf("iteration_num=%v, candidate=%x\n", i, candidateSeed[:])

		var pubBytes [33]byte
		pubBytes[0] = 0x02
		copy(pubBytes[1:], candidateSeed[:])

		// Try to see if this point when intercepted as an x coordinate
		// is actually found on the curve.
		candidatePoint, err := btcec.ParsePubKey(pubBytes[:])
		if err == nil {
			fmt.Printf("Global param generated: %x\n",
				candidatePoint.SerializeCompressed())
			break
		}

		fmt.Printf("not a quadratic residue: %v\n", err)

		i++
	}

swap/htlc.go

carlaKC · 2022-03-29T07:16:52Z

swap/htlc.go

+	// - witness_script_length: 1 byte
+	// - witness_script: len(script) bytes
+	// - control_block_length: 1 byte
+	// - control_block: 4129 bytes


How do you get 4129 for control block?

BIP 341:

the last stack element is called the control block c, and must have length 33 + 32m, for a value of m that is an integer between 0 and 128[6], inclusive.

33 + (32 * 128) = 4129

alternatively could be len(genControlBytes)

I think this should be based on the worst-case size of our control block so that we can get as close to accurate as possible.

I agree with Carla, since our taptree is shallow, our control block is also small compared to the worst theoretical case, so we should calculate the max for our case imo. (Even though we don't use this function).

Edit: we should actually also use these functions in our test cases as a sanity check (once the control block sizes are adjusted)

Set to our worst case, which is 65 bytes - please confirm

swap/htlc.go

carlaKC · 2022-03-29T09:26:28Z

swap/htlc.go

+	TaprootKey     *secp.PublicKey
+	InternalPubKey *secp.PublicKey


[32] byte?

Storing as byte slice adds a lot of conversions all over the place, but better here than throughout the codebase. Storing as array will only add another conversion without reason imo.

(also, I think secp.PublicKey are 33 bytes when compressed)

swap/htlc.go

carlaKC · 2022-03-30T12:48:04Z

swap/htlc.go

+func newHTLCScriptV3(cltvExpiry int32, receiverHtlcKey, senderHtlcKey [33]byte,
+	swapHash lntypes.Hash) (*HtlcScriptV3, error) {
+
+	_, receiverPubKey := btcec.PrivKeyFromBytes(


Pubkey from privkeyfrombytes?

receiverPubkey, err:= btcec.ParsePubkey(receiverHtlcKey[:]) copy(schnorrReceiverKey, schnorr.SerializePubkey(receiverPubkey)

From NewHtlc:

// NewHtlc returns a new instance. For V1 and V2 scripts, receiver and sender
// keys are expected to be in ecdsa compressed format. For V3 scripts, keys are
// expected to be raw private key bytes.

This was feedback I received from laolu and oli in regards to dealing with the serializeLoopContract issue which requires the keys stored to be 33 bytes (which ecdsa compressed are), however schnorr keys are 32 bytes, which throws the serializer off. To mitigate this issue, we store raw private key bytes in the LoopContract in memory and feed those raw bytes to NewHtlc only in the htlcv3 case.

Why can't we just store [33]byte then schnorr.SerializePubkey when we need to use the key in a HtlcV3 context?

Do you have a link to that convo so I can understand the context of that discussion?

Took that approach, addressed

carlaKC · 2022-03-31T06:13:40Z

swap/htlc.go

+	// - witness_script_length: 1 byte
+	// - witness_script: len(script) bytes
+	// - control_block_length: 1 byte
+	// - control_block: 4129 bytes


I think this should be based on the worst-case size of our control block so that we can get as close to accurate as possible.

carlaKC · 2022-03-31T06:14:55Z

swap/htlc.go

+func newHTLCScriptV3(cltvExpiry int32, receiverHtlcKey, senderHtlcKey [33]byte,
+	swapHash lntypes.Hash) (*HtlcScriptV3, error) {
+
+	_, receiverPubKey := btcec.PrivKeyFromBytes(


Why can't we just store [33]byte then schnorr.SerializePubkey when we need to use the key in a HtlcV3 context?

Do you have a link to that convo so I can understand the context of that discussion?

swap/htlc.go

lightninglabs-deploy · 2022-04-07T07:12:32Z

@bhandras: review reminder
@arshbot, remember to re-request review from reviewers when ready

bhandras

Great PR, almost there just a few comments remain.

swap/htlc.go

bhandras · 2022-04-08T10:18:45Z

swap/htlc.go

+	// - witness_script_length: 1 byte
+	// - witness_script: len(script) bytes
+	// - control_block_length: 1 byte
+	// - control_block: 4129 bytes


I agree with Carla, since our taptree is shallow, our control block is also small compared to the worst theoretical case, so we should calculate the max for our case imo. (Even though we don't use this function).

Edit: we should actually also use these functions in our test cases as a sanity check (once the control block sizes are adjusted)

swap/htlc.go

swap/htlc_test.go

bhandras · 2022-04-08T10:34:03Z

swap/htlc_test.go

+				)
+
+				sig := signTx(
+					tx, senderPrivKey, txscript.NewBaseTapLeaf(trHtlc.ClaimScript),


The testcase descriptions and the actual testcases are inverted.

swap/htlc_test.go

swap/htlc.go

arshbot

Made the requested changes! Ready for another pass

swap/htlc.go

arshbot · 2022-04-12T18:56:47Z

swap/htlc.go

+func newHTLCScriptV3(cltvExpiry int32, receiverHtlcKey, senderHtlcKey [33]byte,
+	swapHash lntypes.Hash) (*HtlcScriptV3, error) {
+
+	_, receiverPubKey := btcec.PrivKeyFromBytes(


Took that approach, addressed

arshbot · 2022-04-12T19:29:26Z

swap/htlc.go

+	// - witness_script_length: 1 byte
+	// - witness_script: len(script) bytes
+	// - control_block_length: 1 byte
+	// - control_block: 4129 bytes


Set to our worst case, which is 65 bytes - please confirm

guggero

Looks way better now, but still a lot of nits. We want to be extra careful, explicit and verbose with anything that touches on-chain scripts.

swap/htlc.go

swap/htlc_test.go

guggero · 2022-04-13T12:51:02Z

swap/htlc_test.go

+	preimage := [32]byte{1, 2, 3}
+	p := lntypes.Preimage(preimage)
+	hashedPreimage := sha256.Sum256(p[:])
+	value := int64(800_000 - 500) // TODO(guggero): Calculate actual fee.


Fee calculation is now available in lnd master.

Removed fee because previous tests don't include and it's a distraction from what this is testing

swap/htlc_test.go

arshbot · 2022-04-13T16:24:07Z

Addressed all nits, left a clarifying question! @bhandras @guggero PTAL!!

swap/htlc.go

bhandras

Tried to find everything, hopefully we're now just one round away from being ready.

bhandras · 2022-04-14T19:37:56Z

swap/htlc.go


 	switch outputType {
 	case HtlcNP2WSH:
+		pkScript, err := input.WitnessScriptHash(htlc.Script())


pkScript inside the case will shadow the pkScript declared outside. A way to fix it is to declare the error as well. Example: https://go.dev/play/p/3RXcMVh162P

Good catch, done. Since err is already declared earlier, the redeclaration shouldn't be necessary

It's still wrong?

edit: I see you fixed it in the other commit. Please fix it in this commit.

This Is still wrong in the first commit.

swap/htlc.go

swap/htlc_test.go

arshbot · 2022-04-19T12:55:34Z

@bhandras PTAL!

arshbot · 2022-04-19T14:16:38Z

@guggero I think is ready for another pass from you as well! PTAL!

bhandras

Almost ready :)

swap/htlc.go

bhandras · 2022-04-19T19:21:08Z

swap/htlc.go


 	switch outputType {
 	case HtlcNP2WSH:
+		pkScript, err := input.WitnessScriptHash(htlc.Script())


edit: I see you fixed it in the other commit. Please fix it in this commit.

swap/htlc.go

bhandras

Please fix the aliasing issue (#473 (review)) in the first commit, other than that LGTM 🎉

In this commit we add the version 3 htlc, which is implemented with taproot script spending the two payment paths: the claim path case, and the timeout case.

bhandras · 2022-04-20T14:57:01Z

Uhm, these PRs need two approvals..

arshbot requested review from bhandras and carlaKC March 28, 2022 20:26

arshbot force-pushed the HtlcV3-taproot-base branch 4 times, most recently from 8765ba2 to 83f8a76 Compare March 28, 2022 20:37

carlaKC reviewed Mar 29, 2022

View reviewed changes

arshbot force-pushed the HtlcV3-taproot-base branch 3 times, most recently from 3b3df7b to 5a31825 Compare March 29, 2022 15:15

arshbot requested a review from carlaKC March 29, 2022 15:15

carlaKC suggested changes Mar 30, 2022

View reviewed changes

carlaKC mentioned this pull request Mar 30, 2022

multi: upgrade to P2TR htlcs without keyspend path #475

Closed

arshbot force-pushed the HtlcV3-taproot-base branch from 5a31825 to 3d4ba17 Compare March 30, 2022 17:19

carlaKC reviewed Mar 31, 2022

View reviewed changes

bhandras mentioned this pull request Apr 3, 2022

multi: changes to the taproot HTLC required for both client and server #477

Merged

bhandras reviewed Apr 8, 2022

View reviewed changes

bhandras requested a review from guggero April 8, 2022 10:39

bhandras reviewed Apr 8, 2022

View reviewed changes

swap/htlc.go Outdated Show resolved Hide resolved

bhandras reviewed Apr 8, 2022

View reviewed changes

swap/htlc.go Outdated Show resolved Hide resolved

bhandras reviewed Apr 8, 2022

View reviewed changes

swap/htlc.go Show resolved Hide resolved

yyforyongyu self-requested a review April 11, 2022 14:11

arshbot force-pushed the HtlcV3-taproot-base branch 2 times, most recently from 2a2d675 to 06fd791 Compare April 12, 2022 20:24

arshbot commented Apr 12, 2022

View reviewed changes

arshbot requested a review from bhandras April 12, 2022 20:26

guggero reviewed Apr 13, 2022

View reviewed changes

arshbot force-pushed the HtlcV3-taproot-base branch from 06fd791 to fcc1c35 Compare April 13, 2022 16:22

arshbot requested a review from guggero April 13, 2022 16:23

bhandras reviewed Apr 13, 2022

View reviewed changes

swap/htlc.go Outdated Show resolved Hide resolved

arshbot force-pushed the HtlcV3-taproot-base branch 2 times, most recently from b8dff91 to abce7d1 Compare April 14, 2022 16:41

arshbot requested a review from bhandras April 14, 2022 16:42

bhandras reviewed Apr 14, 2022

View reviewed changes

arshbot force-pushed the HtlcV3-taproot-base branch 2 times, most recently from bcfb001 to b3e4f68 Compare April 18, 2022 18:49

arshbot requested a review from bhandras April 18, 2022 18:49

arshbot force-pushed the HtlcV3-taproot-base branch from b3e4f68 to 2135bcd Compare April 19, 2022 14:17

bhandras reviewed Apr 19, 2022

View reviewed changes

arshbot force-pushed the HtlcV3-taproot-base branch from 2135bcd to 0f75c9b Compare April 19, 2022 23:11

arshbot requested a review from bhandras April 19, 2022 23:12

bhandras approved these changes Apr 20, 2022

View reviewed changes

arshbot added 2 commits April 20, 2022 10:52

swap: refactor NewHtlc so pkScript is generatred per output

cee607f

swap: HTLCV3 added

173c213

In this commit we add the version 3 htlc, which is implemented with taproot script spending the two payment paths: the claim path case, and the timeout case.

arshbot force-pushed the HtlcV3-taproot-base branch from 0f75c9b to 173c213 Compare April 20, 2022 14:53

arshbot merged commit 5446a1f into lightninglabs:master Apr 20, 2022

This was referenced Apr 20, 2022

Revert "swap: HTLCV3 added" #481

Merged

swap: HTLCV3 added #482

Merged

swap: HTLCV3 added #473

swap: HTLCV3 added #473

Uh oh!

Conversation

arshbot commented Mar 28, 2022

Pull Request Checklist

Uh oh!

arshbot commented Mar 28, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arshbot Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhandras Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arshbot Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

lightninglabs-deploy commented Apr 7, 2022

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bhandras Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arshbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arshbot Mar 29, 2022 •

edited

Loading

bhandras Apr 8, 2022 •

edited

Loading

arshbot Mar 29, 2022 •

edited

Loading

bhandras Apr 8, 2022 •

edited

Loading