Skip to content

Add 0.0.116 CHANGELOG entries and prep for 0.0.116rc1 #2414

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

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

Hopefully for release tomorrow, depending on a few last-minute things landing.

@TheBlueMatt TheBlueMatt added this to the 0.0.116 milestone Jul 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (8e2b70d) 90.24% compared to head (5adee54) 90.24%.

❗ 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    #2414      +/-   ##
==========================================
- Coverage   90.24%   90.24%   -0.01%     
==========================================
  Files         106      106              
  Lines       55442    55442              
  Branches    55442    55442              
==========================================
- Hits        50036    50035       -1     
- Misses       5406     5407       +1     

see 1 file 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.

@valentinewallace valentinewallace self-requested a review July 14, 2023 17:16
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. I went through git log --merges v0.0.115..HEAD to check for missing PRs.

@TheBlueMatt
Copy link
Collaborator Author

Added the requested bugfix note:

$ git diff-tree -U1 060387be bd8c60b8
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5d9c046b7..32b080759 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -114,2 +114,4 @@
  * Rapid Gossip Sync processing now fails on an unknown chain hash (#2324).
+ * RouteHintHop::htlc_maximum_msat is now enforced. Note that BOLT11 route hints
+   do not have such a field so this code is generally unused (#2305).

@@ -1,3 +1,120 @@
# 0.0.116rc1 - Jul 14, 2023 - "Anchoring the Roadmap"
Copy link

Choose a reason for hiding this comment

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

Yes I think if we can add a special note “anchor output” resuming the state of the things and announcing how we’re thinking to move forward with anchor deployment.

For now the dedicated compilation flag has been built and you need to manually turn on negotiate_anchors_zero_fee_htlc_tx.

Please if we can add a warning all usage of anchor outputs channels with 0.0.116 is on the responsibility on the user, be conservative and careful with your counterparty selection, you might loss money something like that (and cute kittens might die all over the Internet).

I think we should say with 0.0.117 we hope to have all the fee-bumping reserves computations working correctly, at the very least for a ldk-node style deployment and this be default for 0.0.118 (soft commitment towards our users, all things equal).

What else can be in a special anchor output release note ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I think if we can add a special note “anchor output” resuming the state of the things and announcing how we’re thinking to move forward with anchor deployment.

Do you have any issue with the way its described in the first bullet point?

For now the dedicated compilation flag has been built

This is not true, the cfg flag has been removed as a part of "releasing anchors".

Please if we can add a warning all usage of anchor outputs channels with 0.0.116 is on the responsibility on the user, be conservative and careful with your counterparty selection, you might loss money something like that (and cute kittens might die all over the Internet).

Do you have a specific suggestion that fits into the release notes, otherwise probably best to get it in the config docs (I think the docs there are pretty good, though, no?).

I think we should say with 0.0.117 we hope to have all the fee-bumping reserves computations working correctly

That's news me - implementing reserves in a way that makes sense is super tricky, and we haven't figured out exactly how to do it after some discussion in #2320. Marking that for the 117 milestone is a pretty big task given we are still in the design phase, and 117 already has a ton of things scheduled/tagged. Thus, I'm not sure we want to commit to it as a part of the next release.

Copy link

Choose a reason for hiding this comment

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

Do you have any issue with the way its described in the first bullet point?

Yes, I would favor more a “big warning notice” about anchor outputs to say to our users this is the main thing we have worked for this release, and this is the one thing we would love to have API feedback on it.

Still while being conservative on funds risks coming with a beta feature. I had in mind what Dave did for mempoolfullrbf with 24.0.1: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-24.0.1.md#notice-of-new-option-for-transaction-replacement-policies where the feature is explicitly said as conservative and there is not yet consensus between devs on the risks trade-offs.

This is not true, the cfg flag has been removed as a part of "releasing anchors".

Ah that might be true, what is “releasing anchors” in this context the v0.0.116 rc0 ? Note it doesn’t show up in the GH https://github.com/lightningdevkit/rust-lightning/releases

Do you have a specific suggestion that fits into the release notes, otherwise probably best to get it in the config docs (I think the docs there are pretty good, though, no?).

“Users are recommended to be conservative with the usage of option_anchors_zero_fee_htlc_tx and select their anchor outputs channels counterparty with care. With anchor output come the requiremeent a Lightning node has to manage “hot” fee-bumping reserves, a fundamental change in the operations of a node. Best practice would be to add progressively funds on this channel type until one is more familiar with the effective management of fee-bumping reserves. Further documentation is available around negotiate_anchors_zero_fee_htlc_tx”.

Feel free to tweak it according to your taste, though when a feature is new I think it’s better to say twice to our users a warning both in docs and in the release notes (and here I must say LND has the practice of doing it far better than us).

That's news me - implementing reserves in a way that makes sense is super tricky, and we haven't figured out exactly how to do it after some discussion in #2320. Marking that for the 117 milestone is a pretty big task given we are still in the design phase, and 117 already has a ton of things scheduled/tagged. Thus, I'm not sure we want to commit to it as a part of the next release.

Yeah sorry the “we” might not reflect the position of all of us, though at the very least I believe it reflect all the discussion over the anchor output patchetset (as tracked only recently with ##2297) though I think I can say there is a “rough consensus” between you, Wilmer, Elias and me our fee-bumping reserves API can be improved.

For sure, “100% robust” fee-bumping reserves might be a too high step for 0.0.117 though I would be very happy if we can know clearly what set of PRs and substantial improvement of the fee-bumping reserves we aim with 0.0.117, and that way get us one step closer from marking anchor output “alpha” (somehow we shouldn’t delay too much anchor output once fee-bumping reserves API a bit mature as legacy channels have historical and known dust exposure).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah that might be true, what is “releasing anchors” in this context the v0.0.116 rc0 ? Note it doesn’t show up in the GH https://github.com/lightningdevkit/rust-lightning/releases

Yea, cause we don't have a release up yet :)

Yes, I would favor more a “big warning notice” about anchor outputs to say to our users this is the main thing we have worked for this release, and this is the one thing we would love to have API feedback on it.

Alright, since this has ACKs I'm gonna go ahead and merge it, and will work on adding more context in the 0.0.116-release release notes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #2427

@tnull tnull self-requested a review July 17, 2023 17:54
@TheBlueMatt
Copy link
Collaborator Author

Pushed the requested changes and added one more PR:

$ git diff-tree -U1 5adee5490 cc5fea84e
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3ccc8ec4d..7243b520b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,2 +25,3 @@
    `PaymentParameters`. Note that not all recipients support this (#2156).
+ * A new `ConfirmationTarget::MempoolMinimum` has been added (#2415).
  * `SpendableOutputDescriptor::to_psbt_input` was added (#2286).
@@ -88,3 +89,3 @@
    now disconnected to allow for on-reconnection state machine reset. This
-   works around some issues in lnd prior to 16.3 which can cause channels to
+   works around some issues in LND prior to 16.3 which can cause channels to
    hang and eventually force-close (#2293).
@@ -118,4 +119,4 @@
  * Rapid Gossip Sync processing now fails on an unknown chain hash (#2324).
- * RouteHintHop::htlc_maximum_msat is now enforced. Note that BOLT11 route hints
-   do not have such a field so this code is generally unused (#2305).
+ * `RouteHintHop::htlc_maximum_msat` is now enforced. Note that BOLT11 route
+   hints do not have such a field so this code is generally unused (#2305).
 

@ariard
Copy link

ariard commented Jul 17, 2023

Please feel free to tag v0.0.116rc1, I’m fine with the current release note from a conceptual viewpoint modulo the safety warning for anchor outputs usage with the suggestion proposed. I wanna to focus on my package relay review during the current week and I think it’s nice to not let defer by another week this release candidate.

By the way what is the heuristics to move from a release candidate to another, like addressing all the bugs reported by RC testers I guess and address API changes rough corners ? I think it’s great we finally do release candidate though I don’t know how they fit with our release cycle (which more or less aims to be 8 weeks from historical discussions when we start to do real release iirc).

@TheBlueMatt TheBlueMatt merged commit c5c5f3f into lightningdevkit:main Jul 17, 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.

6 participants