Fix ExtraData field and use BigSize encodine

[yyforyongyu]

This PR changes the encoding from btcutil.Amount to BigSize for efficiency. It also fixes the wrong encoding used in the dyn msg set. For instance, the extra data field is meant to hold extra data, but it actually holds the full tlv data,

before this change, notice the ExtraData holds LocalNonce,

dyn_ack_test.go:68: Encoded msg is (*lnwire.DynAck)(0x140002beea0)({
         ChanID: (lnwire.ChannelID) (len=32 cap=32) 777b0cfd56e5b84a4bbe87f209b9eeb9218208e9a403c75e03d2981ecd91cf79,
         Sig: (lnwire.Sig) {
          bytes: ([64]uint8) (len=64 cap=64) {
           00000000  b0 54 0e e8 6d 02 d1 e6  8f d9 95 a9 9d 49 60 5d  |.T..m........I`]|
           00000010  eb 8e aa 16 4c 7d 26 47  60 10 b2 06 bd 97 8a 3e  |....L}&G`......>|
           00000020  b2 73 05 c0 be 0c 14 ed  fd 64 b4 79 a0 7e 22 15  |.s.......d.y.~".|
           00000030  96 f1 12 86 4f 2f f9 c1  ae d6 68 0d e6 29 68 26  |....O/....h..)h&|
          },
          sigType: (lnwire.sigType) 0
         },
         LocalNonce: (fn.Option[github.com/lightningnetwork/lnd/lnwire.Musig2Nonce]) {
          isSome: (bool) true,
          some: (lnwire.Musig2Nonce) (len=66 cap=66) {
           00000000  2c d4 53 7d aa 7b 7e ae  18 32 a6 c4 29 e9 e0 91  |,.S}.{~..2..)...|
           00000010  32 7a af d1 1c 2b 04 a0  4d b5 6a 6f 8b 6c dc d1  |2z...+..M.jo.l..|
           00000020  80 2d ff 72 d8 3c fc 01  6e 7c 1a c8 5e 3a 16 98  |.-.r.<..n|..^:..|
           00000030  bc 9b 6e 22 58 96 96 ad  88 bf ff 59 90 bd 36 0b  |..n"X......Y..6.|
           00000040  0b 4d                                             |.M|
          }
         },
         ExtraData: (lnwire.ExtraOpaqueData) (len=72 cap=512) {
          00000000  00 42 2c d4 53 7d aa 7b  7e ae 18 32 a6 c4 29 e9  |.B,.S}.{~..2..).|
          00000010  e0 91 32 7a af d1 1c 2b  04 a0 4d b5 6a 6f 8b 6c  |..2z...+..M.jo.l|
          00000020  dc d1 80 2d ff 72 d8 3c  fc 01 6e 7c 1a c8 5e 3a  |...-.r.<..n|..^:|
          00000030  16 98 bc 9b 6e 22 58 96  96 ad 88 bf ff 59 90 bd  |....n"X......Y..|
          00000040  36 0b 0b 4d 6f 02 79 79                           |6..Mo.yy|
         }
        })

after,

dyn_ack_test.go:68: Encoded msg is (*lnwire.DynAck)(0x1400033eea0)({
         ChanID: (lnwire.ChannelID) (len=32 cap=32) 24481a8ac033dc870a059ca802707b72222ecd6e4641578aea581f4b972dd70b,
         Sig: (lnwire.Sig) {
          bytes: ([64]uint8) (len=64 cap=64) {
           00000000  7b ab c5 37 d9 bd c1 cd  0b d9 88 ff 11 9b e1 21  |{..7...........!|
           00000010  c1 af ce b0 2c 34 e3 32  27 25 bd 0c a0 19 80 65  |....,4.2'%.....e|
           00000020  da 35 c3 37 d0 41 ff ce  13 a4 fd f2 4c d2 4c d9  |.5.7.A......L.L.|
           00000030  8b 37 57 80 38 6d e0 6a  56 9b bb c3 c4 1f e1 b7  |.7W.8m.jV.......|
          },
          sigType: (lnwire.sigType) 0
         },
         LocalNonce: (fn.Option[github.com/lightningnetwork/lnd/lnwire.Musig2Nonce]) {
          isSome: (bool) true,
          some: (lnwire.Musig2Nonce) (len=66 cap=66) {
           00000000  2c d4 53 7d aa 7b 7e ae  18 32 a6 c4 29 e9 e0 91  |,.S}.{~..2..)...|
           00000010  32 7a af d1 1c 2b 04 a0  4d b5 6a 6f 8b 6c dc d1  |2z...+..M.jo.l..|
           00000020  80 2d ff 72 d8 3c fc 01  6e 7c 1a c8 5e 3a 16 98  |.-.r.<..n|..^:..|
           00000030  bc 9b 6e 22 58 96 96 ad  88 bf ff 59 90 bd 36 0b  |..n"X......Y..6.|
           00000040  0b 4d                                             |.M|
          }
         },
         ExtraData: (lnwire.ExtraOpaqueData) (len=4 cap=64) {
          00000000  6f 02 79 79                                       |o.yy|
         }
        })

I think we may have other places use the wrong encoding, will check.

[gemini-code-assist]

Summary of Changes

Hello @yyforyongyu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the serialization of dynamic messages by adopting more efficient BigSize encoding for certain monetary values and, critically, corrects the handling of ExtraData fields. These changes ensure that ExtraData accurately represents only unknown or unexpected TLV records, promoting better forward compatibility and adherence to protocol specifications.

Highlights

• Encoding Efficiency: Switched the encoding of btcutil.Amount fields (e.g., DustLimit, ChannelReserve) within dynamic messages to use BigSize encoding, improving serialization efficiency and reducing byte size for smaller values.
• Correct ExtraData Handling: Refactored the encoding and decoding logic for ExtraData fields in DynAck, DynCommit, and DynPropose messages. The ExtraData field now correctly stores only unrecognized TLV records, adhering to the TLV extensibility principle, rather than inadvertently holding all TLV data.
• TLV Type Update: The TLV type for DALocalMusig2Pubnonce in DynAck messages has been updated from 0 to 14. This change ensures proper type assignment and avoids potential conflicts with other TLV types.
• New TLV Parsing Helper: Introduced a new helper function ParseAndExtractExtraData in lnwire/extra_bytes.go to streamline the process of extracting known TLV records from a byte stream and isolating any truly 'extra' or unrecognized data.
• Comprehensive Test Coverage: Added new dedicated tests for DynAck, DynCommit, and DynPropose messages to validate the updated encoding/decoding logic, including the BigSize and ExtraData changes. Existing random message generation tests were also updated to reflect these changes and ensure correct ExtraData population.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the encoding of several dynamic commitment messages (DynAck, DynCommit, DynPropose) to correctly handle ExtraData and use more efficient BigSize encoding for amount fields. The changes are a good improvement, fixing a bug where all TLV data was being packed into ExtraData. The PR also adds comprehensive tests for the affected messages. I've found a critical issue in the decoding logic for DynCommit and DynPropose that would cause a panic or compilation error, and I've provided suggestions to fix it.

[yyforyongyu]

/gemini review

[gemini-code-assist]

Code Review

This PR introduces changes to the encoding from btcutil.Amount to BigSize for efficiency and fixes the wrong encoding used in the dyn msg set. The ExtraData field is now correctly handled.

[yyforyongyu]

/gemini review

[gemini-code-assist]

Code Review

This pull request fixes a critical bug in the dynamic commitment message set and enhances encoding efficiency by using BigSize encoding for amount fields. The core issue of ExtraData incorrectly containing known TLV fields is resolved across DynPropose, DynAck, and DynCommit messages. The introduction of the ParseAndExtractExtraData helper function greatly simplifies and clarifies the decoding logic, and the accompanying test changes are excellent.

[yyforyongyu]

/gemini review

[gemini-code-assist]

Code Review

This pull request fixes an issue with ExtraData encoding and improves efficiency by using BigSize encoding. The new ParseAndExtractExtraData function is a great addition for correctly decoding TLV streams. However, the new Encode methods for DynAck, DynCommit, and DynPropose incorrectly serialize known and unknown TLV records into separate streams, which would break wire compatibility. I've provided suggestions to merge all records into a single stream before encoding.

[ellemouton]

See comment in review - i think the extradata encoding/decoding was correct before.

If we have known fields (like LocalNonce) that we extracted from ExtraTLV bytes - then what we should instead do is just to not explicitly write the LocalNonce bytes & instead just write the ExtraData bytes to disk & then on reading from disk, then we do an in-memory parse and populate LocalNonce from that.

[ellemouton]

oops, forgot to click "review changes".

See comment in thread - just want to make sure we are on same page.

I think the confusion is coming in because legacy protocol messages had fixed fields followed by TLV stream. But now, we are going for a full TLV stream (minus the first channel ID field of course).

And so we need to ensure that the entire TLV stream is ordered (which includes our known fields and our unknown records)

[yyforyongyu]

/gemini review

[gemini-code-assist]

Code Review

This PR fixes an encoding issue with ExtraData and improves efficiency by using BigSize encoding. The changes are well-implemented, and the addition of tests is valuable.

[yyforyongyu]

/gemini review

[gemini-code-assist]

Code Review

This pull request improves the correctness and clarity of TLV handling for dynamic commitment messages by fixing the ExtraData field and using BigSize encoding. The new ParseAndExtractExtraData helper function and the encode/decode tests are valuable additions. The feedback focuses on using more idiomatic Go buffer initialization in the new test files.

[yyforyongyu]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly fixes a significant issue with TLV serialization where known fields were being incorrectly packed into the ExtraData field. The introduction of the ParseAndExtractExtraData helper function centralizes the logic for separating known and unknown TLV records, which greatly improves correctness and maintainability across the dynamic commitment message set. The switch to BigSize encoding for amount fields is also a good efficiency improvement. The new and updated tests are thorough and cover the changes well.

I have one suggestion regarding code formatting to better align with the project's style guide.

PS: There's a small typo in the PR title ("encodine" -> "encoding").

[ellemouton]

lgtm after roasbeef nits addressed!

Love the hard coded byte tests. I guess for those we need to just watch out when we introduce known types for the records we currently define as unknown there.

Thanks for the updates to the commit order - things are much more clear to me now 🙏

[ellemouton]

lovely lovely

[Roasbeef]

LGTM 🦀