Uh oh!
There was an error while loading. Please reload this page.
Drop ChannelKeys Private Key Methods - #632
Conversation
As we drop the requirement that all ChannelKeys expose the private keys used, we should have a way to access the private keys in use when using InMemoryChannelKeys.
Codecov Report
@@ Coverage Diff @@## master #632 +/- ##
==========================================
- Coverage 91.26% 91.26% -0.01%
==========================================
Files 35 35 Lines 20929 20918 -11 ==========================================
- Hits 19101 19090 -11
Misses 1828 1828
Continue to review full report at Codecov.
|
| /// Gets the local htlc secret key used in commitment tx htlc outputs | ||
| fn htlc_base_key<'a>(&'a self) -> &'a SecretKey; | ||
| /// Gets the commitment seed | ||
| fn commitment_seed<'a>(&'a self) -> &'a [u8; 32]; |
There was a problem hiding this comment.
Future work, should we move commitment_seed out-of-memory and behind the interface by moving inside chan_utils::build_commitment_secret ? In case of undetected node compromise, access to the commitment seed let you derive revocation secret for future non-yet-existent local commitment transactions ?
There was a problem hiding this comment.
Yea, I think we have to. If you want an external signer to prevent the host from burning all the coins, you need some kind of exchange here, but we'd also need to get the ordering right, like, to call get_commitment_revocation_secret we'd have to first provide a copy of the new commitment transaction.
| pub struct InMemoryChannelKeys { | ||
| /// Private key of anchor tx | ||
| funding_key: SecretKey, | ||
| pub funding_key: SecretKey, |
There was a problem hiding this comment.
I think even for this concrete implementation, it would be better to not make the private key properties public. Perhaps add a test config constraint?
There was a problem hiding this comment.
You need them to be able to sign transactions when handling SpendableOutput events, so there needs to be some way to get at them.
There was a problem hiding this comment.
Don't know the full context, but might it be better to expose a way to use them (e.g. expose a sign_tx method) rather than exposing the keys themselves?
There was a problem hiding this comment.
If the signing only handles in impl for InMemoryChannelKeys, they may not have to be pub, right?
There was a problem hiding this comment.
No, we dont require signing functions for SpendableOutputs events as those are purely external and can happen a month later. eg the use in the tests at https://github.com/rust-bitcoin/rust-lightning/blob/master/lightning/src/ln/functional_tests.rs#L4299
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
arik-so
left a comment
There was a problem hiding this comment.
Very simple and elegant PR!
09a6ce4 to
e5a7422Compare
This isn't quite sufficient for a secure external signer, as we still expose commitment_seed(), but its a nice cleanup after #610 did all the heavy lifting and its a good step.