multi: log additional information for local force closures by shaurya947 · Pull Request #7787 · lightningnetwork/lnd

shaurya947 · 2023-06-22T22:01:45Z

Change Description

Addresses #7696 by logging the reason for a local force close in the DB, and echoing it within the ClosedChannels response. Since initiating a force close and marking the channel as closed happen at two distinct parts of the code, we utilize the ChannelArbitrator log to persist the necessary information between these two moments in time. Once we mark the channel as closed, we log that info in the closed channels bucket. In this PR we address 3 possible reasons of local force closure: user-initiated, link failure, and HTLC/chainTrigger-related.

Steps to Test

itests have been updated to test for all 3 conditions, but manual testing can be done by simulating any of the 3 local force close situations and then checking the channel information in the ClosedChannels response.

Pull Request Checklist

Testing

Your PR passes all CI checks.
Tests covering the positive and negative (error paths) are included.
Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
Commits follow the Ideal Git Commit Structure.
Any new logging statements use an appropriate subsystem and logging level.
There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

A local force close could be due to one of the following reasons: - User initiated - Link failure (automatic) - On-chain HTLC conditions such as timeout (automatic) This commit adds a new struct that can encapsulate these different reasons, and logs that struct to the channel's ArbitratorLog.

In this commit we use the functional optional for specifying the local force close reason that we built in the previous commit. The option can be for user-initiated FC, or link failure or FC, or just logging the ChainActionMap in the case of chainTrigger.

During the local force close flow, at the time of actually marking the channel as closed, we need to fetch the local force close info from the channel's arbitrator log and write it into the persistent closed channels bucket in the DB. This commit achieves that by creating a new exported struct inside the channeldb package, populates the struct, and writes it to the DB.

We add a new proto message to populate the local force close info in the ClosedChannels response and generate the new RPC files. We also add the info to the response.

shaurya947 · 2023-06-28T16:58:25Z

Hey @saubyk this is mostly ready for review (I just need to investigate some failing tests) but I've fixed all the lint issues and added a release notes entry for 0.17.1 per the milestone you assigned. Let me know whom we can ask for reviews 🙂

saubyk · 2023-06-29T17:31:37Z

Hi @shaurya947 great work so far. thanks. Will start assigning reviewers when we start the dev cycle for 17.1
Excited to get this feature merged.

ziggie1984 · 2023-08-17T19:54:01Z

looking forward to this PR !

guggero

Great idea for approaching this quite complex task. Very nice work, looks pretty far along.
Main comments are around using TLV to serialize the force close info and to share the code between the arbitrator and the channel DB if possible.

guggero · 2023-09-06T12:23:28Z

contractcourt/briefcase.go

+		b := new(bytes.Buffer)
+
+		// first byte is userInitiated bool
+		err = binary.Write(b, endian, info.userInitiated)


We should use TLV for any new data structure that we save to the DB. That will make it way easier to extend this info structure as well. See the example in fetchChanInfo() in channeldb/channel.go.

guggero · 2023-09-06T12:23:48Z

contractcourt/briefcase.go

+			return err
+		}
+
+		// next bytes are actual map data


nit: all inline comments should also be full sentences with proper capitalization and punctuation.

guggero · 2023-09-06T12:25:16Z

contractcourt/chain_arbitrator.go

+func UserInitiatedForceClose(ca *ChannelArbitrator) {
+	// Ignore errors since logging force close info is not a critical part
+	// of the force close flow.
+	_ = ca.log.LogLocalForceCloseInfo(localForceCloseInfo{


Maybe we should at least log any errors?

guggero · 2023-09-06T12:26:17Z

contractcourt/chain_arbitrator.go

+// of ForceCloseContract(). We don't export this type directly, but rather the
+// concrete functions implementing this type, so that callers can choose the
+// concrete function that best describes their use case for force closing.
+type forceCloseContractOption func(*ChannelArbitrator)


Hmm, this doesn't look like the normal option pattern, as it's just a function type. Maybe just call it forceCloseContractCallback instead?

guggero · 2023-09-06T12:26:51Z

contractcourt/chain_arbitrator.go

 	}

 	log.Infof("Attempting to force close ChannelPoint(%v)", chanPoint)
+	opt(arbitrator)


Nice design with the callback 👍

guggero · 2023-09-06T12:30:13Z

contractcourt/channel_arbitrator.go

 		// next state, as we still need to broadcast the commitment
 		// transaction.
 		case chainTrigger:
+			// Ignore errors since logging force close info is not a


Add a named callback for this as well? Then we have all the code in one place.

guggero · 2023-09-06T12:34:24Z

channeldb/channel.go

+// this may be extended in the future. For example, we may decide to log HTLC
+// information that doesn't automatically result in a force close, but leaves
+// the channel in a state whereby the user has no option but to force close.
+type LocalForceCloseInfo struct {


Could we use this in the contractcourt directly? I see that the HtlcActions isn't exactly the same type, but maybe we could convert? Otherwise we need encode/decode code in two places.
If we share this struct, it can have a public Encode() and Decode() method.

guggero · 2023-09-06T12:35:53Z

channeldb/channel.go

 	}

+	// Write whether the local force close info is present.
+	if err := WriteElements(w, cs.LocalFCInfo != nil); err != nil {


This won't work without changes to the WriteElement and ReadElement functions in channeldb/codec.go.

Why do you say this? bool is handled by WriteElement.

Oh, I misread it as just WriteElements(w, cs.LocalFCInfo), missed the != nil part. So maybe extract to a bool to make it more obvious?

That is the general pattern of writing it looking at the function.

guggero · 2023-09-06T12:38:09Z

rpcserver.go

+		for action, htlcs := range localFCInfo.HtlcActions {
+			htlcHashes := make([][]byte, len(htlcs))
+			for i, htlc := range htlcs {
+				htlcHashes[i] = htlc.RHash[:]


We need to use copy() here to avoid running into the famous golang loop variable issues.

guggero · 2023-09-06T12:39:49Z

lnrpc/lightning.proto


+message ListHTLCHash {
+    // Hash of HTLC
+    repeated bytes hash = 1;


nit: should be plural since it's a slice of hashes.

Chinwendu20 · 2023-09-11T15:48:41Z

Thanks @guggero for the review as per @saubyk message on slack, I will work on the PR to implement your suggestions and submit an update soon.

saubyk · 2023-09-11T16:55:40Z

Thanks @guggero for the review as per @saubyk message on slack, I will work on the PR to implement your suggestions and submit an update soon.

Thanks @Chinwendu20

Chinwendu20 · 2023-09-13T11:58:27Z

contractcourt/briefcase.go

+
+	// htlcActions is a non-empty map iff the force close was automatically
+	// initiated by an on-chain trigger such as HTLC timeout.
+	htlcActions ChainActionMap


Hello @guggero would be a good idea to just indicate that the closure was due to ahtlcAction and we just put the particular htlc action responsible for the closure. Would there be need for the actual htlc?

I guess it would be nice to be able to correlate the closure resulting from an HTLC with the actual HTLC in the channel (to see why it timed out, what its CLTV delta was and so on).

Okay thanks.

lightninglabs-deploy · 2023-10-04T15:35:44Z

@shaurya947, remember to re-request review from reviewers when ready

Chinwendu20 · 2023-10-09T05:59:19Z

Continued here: #8072

guggero · 2023-10-09T07:37:26Z

Replaced by #8072.

shaurya947 mentioned this pull request Jun 22, 2023

[feature]: Store the channel force close error message in the closed channels database #7696

Closed

saubyk linked an issue Jun 22, 2023 that may be closed by this pull request

[feature]: Store the channel force close error message in the closed channels database #7696

Closed

saubyk added this to the v0.17.1 milestone Jun 22, 2023

shaurya947 added 5 commits June 28, 2023 12:29

rpc: add local force close info to protobuf for ClosedChannels

3a29f47

We add a new proto message to populate the local force close info in the ClosedChannels response and generate the new RPC files. We also add the info to the response.

itest: update force close itests for testing local force close reason

08ca9fc

shaurya947 force-pushed the force-close-reason branch from 1c67ef3 to 08ca9fc Compare June 28, 2023 16:48

docs: add release notes for 0.17.1

2098b36

shaurya947 marked this pull request as ready for review June 28, 2023 16:58

saubyk modified the milestones: High Priority, v0.18.0 Aug 4, 2023

saubyk assigned shaurya947 Aug 8, 2023

guggero self-requested a review August 18, 2023 07:41

guggero requested changes Sep 6, 2023

View reviewed changes

Chinwendu20 reviewed Sep 13, 2023

View reviewed changes

Chinwendu20 mentioned this pull request Oct 7, 2023

multi: log additional information for local force closures #8072

Closed

guggero closed this Oct 9, 2023

saubyk removed a link to an issue Oct 10, 2023

[feature]: Store the channel force close error message in the closed channels database #7696

Closed

Comments

Conversation

shaurya947 commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Steps to Test

Pull Request Checklist

Testing

Code Style and Documentation

Uh oh!

shaurya947 commented Jun 28, 2023

Uh oh!

saubyk commented Jun 29, 2023

Uh oh!

ziggie1984 commented Aug 17, 2023

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chinwendu20 commented Sep 11, 2023

Uh oh!

saubyk commented Sep 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Oct 4, 2023

Uh oh!

Chinwendu20 commented Oct 9, 2023

Uh oh!

guggero commented Oct 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shaurya947 commented Jun 22, 2023 •

edited

Loading