Skip to content

Add ChannelKeys to ChannelMonitor #466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

devrandom
Copy link
Member

This is the first step to removing secret key handling from ChannelMonitor.

Further steps will include:

  • moving left over signing operations into ChannelKeys
  • eliminating the secret keys

I broke this into steps in order to reduce PR size, even though it does introduce some temporary redundancy.

BTW, I find the generic on ChanSigner to be somewhat painful. It seems that the main reason for this is to have fast inline serialization. Maybe there's a way to do that while keeping the ChanSigner dynamic.

@ariard
Copy link

ariard commented Jan 30, 2020

@devrandom maybe have a high-level look on #462, it's a refactor to make tx generation in one place and split it from txn parsing.

What would you expect as an nterface for ChanSigner concerning non-BOLT3-specified transactions like justice tx on revoked commitment or spending remote HTLC outputs ?

fn sign_justice_tx(Vec<RevokedOutpointDescriptor) {}

RevokedHTLCOutpointDescriptor {
        commitment_number: u32,
        hash: PaymentHash
        lock_time: u32
}

RevokedOutpointDescriptor {
        commitment_number: u32,
        csv_delay: u32,

Assuming Signer have knowledge of redeemScript, preimages, revocation secrets. So roughly it would a SpendableOuputDescripor extension but would be better to use some well-defined data structures for inter-operability like PSBT (or pass some output script descriptor and let the signer implementor generate the PSBT if there is multiple signers ?)

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it up to you and @ariard how you want to proceed given this and #462 conflict heavily.

@devrandom
Copy link
Member Author

@ariard @TheBlueMatt , thank you for bringing #461 / #462 to my attention, I should have looked before getting started. I am totally fine with rebasing this as needed.

@devrandom devrandom force-pushed the chanmon-keys branch 3 times, most recently from dcc247d to 9111585 Compare January 30, 2020 22:22
@devrandom
Copy link
Member Author

@ariard:

Assuming Signer have knowledge of redeemScript, preimages, revocation secrets. So roughly it would a SpendableOuputDescripor extension but would be better to use some well-defined data structures for inter-operability like PSBT (or pass some output script descriptor and let the signer implementor generate the PSBT if there is multiple signers ?)

For closing/justice inputs, each of them is determined by protocol, and the information needed is as you have in your comment AFAICT. I think PSBT is not very suitable for describing the inputs, because much of the information is Lightning specific.

For the outputs, they can be anything, and they could in theory be described by PSBT or some other mechanism. I think that's out of scope for lightning-rust, at least for now. The outputs will of course be examined by the signer to ensure they follow policy (e.g. go to a whitelisted destination).

Also, for rust-lightning, since it's a development kit, it should not concern itself with how the outputs are constructed, but just give the developer freedom to choose.

Did I answer your question?

I'll look at #462 in more detail.

@devrandom
Copy link
Member Author

@ariard #462 looks like a much needed refactoring to reduce the complexity of ChannelMonitor.

Please let me know if you want me to comment on anything specific.

@ariard
Copy link

ariard commented Jan 31, 2020

@devrandom you may use PSBT even for LN, by passing per_commitment in BIP_IN_BIP32_DERIVATION and witnessScript in PSBT_IN_WITNESS_SCRIPT, assuming you also send revocation_secret/preimage to signer.

Also, for rust-lightning, since it's a development kit, it should not concern itself with how the outputs are constructed, but just give the developer freedom to choose.

I agree, we shouldn't enforce any binary format on the external signer implementor while at the same time avoid a messy custom-data structure to describe outputs.

For the outputs, they can be anything, and they could in theory be described by PSBT or some other mechanism.

I was thinking about the in-protocol outputs, the ones you must take actions on before a given time, i.e revoked outputs or commitment HTLC outputs. Not outputs belonging to justice/sweeping txn, this ones are completely up to the signer.

Yes thanks for your answer, it will be easier to see if everything suit well when we remove private keys from ChannelMonitor.

I'm waiting Matt high-level feedback on #462 before to build on top and yes remove private keys completely. It may drop some parts of your PR, but that would be for the best I guess ?

@devrandom
Copy link
Member Author

@devrandom you may use PSBT even for LN, by passing per_commitment in BIP_IN_BIP32_DERIVATION and witnessScript in PSBT_IN_WITNESS_SCRIPT, assuming you also send revocation_secret/preimage to signer.

A signer that understands PSBT but doesn't understand Lightning would not be able to sign, right? So although it's a use of PSBT, it's not standard, and PSBT doesn't solve the entire transmission requirements.

For the outputs, they can be anything, and they could in theory be described by PSBT or some other mechanism.

I was thinking about the in-protocol outputs, the ones you must take actions on before a given time, i.e revoked outputs or commitment HTLC outputs. Not outputs belonging to justice/sweeping txn, this ones are completely up to the signer.

Yes, agreed, the protocol outputs handling has to follow a specific pattern.

Yes thanks for your answer, it will be easier to see if everything suit well when we remove private keys from ChannelMonitor.

I'm waiting Matt high-level feedback on #462 before to build on top and yes remove private keys completely. It may drop some parts of your PR, but that would be for the best I guess ?

Yes, of course.

@TheBlueMatt
Copy link
Collaborator

@ariard given #462 may take a bit, does it make sense for this to go first?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good mod two comments, should be easy enough to resolve.

@TheBlueMatt
Copy link
Collaborator

Other than the 1.22 error, looks good!

@devrandom
Copy link
Member Author

devrandom commented Feb 4, 2020

Squashed to two commits.

@devrandom devrandom force-pushed the chanmon-keys branch 2 times, most recently from 4385be4 to 68c9d43 Compare February 4, 2020 23:37
@TheBlueMatt TheBlueMatt merged commit 8ee626c into lightningdevkit:master Feb 5, 2020
@devrandom devrandom deleted the chanmon-keys branch February 5, 2020 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants