contractcourt: stop lnd when received witness is empty #7811

yyforyongyu · 2023-07-06T15:55:57Z

This is the first commit from #7615. Making a PR for it so affected users can apply the patch first.

This commit adds a length check in isPreimageSpend to make sure it won't ever panic and adds a unit test to verify it.

morehouse

LGTM with a couple nits.

morehouse · 2023-07-07T15:19:49Z

contractcourt/htlc_timeout_resolver_test.go

+
+	// witnessSpendLocal is the remote's witness used to spend the HTLC via
+	// the second stage success tx path on the remote commitment.
+	witnessSpendLocal := wire.TxWitness{


Shouldn't we call this witnessSpendRemote?

named it to witnessSpendSuccess instead. So what I want to differentiate here is not local vs remote but how the preimage is revealed - either via the second stage success tx, or a direct spend. And in the test case setup we then start paying attention to whether it's a local view or a remote view.

morehouse · 2023-07-07T15:20:35Z

contractcourt/htlc_timeout_resolver_test.go

+
+	// witnessSpendDirect is the remote's witness used to spend the HTLC
+	// via the preimage path on the local commitment.
+	witnessSpendDirect := wire.TxWitness{


How about witnessSpendLocal?

The direct spend can happen for both local and remote tho.

You are right in general the Preimage Spent can happen by us on the remote, but refering to the naming of the file htlc_timeout_resolver their are only two possibilities for the preimage spent. Either on the remote via the witnessSpendSuccess or on our local via the witnessSpendLocal ?

Updated the tests to more clearly express the testing objects.

looks great now!

ellemouton · 2023-07-10T07:30:40Z

contractcourt/htlc_timeout_resolver.go

+	// on the local commitment transaction for an outgoing HTLC that is
+	// swept on-chain by us with pre-image.


If it is the local commitment tx, and we are sweeping the pre-image path, then it must be an incoming HTLC. An outgoing htlc on our commitment tx can only be swept by us via the timeout path.

Crypt-iQ · 2023-07-10T13:59:25Z

I don't think this should be done, we should instead figure out why bitcoind isn't serving witness data and, if we're able to, refuse to startup. Also this skips potentially notifying on a preimage spend if the bitcoind were to later right itself via user running -reindex.

morehouse · 2023-07-10T14:26:47Z

I don't think this should be done, we should instead figure out why bitcoind isn't serving witness data and, if we're able to, refuse to startup. Also this skips potentially notifying on a preimage spend if the bitcoind were to later right itself via user running -reindex.

This PR seems valuable to me on its own -- why shouldn't we do a bounds check before accessing the array element? We already do the bounds check for the remote commitment.

Whether this PR fixes the root cause is orthogonal.

Crypt-iQ · 2023-07-10T17:33:22Z

This PR seems valuable to me on its own -- why shouldn't we do a bounds check before accessing the array element? We already do the bounds check for the remote commitment.

I think the PR is fine after we've fixed the issue. I'd prefer that before it's actually fixed, we fail loudly on missing witness data. Otherwise continuing rather than panic means a -reindex won't extract the preimage on-chain.

Whether this PR fixes the root cause is orthogonal.

I don't agree because if we fix this immediately, we may never fix the root cause as it'll fail silently

yyforyongyu · 2023-07-11T10:16:17Z

I think the PR is fine after we've fixed the issue. I'd prefer that before it's actually fixed, we fail loudly on missing witness data.

Yeah we should fix it, tho I don't think it can be fixed inside lnd. And I also don't think we should let it go silently. I've added an error log in the latest push, but don't think it's enough. If we are sure it's an issue from bitcoind, we need to panic there with some info like reindex your bitcoind since it's very critical to have a healthy chain backend.

lightninglabs-deploy · 2023-10-23T08:37:01Z

@Crypt-iQ: review reminder
@yyforyongyu, remember to re-request review from reviewers when ready

ziggie1984

Great work yy 🤝, had some minor nits. Gracefully shutting down LND when no witness data is present makes a lot of sense. Especially because we cannot guarantee the user setting -rpcserialversion .

ziggie1984 · 2023-10-27T21:35:38Z

contractcourt/htlc_timeout_resolver_test.go

+
+	// witnessSpendDirect is the remote's witness used to spend the HTLC
+	// via the preimage path on the local commitment.
+	witnessSpendDirect := wire.TxWitness{


You are right in general the Preimage Spent can happen by us on the remote, but refering to the naming of the file htlc_timeout_resolver their are only two possibilities for the preimage spent. Either on the remote via the witnessSpendSuccess or on our local via the witnessSpendLocal ?

contractcourt/htlc_timeout_resolver_test.go

chainntnfs/interface.go

chainntnfs/txnotifier.go

ziggie1984

Base-Branch can be changed to master.

LGTM 🚀

This commit adds a length check in `isPreimageSpend` to make sure it won't ever panic and adds a unit test to verify it.

This commit adds a check when receiving transactions from `BitcoindClient` so an error is returned when empty witness is found.

This commit uses `Criticalf` to gracefully stop `lnd` when the received witness data is empty.

ffranr mentioned this pull request Jul 6, 2023

[bug]: Channel Transaction has over 800 confirmations but still pending and LND node failure #7803

Closed

morehouse approved these changes Jul 7, 2023

View reviewed changes

ellemouton reviewed Jul 10, 2023

View reviewed changes

yyforyongyu force-pushed the fix-panix branch from 0229b08 to b62dd66 Compare July 11, 2023 10:09

yyforyongyu force-pushed the fix-panix branch from b62dd66 to 0e1f8eb Compare August 18, 2023 10:00

yyforyongyu changed the title ~~contractcourt: make sure isPreimageSpend won't panic~~ contractcourt: stop lnd when received witness is empty Aug 18, 2023

yyforyongyu requested a review from Crypt-iQ August 18, 2023 10:03

yyforyongyu force-pushed the fix-panix branch 4 times, most recently from 91cb102 to 90010c9 Compare August 25, 2023 00:43

yyforyongyu mentioned this pull request Aug 25, 2023

Start watching mempool in htlc outgoing contest resolver #7615

Open

yyforyongyu added security General label for issues/PRs related to the security of the software backend Related to the node backend software/interface (e.g. btcd, bitcoin-core) bitcoind Bitcoin Core backend P2 should be fixed if one has time labels Sep 19, 2023

yyforyongyu requested review from Crypt-iQ and removed request for Crypt-iQ October 9, 2023 07:11

yyforyongyu mentioned this pull request Oct 25, 2023

[bug]: Channel is stuck in pending state. #8028

Closed

ziggie1984 reviewed Oct 28, 2023

View reviewed changes

yyforyongyu force-pushed the fix-panix branch 2 times, most recently from a71591c to e94af84 Compare October 30, 2023 06:26

yyforyongyu changed the base branch from master to 0-18-staging October 30, 2023 06:26

yyforyongyu requested a review from ziggie1984 October 30, 2023 06:26

yyforyongyu force-pushed the fix-panix branch from e94af84 to b9bc021 Compare October 30, 2023 06:28

ziggie1984 approved these changes Oct 30, 2023

View reviewed changes

Roasbeef changed the base branch from 0-18-staging to master October 30, 2023 17:44

yyforyongyu added 4 commits October 31, 2023 01:44

contractcourt: make sure isPreimageSpend won't panic

971d3d8

This commit adds a length check in `isPreimageSpend` to make sure it won't ever panic and adds a unit test to verify it.

chainntnfs: return an error when witness stack is empty

28c3c83

This commit adds a check when receiving transactions from `BitcoindClient` so an error is returned when empty witness is found.

chainntnfs: stop lnd when received witness stack is empty

ee883c3

This commit uses `Criticalf` to gracefully stop `lnd` when the received witness data is empty.

docs: update release notes re empty witness data

28497e9

yyforyongyu force-pushed the fix-panix branch from b9bc021 to 28497e9 Compare October 30, 2023 17:45

Roasbeef enabled auto-merge October 30, 2023 17:49

Roasbeef disabled auto-merge October 31, 2023 22:17

Roasbeef added this to the v0.18.0 milestone Oct 31, 2023

Roasbeef merged commit eb5fc0c into lightningnetwork:master Oct 31, 2023
23 of 26 checks passed

yyforyongyu deleted the fix-panix branch November 1, 2023 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contractcourt: stop lnd when received witness is empty #7811

contractcourt: stop lnd when received witness is empty #7811

yyforyongyu commented Jul 6, 2023

morehouse left a comment

morehouse Jul 7, 2023

yyforyongyu Jul 11, 2023

morehouse Jul 7, 2023

yyforyongyu Aug 18, 2023

ziggie1984 Oct 27, 2023

yyforyongyu Oct 30, 2023

ziggie1984 Oct 30, 2023

ellemouton Jul 10, 2023

yyforyongyu Jul 11, 2023

Crypt-iQ commented Jul 10, 2023

morehouse commented Jul 10, 2023

Crypt-iQ commented Jul 10, 2023

yyforyongyu commented Jul 11, 2023

lightninglabs-deploy commented Oct 23, 2023

ziggie1984 left a comment

ziggie1984 Oct 27, 2023

ziggie1984 left a comment

		// on the local commitment transaction for an outgoing HTLC that is
		// swept on-chain by us with pre-image.

contractcourt: stop lnd when received witness is empty #7811

contractcourt: stop lnd when received witness is empty #7811

Conversation

yyforyongyu commented Jul 6, 2023

morehouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ commented Jul 10, 2023

morehouse commented Jul 10, 2023

Crypt-iQ commented Jul 10, 2023

yyforyongyu commented Jul 11, 2023

lightninglabs-deploy commented Oct 23, 2023

ziggie1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment