Skip to content

Require feature option_shutdown_anysegwit #1018

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

Closed

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jul 28, 2021

Fixes #1006.

TODO: Fix full_stack fuzz test failure likely caused by:

Invalid funding_created signature from peer

@TheBlueMatt
Copy link
Collaborator

Needs rebase on keysend.

@jkczyz jkczyz force-pushed the 2021-07-shutdown-anysegwit branch from 8d84e15 to 1e27816 Compare July 28, 2021 22:00
@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #1018 (8d84e15) into main (f3b63e4) will decrease coverage by 0.07%.
The diff coverage is 84.21%.

❗ Current head 8d84e15 differs from pull request most recent head 1e27816. Consider uploading reports for the commit 1e27816 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
- Coverage   90.79%   90.72%   -0.08%     
==========================================
  Files          60       60              
  Lines       31434    31745     +311     
==========================================
+ Hits        28542    28802     +260     
- Misses       2892     2943      +51     
Impacted Files Coverage Δ
lightning/src/ln/peer_handler.rs 49.69% <50.00%> (+3.70%) ⬆️
lightning/src/ln/functional_tests.rs 97.31% <92.30%> (+0.01%) ⬆️
lightning/src/ln/features.rs 99.60% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b63e4...1e27816. Read the comment docs.

@TheBlueMatt
Copy link
Collaborator

Grrrrrr, I'm more than a little bit pissed about this, but it looks like maybe lnd doesn't support shutdown_anysegwit? At least my immediate peers don't seem to support it. I've asked on their slack but its a bit disappointing that a 2-year-old spec change isn't supported. It's quite annoying, but I guess we could simply document this and then check if the peer supports anysegwit when we open the channel?

@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 29, 2021

Grrrrrr, I'm more than a little bit pissed about this, but it looks like maybe lnd doesn't support shutdown_anysegwit? At least my immediate peers don't seem to support it. I've asked on their slack but its a bit disappointing that a 2-year-old spec change isn't supported. It's quite annoying, but I guess we could simply document this and then check if the peer supports anysegwit when we open the channel?

Hmmm... looks like it was only merged into the spec back on May 24 of this year. Could check at channel open and would also need to check at close after #1019.

@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 29, 2021

Though it would make our interfaces more complicated if the validity of the user's shutdown script provided by KeysInterface depended on the counterparty's features.

@TheBlueMatt
Copy link
Collaborator

Hmmm... looks like it was only merged into the spec back on May 24 of this year

It's been around for much longer, though. At least it's super trivial to implement, too.

Though it would make our interfaces more complicated if the validity of the user's shutdown script provided by KeysInterface depended on the counterparty's features.

I think we should just force-close in that case and document it. Hopefully eventually lnd will implement it and until then warning users is fine I think.

@TheBlueMatt
Copy link
Collaborator

#1019 is going ahead without requiring anysegwit. I don't think there's a reason to require it any time soon.

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.

Require option_anysegwit/Adapt API
2 participants