Draft: phase-2 signer API by devrandom · Pull Request #723 · lightningdevkit/rust-lightning

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom · 2020-09-29T08:51:58Z

This is an early sketch of a modified signer API for review of the concept.

As previously discussed, we would like to pass in the data elements describing the commitment transaction (and HTLCs) instead of the serialized transaction. This makes it easier to validate and ensure that no aspect of the transaction escaped validation.

Let me know if expanding the sketch is required for meaningful review.

devrandom · 2020-09-29T08:53:22Z

 	fn sign_holder_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, holder_commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()>;

+	/// Create a signature for a holder's commitment transaction and attached HTLC transactions.
+	fn sign_holder_commitment_phase2<T: secp256k1::Signing + secp256k1::Verification>(


This replaces the previous function.

devrandom · 2020-09-29T08:56:59Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


This awkwardness is due to HolderCommitmentTransaction holding counterparty signatures. Not sure what's the best thing here - perhaps separate the signature holding to another struct?

I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

I don't see why? HolderCommitmentTransaction, indeed, holds too much data today (specifically, the transaction itself), but communicating things like the counterparty signatures and the per htlc values has to be done. eg here the function signature has a ton of fields, that may be usefully just shoved into a struct, and HolderCommitmentTransaction is basically that. (I assume we agree here, just noting that we may end up wanting a struct either way).

devrandom · 2020-09-29T08:59:26Z

+/// Returns the transaction, the HTLC descriptions with output index populated
+/// and the redeem scripts for the transaction.
+/// Note that the redeem scripts are returned for test purposes.
+pub fn build_commitment_tx(


Should this be DRYed with Channel.build_commitment_transaction?

Would def be nice, also so that its a separate, static function which can be used by a simple external signer to implement of the signer interface with one-line calls to pub-static functions.

devrandom · 2020-09-29T09:00:01Z

+}
+
+/// Get the transaction number obscure factor
+pub fn get_commitment_transaction_number_obscure_factor(


DRY this with Channel.get_commitment_transaction_number_obscure_factor

codecov · 2020-09-29T10:31:06Z

Codecov Report

Merging #723 into main will increase coverage by 1.46%.
The diff coverage is 17.02%.

@@ Coverage Diff @@## main #723 +/- ##
==========================================
+ Coverage 91.95% 93.41% +1.46% 
==========================================
Files 36 40 +4 Lines 20200 25867 +5667 ==========================================
+ Hits 18575 24164 +5589 - Misses 1625 1703 +78

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`97.37% <ø> (ø)`
lightning/src/ln/transaction.rs	`0.00% <0.00%> (ø)`
lightning/src/util/enforcing_trait_impls.rs	`98.76% <50.00%> (ø)`
lightning/src/chain/keysinterface.rs	`94.05% <58.82%> (-2.15%)`	⬇️
lightning/src/ln/channel.rs	`89.79% <100.00%> (+3.56%)`	⬆️
lightning/src/chain/transaction.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.07% <0.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <0.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <0.00%> (ø)`
... and 9 more

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 8640173...05a8af6. Read the comment docs.

ariard

Actually we have HolderCommitmentTransaction which is just a blob of channel data without clear distribution of responsibility between Signer, Channel and ChannelMonitor. I think we should reduce/kill HolderCommitmentTransaction and stick per-update state in both Channel/ChannelMonitor. Static channel data should be initialized in Signer as soon as we receive off-chain messages.

When Channel/ChannelMonitor require either a verification or signing operation they call the Signer interface with the accurate per-update state. Signer can construct the commitment/HTLC transactions from data elements and verify/sign. As signer enforces immutability on static channel data, so an implementation can also serve as a commitment tx provider for any ChannelMonitor caller.

That said, if we agree that's where we want to end-up, I think this PR is moving towards the good direction.

ariard · 2020-09-29T22:43:44Z

+ // We provide a dummy signature for the remote, since we don't require that sig
+ // to be passed in to this call. It would have been better if HolderCommitmentTransaction
+ // didn't require the remote sig.
+ let dummy_sig = Secp256k1::new().sign(


I think we would like to get ride of HolderCommitmentTransaction. It's API can be enforced either by ChannelMonitor or an external signer implementation like InMemoryChannelKeys.
All dynamic state (balances, HTLCs, ...) would be stored in Channel and the rest assumed to be in signer.

TheBlueMatt · 2020-10-01T18:22:43Z

+	pub fn funding_outpoint(&self) -> &OutPoint { self.funding_outpoint.as_ref().unwrap() }
+
+	/// Prepare build information for a commitment tx that the holder can broadcast
+	pub fn build_holder_commitment_tx<T: secp256k1::Signing + secp256k1::Verification>(


Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?

TheBlueMatt

Conceptually looks good, one question but its the right direction.

devrandom · 2020-10-05T19:23:08Z

So, sign_holder_commitment doesn't take TxCreationKeys in this version. It did before (indirectly, via the holder_commitment_tx). I assume that we still want them based on the dev meeting today? Should I just add it as one of the params for now, until we refactor HolderCommitmentTransaction?

Followup: I did the latter

devrandom · 2020-12-27T19:15:51Z

Superseded by #742

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

devrandom force-pushed the phase2 branch from 3e4dbbb to 9d3b4aeCompare September 29, 2020 08:52

devrandom commented Sep 29, 2020
View reviewed changes

devrandom force-pushed the phase2 branch from 10b4723 to 7d3f876Compare September 29, 2020 09:11

devrandom commented Sep 29, 2020
View reviewed changes

Comment threadlightning/src/chain/keysinterface.rs Outdated

devrandom force-pushed the phase2 branch from 7d3f876 to e934813Compare September 29, 2020 09:49

ariard reviewed Sep 29, 2020
View reviewed changes

draft KeysInterface.sign_holder_commitment_phase2
5982347

devrandom force-pushed the phase2 branch from e934813 to 5982347Compare October 1, 2020 11:15

TheBlueMatt reviewed Oct 1, 2020
View reviewed changes

pass in PreCalculatedTxCreationKeys
05a8af6

devrandom closed this Dec 27, 2020

Uh oh!

Conversation

devrandom commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovBot commented Sep 29, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

devrandom commented Oct 5, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented Dec 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

devrandom commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovBot commented Sep 29, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecovBot commented Sep 29, 2020•
edited
Loading

devrandom commented Oct 5, 2020•
edited
Loading

codecovBot commented Sep 29, 2020•
edited
Loading

devrandom commented Oct 5, 2020•
edited
Loading