0.0.11 #608
Merged
0.0.11 #608
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As channel_value last usage was for computing feerate but as this one is static per-commitment and will always-be following specification, we remove it.
In e46e183 we began tracking whether a local commitment transaction had been signed and broadcast in OnchainTxHandler, refusing to update the local commitment transaction state in the ChannelMonitor on that basis. This is fine, except that it doesn't make a lot of sense to store the full local transaction state in OnchainTxHandler - we should be providing it the unsigned local transaction at the time we wish to broadcast and no more (just like we do all other transaction data).
This cleans up sign_local_commitment somewhat by returning a Result<Signaure, ()> over the local commitment transaction instead of modifying the struct which was passed in. This is the first step in making LocalCommitmentTransaction a completely pub struct, using it just to communicate enough information to the user to allow them to construct a signaure instead of having it contain a bunch of logic. This should make it much easier to implement a custom ChannelKeys by disconnecting the local commitment transaction signing from our own datastructures.
1107ab0 introduced an API to have a ChannelKeys implementer sign HTLC transactions by calling into the LocalCommitmentTransaction object, which would then store the tx. This API was incredibly awkward, both because it required an external signer trust our own internal interfaces, but also because it didn't allow for any inspection of what was about to be signed. Further, it signed the HTLC transactions one-by-one in a somewhat inefficient way, and there isn't a clear way to resolve this (as the which-HTLC parameter has to refer to something in between the HTLC's arbitrary index, and its index in the commitment tx, which has "holes" for the non-HTLC outputs and skips some HTLCs). We replace it with a new function in ChannelKeys which allows us to sign all HTLCs in a given commitment transaction (which allows for a bit more effeciency on the signers' part, as well as sidesteps the which-HTLC issue). This may also simplify the signer implementation as we will always want to sign all HTLCs spending a given commitment transaction at once anyway. We also de-mut the LocalCommitmentTransaction passed to the ChanKeys, instead opting to make LocalCommitmentTransaction const and avoid storing any new HTLC-related data in it.
Instead of adding signatures to LocalCommitmentTransactions, we instead leave them unsigned and use them to construct signed Transactions when we want them. This cleans up the guts of LocalCommitmentTransaction enough that we can, and do, expose its state to the world, allowing external signers to have a basic awareness of what they're signing.
We should never be exposing our own TODOs to the world.
We don't need to assert that transaction structure is what we expect when the transaction is created by a function twenty lines up in the same file.
Codecov Report
@@ Coverage Diff @@
## master #608 +/- ##
==========================================
+ Coverage 91.05% 91.08% +0.02%
==========================================
Files 34 34
Lines 20359 20447 +88
==========================================
+ Hits 18538 18624 +86
- Misses 1821 1823 +2
Continue to review full report at Codecov.
|
|
SGTM 8b18d90 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #598, which seems ready to go, bump versions.