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.

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

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

Merged
merged 3 commits into from Jul 26, 2018

Conversation

Projects
None yet
3 participants
@cfromknecht
Copy link
Collaborator

cfromknecht commented Jul 25, 2018

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

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Jul 25, 2018

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

@cfromknecht cfromknecht force-pushed the cfromknecht:to-local-script-size branch 3 times, most recently from 0dc050a to c151078 Jul 25, 2018

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Jul 25, 2018

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 cfromknecht force-pushed the cfromknecht:to-local-script-size branch from 700ce26 to faf34a5 Jul 25, 2018

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

cfromknecht commented Jul 25, 2018

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 added some commits Jul 25, 2018

lnwallet/size: correct commit to-local and 2nd stage script sizes
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
utxonursery: use symmetric second level htlc witness size
This commit switches over the estimates for htlc success/timeout
witness sizes to use a symmetric variable, highlighting their
equivalence in size.

@cfromknecht cfromknecht force-pushed the cfromknecht:to-local-script-size branch from faf34a5 to d41d63a Jul 25, 2018

@halseth
Copy link
Collaborator

halseth left a comment

LGTM 👍

// - zero_length: 1 byte
// - witness_script_length: 1 byte
// - witness_script (to_local_script)
// - OP_CHECKSIG: 1 byte

This comment has been minimized.

@halseth

halseth Jul 25, 2018

Collaborator

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

This comment has been minimized.

@cfromknecht

cfromknecht Jul 25, 2018

Collaborator

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

This comment has been minimized.

@cfromknecht

cfromknecht Jul 25, 2018

Collaborator

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
Copy link
Member

Roasbeef left a comment

LGTM ☄️

@Roasbeef Roasbeef merged commit 8cd6eeb into lightningnetwork:master Jul 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 54.622%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment