BOLT-03: make sats portion of HTLC CLTV tie-breaker more explicit

[Roasbeef]

This commit is intended to fix an ambiguity in the spec that led to a
divergence in the sorting tie breaker between implementations, that can
lead to force closed transaction in practice. BIP 69 operates on the
output level, therefore it examines the satoshi amount of a output
when sorting. The spec however, references BIP 69, but states that an
"identical" HTLC output may have the same amount_msat value.

In the wild this led to some implementations checking the sat value of
an HTLC while others checked the msat value. In the scenario where an
pair HTLC has the same sat value, but differing msat values, then
one will fall through to the tie-breaker, while the other while sort
them according to their msat values.

In this commit, we attempt to make this requirement more explicit by
removing the reference to msat, and more explicitly describing when an
HTLC pair is to be considered identical.

[pm47]

Nice find! One less cause of force closes.

The below patch updates the test vectors and make the issue apparent in eclair:

diff --git a/03-transactions.md b/03-transactions.md
index 29a9885..c3c7037 100644
--- a/03-transactions.md
+++ b/03-transactions.md
@@ -1042,11 +1042,11 @@ and HTLCs 5 and 6 are only used in the "same amount and preimage" test.
     htlc 4 payment_preimage: 0404040404040404040404040404040404040404040404040404040404040404
     htlc 5 direction: local->remote
     htlc 5 amount_msat: 5000000
-    htlc 5 expiry: 505
+    htlc 5 expiry: 506
     htlc 5 payment_preimage: 0505050505050505050505050505050505050505050505050505050505050505
     htlc 6 direction: local->remote
-    htlc 6 amount_msat: 5000000
-    htlc 6 expiry: 506
+    htlc 6 amount_msat: 5000001
+    htlc 6 expiry: 505
     htlc 6 payment_preimage: 0505050505050505050505050505050505050505050505050505050505050505

 <!-- The test vector values are derived, as per Key Derivation, though it's not
@@ -1439,7 +1439,7 @@ And, here are the test vectors themselves:
     # HTLC 1 received amount 2000 wscript 76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c8201208763a9144b6b2e5444c2639cc0fb7bcea5afba3f3cdce23988527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae677502f501b175ac6868
     # HTLC 5 offered amount 5000 wscript 76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c820120876475527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae67a9142002cc93ebefbb1b73f0af055dcc27a0b504ad7688ac6868
     # HTLC 6 offered amount 5000 wscript 76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c820120876475527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae67a9142002cc93ebefbb1b73f0af055dcc27a0b504ad7688ac6868
-    # HTLC 5 and 6 have CLTV 505 and 506, respectively, and preimage 0505050505050505050505050505050505050505050505050505050505050505
+    # HTLC 5 and 6 have CLTV 506 and 505, respectively, and preimage 0505050505050505050505050505050505050505050505050505050505050505
     remote_signature = 3044022044f807aefa41480a5d1df2fc312c486900617fd24493cf41976428cb249ec2c2022007fd1229cfb57d638b9c137b03bc7917d0e418b63e62a3642f1c13354cc71df8
     # local_signature = 304502210098674686a13c7da2d95abea08d27e9324573156d79e5eb08cd96d1e33bb0045002206391216f4fd5fb7b0fe8c43074fd19f485dd47d7b07f7325c5a6121f5b0a591b
     output commit_tx: 02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8005d007000000000000220020748eba944fedc8827f6b06bc44678f93c0f9e6078b35c6331ed31e75f8ce0c2d8813000000000000220020305c12e1a0bc21e283c131cea1c66d68857d28b7b2fce0a6fbc40c164852121b8813000000000000220020305c12e1a0bc21e283c131cea1c66d68857d28b7b2fce0a6fbc40c164852121bc0c62d0000000000160014ccf1af2f2aabee14bb40fa3851ab2301de843110a79f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e040048304502210098674686a13c7da2d95abea08d27e9324573156d79e5eb08cd96d1e33bb0045002206391216f4fd5fb7b0fe8c43074fd19f485dd47d7b07f7325c5a6121f5b0a591b01473044022044f807aefa41480a5d1df2fc312c486900617fd24493cf41976428cb249ec2c2022007fd1229cfb57d638b9c137b03bc7917d0e418b63e62a3642f1c13354cc71df801475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220
@@ -1809,7 +1809,7 @@ Here are the same test vectors, but when `option_static_remotekey` is used:
     # HTLC 1 received amount 2000 wscript 76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c8201208763a9144b6b2e5444c2639cc0fb7bcea5afba3f3cdce23988527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae677502f501b175ac6868
     # HTLC 5 offered amount 5000 wscript 76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c820120876475527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae67a9142002cc93ebefbb1b73f0af055dcc27a0b504ad7688ac6868
     # HTLC 6 offered amount 5000 wscript 76a91414011f7254d96b819c76986c277d115efce6f7b58763ac67210394854aa6eab5b2a8122cc726e9dded053a2184d88256816826d6231c068d4a5b7c820120876475527c21030d417a46946384f88d5f3337267c5e579765875dc4daca813e21734b140639e752ae67a9142002cc93ebefbb1b73f0af055dcc27a0b504ad7688ac6868
-    # HTLC 5 and 6 have CLTV 505 and 506, respectively, and preimage 0505050505050505050505050505050505050505050505050505050505050505
+    # HTLC 5 and 6 have CLTV 506 and 505, respectively, and preimage 0505050505050505050505050505050505050505050505050505050505050505
     remote_signature = 30440220048705bec5288d28b3f29344b8d124853b1af423a568664d2c6f02c8ea886525022060f998a461052a2476b912db426ea2a06700953a241135c7957f2e79bc222df9
     # local_signature = 3045022100c4f1d60b6fca9febc8b39de1a31e84c5f7c4b41c97239ef05f4350aa484c6b5e02200c5134ac8b20eb7a29d0dd4a501f6aa8fefb8489171f4cb408bd2a32324ab03f
     output commit_tx: 02000000000101bef67e4e2fb9ddeeb3461973cd4c62abb35050b1add772995b820b584a488489000000000038b02b8005d007000000000000220020748eba944fedc8827f6b06bc44678f93c0f9e6078b35c6331ed31e75f8ce0c2d8813000000000000220020305c12e1a0bc21e283c131cea1c66d68857d28b7b2fce0a6fbc40c164852121b8813000000000000220020305c12e1a0bc21e283c131cea1c66d68857d28b7b2fce0a6fbc40c164852121bc0c62d0000000000160014cc1b07838e387deacd0e5232e1e8b49f4c29e484a79f6a00000000002200204adb4e2f00643db396dd120d4e7dc17625f5f2c11a40d857accc862d6b7dd80e0400483045022100c4f1d60b6fca9febc8b39de1a31e84c5f7c4b41c97239ef05f4350aa484c6b5e02200c5134ac8b20eb7a29d0dd4a501f6aa8fefb8489171f4cb408bd2a32324ab03f014730440220048705bec5288d28b3f29344b8d124853b1af423a568664d2c6f02c8ea886525022060f998a461052a2476b912db426ea2a06700953a241135c7957f2e79bc222df901475221023da092f6980e58d2c037173180e9a465476026ee50f96695963e8efe436f54eb21030e9f7b623d2ccc7c9bd44d66d5ce21ce504c0acf6385a132cec6d3c39fa711c152ae3e195220

[Roasbeef]

Thanks for the updated test vectors @pm47! Tacked them on in a new commit.

[TheBlueMatt]

Can we drop the BIP 69 reference entirely now?

[Roasbeef]

Why? Things are still based off BIP 69 (sorting inputs and outputs), the only thing we add additionally is the CLTV tie-breaker.

[TheBlueMatt]

Because the "just link BIP 69 and that answers the question" bit has bitten us...3 times now? If we have to write out the details then we probably wouldn't have missed these to begin with.

[TheBlueMatt]

Plus it kinda reads as an endorsement of BIP 69, which we really want to avoid doing.

[rustyrussell]

Ack! I want to test the vectors here before applying though.

[TheBlueMatt]

Suggested change

	Lexicographic ordering: see [BIP69](https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki). In the case of identical HTLC outputs (amount of satoshis as well as the script are the same), the outputs are ordered in increasing `cltv_expiry` order.
	Outputs are ordered first according to their value (in whole satoshis, note that for HTLC outputs, the millisatoshi part must be ignored), followed by script_pubkey, comparing the common-length prefix lexographically as if by `memcmp`, then selecting the shorter script (if they differ). Finally, for HTLC outputs the outputs are ordered in increasing `cltv_expiry` order.

[rustyrussell]

Ack!

[rustyrussell]

Validated that c-lightning agrees with these test vectors.

Applying as per action at meeting 2021-05-24.