Update to `rust-lightning:0.0.117` #15

luckysori · 2023-11-21T08:05:00Z

I've targeted a newly created branch with v0.0.117 as the tip so that we can judge the diff more easily. I guess we'll have to compare this diff to the diff in #13, which is not ideal.

Once we are happy with this PR we can merge it and reset split-tx-experiment to this branch.

holzeis

LGTM 👍but lets wait for the review of @Tibo-lg

lightning/src/ln/channelmanager.rs

Tibo-lg · 2023-11-21T11:08:13Z

Quite hard to review indeed, but basically LGTM, though:

it'd be nice to have CI passing
have you tried using it as a target in a rust-dlc branch? It'd be nice to see all test passing there before resetting the split-tx-experiment branch.

luckysori · 2023-11-21T11:34:03Z

Quite hard to review indeed, but basically LGTM, though:
* it'd be nice to have CI passing

Yep, that's a requirement for merging for me. I haven't investigated it yet, but perhaps you have some ideas since you fixed it last time. There were some conflicts in .github/workflows/build.yml and ci/ci-tests.sh, and perhaps I didn't resolve them correctly.

* have you tried using it as a target in a rust-dlc branch? It'd be nice to see all test passing there before resetting the `split-tx-experiment` branch.

Working on it! I also think we shouldn't merge this until everything works.

Tibo-lg · 2023-11-21T11:52:38Z

perhaps you have some ideas since you fixed it last time

I don't know if there are more issues, but looks like there is a field missing (which I think is only there when compiling with the taproot feature so you need the taproot feature flag): https://github.com/p2pderivatives/rust-lightning/actions/runs/6941222392/job/18881631878?pr=15#step:6:11803

…useful" This reverts commit eb882a6. The reverted commit introduced a regression causing the subchannel open protocol to hang because a particularly large message was not being sent over in full (one of the chunks was seemingly stuck).

I've backported from the 0.0.118 release since it seems like we could easily run into this by attempting to close a channel that is already closed or that never existed in the first place.

`Channel` is only a thing for funded channels. Thus, checking if a channel has not yet been funded is dead code and can simply be elided.

…ht place Because a `Funded` `Channel` cannot possibly be pre-funding, the logic in `ChannelManager::close_channel_internal` to handle pre-funding channels is in the wrong place. Rather than being handled inside the `Funded` branch, it should be in an `else` following it, handling either of the two `ChannelPhases` outside of `Funded`. Sadly, because of a previous control flow management `loop {}`, the existing code will infinite loop, which is fixed here.

…o chan Previously, unfunded channels would be stored outside of `PeerState::channel_by_id`, and thus if there is no channel when we look in `PeerState::channel_by_id`, `close_channel_internal` called `force_close_channel_with_peer` to hunt for unfunded channels. However, that is no longer the case, so the call is redundant, and we can simply return an error instead.

luckysori · 2023-12-18T05:12:07Z

@Tibo-lg: I think this is ready for review again (🤞 hoping CI passes).

The backported patches (9861fcd, 7798852, 0b465c0) diverge slightly from what ended up merging upstream, but the current version did fix all our problems, so I'd rather wait for 0.0.119 (coming soon) and drop these patches then.

Also, I had to add fabcb68 because otherwise the protocol would no longer finish over the internet 🤷 I'm talking to Matt about this, but the revert at least unblocks us for now.

luckysori · 2023-12-20T02:23:32Z

@Tibo-lg: FYI the only reason why the two check_commits jobs failed is because it checks all the commits between the branch and main, which in our case is still 0.0.116. There are hundreds of commits in there, so it ran into a timeout after 6 hours.

I think we should change this on the check-each-commit.sh script. It makes more sense to run against the base branch of the pull request. Let's see if f3bac8b works.

luckysori · 2023-12-20T04:00:06Z

I think we should change this on the check-each-commit.sh script. It makes more sense to run against the base branch of the pull request. Let's see if f3bac8b works.

The patch does work, but it breaks the on-commit-push workflow, because a regular push to a branch doesn't involve the concept of a base branch. This does highlight the fact that we are running CI twice on every push to a PR branch, which is unnecessary. I can't quite figure out how to solve this though. The info online is not very clear.

I would advocate to only run CI on PR pushes since that's the more common workflow. If we want to see what CI says, we can always open a (draft) PR. I propose 98af90f.

If you want to check what CI says you can always just open a (draft) PR.

Most PRs target `main`, but for those PRs that target a different branch it is more efficient to target the PR base branch. Otherwise we can end up running the script for too many commits and run into a timeout on CI.

The (de)serialisation for `ChannelTransactionParameters` changed between 0.0.114 and 0.0.116. This type had already been modified by us when adding the `original_funding_outpoint` field. When first introduced, we chose a type value of 14. After the update, we chose to reduce it down to 12 since the type 12 was no longer being used after changes between 0.0.114 and 0.0.116. Unfortunately, this unnecessarily broke channels created before the update. Eventually we will need to settle on a way to handle these possible conflicts with upstream. Tibo already suggested a few options: - Use much larger numbers for `rust-dlc` custom fields. - Move LDK values around as we rebase on top of new changes. - Add a subdomain for `rust-dlc` custom fields.

It would be even nicer to log the `ChannelDetails`, but then we would have to modify the macro and that is too painful.

Tibo-lg · 2023-12-20T04:19:19Z

Whichever is fine by me (at least for this repo), but another solution would be to run this job only for PR (see this)

luckysori · 2023-12-20T04:29:13Z

Whichever is fine by me (at least for this repo), but another solution would be to run this job only for PR (see this)

Oh, yes, we could do that too. But I thought it would be better to not run anything on branch push so that we can avoid duplicate work on CI and make CI builds a bit faster overall.

luckysori requested review from holzeis and Tibo-lg November 21, 2023 08:05

luckysori assigned holzeis and luckysori Nov 21, 2023

luckysori changed the base branch from split-tx-experiment to split-tx-experiment-117 November 21, 2023 08:08

holzeis approved these changes Nov 21, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

holzeis mentioned this pull request Nov 23, 2023

Upgrade ldk to 0.0.117 get10101/10101#1604

Closed

luckysori force-pushed the split-tx-experiment-117-10101 branch 3 times, most recently from bf675b6 to 342fecb Compare November 29, 2023 10:12

luckysori and others added 7 commits December 18, 2023 15:25

backport: Fix deadlock when closing an unavailable channel

88cabaa

I've backported from the 0.0.118 release since it seems like we could easily run into this by attempting to close a channel that is already closed or that never existed in the first place.

backport: Drop unreachable shutdown code in Channel::get_shutdown

9861fcd

`Channel` is only a thing for funded channels. Thus, checking if a channel has not yet been funded is dead code and can simply be elided.

Use p2pderivatives/main as rebase target in CI

f49cffe

Fix pinned dependency in CI

3f32a40

luckysori force-pushed the split-tx-experiment-117-10101 branch 3 times, most recently from 9fb4454 to 962f2ed Compare December 18, 2023 04:59

luckysori force-pushed the split-tx-experiment-117-10101 branch 5 times, most recently from 53a68fc to 0a2c070 Compare December 20, 2023 03:36

luckysori force-pushed the split-tx-experiment-117-10101 branch from 0a2c070 to ee7f03d Compare December 20, 2023 03:52

luckysori and others added 9 commits December 20, 2023 15:02

Run CI checks only on PR push

98af90f

If you want to check what CI says you can always just open a (draft) PR.

Run check-each-commit.sh based on PR base branch

1ab006b

Most PRs target `main`, but for those PRs that target a different branch it is more efficient to target the PR base branch. Otherwise we can end up running the script for too many commits and run into a timeout on CI.

Enable spliting lightning channel

d553106

Check and return commit tx number

2a4d495

Simplify implementation of get_original_funding_txo

f4822e9

Use original funding TXO to get channel monitors

9283634

Mark check_builds_on argument as unused

23f4cfe

Log channel balances before deleting channel data

15c4ff6

It would be even nicer to log the `ChannelDetails`, but then we would have to modify the macro and that is too painful.

luckysori force-pushed the split-tx-experiment-117-10101 branch from ee7f03d to 15c4ff6 Compare December 20, 2023 04:02

luckysori unassigned holzeis and luckysori Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to `rust-lightning:0.0.117` #15

Update to `rust-lightning:0.0.117` #15

luckysori commented Nov 21, 2023 •

edited

Loading

holzeis left a comment •

edited

Loading

Tibo-lg commented Nov 21, 2023

luckysori commented Nov 21, 2023

Tibo-lg commented Nov 21, 2023

luckysori commented Dec 18, 2023

luckysori commented Dec 20, 2023 •

edited

Loading

luckysori commented Dec 20, 2023 •

edited

Loading

Tibo-lg commented Dec 20, 2023

luckysori commented Dec 20, 2023

Update to rust-lightning:0.0.117 #15

Are you sure you want to change the base?

Update to rust-lightning:0.0.117 #15

Conversation

luckysori commented Nov 21, 2023 • edited Loading

holzeis left a comment • edited Loading

Choose a reason for hiding this comment

Tibo-lg commented Nov 21, 2023

luckysori commented Nov 21, 2023

Tibo-lg commented Nov 21, 2023

luckysori commented Dec 18, 2023

luckysori commented Dec 20, 2023 • edited Loading

luckysori commented Dec 20, 2023 • edited Loading

Tibo-lg commented Dec 20, 2023

luckysori commented Dec 20, 2023

Update to `rust-lightning:0.0.117` #15

Update to `rust-lightning:0.0.117` #15

luckysori commented Nov 21, 2023 •

edited

Loading

holzeis left a comment •

edited

Loading

luckysori commented Dec 20, 2023 •

edited

Loading

luckysori commented Dec 20, 2023 •

edited

Loading