Use 48-bit commitment transaction numbers

[cjamthagen]

TODO: fix deriveStateHintObsfucator to set correct obsfucator for the channel. According to the spec the payment-basepoints from open_channel and accept_channel are needed, and they don't seem to be implemented yet.

Since I am pretty new to writing Go, please let me know if I have done something wrong.

[Roasbeef]

Nice! I was literally just thinking about making this change the other day.

This looks pretty good so far, however the build is currently failing since the change as it stands is incomplete:

• The lnwire.SingleFundingComplete message needs to be altered to accept a 6-byte obfuscator rather than a 4-byte one.

With that change the Travis should be able to successfully build this PR and run all the tests.

You're correct that the current funding workflow in the daemon doesn't yet match the workflow as outlined within the spec. A few pieces are currently being assembled towards adherence in that area which is why I added the StateHintObsfucator field to hold us over until our funding workflow was fully compliant.

One final thing that would be great would be to modify the add an additional set of tests which we can use to generate test-vectors for the spec. The additional set of tests would be in the form of table driven tests. Such a form will allow us to easily add more cases in the future and also easily translate our set of tests to test vectors which the rest of the implementers can use.

[Roasbeef]

We can avoid a heap allocation here by declaring the slice as a fixed-sized array like so:

var obfs [8]byte

[Roasbeef]

Same comment here as above about using a fixed-sized array instead of a dynamically allocated slice.

[cjamthagen]

Fixed.

Question: Is there a specific way to rebase to master that is preferred?

Will work on the tests next.

[Roasbeef]

Great, thanks!

I've answered your question about rebasing in this comment on your other PR.

[cjamthagen]

Table driven tests added.

[cjamthagen]

It seems like the locktime is the culprit for not passing the 'channel force closure' integration test.

The lower 24 bits of the locktime represents the lower 24 bits of the obscured commitment transaction number. If the high byte of locktime is always zero, then the locktime value can only assume a maximum value of 16,777,215, which is below 500,000,000, meaning the locktime will always be compared to blockheight and not MTP, if enabled. Similarly the sequence number will never take the value maxInt (0xFFFFFFFF) because the high byte will always be 0x80. This means that the locktime will not be disabled for the commit transaction, and the commit transaction will not be eligible for inclusion in a block until it has the proper height as constrained by its locktime.

If this is the case, then the locktime cannot be used as defined in the specification to hold the obscured commitment transaction number, because it would risk rendering the commitment transaction unspendable in many cases, only passing as spendable in 2-3% of the time on the mainchain.

Let me know I got anything wrong.

Edit: Although, shouldn't transactions which can not be included due to locktime be rejected from the mempool? During the 'channel force closure' test, the commitment transaction is in the mempool.

Edit2: Looks like SimNetParams sets RelayNonStdTxs to true, so the commitment transaction can be included in the mempool. Right?

Edit3: Changing RelayNonStdTxs to false will still include the commitment transaction.

[cjamthagen]

Alright, setting the 30th bit (1<<29) of the locktime fixes the problem and allows the test to pass. I'll await some comments and then propose an update to BOLT 3.

[Roasbeef]

Nice work! I've completed a final pass of the code and have also done a bit of testing locally. Thanks for expanding the unit tests to exercise those additional cases I missed the first time around when.

There's a set of test vectors pending within the spec that should take care of ensuring the precise algorithm to encoding/decoding the state hints are detailed within the spec.

LGTM!

Two things are currently blocking me merging this PR:

1. These two commits should be squashed together:
   • 5f42ff94c30f7801d93a896e37c19313abddd66e and f6bced9a94e51584abfe2a116de3a1164c35c0ec
2. I'll need to first merge in the PR which adds the init message. If I merge this one first, then nodes will on either side of the latest commits (in master) won't be able to create channels with each other as the initial commitment sigs will fail validation.
   • Temporarily, I'll modify the local feature bits to abort channel creation if bit for "48-bit state hints" isn't set. Within an upcoming major version the temporary local feature bits we add will be eliminated and instead aligned with the bits as specified within the spec.

[Roasbeef]

This commit (175112af510ca5888684c68b760bb6fdbf7e66e1) should also be amended to remove the case for in readElement/writeElement for a [4]byte as it was only used for the 24-bit state hints which have been deprecated by this PR.

[Roasbeef]

Minor nit: there should be a space before the comment here and the comment should be expanded to a complete sentence.

[cjamthagen]

Fixed. I'll take a look at the test vectors in the spec and make sure they get added.

[Roasbeef]

Nice work! This PR inches us a bit closer to having full compatibility with the current specification draft.

This PR now conflicts with master, I think if you remove the last commit all together, then the conflict will go away. The change itself is incomplete, as it doesn't modify the ChannelDelta usage within the database.

Will take another run through after the last commit is removed, likely stamping this with an "LGTM" and getting it merged finally :)

[cjamthagen]

Done. What ChannelDelta usage needed modification?

[Roasbeef]

@cjamthagen this modification: db8c6d4

[Roasbeef]

LGTM 🎉