[custom channels]: generate unique script keys for HTLCs #1209

guggero · 2024-11-21T15:36:30Z

Fixes the LiT integration tests that were broken after merging #1181.
But that basically showed us that colliding script keys for MPP shard HTLCs are actually a problem.

This PR tweaks each script key with the HTLC index, making them unique again, even if they have the same payment hash and timeout (which results in the same BTC level taproot output key).

To make the accompanying litd integration tests pass, we also fix the following two bugs:

Calculate the sweep transaction change output index correctly and dynamically to avoid invalid exclusion proofs when there are multiple HTLCs being swept at the same time.
Fix an issue with UpsertScriptKey that wouldn't allow the tweak of a script key to be updated when it is learned after the key was already added to the DB.

coveralls · 2024-11-21T15:47:47Z

Pull Request Test Coverage Report for Build 12056907010

Details

98 of 205 (47.8%) changed or added relevant lines in 6 files are covered.
9 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.04%) to 40.872%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapchannel/aux_leaf_creator.go	0	2	0.0%
tapchannel/aux_leaf_signer.go	68	71	95.77%
tapchannel/commitment.go	17	34	50.0%
tapchannel/aux_sweeper.go	0	85	0.0%

Files with Coverage Reduction	New Missed Lines	%
tapgarden/planter.go	2	74.12%
tapchannel/aux_sweeper.go	7	0.0%

Totals
Change from base Build 12052477844:	0.04%
Covered Lines:	25782
Relevant Lines:	63080

💛 - Coveralls

tapchannel/commitment.go

tapchannel/aux_leaf_signer_test.go

gijswijs

Very nice. I did a thorough review and left a bunch of comments throughout, but no blockers AFAICT.

tapchannel/aux_leaf_signer.go

tapchannel/aux_leaf_signer_test.go

tapchannel/aux_sweeper.go

tapchannel/aux_leaf_signer.go

guggero · 2024-11-22T14:57:46Z

I rebased this on top of #1154 to verify everything works.
Only the last two commits are for this PR!

@Roasbeef I have run a full itest suite on top of this PR to validate it works.
Since we're tweaking the HTLC revocation key (which is the internal key of the script key), we currently don't need to tweak anything at the signing level, since revocation sweeps aren't currently implemented. Left a big TODO in there so we don't forget when we add that.
So I guess this approach is even more minimal as initially thought (at least for now).

tapchannel/aux_leaf_signer.go

gijswijs · 2024-11-22T16:44:00Z

We are nearly there. Just two minor nits and a comment to check whether I understand the code.

jharveyb · 2024-11-22T17:44:10Z

I rebased this on top of #1154 to verify everything works. Only the last two commits are for this PR!

Should the base branch for PR be changed then, or does this supercede #1154 directly?

guggero · 2024-11-22T19:23:52Z

Should the base branch for PR be changed then, or does this supercede #1154 directly?

I could change the base branch, yes. But because I also rebased it, it will still show all commits. So it doesn't really matter that much, this will need to land after #1154 in any case.

EDIT: I did change the base branch, since that will avoid accidental merge.

gijswijs

LGTM! 🎉

guggero · 2024-11-25T16:01:30Z

Thanks for the review!

Unfortunately while testing this in an itest, I ran into a bit of a road block.
Turns out the tweaking works well. BUT, the thing we tweak the keys with (HtlcIndex) is not stable across the two peers of the channel.
Meaning that for peer A, its HtlcIndex of the outgoing HTLCs is not necessarily exactly the same as for peer B's incoming HTLCs. So after a certain number of HTLCs (the number seems to be around 10 in the itest, but could be pure coincidence), the ordering is out of sync and because of that the two peers have different commitment transactions, which leads to a force close.

I think we might actually need to introduce a stable sort index as a custom records to the HTLC, so both parties can use the correct value when sorting.

Roasbeef · 2024-11-26T03:44:02Z

Does the in-place allocation sort need to factor in the htlc index now?

Before this change, they were identical for a given (amt, payhash, cltv) HTLC tuple. Now as the internal key is tweaked by the index, they'll no longer be identical. That can cause us to mix up two allocations that have the same HTLC tuple, but differ based on their index. By default sort.Sort isn't stable, so the slice ordering can be changed if all the keys match.

guggero · 2024-11-27T15:14:00Z

I think I found and squashed all MPP related bugs now.
Re-requesting review and at the same time will polish and then push the corresponding litd itest changes.

But I expect the CI to be fully green on this now, including the current litd itests.

jharveyb

LGTM! Massive lift 🚀

I think the commit message for the first commit is now wrong btw? Wrt. mentioning that the tweaking for revocation is not implemented.

jharveyb · 2024-11-27T17:50:04Z

tapchannel/allocation_sort.go

@@ -43,7 +43,7 @@ func (s sortableAllocationSlice) Swap(i, j int) {
 }

 // Less is a modified BIP69 output comparison, that sorts based on value, then
-// pkScript, then CLTV value.
+// pkScript, then CLTV value and finally HtlcIndex.
 //
 // NOTE: Part of the sort.Interface interface.
 func (s sortableAllocationSlice) Less(i, j int) bool {


non-blocking nit: AFAICT the stdlib recommendation now is to use slices.SortFunc instead of sort.Sort.

Makes sense. Also found a way to simplify the code quite a bit.

jharveyb · 2024-11-27T17:51:24Z

tapchannel/aux_leaf_signer.go

+	// Convert the tweaked key back to an affine point and create a new
+	// taproot key from it.
+	tweakedKey.ToAffine()
+	return btcec.NewPublicKey(&tweakedKey.X, &tweakedKey.Y)


From the godoc of NewPublicKey; does this output key need to be checked with IsOnCurve?

My guess is no since we know the input was on the curve.

Yeah, we assume the input key is on the curve.

tapchannel/aux_leaf_signer_test.go

ffranr · 2024-11-27T18:26:48Z

tapchannel/aux_leaf_signer.go

+	// We need to avoid the tweak being zero, so we always add 1 to the
+	// index. Otherwise, we'd multiply G by zero.
+	index++
+
+	// If we wrapped around from math.MaxUint64 to 0, we need to make sure
+	// the tweak is 1 to not cause a multiplication by zero. This should
+	// never happen, as it would mean we have more than math.MaxUint64
+	// updates in a channel, which exceeds the protocol's maximum.
+	if index == 0 {
+		return new(secp256k1.ModNScalar).SetInt(1)
+	}
+


To avoid overflowing index we could do something like this instead:

if index == math.MaxUint64 { return new(secp256k1.ModNScalar).SetInt(1) } index++ ...

Also, on the function name, perhaps ScriptKeyTweakFromHtlcIndex.

ffranr · 2024-11-27T18:27:54Z

tapchannel/aux_leaf_signer.go

+	var (
+		pubKeyJacobian, tweakTimesG, tweakedKey btcec.JacobianPoint
+	)
+	pubKey.AsJacobian(&pubKeyJacobian)


nit: ensure that pubKey is not nil.

ffranr · 2024-11-27T18:28:38Z

tapchannel/aux_leaf_signer.go

+func TweakPubKeyWithIndex(pubKey *btcec.PublicKey,
+	index input.HtlcIndex) *btcec.PublicKey {


I feel as if this function and the one above should be generalised into "tweak public key by integer" function(s). Which can live in btcec eventually. None of this logic is unique to taproot assets or HTLCs.

Sure, makes sense. Keeping things as they are for now, but definitely something we can refactor later.

guggero · 2024-11-27T18:52:07Z

I think the commit message for the first commit is now wrong btw? Wrt. mentioning that the tweaking for revocation is not implemented.

No, it's correct. Sweeping an HTLC of a revoked commitment (old state) isn't implemented yet. In that case we would need to tweak the signing key at sign time. In any non-revocation (e.g. normal, latest-state force close), we only need to tweak the internal key when we create the output and then again when we create the control block. But no signing private key needs to be tweaked as of now.

gijswijs

No further remarks, only questions to see if I understand it. Also the last two commits fix problems that are unrelated to HTLC script key tweak, am I right?
The change output index calculation and the upsert of tweaks are unrelated to the initial problem AFAICT.

tl;dr: LGTM! 🎉

gijswijs · 2024-11-27T16:51:50Z

tapchannel/commitment.go

@@ -766,13 +780,13 @@ func CreateAllocations(chanState lnwallet.AuxChanState, ourBalance,
 			NonAssetLeaves: sibling,
 			ScriptKey: asset.ScriptKey{
 				PubKey: asset.NewScriptKey(
-					htlcTree.TaprootKey,
+					tweakedTree.TaprootKey,


Just to double check: The SortTaprootKeyBytes should not be converted to tweakedTree.TaprootKey because that's the output key of the on-chain P2TR output that would be created if there was no asset commitment. So that should remain untweaked?

Yes, correct. We need to make sure the sort order is stable before applying any tweaks (TAP leaf or HtlcIndex). Added a comment to make this clear.

gijswijs · 2024-11-27T16:54:00Z

tapchannel/commitment.go

@@ -1289,12 +1317,12 @@ func createSecondLevelHtlcAllocations(chanType channeldb.ChannelType,
 		),
 		InternalKey:    htlcTree.InternalKey,
 		NonAssetLeaves: sibling,
-		ScriptKey:      asset.NewScriptKey(htlcTree.TaprootKey),
+		ScriptKey:      asset.NewScriptKey(tweakedTree.TaprootKey),
 		SortTaprootKeyBytes: schnorr.SerializePubKey(


Same question here.

Added a comment here too.

gijswijs · 2024-11-27T17:10:46Z

tapchannel/allocation_sort.go

+		return true
+	}
+
+	return allocI.HtlcIndex < allocJ.HtlcIndex


This commit tweaks the internal key of the asset-level script key with the HTLC index to enforce uniqueness of script keys across multiple HTLCs with the same payment hash and timeout (MPP shards of the same payment). The internal key we tweak is the revocation key. So we must also apply that same tweak when doing a revocation/breach sweep transaction. But since that functionality is not yet implemented, we'll just leave a TODO in a follow-up commit for that. Therefore, no tweak will currently need to be applied at _sign_ time.

This fixes a bug where the order of the HTLCs would not be deterministic between two peers if they were identical due to them being shards of the same MPP payment. By adding the HTLC index as a further sort argument, the order is again stable and deterministic.

This commit adds a couple of trace outputs that were helpful in finding two bugs fixed in this series of commits. We also increase the number of levels the spew logger is allowed to print, so we can see a bit more. We need this limit to avoid endless loops in self-referencing structs such as proofs.

This fixes a bug where we previously assumed there would only ever be one asset input in a sweep. But when we sweep multiple HTLCs at the same time, the change output index isn't static and needs to be calculated based on the asset commitment output indexes.

guggero · 2024-11-27T18:57:43Z

Also the last two commits fix problems that are unrelated to HTLC script key tweak, am I right?

Correct. But those changes were required to get the integration tests passing, so they were required follow-up fixes.
Going to update the PR body with what else is fixed for future reference 👍

Roasbeef

LGTM 🦋

tapchannel/allocation_sort.go

tapchannel/aux_sweeper.go

Roasbeef · 2024-11-27T19:08:58Z

tapdb/sqlc/queries/assets.sql

-                       END
+                       END,
+      -- If the tweak was previously unknown, we'll update to the new value.
+      tweak = CASE


Non-blocking, but can extend the recently added TestScriptKeyKnownUpsert test to cover this

Yeah, makes sense. Added.

This fixes a bug where if we ever inserted a script key before knowing its tweak, we could never update the tweak later on when we learn it. This meant that such keys would be seen as BIP-086 keys, even though we later learn they aren't.

guggero requested review from Roasbeef and gijswijs November 21, 2024 15:36

Roasbeef reviewed Nov 22, 2024

View reviewed changes

tapchannel/commitment.go Show resolved Hide resolved

tapchannel/aux_leaf_signer_test.go Outdated Show resolved Hide resolved

gijswijs reviewed Nov 22, 2024

View reviewed changes

guggero force-pushed the htlc-script-key-tweak branch 2 times, most recently from 57ee008 to 1163f5b Compare November 22, 2024 14:53

guggero requested review from gijswijs and Roasbeef November 22, 2024 14:57

gijswijs reviewed Nov 22, 2024

View reviewed changes

tapchannel/aux_leaf_signer.go Show resolved Hide resolved

guggero changed the base branch from main to htlc-resolution November 22, 2024 19:28

dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 23, 2024

dstadulis assigned guggero Nov 23, 2024

dstadulis added tap-channels htlc labels Nov 23, 2024

guggero force-pushed the htlc-script-key-tweak branch from 1163f5b to fda054d Compare November 25, 2024 11:39

guggero requested a review from gijswijs November 25, 2024 11:39

gijswijs approved these changes Nov 25, 2024

View reviewed changes

guggero force-pushed the htlc-script-key-tweak branch from fda054d to c96f369 Compare November 27, 2024 15:06

guggero changed the base branch from htlc-resolution to main November 27, 2024 15:12

guggero force-pushed the htlc-script-key-tweak branch from c96f369 to 1e85b09 Compare November 27, 2024 15:13

guggero requested a review from gijswijs November 27, 2024 15:14

guggero mentioned this pull request Nov 27, 2024

version: bump to version v0.5.0-alpha.rc1 #1221

Merged

jharveyb self-requested a review November 27, 2024 17:48

jharveyb approved these changes Nov 27, 2024

View reviewed changes

ffranr reviewed Nov 27, 2024

View reviewed changes

gijswijs approved these changes Nov 27, 2024

View reviewed changes

guggero added 6 commits November 27, 2024 19:55

tapchannel: apply tweaks when sweeping HTLCs

52ee518

tapchannel: tweak HTLC second level transaction internal key

9b9e4aa

guggero force-pushed the htlc-script-key-tweak branch from 1e85b09 to 63fb86c Compare November 27, 2024 18:58

Roasbeef approved these changes Nov 27, 2024

View reviewed changes

tapchannel/allocation_sort.go Show resolved Hide resolved

tapchannel/aux_sweeper.go Show resolved Hide resolved

tapchannel/aux_sweeper.go Show resolved Hide resolved

Roasbeef reviewed Nov 27, 2024

View reviewed changes

tapdb: allow tweak to be upserted

38b28f2

This fixes a bug where if we ever inserted a script key before knowing its tweak, we could never update the tweak later on when we learn it. This meant that such keys would be seen as BIP-086 keys, even though we later learn they aren't.

guggero force-pushed the htlc-script-key-tweak branch from 63fb86c to 38b28f2 Compare November 27, 2024 19:17

guggero merged commit b192b5b into main Nov 27, 2024
18 checks passed

guggero deleted the htlc-script-key-tweak branch November 27, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[custom channels]: generate unique script keys for HTLCs #1209

[custom channels]: generate unique script keys for HTLCs #1209

guggero commented Nov 21, 2024 •

edited

Loading

coveralls commented Nov 21, 2024 •

edited

Loading

gijswijs left a comment

guggero commented Nov 22, 2024

gijswijs commented Nov 22, 2024

jharveyb commented Nov 22, 2024

guggero commented Nov 22, 2024 •

edited

Loading

gijswijs left a comment

guggero commented Nov 25, 2024

Roasbeef commented Nov 26, 2024

guggero commented Nov 27, 2024

jharveyb left a comment

jharveyb Nov 27, 2024

guggero Nov 27, 2024

jharveyb Nov 27, 2024

guggero Nov 27, 2024

ffranr Nov 27, 2024

ffranr Nov 27, 2024

ffranr Nov 27, 2024

ffranr Nov 27, 2024 •

edited

Loading

guggero Nov 27, 2024

guggero commented Nov 27, 2024

gijswijs left a comment

gijswijs Nov 27, 2024

guggero Nov 27, 2024

gijswijs Nov 27, 2024

guggero Nov 27, 2024

gijswijs Nov 27, 2024

guggero commented Nov 27, 2024

Roasbeef left a comment

Roasbeef Nov 27, 2024

guggero Nov 27, 2024

		func TweakPubKeyWithIndex(pubKey *btcec.PublicKey,
		index input.HtlcIndex) *btcec.PublicKey {

[custom channels]: generate unique script keys for HTLCs #1209

[custom channels]: generate unique script keys for HTLCs #1209

Conversation

guggero commented Nov 21, 2024 • edited Loading

coveralls commented Nov 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12056907010

Details

💛 - Coveralls

gijswijs left a comment

Choose a reason for hiding this comment

guggero commented Nov 22, 2024

gijswijs commented Nov 22, 2024

jharveyb commented Nov 22, 2024

guggero commented Nov 22, 2024 • edited Loading

gijswijs left a comment

Choose a reason for hiding this comment

guggero commented Nov 25, 2024

Roasbeef commented Nov 26, 2024

guggero commented Nov 27, 2024

jharveyb 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

ffranr Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Nov 27, 2024

gijswijs 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

guggero commented Nov 27, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero commented Nov 21, 2024 •

edited

Loading

coveralls commented Nov 21, 2024 •

edited

Loading

guggero commented Nov 22, 2024 •

edited

Loading

ffranr Nov 27, 2024 •

edited

Loading