TLV encoding: add forward-compatibility #1037

guggero · 2024-07-19T12:55:19Z

Replaces #557.

Fixes #997.

With this PR we add forward-compatibility that will allow us in the future to add new optional (odd-numbered) TLV types to any of our TLV structs without breaking old clients.
This will allow us to implement #956 in the future, once everyone has upgraded to v0.4.1 (or whichever version this lands in).

I opted to not go with any version increases, since this doesn't really change anything in the current behavior yet...

This change is

guggero · 2024-07-26T14:08:07Z

Rebased.

Roasbeef

Solid approach!

No major comments, other than one re how we can de-risk enforcing strict decode in a short time horizon.

Also need to circle back re the interaction with the test vectors and the new type constants, as well as some of the forward compat unit tests.

asset/records.go

Roasbeef · 2024-07-29T23:12:32Z

asset/asset.go

@@ -2000,7 +2000,8 @@ func (a *Asset) Decode(r io.Reader) error {
 	if err != nil {
 		return err
 	}
-	return stream.Decode(r)
+
+	return TlvStrictDecode(stream, r, KnownAssetLeafTypes)


I think to be slightly more conservative, we should put this behind a flag that's disabled for now. This protects us in the off chain that someone is using a modified version of the daemon with their own custom flag, or is using a fresh implementation.

We could check for this in the wild by running the new decode method on the existing assets in proofs uploaded to our Universe.

But wouldn't that break the intended forward compatibility?
Isn't the intention that when we ship v0.4.2 with this code enabled (e.g. no flag, allow new unknown odd types) that we could then with v0.5.0 add new fields with odd types, being backward compatible to v0.4.2?
With the intention that users can remain on v0.4.2 without their nodes complaining when syncing new proofs created with v0.5.0. If they first have to turn on a flag to allow that, doesn't that kind of defeat the purpose?

Related, a useful itest could be that, if a proof is created and then extended with an unknown odd field, checking that a client could successfully store that proof (via local Universe, DB, or flat file), and then return the identical proof to another client.

Roasbeef · 2024-07-29T23:17:40Z

asset/asset.go

+		return err
+	}
+
+	a.unknownOddTypes = unknownOddTypes


Dig the implementation!

One gap (still getting through the PR) that may pop up is when we encode a protocol object within the database as a fully featured row vs encoding an encoded blob.

Isn't that what we do for most fields?

We wouldn't actually store the information in the unknown types. Just keep it in memory so we can validate correctly (e.g. arrive at the correct leaf hash for assets or the correct meta hash for meta reveals).
Only when you upgrade to the version that implements the new value, would we store it, with the corresponding database migration. But then it would be a known type and the value wouldn't end up in unknownOddTypes anymore.

asset/records.go

asset/asset.go

jharveyb

Looks good overall! I think it could be useful to have some of this exercised in itest, especially for the composite / nested types like proofs.

asset/records.go

address/address_test.go

asset/records_test.go

proof/records.go

lightninglabs-deploy · 2024-08-08T23:59:56Z

@guggero, remember to re-request review from reviewers when ready

guggero · 2024-08-09T07:08:27Z

Will get back to this PR next week.

guggero · 2024-08-14T15:16:47Z

Also need to circle back re the interaction with the test vectors and the new type constants, as well as some of the forward compat unit tests.

Will look into that tomorrow, but I think for now this is ready for a re-review.

coveralls · 2024-08-14T15:24:35Z

Pull Request Test Coverage Report for Build 10421878251

Details

284 of 294 (96.6%) changed or added relevant lines in 14 files are covered.
15 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.3%) to 40.402%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
asset/asset.go	10	14	71.43%
asset/records.go	54	60	90.0%

Files with Coverage Reduction	New Missed Lines	%
tapdb/addrs.go	2	79.04%
commitment/tap.go	2	84.43%
asset/asset.go	3	81.35%
universe/interface.go	8	50.22%

Totals
Change from base Build 10408357404:	0.3%
Covered Lines:	23914
Relevant Lines:	59190

💛 - Coveralls

jharveyb

Looks good! Left some comment in existing threads on itest behavior we may want to get better assurances on forwards-compat.

Some of the unit tests already extend data from the BIP test vectors, so I'm not sure if we want/need to add data with unknown types as test vectors themselves.

internal/test/generic.go

asset/records_test.go

address/address_test.go

In this commit, as a follow up to the recent even/odd type split, we add some helper functions to assert that when we decode an asset TLV stream, we bail out if we find any unknown even types.

…ntered

This commit makes it possible for the protocol to add new odd (optional) types in the future without old clients breaking. By parsing and keeping any left-over unknown odd types, then re-encoding them when calculating the commitment leaf, old software can still correctly use and validate an asset, even if it doesn't fully understand all of its (optional) new fields.

Fixes a temporary TODO and makes the check nil-safe.

guggero · 2024-08-16T15:36:35Z

I've added test vectors and an integration test.

jharveyb

Awesome! With the itest, I'm convinced that this should work out.

Since that includes proof decode with an unknown type, I don't think there's particular value on running this over the proofs of our testnet and mainnet universes. IIUC any unknown types should have been dropped when those servers were storing the proofs anyway.

jharveyb · 2024-08-16T17:38:52Z

itest/addrs_test.go

+		567: []byte("it's committed"),
+	}
+	cp.TaprootAssetProof.UnknownOddTypes = tlv.TypeMap{
+		789: []byte("there's assets in here..."),


jharveyb · 2024-08-16T17:39:08Z

itest/addrs_test.go

+	// there.
+	require.Equal(t.t, modifiedBlob, transferProof2.RawProofFile)
+	require.True(t.t, bytes.Contains(modifiedBlob, []byte("it's included")))
+	require.True(


Roasbeef

Reviewed 18 of 18 files at r1, 28 of 28 files at r2, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @guggero and @jharveyb)

commitment/commitment_test.go line 1707 at r2 (raw file):

	var knownProofBytes []byte
	test.RunUnknownOddTypeTest(

Nice generalization 👍

guggero mentioned this pull request Jul 19, 2024

asset: update parsing to return error if unknown odd type encountered #557

Closed

guggero requested review from Roasbeef and jharveyb July 19, 2024 13:26

dstadulis added tlv assets labels Jul 19, 2024

dstadulis added this to the v0.4.1 milestone Jul 19, 2024

dstadulis assigned guggero Jul 19, 2024

dstadulis added the proofs label Jul 19, 2024

guggero force-pushed the tlv-carve-out branch from 0d61fa9 to 21ee689 Compare July 26, 2024 14:08

Roasbeef reviewed Jul 29, 2024

View reviewed changes

jharveyb reviewed Aug 1, 2024

View reviewed changes

guggero force-pushed the tlv-carve-out branch from 21ee689 to 7aaf947 Compare August 14, 2024 15:15

guggero requested review from Roasbeef and jharveyb August 14, 2024 15:16

jharveyb reviewed Aug 14, 2024

View reviewed changes

internal/test/generic.go Outdated Show resolved Hide resolved

internal/test/generic.go Outdated Show resolved Hide resolved

asset/records_test.go Outdated Show resolved Hide resolved

address/address_test.go Outdated Show resolved Hide resolved

Roasbeef and others added 5 commits August 16, 2024 13:42

asset: update parsing to return error if unknown even type encountered

2a1d4c0

In this commit, as a follow up to the recent even/odd type split, we add some helper functions to assert that when we decode an asset TLV stream, we bail out if we find any unknown even types.

asset: remove unused TLV type

d34d59c

address: update parsing to return error if unknown even type encountered

84fa388

commitment: update parsing to return error if unknown even type encou…

53904cc

…ntered

proof: update parsing to return error if unknown even type encountered

fa6516e

guggero force-pushed the tlv-carve-out branch from 7aaf947 to 28bbf71 Compare August 16, 2024 14:35

guggero added 4 commits August 16, 2024 16:48

address: make forward-compatible by parsing unknown odd types

7b533ae

commitment: make forward-compatible by parsing unknown odd types

4e78fb2

proof: make forward-compatible by parsing unknown odd types

97697ed

guggero added 2 commits August 16, 2024 16:50

asset: remove TODO in script key check

3c424d4

Fixes a temporary TODO and makes the check nil-safe.

itest: add test for unknown odd TLV types

d06206e

guggero force-pushed the tlv-carve-out branch from 28bbf71 to d06206e Compare August 16, 2024 14:51

guggero requested a review from jharveyb August 16, 2024 15:36

jharveyb approved these changes Aug 16, 2024

View reviewed changes

Roasbeef requested a review from jharveyb August 16, 2024 23:23

Roasbeef approved these changes Aug 16, 2024

View reviewed changes

Roasbeef merged commit 420f246 into main Aug 16, 2024
17 checks passed

guggero deleted the tlv-carve-out branch August 19, 2024 08:16

Roasbeef mentioned this pull request Aug 19, 2024

[enhancement]: return error if unknown odd type encountered #900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLV encoding: add forward-compatibility #1037

TLV encoding: add forward-compatibility #1037

guggero commented Jul 19, 2024 •

edited by Roasbeef

Loading

guggero commented Jul 26, 2024

Roasbeef left a comment

Roasbeef Jul 29, 2024

guggero Aug 14, 2024

jharveyb Aug 14, 2024

Roasbeef Jul 29, 2024

jharveyb Aug 1, 2024

guggero Aug 14, 2024

jharveyb left a comment

lightninglabs-deploy commented Aug 8, 2024

guggero commented Aug 9, 2024

guggero commented Aug 14, 2024

coveralls commented Aug 14, 2024 •

edited

Loading

jharveyb left a comment

guggero commented Aug 16, 2024

jharveyb left a comment

jharveyb Aug 16, 2024

jharveyb Aug 16, 2024

Roasbeef left a comment

TLV encoding: add forward-compatibility #1037

TLV encoding: add forward-compatibility #1037

Conversation

guggero commented Jul 19, 2024 • edited by Roasbeef Loading

guggero commented Jul 26, 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

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

jharveyb left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Aug 8, 2024

guggero commented Aug 9, 2024

guggero commented Aug 14, 2024

coveralls commented Aug 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10421878251

Details

💛 - Coveralls

jharveyb left a comment

Choose a reason for hiding this comment

guggero commented Aug 16, 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

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Jul 19, 2024 •

edited by Roasbeef

Loading

coveralls commented Aug 14, 2024 •

edited

Loading