LDK Feedback & Questions

[q-src]

This PR contains a sample implementation of LDK as the result of following the steps described in Building a Node with LDK in Java. During the implementation, some questions and improvement ideas arose. Those are added as review comments to the diff at the corresponding code line (You may need to explicitly load the diff of LDKSample.java). The idea is to have discussions just there, where the questions / suggestions initially occurred: at code level. So feel free to add your thinking as a reply to those comments or even start a new discussion with a new comment.

All in all, LDK seems to be very promising for mobile lightning wallets but also for hosted nodes with the need for more flexibility / better integration in existing processes / systems. However, we want to share the following overall improvement ideas with you:

• LDK takes away a lot of lightning complexity. Nevertheless, there are still parts which require the dev / user of LDK to have knowledge of specific details of the LN spec (see also comments). IMHO it would help a lot if those parts could be eliminated (e.g. with default implementations provided by LDK). Because as a dev, I use LDK since I want to be sure that my node implementation is BOLT compliant without the need of dealing with the specs.
• In the Java world it is very uncommon to use underscore_case for method names. Instead, camelCase is the defacto standard.
• The documentation sometimes did not 100% stick to the actual library. The differences were not too bad. However, they result in a feeling of uncertainty which one does not like when dealing with Bitcoin stuff ;-). This could be prevented by always maintaining a sample implementation as in this PR alongside the LDK Java bindings. Whenever LDK interfaces change, the sample implementation would no longer compile and therefore break the build. Moreover, the "Building a Node with LDK in Java" guide could be completely generated out of the sample implementation. This would lead to LDK and the mentioned guide to be always 100% compatible.

As already mentioned, we think LDK is very promising for the use cases of lipa and we would like to further build on it. Moreover, we are also eager to support the development of LDK and its ecosystem - by just providing feedback but also by actively helping in implementing such feedback wherever possible.

[q-src]

It would be much more convenient to have LDK on Maven Central (https://search.maven.org/). See also: https://central.sonatype.org/publish/publish-guide/

Moreover, we could assist with publishing to Maven Central / OSSRH as we have some experience in this.

[TheBlueMatt]

We are now published on Central under org.lightningdevkit, artifact ldk-java!

[q-src]

Awesome, I will certainly try out building with ldk as a Maven dependency. I guess this would also solve the issue with the failing test, since all necessary binaries are included (besides that ldk tests are no longer executed when imported as a Maven dependency), right?

[TheBlueMatt]

Yes, it should include the release binaries for Apple amd64/A-1 and Linux amd64. I can include other platforms as needed, though windows will be a pain.

[q-src]

I guess Windows is pretty much always a pain 😜
I've just tried it on my ubuntu machine and it looks good - thanks 👍

[TheBlueMatt]

If you need it I can work on a windows build, its probably less effort than OSX was :).

[q-src]

We currently have no need for a Windows build, thanks :)

[q-src]

I like it a lot to have events and being able to add custom logic when they occur. However, the interface name ChannelManagerPersister may be misleading in this case. IMHO the persistence logic should be separated from the event handling logic (with a different interface).

[TheBlueMatt]

The next release will rename it to EventHandler.

[q-src]

To get a better developer experience a set of default event handlers following the BOLT implementation in terms of invoice regulations would be a big help, As a LDK user I don't want to worry about BOLT compliance as this is why I am using LDK ;-)

[TheBlueMatt]

Hmm, which BOLT compliance questions do you have here? I believe ~all BOLT compliance (and several other issues like privacy concerns etc) are handled internal to LDK, the only things you need to be concerned about are those explicitly stated in the documentation for PaymentReceived and/or claim_funds.

[q-src]

This was actually the only point I noticed that BOLT compliance may be broken by the LDK user. However, it made me wondering whether there are other parts I have to make sure my implementation follows the spec. If this is really the only place LDK users have to ensure BOLT compliance by themselves, I would appreciate having a default implementation for this event which is BOLT compliant.

At this point I am also wondering, why LDK users have to provide own event handler implementations at all. As mentioned, I really like the possibility to add custom logic when specific events occur. But isn't it that the basic stuff will always be the same? I think some sort of default implementations relying on the already existing interfaces (e.g. for transaction broadcast) and maybe an additional one for payment persistence (similar to the Persist interfaces for channels) would help a lot.

[TheBlueMatt]

This was actually the only point I noticed that BOLT compliance may be broken by the LDK user.

I hope not! Even if you forget to claim or fail an HTLC LDK will still fail it for you if it gets close to timing out. I hope there isn't a way you can fall out of BOLT compliance with LDK unless you try very, very hard to :).

At this point I am also wondering, why LDK users have to provide own event handler implementations at all. As mentioned, I really like the possibility to add custom logic when specific events occur. But isn't it that the basic stuff will always be the same?

Not really, for PaymentReceived, largely, yes, but just claiming the payment isn't all that useful, you probably want to tell the user they got paid, maybe store the payment preimage (proof-of-payment) somewhere, track payments, etc. Just naively claiming the payment and doing nothing else may be ok for a sample, but any real application will need to do more to hook it up to their UX.

I think some sort of default implementations relying on the already existing interfaces (e.g. for transaction broadcast)

Transaction broadcasting is somewhat separate from events (it comes in via a different interface). Here as well there isn't a great "default" that will work for most users, but in this case we do provide a Bitcoin Core RPC-based default in Rust. Sadly mapping it to the Java bindings is more than a little difficult, so for now that's one of the few rust-only things.

and maybe an additional one for payment persistence (similar to the Persist interfaces for channels) would help a lot.

Payment persistence we could provide a default, but I'd be a little surprised if any real-world application wouldn't want to store additional metadata attached to nearly each payment - invoice description, etc, and then hook it up explicitly to their UX, where it maybe belongs better.

[q-src]

I hope there isn't a way you can fall out of BOLT compliance with LDK unless you try very, very hard to :).

That's good to hear - thanks for making this clear!

Just naively claiming the payment and doing nothing else may be ok for a sample, but any real application will need to do more to hook it up to their UX.

This is actually more or less of my point: Naively claiming the payment may be ok for a sample but for a real app there are at least some checks required before the claiming. Some of them are even defined by a BOLT, which is why I would appreciate having an implementation provided by LDK that performs those checks (e.g. received amount should not be greater than twice of the requested amount). That's why I wanted to suggest default implementations for the event handlers (see also below).

Payment persistence we could provide a default, but I'd be a little surprised if any real-world application wouldn't want to store additional metadata attached to nearly each payment - invoice description, etc, and then hook it up explicitly to their UX, where it maybe belongs better.

I guess you got me wrong at this point. I was not talking about having a default implementation for payment persistence but about default implementations for the event handlers. For the PaymentReceived event this would require having an interface for payment persistence. This could look as follows:

interface PaymentPersistence {
  PaymentRequest findPaymentRequest(byte[] paymentHash);
}

interface PaymentRequest {
  byte[] getPreimage();
  int getAmountSat();
}

With such an interface, LDK could handle the mentioned checks in the case of an incoming payment event. It could verify whether the requested amount is sufficient but not more than twice as the requested amount. The way payments are actually stored would have to be defined by the LDK user by implementing the above interfaces.

For the other event handlers, similar default implementations would also be useful. I think they could even be realized by relying on the already existing interfaces for e.g. transaction broadcast. However, this does not necessarily mean that a default implementation of e.g. transaction broadcast has to be provided by LDK.

[TheBlueMatt]

This is actually more or less of my point: Naively claiming the payment may be ok for a sample but for a real app there are at least some checks required before the claiming. Some of them are even defined by a BOLT, which is why I would appreciate having an implementation provided by LDK that performs those checks (e.g. received amount should not be greater than twice of the requested amount). That's why I wanted to suggest default implementations for the event handlers (see also below).

The BOLT-prescribed checks (and privacy-important checks) are already implemented in LDK before generating the PaymentReceived event. eg the receive amount check and payment secret checks are done based on the inbound payment calls (eg https://docs.rs/lightning/0.0.100/lightning/ln/channelmanager/struct.ChannelManager.html#method.create_inbound_payment).

[TheBlueMatt]

In general, I'm not sure that users will need to perform their own checks before claiming HTLCs, but I do figure they'll need to persist additional information to track payments. eg you probably want a notification, you probably want to match a payment to an invoice generated in the past and update some local database, etc. Especially for notifications and other related things, I'm not sure how we could provide any kind of default - its going to be platform-specific, so there isn't a lot we can do there.

[q-src]

The BOLT-prescribed checks (and privacy-important checks) are already implemented in LDK before generating the PaymentReceived event.

Ah ok, that was not clear to me - then I see no need for a "default" implementation. So this means whenever a PaymentReceived event occurs, the payment meets all BOLT requirements? I guess this also means that LDK declines a payment if for example the sent amount is not sufficient?

However, this raises more questions: If LDK performs BOLT-prescribed checks itself, this means that LDK needs to know which payment requests were generated when a payment is incoming. How is this persisted? Is it part of the channel manager or is it just in-memory?

It was the following part of the rust docs which made me believe the LDK user has to make sure themselves that the paid amount is correct:

Note that if the preimage is not known or the amount paid is incorrect, you should call ChannelManager::fail_htlc_backwards to free up resources for this HTLC and avoid network congestion. The amount paid should be considered ‘incorrect’ when it is less than or more than twice the amount expected.-

-- https://docs.rs/lightning/0.0.100/lightning/util/events/enum.Event.html#variant.PaymentReceived

So this part is actually wrong / outdated?

[TheBlueMatt]

Ah ok, that was not clear to me - then I see no need for a "default" implementation. So this means whenever a PaymentReceived event occurs, the payment meets all BOLT requirements? I guess this also means that LDK declines a payment if for example the sent amount is not sufficient?

We do all the required checks now, yes, including amount, if the user provided LDK one (though the privacy concerns over amounts no longer apply with required payment secrets).

However, this raises more questions: If LDK performs BOLT-prescribed checks itself, this means that LDK needs to know which payment requests were generated when a payment is incoming. How is this persisted? Is it part of the channel manager or is it just in-memory?

See https://docs.rs/lightning/0.0.100/lightning/ln/channelmanager/struct.ChannelManager.html#method.create_inbound_payment for where we accept that information. It is persisted as a part of the ChannelManager object.

So this part is actually wrong / outdated?

Yes, thanks for reporting, fixed at lightningdevkit/rust-lightning#1079

[q-src]

I noticed that both, register_tx and register_output are always called. Therefore, I wonder if one could use register_tx xor register_outputonly (we currently ignores allregister_output` calls).

[TheBlueMatt]

No, they mean two different things - register_tx "registers interest in a transaction" whereas register_output "registers interest in spends of a transaction output". Do you have any recommendations for how we can make the documentation clearer here?

[q-src]

Ok, I see - thanks for the clarification! Maybe it helps if the documentation explained where the interest in the transaction or output is coming from. I guess register_tx signals interest in funding transactions to inform LDK that a channel is opened whereas register_output signals interest in force close transactions.

[q-src]

Step 6 has to be done after Step 11 due to the dependency on channelManagerConstructor.

[TheBlueMatt]

Indeed, there's no required ordering except for the dependencies. Any suggestions for how we can better capture that in the docs?

[q-src]

I guess most devs will read through the guide and do the implementations step by step. Therefore, it would be helpful if the ordering in the guide corresponds to the dependencies.

[q-src]

We currently use v0.0.98.3 of LDK since all newer versions have a failing test:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running org.ldk.ManualMsgHandlingPeerTest
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0 sec
Running org.ldk.HumanObjectPeerTest
Running test with flags 0
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.021 sec <<< FAILURE!
org.ldk.HumanObjectPeerTest.test_message_handler()  Time elapsed: 0.02 sec  <<< FAILURE!
java.lang.ExceptionInInitializerError
	at org.ldk.structs.Logger.<init>(Logger.java:17)
	at org.ldk.structs.Logger.new_impl(Logger.java:35)
	at org.ldk.HumanObjectPeerTestInstance$Peer.<init>(HumanObjectPeerTest.java:208)
	at org.ldk.HumanObjectPeerTestInstance$Peer.<init>(HumanObjectPeerTest.java:297)
	at org.ldk.HumanObjectPeerTestInstance.do_test_message_handler(HumanObjectPeerTest.java:610)
	at org.ldk.HumanObjectPeerTest.do_test_run(HumanObjectPeerTest.java:879)
	at org.ldk.HumanObjectPeerTest.do_test(HumanObjectPeerTest.java:884)
	at org.ldk.HumanObjectPeerTest.test_message_handler(HumanObjectPeerTest.java:911)
Caused by: java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:208)
	at java.base/java.nio.file.Files.copy(Files.java:3112)
	at org.ldk.impl.bindings.<clinit>(bindings.java:32)
	... 26 more

Running org.ldk.PeerTest
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0 sec

Results :

Failed tests:   org.ldk.HumanObjectPeerTest.test_message_handler()

Tests run: 1, Failures: 1, Errors: 0, Skipped: 0

[TheBlueMatt]

This implies that a matching liblightningjni native library was not found - how are you importing the LDK dependency and what platform are you running on?

[q-src]

LDK is declared as a Maven module ldk:

ldk-sample-java/pom.xml

Line 12 in c5d2fca

<module>ldk</module>

The ldk-sample module then decalres it as a dependency:

ldk-sample-java/ldk-sample/pom.xml

Lines 15 to 19 in c5d2fca

	<dependency>
	<groupId>org.ldk</groupId>
	<artifactId>ldk-java</artifactId>
	<version>1.0-SNAPSHOT</version>
	</dependency>

Tests are run either through the parent module by LD_LIBRARY_PATH=$PWD/ldk ./mvnw verify or within the ldk module / directory directly by LD_LIBRARY_PATH=$PWD mvn verify. Both lead to the same test failure mentioned in the initial comment.

The platform is a Ubuntu 20.04.2 LTS with kernel 5.4.0-77-generic (x86_64) and Java 11.0.11, vendor: Ubuntu, runtime: /usr/lib/jvm/java-11-openjdk-amd64

[TheBlueMatt]

Is it possible the files in resources are being stripped by your build process? Note that the resources files are not included in the git tree, only in the release binaries (though you can fetch them directly from https://git.bitcoin.ninja/ldk-java-bins if you want).

[q-src]

Since LDK is now on maven central, this one is no longer a real blocker. But maybe I will try to find out more anyway.