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.

Sign up for GitHub

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

Jump to bottom

Shutdown and UpdateFee should not be intertwined #1661

Merged
merged 2 commits into from
Feb 15, 2021
Merged

Shutdown and UpdateFee should not be intertwined #1661

merged 2 commits into from
Feb 15, 2021

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Dec 30, 2020 •
edited
Loading

If a plugin (or a very reactive user) tries to mutual-close a channel right when our peer reconnects, we can end up with the following message flow:

UpdateFee ----->
Shutdown  ----->
<-----  Shutdown
CommigSig ----->

Which was not properly handled and ended up in a locked state between shutdown and negotiating.

Just like we don't initiate a mutual close while we have unsigned outgoing htlcs, we should not initiate a mutual close if we have a pending unsigned fee update. We'll be signing it almost immediately, and the CMD_CLOSE can be replayed after a small delay.

We also now correctly handle the transition to the negotiating state in case our peer sends us messages in this order.

@t-bast t-bast requested a review from pm47 December 30, 2020 15:39
pm47
pm47 reviewed Jan 19, 2021
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

I think there is also this scenario, where the fundee initiates the close:

UpdateFee ----->
<-----  Shutdown
Shutdown  ----->
CommigSig ----->

@t-bast
Copy link
Member Author

t-bast commented Jan 19, 2021

I'll add a test case for that, I believe it will be covered by the code change already.

@t-bast
Copy link
Member Author

t-bast commented Jan 20, 2021

I think there is also this scenario, where the fundee initiates the close

Done in 838d2d4
It showed that the channel code was not handling this correctly.
It's a subtle change, I'm not 100% sure I got it right.

pm47
pm47 reviewed Feb 12, 2021
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

The UpdateFee handler should be removed in state SHUTDOWN I think, because IIUC the assumption is now that new changes cannot be added in that state (we treat UpdateFee exactly like UpdateAddHtlc) . It looks like an issue at the spec level.

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated Show resolved Hide resolved
@t-bast
Copy link
Member Author

t-bast commented Feb 12, 2021

The UpdateFee handler should be removed in state SHUTDOWN I think, because IIUC the assumption is now that new changes cannot be added in that state

I think you're right. To avoid being too strict with senders, I changed our behavior to stop emitting update_fee but allow receiving them in 200c2c0, let me know what you think.

@pm47
Copy link
Member

pm47 commented Feb 12, 2021

The UpdateFee handler should be removed in state SHUTDOWN I think, because IIUC the assumption is now that new changes cannot be added in that state

I think you're right. To avoid being too strict with senders, I changed our behavior to stop emitting update_fee but allow receiving them in 200c2c0, let me know what you think.

I think it works but I'm a bit confused as to why. How can it work where the whole purpose of this PR was to precisely prevent that from happening.

@pm47 pm47 mentioned this pull request Feb 15, 2021
Closed
@t-bast
Handle intertwined update_fee and shutdown
240fb31 
We were not correctly handling this scenario and could end up stuck between
shutdown and negotiating.
@t-bast
Copy link
Member Author

t-bast commented Feb 15, 2021

I simplified the PR, it's much closer now to #1691 with additional tests.
I believe it makes sense to skip the CMD_UPDATE_FEE at reconnect when we're shutting down (in the SYNCING handler), we can always wait for the handler of currentBlockCount to update the fee without fear of a race condition.

@t-bast t-bast requested a review from pm47 February 15, 2021 16:50
@codecov-io
Copy link

Codecov Report

Merging #1661 (240fb31) into master (81f15aa) will increase coverage by 0.08%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #1661      +/-   ##
==========================================
+ Coverage   85.95%   86.04%   +0.08%     
==========================================
  Files         151      151              
  Lines       11474    11486      +12     
  Branches      495      483      -12     
==========================================
+ Hits         9863     9883      +20     
+ Misses       1611     1603       -8
Impacted Files Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 86.42% <85.71%> (+0.51%) ⬆️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 93.40% <100.00%> (+0.12%) ⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala 92.30% <0.00%> (-0.61%) ⬇️
...r/acinq/eclair/payment/send/PaymentLifecycle.scala 92.85% <0.00%> (-0.48%) ⬇️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 80.25% <0.00%> (-0.26%) ⬇️
...clair/payment/send/MultiPartPaymentLifecycle.scala 95.10% <0.00%> (-0.14%) ⬇️
...ore/src/main/scala/fr/acinq/eclair/PimpKamon.scala 100.00% <0.00%> (ø)
...main/scala/fr/acinq/eclair/crypto/Monitoring.scala 100.00% <0.00%> (ø)
...la/fr/acinq/eclair/wire/ReplyChannelRangeTlv.scala 100.00% <0.00%> (ø)
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala 86.36% <0.00%> (ø)
... and 15 more

pm47
pm47 reviewed Feb 15, 2021
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

Looking good! Just an OCD nit

eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/ChannelExceptions.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala Outdated Show resolved Hide resolved
@t-bast
Copy link
Member Author

t-bast commented Feb 15, 2021

Done, I added Outgoing in 44db5a5

@t-bast t-bast merged commit 36e8c05 into master Feb 15, 2021
@t-bast t-bast deleted the shutdown-update-fee branch February 15, 2021 17:33
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Feb 23, 2021
@t-bast
Handle intertwined Shutdowb and UpdateFee
92f38b5 
We were not correctly handling intertwined `update_fee` and `shutdown`
and could end up stuck between shutdown and negotiating.

See ACINQ/eclair#1661 for additional details.
@t-bast t-bast mentioned this pull request Feb 23, 2021
Merged
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Feb 26, 2021
@t-bast
Handle intertwined Shutdowb and UpdateFee (#212)
fb6088c 
We were not correctly handling intertwined `update_fee` and `shutdown`
and could end up stuck between shutdown and negotiating.

See ACINQ/eclair#1661 for additional details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pm47 pm47 pm47 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t-bast @pm47 @codecov-io