Fixup fundchannel_cancel #3336
Fixup fundchannel_cancel #3336
Conversation
niftynei
force-pushed
the
nifty/fixup_channel_cancels
branch
from
844de0e
to
6867cd7
Compare
| l1.rpc.connect(l2.info['id'], 'localhost', l2.port) | ||
| l1.rpc.fundchannel_start(l2.info['id'], amount)['funding_address'] | ||
| assert l1.rpc.fundchannel_complete(l2.info['id'], prep['txid'], txout)['commitments_secured'] |
There was a problem hiding this comment.
This could do with a short description of what it tests. I guess i checks that we can still start and complete a fundchannel after cancelling?
There was a problem hiding this comment.
this is a bit of an API assertion, in that it checks that the field commitments_secured is returned and is set to true. if this API changes, this test will fail.
| @@ -1005,6 +1005,27 @@ def test_funding_external_wallet_corners(node_factory, bitcoind): | |||
| # We can cancel channel after fundchannel_complete | |||
| assert l1.rpc.fundchannel_cancel(l2.info['id'])['cancelled'] | |||
|
|
|||
| l1.rpc.connect(l2.info['id'], 'localhost', l2.port) | |||
| l1.rpc.fundchannel_start(l2.info['id'], amount)['funding_address'] | |||
| assert l1.rpc.fundchannel_complete(l2.info['id'], prep['txid'], txout)['commitments_secured'] | |||
There was a problem hiding this comment.
The assert also seems a bit strange, are you trying to check that there is a commitments_secured element in the result? In that case an assert 'commitments_secured' in l1.rpc.fundchannel_complete... seems more appropriate.
There was a problem hiding this comment.
Yeah, 'commitments_secured' is always true. Checking if 'commitments_secured' field exists is more reasonable.
There was a problem hiding this comment.
but if the commitments_secured is ever returned as false then this would not have been a successful fundchannel_complete.
There was a problem hiding this comment.
I suppose you could instead just add a comment before this like # A successful funding_complete will always have a commitments_secured that is true, otherwise it would have failed.?
| @@ -1005,6 +1005,27 @@ def test_funding_external_wallet_corners(node_factory, bitcoind): | |||
| # We can cancel channel after fundchannel_complete | |||
| assert l1.rpc.fundchannel_cancel(l2.info['id'])['cancelled'] | |||
|
|
|||
| l1.rpc.connect(l2.info['id'], 'localhost', l2.port) | |||
| l1.rpc.fundchannel_start(l2.info['id'], amount)['funding_address'] | |||
| assert l1.rpc.fundchannel_complete(l2.info['id'], prep['txid'], txout)['commitments_secured'] | |||
There was a problem hiding this comment.
Yeah, 'commitments_secured' is always true. Checking if 'commitments_secured' field exists is more reasonable.
niftynei
force-pushed
the
nifty/fixup_channel_cancels
branch
3 times, most recently
from
93009d7
to
522e8de
Compare
restrict fundchannel_cancel usage to only the opener side Changelog-Changed: Only the opener of a fundchannel can cancel the channel open with fundchannel_cancel
Break out a method for canceling a channel that will either loop through contacting the peer to tell them of the error or just directly cleans up if the peer is currently disconnected.
Use the new forget_channel method to cancel, which checks that peer is still connected before attempting to send message.
if the channel hasn't been locked in yet, allow for a 'hard' error to kill the channel
niftynei
force-pushed
the
nifty/fixup_channel_cancels
branch
from
14e4c5d
to
8d1d36a
Compare
|
Ack 8d1d36a |
There's a few small, subtle bugs currently in
fundchannel_cancel:fundchannel_cancelwhile waiting for lockin. it's debatable whether or not this is desirable. for now it seems prudent to remove the capability altogether.this PR addresses both of these issues. note that
forget_channelis currently exposed because in a later commit (in the dual funding branch) i'm using it elsewhere.