tlv: Added bool to primitive #8057

Chinwendu20 · 2023-10-04T15:28:10Z

Change Description

This pull request adds bool to the primitive type in tlv as per @guggero's comment in this PR:
https://github.com/lightningnetwork/lnd/pull/7787/files#r1317199032
to persist new data structure in the DB using tlv. While doing that I noticed that one of the data types in the LocalCloseInfo type is boolean so that motivated that change.

I know we could just change the bool type to an existing integer primitive type and indicate true as 1 and false as 0 but
since bool is part of the primitive data types in golang, it would make sense to include it here.
Made a comment about it in the PR that introduced tlv to lnd 3 weeks ago: #3061 (comment)
to get your thoughts and perspective, yet to receive a reply.

If the concept is acked, I will go ahead to add the release notes to make the CI happy.
I would be happy to close this PR if this makes no sense.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

guggero

Thanks for the PR!
Did a quick pass and approved the CI run.

Just a reminder to everyone that these are changes in a submodule, so they will only be accessible in the main lnd code once we merge the PR and push a new tag for the submodule.

tlv/record.go

tlv/primitive.go

Chinwendu20 · 2023-10-05T05:11:06Z

Thanks @guggero for the review, as per your comment on it being accessible only after there is a tagged release for lnd, would that mean my next PR on persisting channel force close issue on DB which depends on this change would have to wait?
Also why can't we just use the local tlv package in development, why wait for release?

guggero

Two more requests, otherwise looks good!

Also why can't we just use the local tlv package in development, why wait for release?

I didn't mean we have to wait for a release of lnd itself. I just have to push a new tag for the tlv package after this PR was merged. Like this for example: https://github.com/lightningnetwork/lnd/releases/tag/tlv%2Fv1.1.1

In the other PR you can temporarily use a replace directive in the go.mod file:

replace github.com/lightningnetwork/lnd/tlv => ./tlv

That will cause the linter to fail but should allow you access to the changed code. And when the tag is pushed after merging this PR, you simply remove the replace and instead update the entry for github.com/lightningnetwork/lnd/tlv to the new version (will likely be v1.1.2).

guggero · 2023-10-05T07:59:38Z

docs/release-notes/release-notes-0.17.0.md

@@ -134,6 +134,11 @@ None
  is updated](https://github.com/lightningnetwork/lnd/pull/7788) so that LND can
  take advantage of the latest filter fetching performance improvements.

+### Tlv


The v0.17.0-beta version has already been released, please move this to the file for 0.18.

Can't find it https://github.com/lightningnetwork/lnd/tree/master/docs/release-notes

Ah, that's only in the staging branch. We'll need to wait for this to be merged: #8059.

Should we change the base branch to 0-18-staging ?

The staging branch is about to be merged into master, so I think we can just wait a moment and then re-base on master.

Or if you want to update this PR without waiting, you can change the base to 0-18-staging-rebased instead.

Can be updated now, the 0.18 staging branch was merged into master.

thanks, done

tlv/primitive_test.go

ziggie1984 · 2023-10-05T12:58:28Z

tlv/primitive.go

@@ -362,3 +362,29 @@ func SizeBigSize(val interface{}) SizeFunc {
 		return size
 	}
 }
+
+// EBool encodes a boolean. An error is returned if val is not a boolean.
+func EBool(w io.Writer, val interface{}, buf *[8]byte) error {


Can we move this primitive type to the types above in the file where all the integers are defined (first the Encoding and followed by the Encoding functions), and also include a EBoolT for custom-like boolean types or is this not really necessary ?

Thanks for the review. I noticed that the new functions I added in this file should be above the SizeBigSize function, is that what you are referring to? as for EBoolT, never thought about that if you feel that there will be a good use case fir that, I will include it.

Yes either before SízeBigSize but I even recommend to sort it anywhere after the func DUint64 and before func EBytes32 because to me a bool is way more equal to the basic type like the integers rather than bytes (arrays) and custom types which follow after them.
Yeah I mean I would add EBoolT because if you know you are encoding a boolean you could save a heap allocation as well so better have it and stay inline with the other primitive golang types, but I have not strong feeling about it.

Chinwendu20 · 2023-10-05T18:57:23Z

Thanks @guggero really appreciate your explanation. What I want to know is why we cannot make the replace directive for internal packages like tlv permanently incorporated in the codebase without having it raise lint errors as that would be great for development but I guess this arrangement is best so that there can be separate PRs for respective changes made to each internal submodule.

guggero · 2023-10-06T07:33:02Z

Thanks @guggero really appreciate your explanation. What I want to know is why we cannot make the replace directive for internal packages like tlv permanently incorporated in the codebase without having it raise lint errors as that would be great for development but I guess this arrangement is best so that there can be separate PRs for respective changes made to each internal submodule.

The problem with using a replace directive permanently (which we used to have a couple of years back) is that those are ignored if you pull in lnd as a library in another Go project. So we tended to forget to push new tags for those submodules and update their versions in go.mod and then whenever we bumped the version of lnd in Loop/Pool/Faraday we ran into issues as the modules were outdated.

Chinwendu20 · 2023-10-09T10:35:32Z

Thanks @guggero really appreciate your explanation. What I want to know is why we cannot make the replace directive for internal packages like tlv permanently incorporated in the codebase without having it raise lint errors as that would be great for development but I guess this arrangement is best so that there can be separate PRs for respective changes made to each internal submodule.

The problem with using a replace directive permanently (which we used to have a couple of years back) is that those are ignored if you pull in lnd as a library in another Go project. So we tended to forget to push new tags for those submodules and update their versions in go.mod and then whenever we bumped the version of lnd in Loop/Pool/Faraday we ran into issues as the modules were outdated.

Thank you

ziggie1984

Nice PR, LGTM

morehouse

For testing completeness, perhaps we should extend the new TLV fuzz tests in tlv/fuzz_test.go for the new bool primitive.

morehouse · 2023-10-11T20:16:06Z

tlv/primitive.go

+		if _, err := io.ReadFull(r, buf[:1]); err != nil {
+			return err
+		}
+		*i = buf[0] != 0


Maybe we should return an error if val is not 0 or 1... If that happens when decoding from the DB it's a strong indication that there's a bug or data corruption somewhere.

Thanks for the review, pushed new changes with respect to this.

guggero · 2023-10-16T07:21:59Z

tlv/fuzz_test.go

@@ -97,6 +97,13 @@ func FuzzVarBytes(f *testing.F) {
 	})
 }

+func FuzzBool(f *testing.F) {
+	f.Fuzz(func(t *testing.T, data []byte) {
+		var val []byte


val needs to be a bool, otherwise this won't go past the first if in EBool. The rest looks good!

Thanks all done

Signed-off-by: Ononiwu Maureen <[email protected]>

guggero

Thanks, LGTM 🎉

morehouse

LGTM

guggero · 2023-10-16T15:26:36Z

@Chinwendu20 I pushed a new tag tor/v1.1.3 which you can use in the dependent version (meaning you can bump the version of github.com/lightningnetwork/lnd/tor to v1.1.3 in go.mod of your other PR.

Chinwendu20 · 2023-10-17T11:42:28Z

Woohoo! thanks!

guggero reviewed Oct 4, 2023

View reviewed changes

tlv/record.go Show resolved Hide resolved

tlv/primitive.go Outdated Show resolved Hide resolved

Chinwendu20 force-pushed the tlv-bool branch from 9126e7c to 8e5d574 Compare October 5, 2023 05:06

Chinwendu20 force-pushed the tlv-bool branch from 8e5d574 to d0b16ed Compare October 5, 2023 05:14

Chinwendu20 requested a review from guggero October 5, 2023 05:14

guggero reviewed Oct 5, 2023

View reviewed changes

ziggie1984 reviewed Oct 5, 2023

View reviewed changes

Chinwendu20 force-pushed the tlv-bool branch 2 times, most recently from 120e552 to 76b8755 Compare October 7, 2023 09:02

Chinwendu20 requested review from guggero and ziggie1984 October 7, 2023 11:49

Chinwendu20 force-pushed the tlv-bool branch from 76b8755 to cd1a935 Compare October 11, 2023 18:11

ziggie1984 approved these changes Oct 11, 2023

View reviewed changes

morehouse reviewed Oct 11, 2023

View reviewed changes

Chinwendu20 force-pushed the tlv-bool branch from cd1a935 to 43872f5 Compare October 12, 2023 19:53

Chinwendu20 requested a review from morehouse October 12, 2023 22:52

Chinwendu20 mentioned this pull request Oct 13, 2023

multi: log additional information for local force closures #8072

Open

guggero reviewed Oct 16, 2023

View reviewed changes

Chinwendu20 added 2 commits October 16, 2023 14:21

tlv: Added bool to primitive

206f773

Signed-off-by: Ononiwu Maureen <[email protected]>

docs: Added release note

844a792

Signed-off-by: Ononiwu Maureen <[email protected]>

Chinwendu20 force-pushed the tlv-bool branch from 43872f5 to 844a792 Compare October 16, 2023 13:23

guggero approved these changes Oct 16, 2023

View reviewed changes

morehouse approved these changes Oct 16, 2023

View reviewed changes

guggero merged commit a6f044b into lightningnetwork:master Oct 16, 2023
21 of 25 checks passed

saubyk added this to the v0.18.0 milestone Oct 16, 2023

morehouse mentioned this pull request Nov 14, 2023

tlv: seeds for new bool primitive lightninglabs/lnd-fuzz#19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tlv: Added bool to primitive #8057

tlv: Added bool to primitive #8057

Chinwendu20 commented Oct 4, 2023 •

edited

Loading

guggero left a comment

Chinwendu20 commented Oct 5, 2023

guggero left a comment

guggero Oct 5, 2023

Chinwendu20 Oct 5, 2023

guggero Oct 6, 2023

ziggie1984 Oct 9, 2023

guggero Oct 9, 2023 •

edited

Loading

guggero Oct 10, 2023

Chinwendu20 Oct 11, 2023

ziggie1984 Oct 5, 2023

Chinwendu20 Oct 5, 2023

ziggie1984 Oct 6, 2023

Chinwendu20 commented Oct 5, 2023

guggero commented Oct 6, 2023

Chinwendu20 commented Oct 9, 2023

ziggie1984 left a comment

morehouse left a comment

morehouse Oct 11, 2023

Chinwendu20 Oct 12, 2023

guggero Oct 16, 2023

Chinwendu20 Oct 16, 2023

guggero left a comment

morehouse left a comment

guggero commented Oct 16, 2023 •

edited

Loading

Chinwendu20 commented Oct 17, 2023

tlv: Added bool to primitive #8057

tlv: Added bool to primitive #8057

Conversation

Chinwendu20 commented Oct 4, 2023 • edited Loading

Change Description

Pull Request Checklist

Testing

Code Style and Documentation

guggero left a comment

Choose a reason for hiding this comment

Chinwendu20 commented Oct 5, 2023

guggero 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

guggero Oct 9, 2023 • edited Loading

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

Chinwendu20 commented Oct 5, 2023

guggero commented Oct 6, 2023

Chinwendu20 commented Oct 9, 2023

ziggie1984 left a comment

Choose a reason for hiding this comment

morehouse 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

guggero left a comment

Choose a reason for hiding this comment

morehouse left a comment

Choose a reason for hiding this comment

guggero commented Oct 16, 2023 • edited Loading

Chinwendu20 commented Oct 17, 2023

Chinwendu20 commented Oct 4, 2023 •

edited

Loading

guggero Oct 9, 2023 •

edited

Loading

guggero commented Oct 16, 2023 •

edited

Loading