Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace opt_anchors with ChannelTypeFeatures #2361

Merged

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Jun 16, 2023

This is meant to replace #2321.

Fixes #1971.

@TheBlueMatt TheBlueMatt added this to the 0.0.116 milestone Jun 16, 2023
@TheBlueMatt
Copy link
Collaborator

If we want to do this it has to happen in 116. Its by no means blocking 116, but tagging it as 116 so that we don't forget it just by accident. Will take a look when its non-draft/CI passes.

@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch 6 times, most recently from bf40449 to 51be419 Compare June 16, 2023 19:56
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023

Codecov Report

Patch coverage: 95.02% and project coverage change: +0.82 🎉

Comparison is base (c5214c2) 90.28% compared to head (4b1f6ca) 91.11%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2361      +/-   ##
==========================================
+ Coverage   90.28%   91.11%   +0.82%     
==========================================
  Files         106      106              
  Lines       54323    63856    +9533     
  Branches    54323    63856    +9533     
==========================================
+ Hits        49047    58180    +9133     
- Misses       5276     5676     +400     
Impacted Files Coverage Δ
lightning/src/ln/functional_test_utils.rs 89.29% <0.00%> (+2.00%) ⬆️
lightning/src/chain/package.rs 91.43% <88.99%> (+0.39%) ⬆️
lightning/src/chain/channelmonitor.rs 94.72% <95.45%> (-0.01%) ⬇️
lightning/src/ln/chan_utils.rs 94.51% <95.83%> (-0.34%) ⬇️
lightning/src/ln/channel.rs 92.49% <97.26%> (+2.67%) ⬆️
lightning/src/chain/onchaintx.rs 92.72% <100.00%> (-0.33%) ⬇️
lightning/src/events/bump_transaction.rs 41.55% <100.00%> (+0.76%) ⬆️
lightning/src/ln/features.rs 97.09% <100.00%> (+0.79%) ⬆️
lightning/src/ln/functional_tests.rs 99.13% <100.00%> (+0.90%) ⬆️
lightning/src/ln/monitor_tests.rs 98.05% <100.00%> (-0.23%) ⬇️
... and 2 more

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from d3e1130 to 0821c74 Compare June 16, 2023 22:27
@arik-so arik-so marked this pull request as ready for review June 16, 2023 22:27
@arik-so
Copy link
Contributor Author

arik-so commented Jun 16, 2023

Hey @devrandom @ksedgwic, tagging you here to let you know that this is the serialization-changing PR I was talking about.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor drawback of using ChannelTypeFeatures instead of our own enum of supported channel types is that we'll store the full feature bit vector in all these different places. We're already up to 8 bytes per occurrence and it will only get bigger with new features, but not something we really need to worry about for now.

lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/sign/mod.rs Outdated Show resolved Hide resolved
lightning/src/chain/package.rs Outdated Show resolved Hide resolved
@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from 52c6e24 to d4b6b8b Compare June 17, 2023 02:54
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really needs to be more than one commit

lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/ln/features.rs Outdated Show resolved Hide resolved
@arik-so
Copy link
Contributor Author

arik-so commented Jun 19, 2023

It's not really super splittable. I could modify the adjusted structs one by one, but the unit test modification onus would still be quite major for each commit, and it would make less sense than modifying all six structs at once.

The only thing that really lends itself to a split is the new feature and the new methods on channel type features.

@TheBlueMatt
Copy link
Collaborator

You could do some of the new utility functions separately. The biggest issue with it all being in the same commit isn't the raw diff size but more the various different changes across different files in the same commit. A commit that is super repetitive and just making the same change over and over again in a ton of places is fine, but limiting the number of different changes is good.

@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch 2 times, most recently from a65221d to a602822 Compare June 19, 2023 19:15
@arik-so
Copy link
Contributor Author

arik-so commented Jun 19, 2023

I will squash the last two commits into the others later. Just wanna run cloud CI first before messing up too much at once.

@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from 869f240 to 4655816 Compare June 19, 2023 21:35
@TheBlueMatt
Copy link
Collaborator

Needs rebase now that anchors handling bits landed.

@arik-so
Copy link
Contributor Author

arik-so commented Jun 19, 2023

Yeah I can see that

@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from 4655816 to c5b915e Compare June 19, 2023 22:55
@TheBlueMatt
Copy link
Collaborator

Fixes #1971

@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch 3 times, most recently from 80ec785 to d7064c7 Compare June 22, 2023 00:32
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have a commit which isn't word-wrapped at 60 chars.

lightning/src/ln/features.rs Outdated Show resolved Hide resolved
lightning/src/chain/onchaintx.rs Outdated Show resolved Hide resolved
lightning/src/chain/package.rs Outdated Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved
lightning/src/chain/package.rs Outdated Show resolved Hide resolved
@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from d7064c7 to a271abc Compare June 22, 2023 21:54
Specifically, introduce a new constructor for an anchors-
supporting feature set, as well as methods that will
maintain forwards-compatible deserialization in older
versions.
@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from a271abc to 9abdd4c Compare June 22, 2023 22:24
lightning/src/sign/mod.rs Outdated Show resolved Hide resolved
lightning/src/ln/functional_test_utils.rs Show resolved Hide resolved
lightning/src/chain/package.rs Show resolved Hide resolved
lightning/src/chain/package.rs Show resolved Hide resolved
(10, built, required),
(12, htlcs, vec_type),
(14, opt_anchors, option),
(16, opt_non_zero_fee_anchors, option),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't we break VLS if they're setting this and storing them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they did say they weren't, but we should confirm @devrandom @ksedgwic

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch 2 times, most recently from fff7da5 to 4b1f6ca Compare June 23, 2023 04:06
This change modifies six structs that were keeping
track of anchors features with an `opt_anchors` field,
as well as another field keeping track of nonzero-fee-
anchor-support.
@arik-so arik-so force-pushed the 2023-06-anchor-channel-type-features branch from 4b1f6ca to 82b5359 Compare June 23, 2023 17:37
@TheBlueMatt TheBlueMatt merged commit 3973865 into lightningdevkit:main Jun 23, 2023
@TheBlueMatt TheBlueMatt modified the milestones: 0.0.116alpha, 0.0.116 Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't store opt_anchors explicitly in Channel
5 participants