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

Fix crash when trying to forget a channel with HTLCs #987

Closed
wants to merge 3 commits into from

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Feb 12, 2018

Reported by @sfmcnally in #946

Will likely need to adapted should #975 get in first.

Fixes #946

Will be used when forgetting peer as well.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Check if we have HTLCs, if yes force=true is required.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Signed-off-by: Christian Decker <decker.christian@gmail.com>
@rustyrussell
Copy link
Contributor

Yech :(

I'm pretty sure that this code will just destabalize the system, by unpairing existing HTLCs which are coming/going to other peers. We assume that doesn't happen as the entire system is designed to avoid that.

IOW, dev-forgetpeer was much harder than we thought. It needs to fail the corresponding incoming HTLCs for every outgoing HTLC we just deleted, and similarly make sure we now handle the replies for outgoing HTLCs which now don't have a matching incoming...

@cdecker
Copy link
Member Author

cdecker commented Feb 13, 2018

That's the reason to have it being forced, we definitely don't want to have this used in production, it's meant for testing only.

That being said, with #975 it should become much less important to clear a channel since a dead channel no longer prevents you from opening a new one.

@rustyrussell
Copy link
Contributor

No, the user is explicitly using it for non-testing!

We need to fix the real problem, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants