crisdut commented 1 year ago

This PR completes all steps to open a channel between lnp-nodes, according to Bolt2, except a initial "refund tx" signature, because I will solve that in other PR.

Also, I will create another PR to fix the connect (bolt1) and open channel (bolt2) operations with other LN implementations because I found issues related a parsing messages.

Changes

Add ChanelUpdate event to force accept peer change the channel id.
Allow accept node load your keyset.
Allow electrum worker track and untrack transactions (only to "zero conf") [1]
Implements accept peer finish_accepted, finish_signed and finish_funded methods.
Review propose peer and accept peer lifecycles.

Dependencies

LNP-WG/lnp-core/pull/25
LNP-WG/lnp-core/pull/24

[1] The current rust-electrum-client does not get transaction with "verbose" informations. I will find a way to get the information. Details here

codecov-commenter commented 1 year ago

Codecov Report

Merging #69 (d5f28d6) into master (35f921e) will decrease coverage by 0.0%. The diff coverage is 0.0%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #69     +/-   ##
=======================================
- Coverage     0.1%   0.1%   -0.0%     
=======================================
  Files          41     41             
  Lines        3273   3355     +82     
=======================================
  Hits            3      3             
- Misses       3270   3352     +82

Impacted Files	Coverage Δ
src/bus/ctl.rs	`0.0% <0.0%> (ø)`
src/channeld/automata/accept.rs	`0.0% <0.0%> (ø)`
src/channeld/automata/mod.rs	`0.0% <0.0%> (ø)`
src/channeld/automata/propose.rs	`0.0% <0.0%> (ø)`
src/channeld/runtime.rs	`0.0% <0.0%> (ø)`
src/lnpd/runtime.rs	`0.0% <0.0%> (ø)`
src/routed/runtime.rs	`0.0% <0.0%> (ø)`
src/watchd/runtime.rs	`0.0% <0.0%> (ø)`
src/watchd/worker.rs	`0.0% <0.0%> (ø)`
src/opts.rs	`0.0% <0.0%> (ø)`
... and 19 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

dr-orlovsky commented 1 year ago

@crisdut looks interesting! Can you provide some comments which functionality should it add to the node specifically?

crisdut commented 1 year ago

@crisdut looks interesting! Can you provide some comments which functionality should it add to the node specifically?

Hi @dr-orlovsky,

Yes, sure.

For now, I'm adding the final steps to create a channel and correctly persist the state channel. After that, I will finish the track transactions worker on watchd daemon.

Also, I started updating the workflows (here) to help us to review the bolts implementations. I will split each workflow into two flows: internal and external. The first flow is how daemons interact with others along the channel events. The other flow is how the lnp node needs to exchange messages with other peers (independently of the node implementation).

Does it make sense to you?

BTW, I saw your comment here, and I was wondering if it's worth continuing to correct the code or is it better to wait for the re-architecture?

dr-orlovsky commented 1 year ago

I will split each workflow into two flows: internal and external. The first flow is how daemons interact with others along the channel events. The other flow is how the lnp node needs to exchange messages with other peers (independently of the node implementation).

Not sure how this can work, since these two workflows are not independent...

BTW, I saw your comment https://github.com/RGB-WG/rgb-node/discussions/238#discussioncomment-4718147, and I was wondering if it's worth continuing to correct the code or is it better to wait for the re-architecture?

With the new architecture the things which would change are not parts of the business logic; it is just that connections will run on the same thread (while today we have a thread per each remote peer connection). Watchd and channels will remain on an independent threads. So not sure whether it worth waiting for the new architecture, I think parts which you are doing shouldn't be affected by that much.

crisdut commented 1 year ago

With the new architecture the things which would change are not parts of the business logic; it is just that connections will run on the same thread (while today we have a thread per each remote peer connection). Watchd and channels will remain on an independent threads. So not sure whether it worth waiting for the new architecture, I think parts which you are doing shouldn't be affected by that much.

OK sure.

This week I have worked on this issue less than I'd liked. I'm finishing testing this first part. Next week, I'll update this branch again. Thanks for review.

crisdut commented 1 year ago

Hi @dr-orlovsky

I finished the PR, I updated description https://github.com/LNP-WG/lnp-node/pull/69#issue-1532958772

dr-orlovsky commented 1 year ago

Amplify with FlagVec fixes is release, as well as LNP Core (v0.9). I am planning to release v0.9 of LNP Node soon. Can you pls update the PR such that I can merge it before doing the release?

LNP-WG / lnp-node

Fix open channel management (Bolt 2) #69

Codecov Report