[input/size] Check tx size constants#4775
[input/size] Check tx size constants#4775cfromknecht merged 4 commits intolightningnetwork:masterfrom
Conversation
c1544d2 to
5febf58
Compare
|
Moving to v0.13, will cherry pick what is needed for sweeper changes. |
ccb8d5a to
6121926
Compare
bjarnemagnussen
left a comment
There was a problem hiding this comment.
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
input/size.go
Outdated
There was a problem hiding this comment.
Just a typo: uses two times "for the for the"
input/size.go
Outdated
There was a problem hiding this comment.
Just a typo: uses two times "for the for the"
32f9bd4 to
6dd5bfe
Compare
input/size.go
Outdated
There was a problem hiding this comment.
Why was this commented out in the past again?
input/size.go
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
input/size_test.go
Outdated
There was a problem hiding this comment.
👍
Scratch my other comment re extending the tests.
3720f15 to
fc104fd
Compare
input/size.go
Outdated
There was a problem hiding this comment.
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
fc104fd to
9f3d7d8
Compare
|
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.
9f3d7d8 to
d30aae4
Compare
|
@halseth looks fine to me? Have you review comments been addressed @cfromknecht ? |
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.