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

2760 #27

Merged
merged 1 commit into from
Jan 8, 2024
Merged

2760 #27

merged 1 commit into from
Jan 8, 2024

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Dec 6, 2023

Copy link

netlify bot commented Dec 6, 2023

Deploy Preview for courageous-frangipane-fab648 ready!

Name Link
🔨 Latest commit 3e9f89a
🔍 Latest deploy log https://app.netlify.com/sites/courageous-frangipane-fab648/deploys/65954e417b21f50008de697b
😎 Deploy Preview https://deploy-preview-27--courageous-frangipane-fab648.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dunxen
Copy link
Collaborator

dunxen commented Dec 6, 2023

Thanks, @jbesraa! Looking good so far. Did you want a little more time to flesh out the other questions on V2 establishment or do you need some assistance? I think we don't really need to focus on V1 vs V2 stuff here and rather dig deeper into the cause of the bug! :D

@jbesraa
Copy link
Contributor Author

jbesraa commented Dec 7, 2023

Thanks, @jbesraa! Looking good so far. Did you want a little more time to flesh out the other questions on V2 establishment or do you need some assistance? I think we don't really need to focus on V1 vs V2 stuff here and rather dig deeper into the cause of the bug! :D

Yea no problem.
I dont have other questions to add here really, so feel free to add yours if you have something in mind.

Are we planning to run this tomorrow?

Copy link
Collaborator

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Are we planning to run this tomorrow?

If you need some more time, then just let Jeff know! There is no Rush and just want to make sure you're ready and prepared :D


+-------+ +-------+
| |--(1)- open_channel ---->| |
| |<-(2)- accep_channel ----| |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| |<-(2)- accep_channel ----| |
| |<-(2)- accept_channel ----| |

## Notes
Creating a channel involves exchanging multiple messages with a
connected peer, which does not always end up successfully.
Specificly, in the current implemntation an infinite loop might occur
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Specificly, in the current implemntation an infinite loop might occur
Specifically, in the current implementation an infinite loop might occur

Comment on lines 14 to 17
Creating a channel involves exchanging multiple messages with a
connected peer, which does not always end up successfully.
Specificly, in the current implemntation an infinite loop might occur
in the funding step.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should mention that this bug resulted from lightningdevkit/rust-lightning#2077 specifically.

1. Did you review the PR? [Concept ACK, approach ACK, tested ACK, or NACK](https://github.com/lightningdevkit/rust-lightning/blob/master/CONTRIBUTING.md#peer-review)?
2. In the original code we have a loop inside `close_channel_internal`,
in which scenario it goes indefintly?
3. why are we able to drop `force_close_channel_with_peer` from
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
3. why are we able to drop `force_close_channel_with_peer` from
3. Why are we able to drop `force_close_channel_with_peer` from

Comment on lines 39 to 40
4. question about v2 channel establishemnt
5. question about v2 channel establishemnt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
4. question about v2 channel establishemnt
5. question about v2 channel establishemnt
4. What was the reasoning for promoting an `OutboundV1Channel` to a `Channel` before receiving `funding_signed` prior to this PR? Why do we no only do that promotion after receiving `funding_signed`?

@@ -0,0 +1,46 @@
---
layout: pr
date: 2023-00-00
Copy link
Collaborator

Choose a reason for hiding this comment

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

We still need a valid date here otherwise it won't compile :)

@dunxen
Copy link
Collaborator

dunxen commented Jan 8, 2024

LGTM if you're happy! I'll merge and then we can still make adjustments if needed. At least we can make the announcement today.

@dunxen dunxen merged commit b254a0d into lightningdevkit:main Jan 8, 2024
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.

2 participants