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

Upgrade to rust-lightning 0.0.116 and bump rust-dlc #1482

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

bonomat
Copy link
Contributor

@bonomat bonomat commented Oct 13, 2023

Fix #1084.
Fix #1472.

As far as we can tell, positions opened before this update will not be loaded from storage. It is therefore recommended to close all positions before upgrading to this patch.

Some notes:

  • We've had to increase the payment amounts in the multi_hop_payment and just_in_time_channel_with_multiple_payments tests, as otherwise we would run into route not found errors. It's probably worth investigating why this is the case.

  • We have decided to eagerly send rust-dlc messages because: (1) after the upgrade they were not being automatically sent as fast as they used to and (2) we can speed up the protocols this way anyway.

@holzeis

This comment was marked as resolved.

@bonomat

This comment was marked as resolved.

@holzeis

This comment was marked as resolved.

Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

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

LGMT 🚀

We might want to schedule the release of this though 🙂

CHANGELOG.md Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/dlc_custom_signer.rs Show resolved Hide resolved
crates/ln-dlc-node/src/fee_rate_estimator.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/ln/app_event_handler.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/ln/app_event_handler.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/ln/app_event_handler.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/ln/coordinator_event_handler.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/node/mod.rs Outdated Show resolved Hide resolved
crates/ln-dlc-node/src/node/mod.rs Outdated Show resolved Hide resolved
maker/src/ln/event_handler.rs Outdated Show resolved Hide resolved
@bonomat bonomat force-pushed the chore/upgrade-to-rust-lightning-116 branch from b8b46be to c136039 Compare October 19, 2023 13:00
@bonomat
Copy link
Contributor Author

bonomat commented Oct 19, 2023

@luckysori : feel free to grab it tomorrow and continue on it. It looks like some of the lnd_dlc tests are now failing.

@luckysori luckysori assigned luckysori and unassigned bonomat Oct 20, 2023
@luckysori luckysori force-pushed the chore/upgrade-to-rust-lightning-116 branch from 7222d84 to 949fcfa Compare October 24, 2023 05:30
@luckysori luckysori changed the title chore: upgrade to rust-lightning 0.0.116 and bump rust-dlc Upgrade to rust-lightning 0.0.116 and bump rust-dlc Oct 24, 2023
@luckysori luckysori force-pushed the chore/upgrade-to-rust-lightning-116 branch 8 times, most recently from dd7a857 to 172807d Compare October 25, 2023 02:32
@luckysori luckysori force-pushed the chore/upgrade-to-rust-lightning-116 branch from 172807d to c0daf29 Compare November 1, 2023 14:24
@luckysori
Copy link
Contributor

luckysori commented Nov 1, 2023

@bonomat, @holzeis: Please have another look at this. There was a slightly tricky rebase in between which forced me to touch the collaborative revert functionality that was recently introduced.

I've tested this a few times locally:

  • Open channel + position on 0.0.114.
  • Upgrade app and coordinator to 0.0.116.
  • Restart app and coordinator without hiccups.
  • Collaboratively revert the open subchannel, with the money arriving back in the app's on-chain wallet.

I also did the same (except the last step, since it's impossible) for a channel without an open position.

Cargo.toml Outdated
p2pd-oracle-client = { git = "https://github.com/p2pderivatives/rust-dlc", rev = "9815582" }
dlc-trie = { git = "https://github.com/p2pderivatives/rust-dlc", rev = "9815582" }
simple-wallet = { git = "https://github.com/p2pderivatives/rust-dlc", rev = "9815582" }
dlc-manager = { git = "https://github.com/p2pderivatives/rust-dlc", rev = "d1e08a0" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we will need to bump this once more because this fix landed: p2pderivatives/rust-dlc#162

// Enqueue the message.
dlc_message_handler.send_message(node_id, msg);

// According to the LDK docs, you don't _have_ to call this function explicitly if you are
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 👍

Comment on lines +740 to +742
// TODO: Should get the funding TXO from the coordinator, since we pass it in as an argument to
// the API to collaboratively revert anyway. This way we can avoid depending on the channel
// still being tracked by the `ChannelManager`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, we can definitely do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mentioned in this ticket: #1546

@bonomat
Copy link
Contributor Author

bonomat commented Nov 1, 2023

@bonomat, @holzeis: Please have another look at this. There was a slightly tricky rebase in between which forced me to touch the collaborative revert functionality that was recently introduced.

I've tested this a few times locally:

* Open channel + position on 0.0.114.

* Upgrade app and coordinator to 0.0.116.

* Restart app and coordinator without hiccups.

* Collaboratively revert the open subchannel, with the money arriving back in the app's on-chain wallet.

I also did the same (except the last step, since it's impossible) for a channel without an open position.

Unfortunately I cannot confirm that this works for me :(

  1. start coordinator/app/maker on 0c0a33a
  2. trade with app 1 keep position open
  3. trade with app 2 open and close position
  4. upgrade all to this PR
  5. --> can't start the coordiantor:
     Running `target/debug/coordinator`
2023-11-01 19:46:27  INFO coordinator::logger: Initialized logger
2023-11-01 19:46:27  INFO coordinator: Data-dir: "/Users/bonomat/src/github/itchysats/10101/data/coordinator/regtest"
2023-11-01 19:46:27 DEBUG coordinator::cli: Skipping node announcement on '0.0.0.0'.
2023-11-01 19:46:27  INFO coordinator::cli: Adding node announcement within local network 192.168.178.56. Do not use for production!
2023-11-01 19:46:27  INFO ln_dlc_node::on_chain_wallet: Creating the wallet network=Regtest
2023-11-01 19:46:27  INFO ln_dlc_node::node::channel_manager: Found channel manager data on disk. Recovering from stored state
Error: Unknown required feature preventing decode

@luckysori
Copy link
Contributor

Unfortunately I cannot confirm that this works for me :(

1. start coordinator/app/maker on [0c0a33a](https://github.com/get10101/10101/commit/0c0a33a157db4ad873147badb12ffea40d28b1bb)

2. trade with app 1 keep position open

3. trade with app 2 open and close position

4. upgrade all to this PR

5. --> can't start the coordiantor:
     Running `target/debug/coordinator`
2023-11-01 19:46:27  INFO coordinator::logger: Initialized logger
2023-11-01 19:46:27  INFO coordinator: Data-dir: "/Users/bonomat/src/github/itchysats/10101/data/coordinator/regtest"
2023-11-01 19:46:27 DEBUG coordinator::cli: Skipping node announcement on '0.0.0.0'.
2023-11-01 19:46:27  INFO coordinator::cli: Adding node announcement within local network 192.168.178.56. Do not use for production!
2023-11-01 19:46:27  INFO ln_dlc_node::on_chain_wallet: Creating the wallet network=Regtest
2023-11-01 19:46:27  INFO ln_dlc_node::node::channel_manager: Found channel manager data on disk. Recovering from stored state
Error: Unknown required feature preventing decode

Hmm, I haven't been able to reproduce your problem yet.

@holzeis
Copy link
Contributor

holzeis commented Nov 2, 2023

I can confirm that it's working for me 🚀

  1. just wipe
  2. start coordinator, maker
  3. just fund
  4. start app / create wallet
  5. fund wallet with faucet
  6. open position
  7. stop coordinator
  8. change to head of branch chore/upgrade-to-rust-lightning-116
  9. start coordinator / restart maker
  10. just gen ios run
  11. close position 🎉

@bonomat
Copy link
Contributor Author

bonomat commented Nov 2, 2023

Unfortunately I have severe problems with this branch :(

Even if I just use this branch to do a restart roundtrip I get errors, e.g.

  1. checkout this branch
  2. start everything up on a new env
  3. fund an open position through app
  4. close coordinator
  5. start coordinator

Get same error :(

2023-11-02 16:33:30  INFO coordinator::logger: Initialized logger
2023-11-02 16:33:30  INFO coordinator: Data-dir: "/Users/bonomat/src/github/itchysats/10101/data/coordinator/regtest"
2023-11-02 16:33:30 DEBUG coordinator::cli: Skipping node announcement on '0.0.0.0'.
2023-11-02 16:33:30  INFO coordinator::cli: Adding node announcement within local network 192.168.178.56. Do not use for production!
2023-11-02 16:33:30  INFO ln_dlc_node::on_chain_wallet: Creating the wallet network=Regtest
2023-11-02 16:33:30  INFO ln_dlc_node::node::channel_manager: Found channel manager data on disk. Recovering from stored state
2023-11-02 16:33:30  INFO lightning::ln::channelmanager: Successfully loaded channel 1653c2e484433eb51a01a0550bd3a22032f6ffff08658b4311dd8c687eebb992 at update_id 0 against monitor at update id 0
Error: Unknown required feature preventing decode

Edit: I thought I'll give upgrading rust to 1.73 a change but without luck.

@bonomat
Copy link
Contributor Author

bonomat commented Nov 2, 2023

I did a full wipe of all cached dependencies, data and docker and now I get a new error 😅

2023-11-02 18:40:46  INFO coordinator::logger: Initialized logger
2023-11-02 18:40:46  INFO coordinator: Data-dir: "/Users/bonomat/src/github/10101/10101/data/coordinator/regtest"
2023-11-02 18:40:46 DEBUG coordinator::cli: Skipping node announcement on '0.0.0.0'.
2023-11-02 18:40:46  INFO coordinator::cli: Adding node announcement within local network 192.168.178.56. Do not use for production!
2023-11-02 18:40:46  INFO ln_dlc_node::on_chain_wallet: Creating the wallet network=Regtest
2023-11-02 18:40:47  INFO ln_dlc_node::node::channel_manager: Found channel manager data on disk. Recovering from stored state
2023-11-02 18:40:47  INFO lightning::ln::channelmanager: Successfully loaded channel 190c731887bfd7644d16ee22da3e486327d99cbae0488ad0e6cf2f2be55d1c5b at update_id 0 against monitor at update id 0
2023-11-02 18:40:47  INFO lightning::ln::channelmanager: Successfully loaded channel 84fa01f9fae2999302f145746559758b11c27a36467c5539ce49c2dd17013e40 at update_id 13 against monitor at update id 13
2023-11-02 18:40:47  INFO lightning::ln::channelmanager: Successfully loaded channel 1e41104c8dd12cfc1c396ddf95dfa20a516a6e0f9eebdbf156ab3df2b2f4331d at update_id 5 against monitor at update id 5
2023-11-02 18:40:47 ERROR coordinator: Aborting after panic in task info=panicked at 'We already checked for monitor presence when loading channels', /Users/bonomat/.cargo/git/checkouts/rust-lightning-bee3233422fa4387/b13daab/lightning/src/ln/channelmanager.rs:8879:22
   0: std::backtrace_rs::backtrace::libunwind::trace
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
   1: std::backtrace_rs::backtrace::trace_unsynchronized
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2: std::backtrace::Backtrace::create
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/backtrace.rs:332:13
   3: coordinator::main::{{closure}}::{{closure}}
             at ./coordinator/src/bin/coordinator.rs:59:29
   4: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/alloc/src/boxed.rs:1987:9
   5: std::panicking::rust_panic_with_hook
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:695:13
   6: std::panicking::begin_panic_handler::{{closure}}
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:582:13
   7: std::sys_common::backtrace::__rust_end_short_backtrace
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/sys_common/backtrace.rs:150:18
   8: rust_begin_unwind
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:578:5
   9: core::panicking::panic_fmt
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:67:14
  10: core::panicking::panic_display
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:150:5
  11: core::panicking::panic_str
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:134:5
  12: core::option::expect_failed
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/option.rs:2025:5
  13: core::option::Option<T>::expect
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/option.rs:913:21
  14: lightning::ln::channelmanager::<impl lightning::util::ser::ReadableArgs<lightning::ln::channelmanager::ChannelManagerReadArgs<M,T,ES,NS,SP,F,R,L>> for (bitcoin::hash_types::newtypes::BlockHash,lightning::ln::channelmanager::ChannelManager<M,T,ES,NS,SP,F,R,L>)>::read
             at /Users/bonomat/.cargo/git/checkouts/rust-lightning-bee3233422fa4387/b13daab/lightning/src/ln/channelmanager.rs:8878:19
  15: ln_dlc_node::node::channel_manager::build
             at ./crates/ln-dlc-node/src/node/channel_manager.rs:98:27
  16: ln_dlc_node::node::Node<S>::new
             at ./crates/ln-dlc-node/src/node/mod.rs:340:31
  17: coordinator::main::{{closure}}
             at ./coordinator/src/bin/coordinator.rs:106:25
  18: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tracing-0.1.37/src/instrument.rs:272:9
  19: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/park.rs:283:63
  20: tokio::runtime::coop::with_budget
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/coop.rs:107:5
  21: tokio::runtime::coop::budget
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/coop.rs:73:5
  22: tokio::runtime::park::CachedParkThread::block_on
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/park.rs:283:31
  23: tokio::runtime::context::BlockingRegionGuard::block_on
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/context.rs:315:13
  24: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/scheduler/multi_thread/mod.rs:66:9
  25: tokio::runtime::runtime::Runtime::block_on
             at /Users/bonomat/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.28.2/src/runtime/runtime.rs:304:45
  26: coordinator::main
             at ./coordinator/src/bin/coordinator.rs:320:5
  27: core::ops::function::FnOnce::call_once
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/ops/function.rs:250:5
  28: std::sys_common::backtrace::__rust_begin_short_backtrace
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/sys_common/backtrace.rs:134:18
  29: std::rt::lang_start::{{closure}}
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/rt.rs:166:18
  30: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/ops/function.rs:287:13
  31: std::panicking::try::do_call
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:485:40
  32: std::panicking::try
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:449:19
  33: std::panic::catch_unwind
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panic.rs:140:14
  34: std::rt::lang_start_internal::{{closure}}
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/rt.rs:148:48
  35: std::panicking::try::do_call
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:485:40
  36: std::panicking::try
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:449:19
  37: std::panic::catch_unwind
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panic.rs:140:14
  38: std::rt::lang_start_internal
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/rt.rs:148:20
  39: std::rt::lang_start
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/rt.rs:165:17
  40: _main

[1]    90478 abort      cargo run --bin coordinator

error comes from here:

https://github.com/p2pderivatives/rust-lightning/blob/b13daab906b04efb50a2e467c0299ed6ea81b058/lightning/src/ln/channelmanager.rs#L8878-L8880

@luckysori
Copy link
Contributor

luckysori commented Nov 2, 2023

I did a full wipe of all cached dependencies, data and docker and now I get a new error 😅

Did you check out the right commit? That's a problem that was solved with p2pderivatives/rust-lightning@a57281b for me.

@bonomat
Copy link
Contributor Author

bonomat commented Nov 3, 2023

Mhm, thanks... I suspect I became the unsuspecting victim of cosmic radiation, which decided to play a little prank by flipping a few bits. This cosmic shenanigan led to the birth of a version control monster! This sneaky creature was a master at hide and seek and thwarted all my attempts to reset the git branch against the remote. But fear not, for after a good old-fashioned restart of my machine, I finally managed to outwit and defeat the cosmic trickster, and now we're on the path to world peace, one git commit at a time!

@luckysori
Copy link
Contributor

Mhm, thanks... I suspect I became the unsuspecting victim of cosmic radiation, which decided to play a little prank by flipping a few bits. This cosmic shenanigan led to the birth of a version control monster! This sneaky creature was a master at hide and seek and thwarted all my attempts to reset the git branch against the remote. But fear not, for after a good old-fashioned restart of my machine, I finally managed to outwit and defeat the cosmic trickster, and now we're on the path to world peace, one git commit at a time!

Hahaha, I knew we would get there eventually!

Only 15 seconds was not always enough. Giving it a minute seems to
always work locally.
@luckysori luckysori force-pushed the chore/upgrade-to-rust-lightning-116 branch from 32a9226 to 8ef9415 Compare November 3, 2023 10:23
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

Nice.

Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

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

Works for me!

luckysori and others added 2 commits November 3, 2023 22:07
Some notes:

- We've had to increase the payment amounts in the `multi_hop_payment`
and `just_in_time_channel_with_multiple_payments` tests, as otherwise
we would run into `route not found` errors. It's probably worth
investigating why this is the case.

- We have decided to eagerly send `rust-dlc` messages because: (1)
after the upgrade they were not being automatically sent as fast as
they used to and (2) we can speed up the protocols this way anyway.

Co-authored-by: Philipp Hoenisch <philipp@hoenisch.at>
The same `forwarding_fee_proportional_millionths` field already exists
in `LnDlcNodeSettings`.
@luckysori luckysori force-pushed the chore/upgrade-to-rust-lightning-116 branch from 8ef9415 to 247a892 Compare November 3, 2023 11:07
@luckysori luckysori added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit f553126 Nov 3, 2023
8 checks passed
@luckysori luckysori deleted the chore/upgrade-to-rust-lightning-116 branch November 3, 2023 11:57
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.

Upgrade rust-dlc (rust-lightning 116) Upgrade rust-lightning to 0.0.116
3 participants