Skip to content

Cut 0.0.106 #1397

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
Apr 3, 2022
Merged

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Mar 30, 2022

No description provided.

@valentinewallace valentinewallace self-requested a review March 30, 2022 17:11
CHANGELOG.md Outdated
* Fixed a compilation error in `ProbabilisticScorer` under `--feature=no-std`
resulting from `core` not having `f64::log10`. A log approximation is used
instead independent of feature settings (#1347).
* Fixed an integer underflow in `find_route` when
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm? I don't think there's an integer underflow in 105.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps I misunderstood this. Does this PR fix anything in 105?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It changes the definition of the cltv max to include the last-hop, whereas in 105 it was just a limit on all hops except the last. the new definition is likely more sensible to users, but technically its not a bugfix against 105.

@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2022

Codecov Report

Merging #1397 (3b576ea) into main (7671ae5) will increase coverage by 0.26%.
The diff coverage is n/a.

❗ Current head 3b576ea differs from pull request most recent head de8bb8d. Consider uploading reports for the commit de8bb8d to get more accurate results

@@            Coverage Diff             @@
##             main    #1397      +/-   ##
==========================================
+ Coverage   90.76%   91.02%   +0.26%     
==========================================
  Files          73       73              
  Lines       41195    44165    +2970     
  Branches    41195    44165    +2970     
==========================================
+ Hits        37392    40203    +2811     
- Misses       3803     3962     +159     
Impacted Files Coverage Δ
lightning/src/ln/channel.rs 88.36% <0.00%> (+0.07%) ⬆️
lightning-invoice/src/de.rs 81.27% <0.00%> (+0.20%) ⬆️
lightning-persister/src/lib.rs 93.93% <0.00%> (+0.33%) ⬆️
lightning/src/routing/network_graph.rs 89.61% <0.00%> (+0.55%) ⬆️
lightning/src/routing/router.rs 93.21% <0.00%> (+0.58%) ⬆️
lightning/src/ln/channelmanager.rs 86.16% <0.00%> (+1.42%) ⬆️
lightning/src/routing/scoring.rs 96.25% <0.00%> (+2.21%) ⬆️
lightning-background-processor/src/lib.rs 95.35% <0.00%> (+2.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7671ae5...de8bb8d. Read the comment docs.

# 0.0.106 - 2022-03-30

## API Updates
* Minimum supported rust version (MSRV) is now 1.41.1 (#1310).
Copy link

Choose a reason for hiding this comment

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

For now our user base is reasonable so we can follow quickly the still fast-moving rust ecosystem. Though in the future we might give a release or two of warning to our users, it can be a time-consuming task ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

1.41 is really old - instead of trying to give warning, we instead just only use things that every major linux distro is shipping already, which means they shouldn't have to do anything to an up-to-date system :)

* `ChannelManager::create_inbound_payment_for_hash`'s documentation has been
corrected to remove the one-year restriction on `invoice_expiry_delta_secs`,
which is only applicable to the deprecated `create_inbound_payment_legacy`
and `create_inbound_payment_for_hash_legacy` methods (#1341).
Copy link

Choose a reason for hiding this comment

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

IMHO, it's better to announce a timeline or release number in the deprecation warning, instead to say "will be removed soon" which opens the way for interpretation and thus user bargaining. I know, for a maintainer it might be preferable to use "soon" as it give more room to adapt in function of the user base, though let's be aware it's a double-edge (Yeah the project is young, we might not have a consistent deprecation process that early).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, though note this isn't related to any changes for this release.

CHANGELOG.md Outdated
feature is on by default (#1388).
* Lightning feature `option_scid_alias` is now supported and negotiated when
opening a channel with a peer. It can be configured via
`ChannelHandshakeConfig::negotiate_scid_privacy` and is off by default.
Copy link

Choose a reason for hiding this comment

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

"It is considered to be turn on by default in the future" ?

CHANGELOG.md Outdated
set to two weeks ago for the first five peers or otherwise to one hour ago
(#1382).
* `ChannelManager::timer_tick_occurred` will now timeout a received multi-path
payment (MPP) after three ticks if not received in full (#1353).
Copy link

Choose a reason for hiding this comment

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

FYI, I think one of the use-case idea of MPP was multi-party payment where the shards would come from different senders. E.g splitting bills at a dinner. I think there is no wallet implementing that right now but time-bounding this change to 3 ticks would be likely a nuisance for that use-case ? Just for awareness, it can be changed back if multi-party MPP become a thing.

@jkczyz jkczyz added this to the 0.0.106 milestone Apr 1, 2022
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.

I didn't bother making sure you included all the relevant PRs.

CHANGELOG.md Outdated
@@ -1,3 +1,112 @@
# 0.0.106 - 2022-03-30
Copy link
Collaborator

Choose a reason for hiding this comment

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

Never try to predict the date, you'll jinx yourself :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol

CHANGELOG.md Outdated

## API Updates
* Minimum supported rust version (MSRV) is now 1.41.1 (#1310).
* `PhantomRouteHints`, `FixedPenaltyScorer`, and `ScoringParameters` now
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this should be sorted almost at the end - its definitely not the most important thing in 106.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the end. I was just going by merge order.

CHANGELOG.md Outdated
(#1286). The seed is also used to randomize candidate paths during route
selection (#1359).
* `PeerManager::timer_tick_occurred` will disconnect an unresponsive peer after
four ticks, corresponding to 40 seconds with the new recommendation to call
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, no, we still disconnect an unresponsive peer after 10 seconds, but we disconnect a peer that is actively sending us messages but not responding to our pings in 40 seconds. I dunno if we even really need to mention it given the "main" disconnection limit is still 10 seconds, but if we do I'd sort it further down and say something about a peer "not responding to our pings during gossip sync".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropping it.

CHANGELOG.md Outdated
* The `lightning` crate has a `grind_signatures` feature used to produce
signatures with low r-values for more predictable transaction weight. This
feature is on by default (#1388).
* Lightning feature `option_scid_alias` is now supported and may be negotiated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this a bit more important than some of the "basically trivial changes" above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was going by merge order but moved now along with the points on #1311. Let me know if you want anything else re-ordered. I can do something more reasonably on my own but don't want to disturb the diff order too much... probably should have made more atomic fixup commits...

# 0.0.106 - 2022-03-30

## API Updates
* Minimum supported rust version (MSRV) is now 1.41.1 (#1310).
Copy link
Collaborator

Choose a reason for hiding this comment

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

1.41 is really old - instead of trying to give warning, we instead just only use things that every major linux distro is shipping already, which means they shouldn't have to do anything to an up-to-date system :)

CHANGELOG.md Outdated
* Reduced time spent processing `channel_update` messages by checking
signatures after checking if no newer messages have already been processed
(#1380).
* `BackgroundProcessor` now persists `NetworkGraph` upon removing stale
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/upon removing stale channels/on a timer/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Should this be split to have a bullet in API change, too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yea, I feel like this is an API change, not a bugfix.

CHANGELOG.md Outdated
* Channels open with `option_scid_alias` negotiated (enabled via
`ChannelHandshakeConfig::negotiate_scid_privacy`) will not be compatible with
prior releases (#1351). Similarly for inbound channels where the
`accept_channel` message has `ChannelTypeFeatures::supports_scid_privacy`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd really prefer we explicitly mention the OpenChannelRequest event here - this is quite a big deal so we should be painfully explicit, unlike lots of things where we can be vague and it doesn't matter.

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.

Otherwise looks good, one further nit, take it or leave it.

(#1361).
* `RoutingMessageHandler::sync_routing_table` has been renamed `peer_connected`
(#1368).
* `MessageSendEvent::SendGossipTimestampFilter` has been added to indicate that
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I do kinda feel like we can leave out stuff that is painfully obvious what to do with it and which will cause downstream compile failures if there's anything a downstream user needs to do with it. ie this wont impact anyone, and in the off-chance it does, its obvious to them what to do and they'll get a compile failure if they dont.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that I was thinking release notes also serve to raise awareness of new features before a user decides to upgrade.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, I don't feel strongly. Its just not clear what a user would do with this - we still exhibit the ~same (or, well, the expected, not the buggy) sync behavior users were expecting, we just have a new enum variant to send a message, but I don't think any users ever actually send manual messages today.

Copy link
Contributor Author

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Let me know if you want me to address #1397 (comment)

(#1361).
* `RoutingMessageHandler::sync_routing_table` has been renamed `peer_connected`
(#1368).
* `MessageSendEvent::SendGossipTimestampFilter` has been added to indicate that
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that I was thinking release notes also serve to raise awareness of new features before a user decides to upgrade.

* Matt Corallo
* Omar Shamardy
* Viktor Tigerström
* dependabot[bot]
Copy link
Contributor

@tnull tnull Apr 2, 2022

Choose a reason for hiding this comment

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

Not to hurt its feelings, but maybe dependabot doesn't need to be mentioned here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol. I, for one, welcome our new robot overlords. #1322 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, bots are people, too.

@jkczyz
Copy link
Contributor Author

jkczyz commented Apr 2, 2022

@TheBlueMatt Let me know if you're good for me to squash

@TheBlueMatt
Copy link
Collaborator

Yes, feel free to.

@jkczyz jkczyz force-pushed the 2022-03-release-0.0.106 branch from ea256d4 to 3b576ea Compare April 2, 2022 17:28
TheBlueMatt
TheBlueMatt previously approved these changes Apr 2, 2022
CHANGELOG.md Outdated
* Channels open with `option_scid_alias` negotiated (enabled via
`ChannelHandshakeConfig::negotiate_scid_privacy`) will be incompatible with
prior releases. Similarly, inbound channels accepted from an
`OpenChannelRequest` with a `channel_type` that has
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: also inbound channels which have that bit and which arent manually accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... does this mean if a peer requests option_scid_alias when opening an inbound channel, the channel will be incompatible with prior versions? Seems like the other two cases require the user to do something explicitly to become incompatible while this case would not based on my reading of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, we had some brief discussion about this prior to merge. Its definitely far from ideal, the only reason we did it is no one else is going to open these channels yet (only LDK supports any of it as of yet, or will in the immediate future), and adding yet another config flag didn't feel worth it. I'm open to revisiting it, adding a config flag is trivial and we could consider it a bugfix, but I don't feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't have a strong opinion but wonder if negotiate_scid_privacy could be used. If false, send an error to the peer if requesting that channel type.

TheBlueMatt
TheBlueMatt previously approved these changes Apr 3, 2022
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.

Feel free to squash - for stuff like this that's relatively timely, at least if I'm the only reviewer I don't mind squash-pushes regularly.

arik-so
arik-so previously approved these changes Apr 3, 2022
* Matt Corallo
* Omar Shamardy
* Viktor Tigerström
* dependabot[bot]
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, bots are people, too.

@jkczyz
Copy link
Contributor Author

jkczyz commented Apr 3, 2022

Updated date to today

Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

Re-ACKing

@TheBlueMatt TheBlueMatt merged commit 0a0f87c into lightningdevkit:main Apr 3, 2022
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Post-merge ACK 4afb697

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