Add lncli command / RPC for manually setting channel state #5033

robot-dreams · 2021-02-16T22:28:41Z

Currently, there's no mechanism for manually setting the network state of a given channel. This PR adds an RPC endpoint to the routing sub-server, as well as a corresponding lncli command, so that users can manually update channel state.

Note that such updates only the channel state that's broadcast over the network (i.e. whether or not the lnwire.ChanUpdateDisabled flag is set). In particular, disabling a channel does NOT close it.

This change also interacts with the pre-existing mechanism for automtically enabling / disabling channels (e.g. when a peer disconnects or reconnects):

If a user manually disables a channel, then subsequent automatic requests to re-enable the channel will be ignored. The user has to manually re-enable the channel, or use the lncli setchannelstatus command to set the state back to auto.
If a user manually enables a channel, then subsequent automatic requests to re-enable the channel will be respected. Such requests usually arise from a channel being closed or a peer going offline; in these cases, we do not want to clutter the network with false information that the channel is still enabled.

Sample behavior:

$ <shut down peer>

[DBG] NANN: Marking channel(242...:0) pending-inactive
...
[INF] NANN: Announcing channel(242...:0) disabled [detected]

$ <restart peer>

[INF] NANN: Announcing channel(242...:0) enabled

$ lncli setchannelstatus --funding_txid=242... --output_index=0 --action=disable

[DBG] RRPC: SetChannelStatus called for channel(242...:0) with action DISABLE
[INF] NANN: Announcing channel(242...:0) disabled [requested]

$ <restart peer>

[INF] PEER: Channel(242...:0) was manually disabled, ignoring automatic enable request

Fixes: #4543

Roasbeef

Great first-time contribution!

I've done an initial pass of the diff, but plan to do another to really grok some of the state machine transition modifications. I think we can also test the return to auto management by closing a channel in the itest to ensure that triggers a disable to be sent.

contractcourt/chain_arbitrator.go

netann/chan_status_manager.go

lnrpc/routerrpc/router.proto

lnrpc/rpc_utils.go

lntest/itest/lnd_test.go

peer/brontide.go

robot-dreams · 2021-02-18T21:33:17Z

Thanks for taking a look! I've hopefully addressed your initial feedback.

Updating the itest according to your suggestions did reveal one additional subtlety though: the updatechanstatus commands only updates the policy on our end, not on the peer's end. Is this what we want, or should we re-think the approach to account for the peer's policy as well?

robot-dreams · 2021-02-18T22:41:12Z

Actually I think there might be a blocking issue here. Converting to draft until I confirm it's fixed...

robot-dreams · 2021-02-19T05:28:35Z

OK, my main concern was around the following issue.

Say Alice/Bob have a channel and Alice/Charlie have a channel. If we disable the Alice/Bob channel from Alice's end but then immediately send a payment Charlie -> Bob (i.e. before Alice broadcasts the ChannelUpdate message) then Charlie's attempt to route the payment via Charlie -> Alice -> Bob will still succeed.

Per discussion with @Roasbeef , we can proceed with the current PR and address the "eventual consistency" concern as a follow-up.

cfromknecht

@robot-dreams great work on this PR, I know a lot of people will be excited to have this feature! A+ commit structure as well, very easy to follow the changes

Say Alice/Bob have a channel and Alice/Charlie have a channel. If we disable the Alice/Bob channel from Alice's end but then immediately send a payment Charlie -> Bob (i.e. before Alice broadcasts the ChannelUpdate message) then Charlie's attempt to route the payment via Charlie -> Alice -> Bob will still succeed.

Per discussion with @Roasbeef , we can proceed with the current PR and address the "eventual consistency" concern as a follow-up.

Good observation here! Indeed we don't honor this in the switch currently, there have been some discussions in the past as to whether we treat the disable bit as more of a network signaling (aka you shouldn't try this channel) vs actually enforcing it. I can imagine cases where each mode might be useful, it could be worthwhile to explore adding a configuration-level toggle whether one wants to enforce "strict disables" or not.

I'd say this PR looks just about merge-ready, only a few nits from me. I really like the approach of adding the ability to return the status management back to auto, makes the API very clean 👍

cfromknecht · 2021-02-18T04:06:54Z

netann/chan_status_manager.go

-	switch curState.Status {
-
-	// Channel is already disabled, nothing to do.
-	case ChanStatusDisabled:


does this imply the entry for outpoint wasn't being cleaned up on channel close?

The entry was getting cleaned up for a cooperative close, since the channel would've started out in ChanStatusEnabled so we don't hit this case.

However, it was not getting cleaned up if the channel was already in ChanStatusDisabled (e.g. peer went offline for a while) and you do a force close.

More specifically (with extra debug logging for removing the channel from the map):

Before this PR

cooperative close:

[INF] NANN: Announcing channel(...) disabled [requested] [DBG] NANN: Removing channel(...) from map

peer goes down + force close disabled channel:

[INF] NANN: Announcing channel(...) disabled [detected] >>> force close doesn't remove channel from map

After this PR

cooperative close:

[INF] NANN: Announcing channel(...) disabled [requested] [DBG] NANN: Removing channel(...) from map

peer goes down + force close disabled channel:

[INF] NANN: Announcing channel(...) disabled [detected] [DBG] NANN: Removing channel(...) from map

Very cool, great catch! Always good to make ensure we clean up after ourselves.

peer/brontide.go

rpcserver.go

lnrpc/routerrpc/router_backend.go

robot-dreams · 2021-02-24T07:17:11Z

Thanks for the encouraging review and the helpful context!

Add a new boolean parameter without changing any existing functionality. The parameter will be used to indicate whether a request to update a channel's status was made manually by a user (currently always false).

Add new value for ChanStatus without changing any existing functionality. The new value indicates that a user manually requested disabling a channel.

Add boolean parameter for test functions without changing any existing functionality. All current tests set manual = false, but Future tests can set manual = true to test manual requests to update channel state.

If a channel was manually disabled, subsequent automatic / background requests to update that channel's state will be ignored. A RequestAuto call restores automatic / background state management and indicates that such requests should no longer be ignored.

Update router.proto and rest-annotations.yaml, recompile protos, add a stub implementation to routerrpc.Server.

Allow router RPC requests to call into the ChanStatusManager to manually update channel state.

This refactor-only change makes the GetChanPointFundingTxid helper function available from sub-systems outside of the root lnd package.

Update UpdateChanStatus stub implementation to call into exposed ChanStatusManager methods in RouterBackend.

The updatechanstatus command calls into the UpdateChanStatus RPC in the router server.

cfromknecht

LGTM 🔥

cfromknecht · 2021-02-25T21:37:24Z

lntest/itest/lnd_test.go

+	// update is propagated.
+	sendReq(alice, chanPoint, routerrpc.ChanStatusAction_DISABLE)
+	expectedPolicy.Disabled = true
+	waitForChannelUpdate(


neutrino integration test failed on this line with:

=== RUN TestLightningNetworkDaemon/18-of-72/neutrino/update_channel_status test_harness.go:88: Failed: (update channel status): exited with error: *errors.errorString did not receive channel update /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:1814 (0xe7afb2) waitForChannelUpdate: t.Fatalf("did not receive channel update") /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:5344 (0xe8ec6f) testUpdateChanStatus: waitForChannelUpdate( /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/test_harness.go:112 (0xe4c3ee) (*harnessTest).RunTestCase: testCase.test(h.lndHarness, h) /home/travis/gopath/src/github.com/lightningnetwork/lnd/lntest/itest/lnd_test.go:14885 (0xeef005) TestLightningNetworkDaemon.func4: ht.RunTestCase(testCase) /home/travis/.gimme/versions/go1.15.7.linux.amd64/src/testing/testing.go:1123 (0x51c2ef) tRunner: fn(t) /home/travis/.gimme/versions/go1.15.7.linux.amd64/src/runtime/asm_amd64.s:1374 (0x46f821) goexit: BYTE $0x90 // NOP

I think this related to another issue ( #4866) however, since the test passes locally for me. I think it's safe to ignore this for now and revisit once the other issue is fixed. I restarted the itest and it passed 👍

Roasbeef

LGTM 🧨

dskvr · 2023-06-09T09:07:33Z

The road to figure out how to update a channel status lands here, but the command has changed. So anyone looking

setchannelstatus -> updatechanstatus ... same arguments.

Roasbeef requested review from cfromknecht and Roasbeef February 17, 2021 01:43

Roasbeef requested changes Feb 18, 2021

View reviewed changes

robot-dreams force-pushed the set-channel-status branch from 4b4586e to 7427585 Compare February 18, 2021 21:18

robot-dreams marked this pull request as draft February 18, 2021 22:41

robot-dreams marked this pull request as ready for review February 19, 2021 05:28

cfromknecht reviewed Feb 23, 2021

View reviewed changes

robot-dreams force-pushed the set-channel-status branch from 7427585 to 70804ab Compare February 24, 2021 07:16

robot-dreams added 14 commits February 24, 2021 07:13

multi: add bool param to channel enable/disable methods

e9baf0e

Add a new boolean parameter without changing any existing functionality. The parameter will be used to indicate whether a request to update a channel's status was made manually by a user (currently always false).

netann: add ChanStatusManuallyDisabled state

c40d291

Add new value for ChanStatus without changing any existing functionality. The new value indicates that a user manually requested disabling a channel.

netann: update channel status transitions to handle manual requests

542c89a

netann: add bool param to chan_status_manager_test

dbabc2e

Add boolean parameter for test functions without changing any existing functionality. All current tests set manual = false, but Future tests can set manual = true to test manual requests to update channel state.

netann: add tests for manually enabling/disabling channels

e1709f2

netann: add test for RequestAuto

a259317

routerrpc: add UpdateChanStatus RPC to Router service

4e4f4bc

Update router.proto and rest-annotations.yaml, recompile protos, add a stub implementation to routerrpc.Server.

routerrpc: expose SetChannel* methods from Router backend

db76b97

Allow router RPC requests to call into the ChanStatusManager to manually update channel state.

multi: move GetChanPointFundingTxid from lnd to lnrpc

ce27962

This refactor-only change makes the GetChanPointFundingTxid helper function available from sub-systems outside of the root lnd package.

routerrpc: implement UpdateChanStatus RPC in router server

6d01f14

Update UpdateChanStatus stub implementation to call into exposed ChanStatusManager methods in RouterBackend.

lncli: add updatechanstatus command

19e9ed5

The updatechanstatus command calls into the UpdateChanStatus RPC in the router server.

brontide: log at debug, not error, when ignoring enable request

5a54d78

itest: add test for new UpdateChanStatus RPC

215bf63

robot-dreams force-pushed the set-channel-status branch from 70804ab to 215bf63 Compare February 24, 2021 15:16

cfromknecht approved these changes Mar 1, 2021

View reviewed changes

cfromknecht added channel management The management of the nodes channels discovery Peer and route discovery / whisper protocol related issues/PRs labels Mar 1, 2021

cfromknecht added rpc Related to the RPC interface v0.13 labels Mar 1, 2021

Roasbeef added this to the 0.13.0 milestone Mar 10, 2021

Roasbeef added this to In progress in v0.13.0-beta via automation Mar 10, 2021

v0.13.0-beta automation moved this from In progress to Reviewer approved Mar 10, 2021

Roasbeef approved these changes Mar 10, 2021

View reviewed changes

Roasbeef merged commit 65b0bbc into lightningnetwork:master Mar 10, 2021

v0.13.0-beta automation moved this from Reviewer approved to Done Mar 10, 2021

robot-dreams deleted the set-channel-status branch March 10, 2021 07:07

cfromknecht mentioned this pull request Apr 7, 2021

peer: more interfaces, refactor unit tests #5067

Closed

C-Otto mentioned this pull request May 8, 2021

Proposal: disable the channel from our side if local_balance == 0 #3717

Closed

Add lncli command / RPC for manually setting channel state #5033

Add lncli command / RPC for manually setting channel state #5033

robot-dreams commented Feb 16, 2021

Roasbeef left a comment

robot-dreams commented Feb 18, 2021

robot-dreams commented Feb 18, 2021

robot-dreams commented Feb 19, 2021

cfromknecht left a comment

cfromknecht Feb 18, 2021

robot-dreams Feb 24, 2021

cfromknecht Feb 25, 2021

robot-dreams commented Feb 24, 2021

cfromknecht left a comment

cfromknecht Feb 25, 2021

Roasbeef left a comment

dskvr commented Jun 9, 2023

Add lncli command / RPC for manually setting channel state #5033

Add lncli command / RPC for manually setting channel state #5033

Conversation

robot-dreams commented Feb 16, 2021

Roasbeef left a comment

Choose a reason for hiding this comment

robot-dreams commented Feb 18, 2021

robot-dreams commented Feb 18, 2021

robot-dreams commented Feb 19, 2021

cfromknecht left a comment

Choose a reason for hiding this comment

cfromknecht Feb 18, 2021

Choose a reason for hiding this comment

robot-dreams Feb 24, 2021

Choose a reason for hiding this comment

Before this PR

After this PR

cfromknecht Feb 25, 2021

Choose a reason for hiding this comment

robot-dreams commented Feb 24, 2021

cfromknecht left a comment

Choose a reason for hiding this comment

cfromknecht Feb 25, 2021

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

dskvr commented Jun 9, 2023