Skip to content

[anchor commitment] Make the anchor commitment type spec compliant#4558

Merged
cfromknecht merged 6 commits into
lightningnetwork:masterfrom
halseth:anchors-spec
Sep 11, 2020
Merged

[anchor commitment] Make the anchor commitment type spec compliant#4558
cfromknecht merged 6 commits into
lightningnetwork:masterfrom
halseth:anchors-spec

Conversation

@halseth
Copy link
Copy Markdown
Contributor

@halseth halseth commented Aug 21, 2020

This PR makes the necessary changes to the anchor commitment type to make it compliant with the final spec version as defined in lightning/bolts#688

There are two changes needed:

  1. We set the feature bit to 20/21. This means that we now require this bit to be advertised to signal anchor support. The old bit will be ignored by new nodes.
  2. The anchor amounts are given back to the initiator in a coop close scenario. This means that existing nodes having anchor channels will disagree on the final coop close fee if only one of the channel parties update. However if both update closing proceeds as normal. In the worst case force closing works as before.
  3. Anchors are no longer experimental.

NOTE: Watchtowers are still not supported for anchor commitment types, and anchor support is not advertised by default but must be enabled through the --protocol.anchors option.

@halseth halseth requested a review from Roasbeef as a code owner August 21, 2020 11:39
@halseth halseth changed the title [anchor commitment] Make the anchor commitment type spec [anchor commitment] Make the anchor commitment type spec compliant Aug 21, 2020
@halseth halseth added this to the 0.12.0 milestone Aug 21, 2020
@halseth halseth requested review from cfromknecht and joostjager and removed request for Roasbeef August 21, 2020 11:53
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first pass looks good

Comment thread lnwallet/commitment.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible underflow, here and below

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but I think it is safe to add the extra check: ca22db6

Comment thread lnwallet/commitment.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be simplified with +=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe coopCloseFee + commitFee + anchorValue can be extracted to prevent a future bug where only one is updated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted into initiatorDelta: a8ec11a

Comment thread lncfg/protocol.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preexisting: help message is a little misleading. it will work with watchtowers, but state updates for anchor channels won't be backed up. this should change before 0.12 though with anchor tower support in the works

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: 83fe313

Copy link
Copy Markdown
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested on regtest and inspected coop close tx. Anchor value is indeed added back to the initiator balance. Two questions:

  • Did you (manually) test coop closing an old-format anchor channel?
  • Is the itest coop closing with anchors anywhere?

Comment thread lnwallet/channel.go Outdated
Comment thread lnwallet/commitment.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe coopCloseFee + commitFee + anchorValue can be extracted to prevent a future bug where only one is updated

Comment thread lnwallet/commitment.go Outdated
@halseth
Copy link
Copy Markdown
Contributor Author

halseth commented Aug 24, 2020

  • Did you (manually) test coop closing an old-format anchor channel?

Do you mean one node running the updated code, one node running the old code? If yes, then I tried this in one of the integration tests, and closing expectedly failed.

  • Is the itest coop closing with anchors anywhere?

Yes, most tests using anchors are closing the channel at the end, most notably the basic funding flow test. Checks all combinations of channel types.

@cfromknecht
Copy link
Copy Markdown
Contributor

@halseth looks good, can squash now

Comment thread lnwallet/commitment.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make clear in a comment that this is a sanity check and not supposed to be hit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

narrator: it was hit

@halseth
Copy link
Copy Markdown
Contributor Author

halseth commented Aug 26, 2020

Squashed and rebased. Removed the commit making anchors non-experimental as discussed, such that they are still behind a build tag.

@halseth halseth modified the milestones: 0.12.0, 0.11.1 Aug 26, 2020
@cfromknecht
Copy link
Copy Markdown
Contributor

@halseth appears related to newly added underflow check

--- FAIL: TestCooperativeCloseDustAdherence (0.03s)
    channel_test.go:2291: unable to close channel: initiator cannot afford proposed coop close fee

@halseth
Copy link
Copy Markdown
Contributor Author

halseth commented Aug 28, 2020

@halseth appears related to newly added underflow check

--- FAIL: TestCooperativeCloseDustAdherence (0.03s)
    channel_test.go:2291: unable to close channel: initiator cannot afford proposed coop close fee

Fixed!

@cfromknecht
Copy link
Copy Markdown
Contributor

lnwallet/channel_test.go:2285:5: `Attemtping` is a misspelling of `Attempting` (misspell)
	// Attemtping to close with this fee now should fails, since Alice

@halseth
Copy link
Copy Markdown
Contributor Author

halseth commented Sep 3, 2020

@cfromknecht Should be good now.

Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🤓

To be spec compliant, we require the initiator to not pay the anchor
values into fees on coop close. We extract the balance calculation into
commitment.go, and add back the value of the anchors to the initiator's
balance.
Also modify the test to check for this condition.
@cfromknecht cfromknecht merged commit c704c57 into lightningnetwork:master Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants