Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lnwallet/size: correct commit to-local and 2nd stage script/witness sizes #1617

Merged
merged 3 commits into from Jul 26, 2018
Merged

lnwallet/size: correct commit to-local and 2nd stage script/witness sizes #1617

merged 3 commits into from Jul 26, 2018

Conversation

cfromknecht
Copy link
Contributor

@cfromknecht cfromknecht commented Jul 25, 2018 •
edited

In this commit, we correct our size estimates for to-local scripts,
which are used on the commitment transaction and the htlc
success/timeout transactions. There have been observed cases of
transactions getting stuck because our estimates were too low, and cause
the transactions to not be relayed.

Our previous estimate for the commitment to-local script was correct
though comment had outdated script. Though the estimate is greater
than the actual size, this has been updated with a proposed estimate
of 79 bytes.

This estimate makes the assumption that CSV delays will be at most
4 bytes when serialized. Since this value is expressed in relative block
heights, this should be more than sufficient for our needs, even though
the maximum possible size for the little-endian int64 is 9 bytes (plus
an OP_DATA).

The other correction is to use the ToLocalScriptSize as our estimate for
htlc timeout/success scripts, as they are the same script. Previously,
our estimate was derived from the proper script, though we were 6 bytes
shy of the new to-local estimate, since we counted the csv_delay as 1
byte, and missed some other OP_DATAs.

All derived estimates have been updating depending on the new and
improved ToLocalScriptSize estimate, and fix some estimates that did not
include the witness length in the estimate.

Finally, we correct some weight miscalculations in:

  • AcceptedHtlcTimeoutWitnessSize: missing data push lengths
  • OfferedHtlcSuccessWitnessSize: extra 73 byte sig, missing data push lengths
  • OfferedHtlcPenaltyWitnessSize: missing 33 byte pubkey

@cfromknecht
Copy link
Contributor Author

cfromknecht commented Jul 25, 2018 •
edited

This addresses the (believed) cause of #1610, tho on its own is not the fix for afflicted users

@cfromknecht cfromknecht force-pushed the to-local-script-size branch 3 times, most recently from 0dc050a to c151078 Compare July 25, 2018 04:31
@cfromknecht
Copy link
Contributor Author

fwiw, the spec estimates 8 bytes for the CSV delay, i can also change the PR to match. that being said, this does seem a place we could deviate if our policy on accepted CSV ranges is properly enforced

@cfromknecht
Copy link
Contributor Author

This fix was tested by setting defaultBitcoinStaticFeeRate = 1 (in sats/vbyte) and running the integration tests. As expected as per #1610, channel_force_closure fails due to the underestimated weight of the final sweep transaction. Applying the changes causes the test to pass again even with the low feerate.

In had to comment out the other tests because some of them check sweep balance, which is altered by changing the fee rate. The selected test suite was:

channel_force_closure                                                                                                     
test_multi-hop_htlc_local_force_close_immediate_expiry                                                                    
test_multi-hop_htlc_receiver_chain_claim                                                                                  
test_multi-hop_local_force_close_on-chain_htlc_timeout                                                                    
test_multi-hop_remote_force_close_on-chain_htlc_timeout                                                                   
test_multi-hop_htlc_local_chain_claim
test_multi-hop_htlc_remote_chain_claim
revoked_uncooperative_close_retribution
revoked_uncooperative_close_retribution_zero_value_remote_output
revoked_uncooperative_close_retribution_remote_hodl

Of these, the integration tests failed for:

  • channel_force_closure
  • hop_htlc_local_force_close_immediate_expiry
  • hop_htlc_receiver_chain_claim
  • test_multi-hop_local_force_close_on-chain_htlc_timeout
  • test_multi-hop_htlc_local_chain_claim

All of these now pass after correcting the size estimates.

@cfromknecht
lnwallet/size: correct commit to-local and 2nd stage script sizes
ee2f257 
In this commit, we correct our size estimates for to-local scripts,
which are used on the commitment transaction and the htlc
success/timeout transactions. There have been observed cases of
transactions getting stuck because our estimates were too low, and cause
the transactions to not be relayed.

Our previous estimate for the commitment to-local script was derived
from an older version of the script. Though the estimate is greater than
the actual size, this has been updated with the current estimate of 79
bytes.

This estimates makes the assumption that CSV delays will be at most
4 bytes when serialized. Since this value is expressed in relative block
heights, this should be more than sufficient for our needs, even though
the maximum possible size for the little-endian int64 is 9 bytes (plus
an OP_DATA).

The other correction is to use the ToLocalScriptSize as our estimate for
htlc timeout/success scripts, as they are the same script. Previously,
our estimate was derived from the proper script, though we were 6 bytes
shy of the new to-local estimate, since we counted the csv_delay as 1
byte, and missed some other OP_DATAs.

All derived estimates have been updating depending on the new and
improved ToLocalScriptSize estimate, and fix some estimates that did not
include the witness length in the estimate.

Finally, we correct some weight miscalculations in:
 - AcceptedHtlcTimeoutWitnessSize: missing data push lengths
 - OfferedHtlcSuccessWitnessSize: extra 73 byte sig, missing data push lengths
 - OfferedHtlcPenaltyWitnessSize: missing 33 byte pubkey
@cfromknecht
utxonursery: use symmetric second level htlc witness size
3815794 
This commit switches over the estimates for htlc success/timeout
witness sizes to use a symmetric variable, highlighting their
equivalence in size.
@cfromknecht
breacharbiter: use ToLocalPenaltyWitnessSize for stage 2 htlcs
d41d63a
@cfromknecht cfromknecht added HTLC fees Related to the fees paid for transactions (both LN and funding/commitment transactions) P1 MUST be fixed or reviewed needs review PR needs review by regular contributors safety General label for issues/PRs related to the safety of using the software labels Jul 25, 2018
halseth
halseth approved these changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

lnwallet/size.go
// - zero_length: 1 byte
// - witness_script_length: 1 byte
// - witness_script (to_local_script)
// - OP_CHECKSIG: 1 byte
Copy link
Contributor

Choose a reason for hiding this comment

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

DId the scripts change at some point here, or was it just always wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The size of this one is correct according to the spec, just the script in the comment is wrong. The other ones were actually incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec says 8 bytes for the csv, but in practice this is more like one or two bytes as it's in relative block height. The spec doesn't account for the OP_DATA as we do here, where we allocate 1 + 4 bytes for the CSV. The spec's value overestimates the total size, even without the OP_DATA. I'd be fine with making it 8 as well just to match the spec.

Roasbeef
Roasbeef approved these changes Jul 26, 2018
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM ☄️

@Roasbeef Roasbeef merged commit 8cd6eeb into lightningnetwork:master Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

@halseth halseth halseth approved these changes

Assignees
No one assigned
Labels
fees Related to the fees paid for transactions (both LN and funding/commitment transactions) HTLC needs review PR needs review by regular contributors P1 MUST be fixed or reviewed safety General label for issues/PRs related to the safety of using the software
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cfromknecht @Roasbeef @halseth