Draft: phase-2 signer API #723
Conversation
| @@ -244,6 +245,17 @@ pub trait ChannelKeys : Send+Clone { | |||
| // TODO: Add more input vars to enable better checking (preferably removing commitment_tx and | |||
| 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>( | |||
There was a problem hiding this comment.
This replaces the previous function.
| // 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| /// 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( |
There was a problem hiding this comment.
Should this be DRYed with Channel.build_commitment_transaction?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| /// Get the transaction number obscure factor | ||
| pub fn get_commitment_transaction_number_obscure_factor( |
There was a problem hiding this comment.
DRY this with Channel.get_commitment_transaction_number_obscure_factor
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
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.
| // 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( |
There was a problem hiding this comment.
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.
| 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>( |
There was a problem hiding this comment.
Would this need to be in the public interface, or are you imaging this is only a utility function in the InMemory version?
|
So, Followup: I did the latter |
|
Superseded by #742 |
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.