[input/size] Check tx size constants #4775

halseth · 2020-11-17T14:38:54Z

We ensure the size constants and their godoc checks out.

See lightning/bolts#815 for a disagreement with the spec that was found in the process.

halseth · 2020-11-18T11:05:49Z

Moving to v0.13, will cherry pick what is needed for sweeper changes.

bjarnemagnussen

Although not touched in this PR I fell over the size description for BaseCommitmentTxSize in line 158, which says "125 + 43 * num-htlc-outputs bytes". Maybe the description should be changed to "125 bytes", and instead the "43 * num-htlc-outputs" explained in a line beneath it, as it is not part of the size calculation for BaseCommitmentTxSize?

Similarly for BaseAnchorCommitmentTxSize in line 179

bjarnemagnussen · 2020-12-02T09:47:15Z

input/size.go

-	//              - OP_CSV: 1 byte	// HTLC script types. The size won't be correct in all cases,
-	//              - OP_DROP: 1 byte	// but it is just an upper bound used for fee estimation in any case.
+	//              - OP_1: 1 byte		// These 3 extra bytes are only
+	//              - OP_CSV: 1 byte	// present for the for the


Just a typo: uses two times "for the for the"

Fixed! 👍

bjarnemagnussen · 2020-12-02T09:47:35Z

input/size.go

-	//              - OP_CSV: 1 byte
-	//              - OP_DROP: 1 byte
+	//              - OP_1: 1 byte		// These 3 extra bytes are only
+	//              - OP_CSV: 1 byte	// present for the for the


Just a typo: uses two times "for the for the"

Fixed! 👍

Roasbeef

LGTM 🌸

Roasbeef · 2021-02-02T02:25:59Z

input/size.go

-	//
-	// TODO(halseth): the non-confirmed version currently includes the
-	// overhead.
-	AcceptedHtlcScriptSizeConfirmed = AcceptedHtlcScriptSize // + HtlcConfirmedScriptOverhead


Why was this commented out in the past again?

Ah nvm, I see the TODO now

Roasbeef · 2021-02-02T02:31:17Z

input/size.go

@@ -115,6 +115,7 @@ const (
 	//	- Sequence: 4 bytes
 	InputSize = 32 + 4 + 1 + 4

+	// FundingInputSize 41 bytes


Where's the script that was run? Should we consider adding it to the repo, or instead extending our tests to craft the actual transaction to compare to the estimate?

It was a pretty rough script I wrote, not sure if worth adding it since we shouldn't be changing these very often.

Posting for reference: halseth@d86684c

my vote would be to add the script and have it generate a test suite that verifies the actual size. we can fail the build if the tests are not properly committed in git or fail the size checks. I'd be okay with making that a follow up as well

Roasbeef · 2021-02-02T02:32:21Z

input/size_test.go

+}
+
+// TestWitnessSizes asserts the correctness of our magic tx size constants.
+func TestTxSizes(t *testing.T) {


👍

Scratch my other comment re extending the tests.

input/size_test.go

cfromknecht · 2021-02-17T16:06:23Z

input/size.go

@@ -115,6 +115,7 @@ const (
 	//	- Sequence: 4 bytes
 	InputSize = 32 + 4 + 1 + 4

+	// FundingInputSize 41 bytes


my vote would be to add the script and have it generate a test suite that verifies the actual size. we can fail the build if the tests are not properly committed in git or fail the size checks. I'd be okay with making that a follow up as well

Roasbeef · 2021-03-05T00:44:49Z

What's needed to move this forward?

halseth · 2021-03-05T09:57:08Z

What's needed to move this forward?

Travis 🤪

This to more easily track mismatches if constants and get more accurate fee estimates for the two channel types. The non-anchor weight estimates will now be smaller, this is okay since these constants are only being used for fee estimation (and will now be more accurate).

We run a script that ensures the constant sizes listed is actually the value of the constant.

Similar to what we do for witnesses, check that the HTLC weight constants check out. They actually do not, since the spec is off by one. We ensure we agree with the spec.

Roasbeef · 2021-03-10T00:21:20Z

@halseth looks fine to me?

Have you review comments been addressed @cfromknecht ?

cfromknecht

LGTM 🏅

halseth requested a review from cfromknecht November 17, 2020 14:38

halseth requested a review from Roasbeef as a code owner November 17, 2020 14:38

halseth force-pushed the input-tx-size-constants branch from c1544d2 to 5febf58 Compare November 17, 2020 14:45

halseth modified the milestones: 0.12.0, 0.13.0 Nov 17, 2020

halseth force-pushed the input-tx-size-constants branch 3 times, most recently from ccb8d5a to 6121926 Compare November 18, 2020 11:48

This was referenced Nov 23, 2020

[anchors] HTLC second level aggregation in the sweeper #4779

Merged

watchtower: anchor channel support #4782

Merged

bjarnemagnussen reviewed Dec 2, 2020

View reviewed changes

Roasbeef added bug fix fees Related to the fees paid for transactions (both LN and funding/commitment transactions) P1 MUST be fixed or reviewed labels Jan 28, 2021

halseth force-pushed the input-tx-size-constants branch 4 times, most recently from 32f9bd4 to 6dd5bfe Compare February 1, 2021 13:21

Roasbeef approved these changes Feb 2, 2021

View reviewed changes

Roasbeef requested review from cfromknecht and removed request for cfromknecht February 9, 2021 03:31

halseth force-pushed the input-tx-size-constants branch 2 times, most recently from 3720f15 to fc104fd Compare February 11, 2021 14:16

cfromknecht reviewed Feb 17, 2021

View reviewed changes

cfromknecht added the v0.13 label Feb 18, 2021

halseth force-pushed the input-tx-size-constants branch from fc104fd to 9f3d7d8 Compare February 25, 2021 11:45

halseth requested a review from cfromknecht February 25, 2021 11:45

halseth added 4 commits March 5, 2021 10:58

input/size: fix comments for constants

3a30763

We run a script that ensures the constant sizes listed is actually the value of the constant.

lnwallet: export HTLC generating methods

2bc37db

input/size: add txSize test

d30aae4

Similar to what we do for witnesses, check that the HTLC weight constants check out. They actually do not, since the spec is off by one. We ensure we agree with the spec.

halseth force-pushed the input-tx-size-constants branch from 9f3d7d8 to d30aae4 Compare March 5, 2021 09:58

cfromknecht approved these changes Mar 10, 2021

View reviewed changes

cfromknecht merged commit 7e33548 into lightningnetwork:master Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[input/size] Check tx size constants #4775

[input/size] Check tx size constants #4775

halseth commented Nov 17, 2020

halseth commented Nov 18, 2020

bjarnemagnussen left a comment

bjarnemagnussen Dec 2, 2020

halseth Feb 1, 2021

bjarnemagnussen Dec 2, 2020

halseth Feb 1, 2021

Roasbeef left a comment

Roasbeef Feb 2, 2021

Roasbeef Feb 2, 2021

Roasbeef Feb 2, 2021

halseth Feb 11, 2021

cfromknecht Feb 17, 2021

Roasbeef Feb 2, 2021

cfromknecht Feb 17, 2021

Roasbeef commented Mar 5, 2021

halseth commented Mar 5, 2021

Roasbeef commented Mar 10, 2021

cfromknecht left a comment

[input/size] Check tx size constants #4775

[input/size] Check tx size constants #4775

Conversation

halseth commented Nov 17, 2020

halseth commented Nov 18, 2020

bjarnemagnussen 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

Roasbeef 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

Roasbeef commented Mar 5, 2021

halseth commented Mar 5, 2021

Roasbeef commented Mar 10, 2021

cfromknecht left a comment

Choose a reason for hiding this comment