Shutdown Updates #233
Shutdown Updates #233
Conversation
|
Okay, will take time to review it tomorrow |
We always require that any changes to Channel state be committed immediately (within the same lock) so we should never have uncommitted changes which would prevent us from sending a Shutdown response.
TheBlueMatt
force-pushed
the
2018-10-shutdown-updates
branch
from
1283bd1 to
5125358
Compare
|
Should be good to review now. |
| // Spec says we should fail the connection, not the channel, but that's nonsense, there | ||
| // are plenty of reasons you may want to fail a channel pre-funding, and spec says you | ||
| // can do that via error message without getting a connection fail anyway... | ||
| return Err(ChannelError::Close("Peer sent shutdown pre-funding generation")); |
There was a problem hiding this comment.
"if it hasn't received a funding_signed (if it is a funder) or a funding_created (if it is a fundee): SHOULD fail the connection" if spec is nonsense and it's not mandatory we're free to just close channel maybe ?
There was a problem hiding this comment.
Yea well the spec is kinda out-of-date, in a few places it references "fail the connection" but thats from originally it assumed one channel per connection, but then they added support for more. So closing the channel should be more than sufficient.
| @@ -1233,6 +1233,9 @@ impl ChannelManager { | |||
|
|
|||
| /// Call this upon creation of a funding transaction for the given channel. | |||
| /// | |||
| /// Note that ALL inputs in the transaction pointed to by funding_txo MUST spend SegWit outputs | |||
| /// or your counterparty can steal your funds! | |||
There was a problem hiding this comment.
Hmm may we prevent that with a call to ChainWatchInterface to be sure that inputs are all is_p2wsh/is_p2wpkh true ?
There was a problem hiding this comment.
Sadly we're not watching it and registering it may be expensive. The other alternative is to require the transaction be provided to funding_generated instead of just the txid and index. Dunno if its worth worrying about too much, though, as long as its well-documented.
There was a problem hiding this comment.
Okay, even if would prefer trigger guard better than documentation but may add surety later, if user didn't get it
There was a problem hiding this comment.
We can change funding_generated to panic if the user forgets to do it, certainly, but is definitely orthogonal to the rest of the patchset, I just slipped in the comments cause I realized we were missing them.
|
Reviewed until 1993ec0, will finish tomorrow as first thing |
See lightning/bolts#499 for a bit more about the ambiguity we're addressing here. Also drop holding cell update_fee the same way we drop holding cell update_add_htlcs when sending shutdown, resolving a bug.
TheBlueMatt
force-pushed
the
2018-10-shutdown-updates
branch
from
5125358 to
182affc
Compare
| // If the update_add is completely bogus, the call will Err and we will close, | ||
| // but if we've sent a shutdown and they haven't acknowledged it yet, we just | ||
| // want to reject the new HTLC and fail it backwards instead of forwarding. | ||
| if let PendingHTLCStatus::Forward(PendingForwardHTLCInfo { incoming_shared_secret, .. }) = pending_forward_info { |
There was a problem hiding this comment.
Does this part is tested in new test ? not sure
There was a problem hiding this comment.
Comment it out, and I see test ln::channelmanager::tests::htlc_fail_async_shutdown ... FAILED
|
Okay, read again BOLT-2 on closing, seems good to me (but don't have the shutdown/closing_signed meddling with commitment edges cases well in head) |
This fixes a ton of bugs/implements missing behavior/etc in shutdown/closing_signed handling. Adds 5 new tests to channelmanager network test.