Feat/teos ldk client

[dzdidi]

@sangbida Things done for now

Most of the logic is a copy/paste of core-lightning client removal of what was standing out as unnecessary
SQLite storage is replaced with LDK's KV Store (in the most naive way)
All original unit tests are passing
I guess we can just comment on diff with things that needs to be done and start picking them one by one.

Also if you have a better process in mind please let me know, this was the first thing I've came up with

[dzdidi]

@sangbida I have not considered earlier to implement it into core-lightning plugin, maybe as a feature flag (at least for db implementation). What do you think?

Here is how it could look approx dzdidi#4

[TheBlueMatt]

Have you seen lightningdevkit/rust-lightning#2552 and the PR that it depends on (that was merged)? That was previous work on the LDK end to make this easier (I assume you're using those APIs?)

[sangbida]

(pasting this comment again on the right PR)

Hey,
I was thinking we could compile some requirements in a doc somewhere, share it in the ldk discord just to make sure we're on the right track. Happy to compile this and share it around for feedback. It might help us to find things we can work on parallely. What do you think?

I'll take some time and go through this PR today, awesome work on it so far :)

[TheBlueMatt]

Yes, absolutely. I hope that the current LDK code is sufficient to get what you need from LDK for TEOS, though the above-linked PR would make it way easier and more robust (would love it if someone wants to pick that up!). Happy to chat more, though I need to remind myself the status of everything.

[mariocynicys]

I have not considered earlier to implement it into core-lightning plugin, maybe as a feature flag

I think we better have separate modules for different clients since the end binary would be "either" for LDK "or" for CLN (and never "mix of features").
We can have a new module though (e.g. teos-client-common) that has the common concepts shared between different clients/plugins, like retrier etc...

[dzdidi]

Me and @tnull couple of weeks ago we chatted about it and here are his key points:

• adding/enabling watchower should be simply have a set_watchtower(&self, url: String) method on Builder
• we can add an interface extension, i.e., have Node::watchtower_handler return a WatchtowerHelper API object where methods could live, and then in turn have them re-exposed in LDK Server
• KVStore should be used for persistence so that LDK-Node can do both local and VSS modes if needed

[TheBlueMatt]

I'm a little confused how you implemented this without ever calling counterparty_commitment_txs_from_update at all. The point of a watchtower isn't just to store data (i.e. implement LDK's KVStore), but rather to have enough information to punish a counterparty for misbehaving. It looks like that isn't implemented here (but intends to be implemented via CommitmentRevocation?). I don't think it makes sense to eventually implement that by hooking LDK's KVStore as you won't get nearly enough info to figure out what tx data you need, rather, a watchtower should likely be implemented by hooking LDK's Persist trait.

[tnull]

rather, a watchtower should likely be implemented by hooking LDK's Persist trait.

Yes, without having reviewed this PR yet, overriding Persist is what had discussed so far, but the client itself should use a KVStore for persistence of any additional metadata (rather than say, take another explicit depedency on SQLite).

[dzdidi]

Do you I understand correctly that for integration instead of on_commitment_revocation you want client to implement https://docs.rs/lightning/latest/lightning/chain/chainmonitor/trait.Persist.html ?

[TheBlueMatt]

Yes, for safety you need to use Persist because by hooking Persist (and using its "async" mode) you can cause LDK to not send peers an updated commitment state until the watchtower is ready to punish the newly-stale states. While many watchtower users might not care about the race condition its nice to have it be avoided where possible, otherwise a channel counterparty could run the channel state ahead of the watchtower and possibly get a stale state through if the watchtower client is offline.

[sangbida]

I'm still compiling and reading through things, but I'm happy to work on this one: lightningdevkit/rust-lightning#2552. Since it seems like it would be good to utlise that work in this PR? @dzdidi let me know if that is okay?

[mariocynicys]

@TheBlueMatt I am not really familiar with LDK and that Persist trait, but I think the gist is that the watchtower client only needs a copy of each revocation and that's it.

you can cause LDK to not send peers an updated commitment state until the watchtower is ready to punish the newly-stale states.

why do that? the watchtower client can retry the unreachable watchtowers later on its own but it doesn't have to halt the LDK node for that.

otherwise a channel counterparty could run the channel state ahead of the watchtower.

I guess you mean "the watchtower client" here?
The watchtower client never goes offline as it's hooked up to the LDK node in the same process. It should never miss any revocation notification from LDK. It might have problems pushing them to the subscribed towers, but that's something for the watchtower client to worry about not LDK.

[dzdidi]

@sangbida sure 🙏

[dzdidi]

by hooking Persist (and using its "async" mode) you can cause LDK to not send peers an updated commitment state until the watchtower is ready to punish the newly-stale states.

I understand this as effectively disabling entire node until watchtower replies. If so - why would somebody want to allow third party determine reliability of their own service? Especially given the support of multiple towers and aforementioned retry.

At the same time, it makes perfect sense to me have LDK to dictate which interface client should implement

[TheBlueMatt]

why do that? the watchtower client can retry the unreachable watchtowers later on its own but it doesn't have to halt the LDK node for that.

Depends on what the user wants. If the point of the watchtower is to protect a mobile client (or generally one that might be offline often) then the watchtower is pretty critical. If a counterparty just DDoS' the watchtower and advances the state a few times with the client then they can pretty reliably broadcast a stale state (if you assume the client is offline often enough that this is practical - if you dont assume that then you don't need a watchtower at all...) because the client won't come back online to finish the upload.

Having the option to pause the node for watchtower responses (or, eg, pausing the node to wait for at least one of N watchtowers to respond) is a pretty important option imo.

[mariocynicys]

aha i see ur point. imo, i would still not compromise usability for non-guaranteed security. i think the async/non-halting fashion is still pretty robust.

if u have a chunry client, each time they come back online (open the app), they are reinforcing any old revocations that were not uploaded.
it's the last couple of revocations they didn't upload before going offline for a long time are the compromised ones. it's not easy for an adversary to predict that or know that the client never opened the app after the DDoS on the tower was over.

the thing is, even if u halt the node to submit each single revocation to the tower, this isn't a 100% guarantee that the tower will respond to your channel breaches, so imo i would rather not degrade the usability for that.

--

pausing the node to wait for at least one of N watchtowers to respond

we can also let this be a configuration param for the developers to choose on their own. something like teos_client.set_minimum_required_attestations(a number in range [0, N]) where 0 means we never halt the node and N means we halt the node till all the N watchtowers confirm receipt of the revocation.

[sangbida]

Hey,
I'm a bit confused about how the code responsibilities are divided between projects. This PR and lightningdevkit/rust-lightning#2552 both seem to be working toward watchtower client functionality for LDK.
From my understanding, there appears to be significant overlap between what's being implemented in the LDK PR and this one. The Justice Transaction Tracker in LDK (once complete) will already handle creating and storing justice transactions for each channel state update.
If the signing and broadcasting functionality also resides in LDK, what specific components would still need to be implemented in TEOS? I feel like I'm missing something about how these projects are meant to interact.
Could someone clarify the separation of concerns between these implementations?