0.0.11 by TheBlueMatt · Pull Request #608 · lightningdevkit/rust-lightning

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

TheBlueMatt · 2020-04-25T02:11:27Z

Based on #598, which seems ready to go, bump versions.

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 · 2020-04-25T02:21:35Z

Codecov Report

Merging #608 into master will increase coverage by 0.02%.
The diff coverage is 96.42%.

@@ 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

Impacted Files	Coverage Δ
lightning/src/ln/onchaintx.rs	`94.86% <93.69%> (-0.26%)`	⬇️
lightning/src/ln/channelmonitor.rs	`95.50% <95.34%> (-0.01%)`	⬇️
lightning/src/chain/keysinterface.rs	`96.98% <100.00%> (-0.07%)`	⬇️
lightning/src/ln/chan_utils.rs	`97.19% <100.00%> (+0.44%)`	⬆️
lightning/src/ln/channel.rs	`86.41% <100.00%> (-0.01%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.04% <0.00%> (ø)`
lightning/src/util/ser_macros.rs	`97.27% <0.00%> (+0.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dc0dd1...8b18d90. Read the comment docs.

ariard · 2020-04-25T05:45:14Z

SGTM 8b18d90

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Antoine Riardand others added 8 commits April 24, 2020 18:51

Dry-up InputMaterial::Funding
359b3d5
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.

Remove TODOs from documentation in keysinterface
92c0698
We should never be exposing our own TODOs to the world.

Remove trivial assertions in LocalCommitmentTransaction fns
03316cd
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.

Bump versions to 0.0.11/net-tokio 0.0.3
8b18d90

TheBlueMatt added this to the 0.0.11 milestone Apr 25, 2020

TheBlueMatt merged commit 12e2a81 into lightningdevkit:masterApr 25, 2020

Uh oh!

Conversation

TheBlueMatt commented Apr 25, 2020

Uh oh!

codecovBot commented Apr 25, 2020

Codecov Report

Uh oh!

ariard commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

TheBlueMatt commented Apr 25, 2020

Uh oh!

codecovBot commented Apr 25, 2020

Codecov Report

Uh oh!

ariard commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

TheBlueMatt commented Apr 25, 2020

Uh oh!

codecovBot commented Apr 25, 2020

Codecov Report

Uh oh!

ariard commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

TheBlueMatt commented Apr 25, 2020

Uh oh!

codecovBot commented Apr 25, 2020

Codecov Report

Uh oh!

ariard commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

TheBlueMatt commented Apr 25, 2020

Uh oh!

codecovBot commented Apr 25, 2020

Codecov Report

Uh oh!

ariard commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels