Proposal: implement custom TLV records for spontaneous payments #273
Conversation
The fee as returned from `PaymentSent` event generated by LDK and is saved in `PaymentSuccessful` event.
.. which will tell Rust not to treat it as an integration test module.
.. no need to use a `Result` here
.. we replace the simple `is_running` with a more verbose `status` method returning a `NodeStatus` struct, giving more information on syncing states etc.
…atus Introduce `status` method allowing to query the `Node`'s status
| @@ -456,3 +456,12 @@ impl Default for ChannelConfig { | |||
| LdkChannelConfig::default().into() | |||
| } | |||
| } | |||
|
|
|||
| /// Custom TLV entry. | |||
| pub struct TlvEntry { | |||
There was a problem hiding this comment.
In the protocol the TLV entry is defined with a length
https://github.com/lightning/bolts/blob/master/01-messaging.md#type-length-value-format
Whats the reason of not including it here?
There was a problem hiding this comment.
I see that LDK is expecting type and value to set the tlv. I guess then the length is set by ldk when sending the message.
Maybe add a bit more info on the type and value describing the restrictions or characterizes of the props:
for example, for type:
type identifiers below 2^16 are reserved for use in this specification. type identifiers greater than or equal to 2^16 are available for custom records. (from the blip)
There was a problem hiding this comment.
In the protocol the TLV entry is defined with a length https://github.com/lightning/bolts/blob/master/01-messaging.md#type-length-value-format
Whats the reason of not including it here?
The length of the data buffer is used for that. I shall see if we can improve the representation, though
| @@ -490,8 +490,11 @@ pub(crate) fn do_channel_full_cycle<K: KVStore + Sync + Send, E: ElectrumApi>( | |||
| // Test spontaneous/keysend payments | |||
| println!("\nA send_spontaneous_payment"); | |||
| let keysend_amount_msat = 2500_000; | |||
| let keysend_payment_hash = | |||
| node_a.send_spontaneous_payment(keysend_amount_msat, node_b.node_id()).unwrap(); | |||
| let tlv1 = TlvEntry { r#type: 131073, value: vec![0x00, 0x11, 0x22, 0x33] }; | |||
There was a problem hiding this comment.
does this test validate that the counter-party node received the message?
src/lib.rs
Outdated
| @@ -1222,7 +1222,7 @@ impl<K: KVStore + Sync + Send + 'static> Node<K> { | |||
|
|
|||
| /// Send a spontaneous, aka. "keysend", payment | |||
| pub fn send_spontaneous_payment( | |||
| &self, amount_msat: u64, node_id: PublicKey, | |||
| &self, amount_msat: u64, node_id: PublicKey, custom_tlvs: Vec<TlvEntry>, | |||
There was a problem hiding this comment.
are the custom_tlvs required in order to send a spontaneous payment?
There was a problem hiding this comment.
Not really required but may be useful. But I see the point, we'll probably take tnull's suggestion to base this change off PR 270; probably once it's merged
There was a problem hiding this comment.
Thanks for looking into this! While adding custom TLV support is stretching our 'keep API minimal' guidelines, I see that it can be of value for certain usecases.
I however agree with @jbesraa's question above regarding the optionality of custom TLVs. Rather than just adding it to the API method, could you base this entire PR off #270 and add it as a new SpontaneousPayment::send_with_custom_tlvs variant to and keep the default send method untouched?
|
Is there a use for this people want in the immediate future aside from the messages thing? Is the podcast streaming sats thing using the messages field or are there other fields it uses? |
…-for-msrv Pin `reqwest` to fix MSRV builds in CI
Yes, the main usecase is Podcasting 2.0: https://github.com/lightning/blips/blob/master/blip-0010.md |
|
Ah, damn, that uses a different TLV than other messages, okay, fair enough, hard to capture that in a single API that isn't full-blown custom TLVs 😭 |
|
should the |
…LDK-v0.0.122 Upgrade to LDK v0.0.123-beta
.. switching to `dyn KVStore + Send + Sync`
... now that we don't have any generic to hide, we can just use the `Node` type directly.
.. now that we can.
…yn-kvstore Drop `KVStore` type parameter from `Node`
.. just a minor cleanup to further modularize the codebase. Also, we'll be reusing these methods in `Event::ConnectionNeeded` soon.
.. we previously got reports from users trying to pay their own JIT invoice, which we currently don't support (and possibly never will). In order to avoid entering any weird states, we just reject our own circular payment.
.. which allows to filter and sort payment based on how recent they are.
…ent-hash-registrations
Unfortunately BDK's current wallet design requires us to have it live in `Mutex` that is locked for long periods of time during syncing. This is especially painful for short-lived operations that just operate locally, such as retrieving the current balance, which we now do in several places to be able to check Anchor channels limitations, e.g., in event handling. In order to avoid blocking during balance retrieval, we introduce a `balance` cache that will be refreshed whenever we're done with syncing *or* when we can successfully get the wallet lock. Otherwise, we'll just return the cached value, allowing us to make progress even though a background sync of the wallet might be in-progress.
Using a `Condvar` could be potentially dangerous in async contexts as `wait`ing on it might block the current thread potentially hosting more than one task. Here, we drop the `Condvar` and adopt a pub/sub scheme instead, similar to the one we already implemented in `ConnectionManager`.
It's not super clear that it achieves much in the face of a rate-limited Esplora server, and having a custom sleep there is just awkward. So we drop it and hope we still get a chance to sync our on-chain wallet now and then.
.. as we're not sure it actually increases reliability. We now only log failures, ignoring HTTP 400 as this is bitcoind's error code for "transaction already in mempool".
.. to make progress and unblock the `Mutex` even if BDK's wallet `sync` would never return.
.. even though we don't expect this to block, we're better safe than sorry and start to introduce timeouts for any calls we make to remote servers.
.. even though we don't expect this to block, we're better safe than sorry and start to introduce timeouts for any calls we make to remote servers.
.. even though we don't expect this to block, we're better safe than sorry and start to introduce timeouts for any calls we make to remote servers.
.. before initiating the Runtime shutdown.
.. as we use `Clone` for `tokio::sync::watch::Sender`, which was only introduced with 1.37.
…wallet-friction Improve on (potentially) blocking wallet behaviors
…lease Cut v0.3.0 release
Fix inbound capacity comment
…-upsteam' into keysend-custom-tlvs-upsteam # Conflicts: # bindings/ldk_node.udl # src/lib.rs # src/types.rs # tests/common/mod.rs
|
I screwed up merges on this one, and it's getting quite hairy; I will create a new PR and address all the comments. |
A very simple implementation of custom TLV records for keysend requests.