Skip to content

Add a commitment revocation hook ⚡🗼 #1776

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

Closed
sr-gi opened this issue Oct 17, 2022 · 10 comments · Fixed by #2337
Closed

Add a commitment revocation hook ⚡🗼 #1776

sr-gi opened this issue Oct 17, 2022 · 10 comments · Fixed by #2337
Milestone

Comments

@sr-gi
Copy link
Contributor

sr-gi commented Oct 17, 2022

In order to implement a watchtower client for LDK-based nodes the commitment revocation data should be exposed in some way (through a public API). The client will need to get access to this for each channel update.

The minimal required information to be exposed should be the revoked_commitment_txid:penalty_tx pair (or the bits and bolts required to build those).

@TheBlueMatt TheBlueMatt added this to the 0.1.1 milestone Oct 17, 2022
@sr-gi
Copy link
Contributor Author

sr-gi commented Mar 8, 2023

Hey guys, is there any ETA for this?

@alecchendev
Copy link
Contributor

I can start looking into this over the next couple of days!

@TheBlueMatt
Copy link
Collaborator

Hmmm, have a look at the current ChannelSigner::validate_counterparty_revocation. I think that may actually suffice if you hook that.

@alecchendev
Copy link
Contributor

@sr-gi did this end up working for you?

@sr-gi
Copy link
Contributor Author

sr-gi commented Mar 13, 2023

@sr-gi did this end up working for you?

I didn't have a chance to try it out yet :(

@alecchendev
Copy link
Contributor

I didn't have a chance to try it out yet :(

All good I'm still going to look into it more :)

@sr-gi
Copy link
Contributor Author

sr-gi commented Apr 13, 2023

Hmmm, have a look at the current ChannelSigner::validate_counterparty_revocation. I think that may actually suffice if you hook that.

I've created some wraps to make this work based on ldk-sample (sr-gi/ldk-sample@9d192a8) but I'm not sure all the bits are in place in ChannelSigner::validate_counterparty_revocation to build both the commitment_txid and the justice_tx. The method is called at the right time, but I haven't found how to pull the necessary data there.

I've seen that the commitment_tx can be built at ChannelSigner::sign_counterparty_commitment, however, only the signature(s) are returned there so wrapping is not enough, and re-implementing it in the wrapper isn't either, given some of the methods/members that are used there are private:

sign cannot be used because lightning::util::crypto is private
hash_to_message cannot be used because fuzz_wrappers is pub(crate)
commitment_sig cannot be created because channel_value_satoshis is private

With respect to the justice_tx, I'm guessing something could be done by wrapping ChannelSigner::sign_justice_revoked_output, but that is not called when revoking a previous state.

If the commitment_txid and the signed justice_tx were fed to ChannelSigner::validate_counterparty_revocation (or any other method that could be made part of that trait) then that would work. Let me know if there is any workaround I've missed though.

@TheBlueMatt
Copy link
Collaborator

Sorry for the delay following up. In general we should probably expose more as required here, though some of the particular utils you mention there shouldn't be required - sign is just a utility to call secp256k1, hash_to_message is just a type munger across secp/bitcoin_hashes types, and the channel_value is passed to the signer in general, so shouldn't be needed on a per-call basis.

@alecchendev
Copy link
Contributor

Btw I'll be working on this for Summer of Bitcoin so I should have something up for this within the first couple weeks of the program (starting May 15) just to give a general timeline @sr-gi

@sr-gi
Copy link
Contributor Author

sr-gi commented Apr 26, 2023

Sorry for the delay following up. In general we should probably expose more as required here, though some of the particular utils you mention there shouldn't be required - sign is just a utility to call secp256k1, hash_to_message is just a type munger across secp/bitcoin_hashes types, and the channel_value is passed to the signer in general, so shouldn't be needed on a per-call basis.

No worries. w.r.t the parts of code I mentioned, that was just to confirm that this does not seem to be solvable with the current interfaces (or if so, it'll look pretty hacky).

Btw I'll be working on this for Summer of Bitcoin so I should have something up for this within the first couple weeks of the program (starting May 15) just to give a general timeline @sr-gi

That's fantastic. I didn't know this was offered as a SoB project. Let me know if you need help co-mentoring this, I'll be happy to (or feel free to reach out if you need to chat about how this will be used downstream).

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 a pull request may close this issue.

3 participants