Support third-party watchtowers in persistence pipeline #2337

alecchendev · 2023-06-05T03:46:02Z

Closes #1776. This PR aims to make it easier to form a justice/penalty transaction within the channel monitor persistence pipeline, which would help with building third-party watchtowers to support LDK-based nodes.

An internal test implementation of Persist is provided to show that we can form a justice tx after these changes as an example of how I'd imagine a user to be able to implement watchtower-like functionality.

Channel monitor persistence seems like the best place to have watchtower support so that we can have control over the channel pausing/resuming normal operation depending on the success/failure of persisting at the watchtower level, so that's mainly where the focus of this PR is.

Note: when I refer to "watchtowers" here, I'm mainly referring to third-party watchtowers, where a node would send a pre-signed, encrypted justice transaction to a externally run watchtower, e.g. teos. This API is meant to make it easier to build the client side support for these kinds of watchtowers.

To do

Accommodate the the first monitor update not being sent through persistence
- ~~Explore if refactoring this would be viable~~
- OR add some state to ChannelMonitor and make sure it gets cleaned up after the initial state

lightning/src/util/test_utils.rs

lightning/src/chain/channelmonitor.rs

lightning/src/util/test_utils.rs

alecchendev · 2023-06-15T23:36:22Z

Rewrote a bunch of this with a new approach which I think is better now. Still figuring out exactly the right API. I tried to elaborate in the commit messages on my thought process so far.

Also I still need to figure out how to best accommodate the very first monitor update with the counterparty's commitment, whether that be storing some state in the ChannelMonitor that gets cleaned up, or possibly trying to refactor to push the update through persistence, or something else.

codecov-commenter · 2023-06-16T00:43:24Z

Codecov Report

Patch coverage: 96.82% and project coverage change: +0.03% 🎉

Comparison is base (0c25046) 90.53% compared to head (b20b1db) 90.56%.
Report is 14 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2337      +/-   ##
==========================================
+ Coverage   90.53%   90.56%   +0.03%     
==========================================
  Files         107      107              
  Lines       56923    57171     +248     
  Branches    56923    57171     +248     
==========================================
+ Hits        51533    51776     +243     
- Misses       5390     5395       +5

Files Changed	Coverage Δ
lightning/src/chain/chainmonitor.rs	`95.03% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`94.63% <92.68%> (-0.10%)`	⬇️
lightning/src/ln/chan_utils.rs	`95.04% <97.16%> (+0.42%)`	⬆️
lightning/src/util/test_utils.rs	`73.61% <98.18%> (+2.14%)`	⬆️
lightning/src/ln/channel.rs	`89.82% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`88.86% <100.00%> (-0.04%)`	⬇️
lightning/src/ln/functional_tests.rs	`98.17% <100.00%> (+0.01%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ariard

Still WIP, figuring out what is the best API for this. I currently have an implementation of
Persist that can form a justice tx after exposing a few things on ChannelMonitor as an
example of how I'd imagine a user being able to implement some sort of watchtower
support. Open to feedback about how this should work.

I think this would benefit the review of this PR if we have a more detailed flow of how do you assume third-party watchtowers flows to work.

If we assume “local” watchtowers or “outsourced” ones (i.e run by an external entity where the pre-signed justice transactions are potentially encrypted thanks to BOLT3’s obscured commitment number). And if the flow should work for basic watchtower (justice transaction-only) or monitor replica (justice txn + time-sensitive ones like HTLC claims), see our GLOSSARY here.

There is a historical BOLT13 on how watchtower might work: https://github.com/sr-gi/bolt13/blob/master/13-watchtowers.md, however to the best of my knowledge there is no documentation on watchtower architecture (which might be helpful not only for cross-implementation compatibility but also guarantees compatibility between versions of LDK).

ariard · 2023-06-30T17:33:46Z

lightning/src/chain/channelmonitor.rs

+		let their_per_commitment_point = PublicKey::from_secret_key(&self.onchain_tx_handler.secp_ctx, &per_commitment_key);
+		let revokeable_redeemscript = self.revokeable_redeemscript(&their_per_commitment_point);
+
+		let sig = self.onchain_tx_handler.signer.sign_justice_revoked_output(&justice_tx, input_idx, value, &per_commitment_key, &self.onchain_tx_handler.secp_ctx)?;


Between the two approaches mentioned “Get counterparty commitment txs from monitor update” and “Get revoked output data from update, build/sign separately”, I think one of the determinant factor to pick up which one might be the most advantageous is the flexibility preserved for the watchtower.

Namely, they are few characteristics than a watchtower signing policy can enforce:

the amount of fees paid by the justice_tx

the destination scriptPubkey

the claim aggregation of one or more RevokeableOutput

In term of a potential watchtower signing policy, from my understanding the two approaches are equivalent:

with approach 1, as we’re calling into OnchainTxHandler’s signer, a signing policy can be implemented behind EcdsaChannelSigner::sign_justice_revoked_output

with approach 2, build_justice_tx is a pub method, the caller can verify the checks on the Transaction and then give it back to sign_justice_tx.

As we have already refactored our signing interface to let custom signing policy be implemented there, this is unclear to me what are the other trade-off between both approaches ? Do we wanna the transaction broadcast to be managed by the own transaction-relay module of the watchtower ? Or rather extend OnchainTxHandler to have justice_tx re-scheduled by the watchtower.

Thanks for this. I've mainly been thinking of this API in terms of what will be easiest/most intuitive for a user building a watchtower without exposing too many unnecessary details, while still allowing for some flexibility in terms of transaction creation (in case in the future a watchtower module wanted to do anything beyond the most basic justice tx, e.g. add an output to the justice tx like mentioned here or claiming revoked HTLC outputs). This is where I was thinking about whether we should expose the full commitment transaction data in approach 1--where they'd have more data needed to possibly claim other outputs, at the expense of possibly offering up excess data--versus just exposing the counterparty's to_local revokable output info in approach 2 (or some combination of both).

Do we wanna the transaction broadcast to be managed by the own transaction-relay module of the watchtower ? Or rather extend OnchainTxHandler to have justice_tx re-scheduled by the watchtower.

As I've had the basic "outsourced" watchtower in mind for this, I imagined that this would expose the necessary information to build, sign, and encrypt the justice tx, and send it to the watchtower server which would take care of broadcasting the tx separately.

(in case in the future a watchtower module wanted to do anything beyond the most basic justice tx, e.g. add an output to the justice tx like mentioned here or claiming revoked HTLC outputs).

Yes adding an output as a “service bounty” for the outsourced watchtower makes a lot of sense (or fee-bumping purpose if you wanna do CPFP). Note, I would favor to expose the full commitment transaction data that way the watchtower can build claim on the revoked HTLC outputs, without waiting counterparty confirmation of second-stage HTLC transaction (which might never happen!). The anchor output could be swept too by the watchtower as recommended by BOLT3 after the CSV 16: https://github.com/lightning/bolts/blob/master/03-transactions.md#to_local_anchor-and-to_remote_anchor-output-option_anchors (not punishment per se though as you have the infrastructure laid out).

As I've had the basic "outsourced" watchtower in mind for this, I imagined that this would expose the necessary information to build, sign, and encrypt the justice tx, and send it to the watchtower server which would take care of broadcasting the transaction separately.

“Outsourced” watchtower sounds good. There is one design option moving forward, making OnchainTxHandler an interface to which transaction issuer (watchtower, dual-funding rbfing) can subscribe broadcasting transaction. A robust transaction rebroadcast and fee-bumping module is easier said than done :) (especially with all transaction relay policy maintenance).

alecchendev

I think this would benefit the review of this PR if we have a more detailed flow of how do you assume third-party watchtowers flows to work.

Ah thanks, I'll add that to the PR description. I've been mainly designing this with the "basic" (justice tx-only) "outsourced" (pre-signed, encrypted justice tx sent to an external entity) watchtower in mind.

alecchendev · 2023-06-30T23:55:56Z

lightning/src/chain/channelmonitor.rs

+		let their_per_commitment_point = PublicKey::from_secret_key(&self.onchain_tx_handler.secp_ctx, &per_commitment_key);
+		let revokeable_redeemscript = self.revokeable_redeemscript(&their_per_commitment_point);
+
+		let sig = self.onchain_tx_handler.signer.sign_justice_revoked_output(&justice_tx, input_idx, value, &per_commitment_key, &self.onchain_tx_handler.secp_ctx)?;


Thanks for this. I've mainly been thinking of this API in terms of what will be easiest/most intuitive for a user building a watchtower without exposing too many unnecessary details, while still allowing for some flexibility in terms of transaction creation (in case in the future a watchtower module wanted to do anything beyond the most basic justice tx, e.g. add an output to the justice tx like mentioned here or claiming revoked HTLC outputs). This is where I was thinking about whether we should expose the full commitment transaction data in approach 1--where they'd have more data needed to possibly claim other outputs, at the expense of possibly offering up excess data--versus just exposing the counterparty's to_local revokable output info in approach 2 (or some combination of both).

Do we wanna the transaction broadcast to be managed by the own transaction-relay module of the watchtower ? Or rather extend OnchainTxHandler to have justice_tx re-scheduled by the watchtower.

As I've had the basic "outsourced" watchtower in mind for this, I imagined that this would expose the necessary information to build, sign, and encrypt the justice tx, and send it to the watchtower server which would take care of broadcasting the tx separately.

TheBlueMatt

Sorry for the delay, this is looking pretty good to me in general, I don't think we need to worry about any other watchtower models here - the only other realistic one is "copies of my ChannelMonitor", which is already doable :).

lightning/src/chain/channelmonitor.rs

sr-gi · 2023-07-10T19:01:59Z

I've tried this downstream on ldk-sample. The workflow, from an API standpoint, is what I'd expect.

If I've got this right, for a client to implement the functionality something like WatchtowerPersister would need to be implemented so the state can be backed up to the remote tower. In my test, I have just logged the methods on the persister and tried this on regtest against a CLN node. In a prod setup, it would make more sense to have a queue that the client process can subscribe to, rather than a map, but I guess that's up to the implementer of the persister to pick whatever they find more useful.

Here's the patched version of ldk-sample building on top of @tnull's 0.0.116 update WIP and pulling from a custom rust-lightning with this patch: https://github.com/sr-gi/ldk-sample/tree/ldk-towers

alecchendev · 2023-07-10T20:45:14Z

Thanks for testing it out @sr-gi!

If I've got this right, for a client to implement the functionality something like WatchtowerPersister would need to be implemented so the state can be backed up to the remote tower.

Yep, your patch looks like how I was imagining it to be used

In a prod setup, it would make more sense to have a queue that the client process can subscribe to, rather than a map, but I guess that's up to the implementer of the persister to pick whatever they find more useful.

Makes sense, yea I just used a map off the top of my head for testing :)

ariard · 2023-07-11T23:47:26Z

Concept ACK, I’ll favor approach 1 over approach 2 as exposing the full commitment data allows more flexibility for the outsourced tower service (e.g fee-bumping output).

alecchendev · 2023-07-12T21:35:54Z

Rebased, cleaned things ups, added docs, and reworked some things according to all the feedback so far

lightning/src/chain/channelmonitor.rs

lightning/src/ln/chan_utils.rs

lightning/src/chain/channelmonitor.rs

alecchendev · 2023-07-31T16:14:22Z

Just fixed a docs build error

lightning/src/ln/channel.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/chan_utils.rs

lightning/src/util/test_utils.rs

alecchendev · 2023-08-03T01:14:26Z

Thanks for all the feedback. CI is gonna fail because of certain fixups spanning multiple commits - let me know when I should squash since there are quite a few :)

TheBlueMatt · 2023-08-16T18:29:13Z

Feel free to squash imo, will take another look after lunch but I think this is good.

alecchendev · 2023-08-16T18:44:47Z

Squashed without further changes

lightning/src/chain/channelmonitor.rs

alecchendev · 2023-08-16T19:18:37Z

Just simplified filtering the dust outputs and removed the commit adding counterparty_dust_limit_satoshis, squashed immediately

lightning/src/ln/channel.rs

lightning/src/ln/chan_utils.rs

lightning/src/chain/channelmonitor.rs

wpaulino · 2023-08-16T23:35:36Z

lightning/src/chain/channelmonitor.rs

+	/// This method will only succeed if this monitor has received the revocation secret for the
+	/// provided `commitment_number`. If a commitment number is provided that does not correspond
+	/// to the commitment transaction being revoked, this will return a signed transaction, but
+	/// the signature will not be valid.


I'd imagine this would be pretty hard to mess up, but if we also require they pass in the prevout, we could check the re-derived witness script corresponds to it and return an error.

yea hmm, I feel like they'd have to really go out of their way to spend the wrong output/commitment number since they should just be using our helper(s). I don't feel that strongly either way, do you feel like this will meaningfully catch user errors?

TheBlueMatt

LGTM, some tiny nits that you can address while squashing, IMO, plus we should probably have some util to do the tracking for users who want it.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/chan_utils.rs

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2023-08-21T20:48:09Z

lightning/src/util/test_utils.rs

+	) -> chain::ChannelMonitorUpdateStatus {
+		let res = self.persister.update_persisted_channel(funding_txo, update, data, update_id);
+
+		if let Some(update) = update {


As a followup, can we expose the code to track unsigned_justice_tx_data and generate the ultimate signed transactions?

Sure, will try and get to this over the next couple of days

Sorry for the delay here, getting to this now. Were you thinking to expose basically a cleaned up version of this test util? I'm wondering who would this be for, since I imagine someone using this interface would want to return a different ChannelMonitorUpdateStatus depending on however they're using these signed justice txs (e.g. sending to remote watchtower/other server).

Hmm although... I could extend this util to take some user-implemented interface that takes a signed justice tx, does something, and returns a ChannelMonitorUpdateStatus, so our util handles the tracking and the user can focus on handling what to do with the tx? Would that make sense?

Yea basically. There's quite a bit of code required here to track transactions and their value and then eventually get them signed. I don't think the interface needs to be the same, however - we don't need to actually do the returning of the status/impl Persist, but rather we can just have a method that says "I got an update, please, state object, handle that update and give me any finalized, broadcastable, transactions that I should hand to the watchtower.

Sadly, this state object is going to need to be able to persist itself in some way or another - if we get a commitment transaction then restart, then see the revocation data for it, we'll need to be able to get that revocation data to the watchtower combined with the commitment tx.

Ah ok that makes sense!

This adds the feerate and local and remote output values to this channel monitor update step so that a monitor can reconstruct the counterparty's commitment transaction from an update. These commitment transactions will be exposed to users in the following commits to support third-party watchtowers in the persistence pipeline. With only the HTLC outputs currently available in the monitor update, we can tell how much of the channel balance is in-flight and towards which side, however it doesn't tell us the amount that resides on either side. Because of dust, we can't reliably derive the remote value from the local value and visa versa. Thus, it seems these are the minimum fields that need to be added.

ariard

Code Review ACK a21f242, haven’t look deep into the tests.

ariard · 2023-08-23T16:01:46Z

lightning/src/chain/channelmonitor.rs

+	///
+	/// It is expected that a watchtower client may use this method to retrieve the latest counterparty
+	/// commitment transaction(s), and then hold the necessary data until a later update in which
+	/// the monitor has been updated with the corresponding revocation data, at which point the


nit: s/revocation data/per_commitment_secret/g

Please no, lets keep our docs assuming users don't know all the inner details of the BOLTs including variable names like this.

More confusing to have loosely names imho.

lightning/src/chain/channelmonitor.rs

ariard · 2023-08-23T16:16:50Z

lightning/src/ln/chan_utils.rs

+	/// The built transaction will allow fee bumping with RBF, and this method takes
+	/// `feerate_per_kw` as an input such that multiple copies of a justice transaction at different
+	/// fee rates may be built.
+	pub fn build_to_local_justice_tx(&self, feerate_per_kw: u64, destination_script: Script)


This might sounds obvious though the destination_script and all the descendant scripts should be only controlled by the local lightning node, to avoid all kind of boring pinnings. If you like more doc.

Ah, I think I'll just leave it how it is, I think it shouldn't be too big of a concern given this is expected to just help with a fairly specific use case

alecchendev · 2023-08-23T17:01:15Z

Oops forgot to say I rebased because of conflicts

TheBlueMatt · 2023-08-23T17:19:47Z

Oops, acked before you pushed fixups, feel free to just go ahead and squash I think.

Upon creating a channel monitor, it is provided with the initial counterparty commitment transaction info directly before the very first time it is persisted. Because of this, the very first counterparty commitment is not seen as an update in the persistence pipeline, and so our previous changes to the monitor and updates cannot be used to reconstruct this commitment. To be able to expose the counterparty's transaction for the very first commitment, we add a thin wrapper around `provide_latest_counterparty_commitment_tx`, that stores the necessary data needed to reconstruct the initial commitment transaction in the monitor.

For watchtowers to be able to build justice transactions for our counterparty's revoked commitments, they need to be able to find the revokeable output for them to sweep. Here we cache `to_self_delay` in `CommitmentTransaction` to allow for finding this output on the struct directly. We also add a simple helper method to aid in building the initial spending transaction. This also adds a unit test for both of these helpers, and refactors a bit of a previous `CommitmentTransaction` unit test to make adding these easier.

Here we implement `WatchtowerPersister`, which provides a test-only sample implementation of `Persist` similar to how we might imagine a user to build watchtower-like functionality in the persistence pipeline. We test that the `WatchtowerPersister` is able to successfully build and sign a valid justice transaction that sweeps a counterparty's funds if they broadcast an old commitment.

alecchendev · 2023-08-23T17:33:30Z

Squashed with no further changes

alecchendev commented Jun 5, 2023

View reviewed changes

lightning/src/util/test_utils.rs Outdated Show resolved Hide resolved

tnull mentioned this pull request Jun 5, 2023

Add Watchtowers lightningdevkit/ldk-node#110

Open

wpaulino self-requested a review June 5, 2023 21:45

TheBlueMatt reviewed Jun 6, 2023

View reviewed changes

alecchendev force-pushed the 2023-06-watchtower-support branch from 7079d06 to 3b3492a Compare June 15, 2023 23:35

ariard reviewed Jun 30, 2023

View reviewed changes

alecchendev commented Jul 1, 2023

View reviewed changes

sr-gi added a commit to sr-gi/ldk-sample that referenced this pull request Jul 7, 2023

WIP: testing lightningdevkit/rust-lightning#2337

c26c21c

TheBlueMatt reviewed Jul 8, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

sr-gi added a commit to sr-gi/ldk-sample that referenced this pull request Jul 10, 2023

WIP: testing lightningdevkit/rust-lightning#2337

97a4ca5

sr-gi added a commit to sr-gi/ldk-sample that referenced this pull request Jul 10, 2023

WIP: testing lightningdevkit/rust-lightning#2337

fbc0fe1

sr-gi added a commit to sr-gi/ldk-sample that referenced this pull request Jul 10, 2023

WIP: testing lightningdevkit/rust-lightning#2337

e158634

alecchendev force-pushed the 2023-06-watchtower-support branch 2 times, most recently from 444db49 to 8bfbe68 Compare July 12, 2023 21:18

alecchendev marked this pull request as ready for review July 12, 2023 21:35

TheBlueMatt reviewed Jul 27, 2023

View reviewed changes

alecchendev force-pushed the 2023-06-watchtower-support branch 2 times, most recently from 7e3dd9a to c5f5f2b Compare July 31, 2023 16:13

alecchendev force-pushed the 2023-06-watchtower-support branch 2 times, most recently from 0e58741 to e25593d Compare July 31, 2023 19:46

wpaulino reviewed Aug 1, 2023

View reviewed changes

alecchendev force-pushed the 2023-06-watchtower-support branch from e25593d to 5735e75 Compare August 3, 2023 01:12

alecchendev force-pushed the 2023-06-watchtower-support branch from 1441fb4 to 9c13db6 Compare August 15, 2023 23:53

alecchendev force-pushed the 2023-06-watchtower-support branch from 9c13db6 to 26e60b7 Compare August 16, 2023 18:44

alecchendev commented Aug 16, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

wpaulino reviewed Aug 16, 2023

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

alecchendev force-pushed the 2023-06-watchtower-support branch from 26e60b7 to 387c743 Compare August 16, 2023 19:16

wpaulino reviewed Aug 16, 2023

View reviewed changes

alecchendev force-pushed the 2023-06-watchtower-support branch from 387c743 to d1888bc Compare August 17, 2023 00:40

TheBlueMatt reviewed Aug 21, 2023

View reviewed changes

alecchendev force-pushed the 2023-06-watchtower-support branch from d1888bc to a21f242 Compare August 23, 2023 15:48

ariard approved these changes Aug 23, 2023

View reviewed changes

alecchendev force-pushed the 2023-06-watchtower-support branch from a21f242 to fbf9e58 Compare August 23, 2023 17:07

TheBlueMatt approved these changes Aug 23, 2023

View reviewed changes

alecchendev added 5 commits August 23, 2023 12:33

Build and expose counterparty commitments from monitor update

966465a

Enable signing a justice tx using the channel monitor

2cb2557

alecchendev force-pushed the 2023-06-watchtower-support branch from fbf9e58 to b20b1db Compare August 23, 2023 17:33

TheBlueMatt approved these changes Aug 23, 2023

View reviewed changes

wpaulino approved these changes Aug 23, 2023

View reviewed changes

TheBlueMatt merged commit 32d6e91 into lightningdevkit:main Aug 23, 2023
14 checks passed

alecchendev mentioned this pull request Sep 4, 2023

Add util to track justice transactions for watchtowers #2552

Open

2 tasks

sr-gi mentioned this pull request Sep 14, 2023

Build a tower client for LDK talaia-labs/rust-teos#133

Open

2 tasks

Support third-party watchtowers in persistence pipeline #2337

Support third-party watchtowers in persistence pipeline #2337

Conversation

alecchendev commented Jun 5, 2023 • edited

alecchendev commented Jun 15, 2023

codecov-commenter commented Jun 16, 2023 • edited

Codecov Report

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

sr-gi commented Jul 10, 2023

alecchendev commented Jul 10, 2023

ariard commented Jul 11, 2023

alecchendev commented Jul 12, 2023

alecchendev commented Jul 31, 2023

alecchendev commented Aug 3, 2023

TheBlueMatt commented Aug 16, 2023

alecchendev commented Aug 16, 2023

alecchendev commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecchendev commented Aug 23, 2023

TheBlueMatt commented Aug 23, 2023

alecchendev commented Aug 23, 2023

alecchendev commented Jun 5, 2023 •

edited

codecov-commenter commented Jun 16, 2023 •

edited