Upgrade to LDK 0.0.125 #136
Conversation
Onion messages are now useful for various things so having a demo use for them isn't all that interesting anymore.
TheBlueMatt
force-pushed
the
main
branch
5 times, most recently
from
f87f634 to
c6bf6ab
Compare
There was a problem hiding this comment.
LGTM, although one question regarding dropping sendonionmessage:
Mh, so having this command available was a real easy way to start playing with onion messages. While we don't think of LDK sample as a production node, it at least used to be the defacto default way to get started quickly with LDK. I wonder if should reconsider doing lightningdevkit/ldk-node#121 if we drop this functionality here, really just to allow users to experiment with custom onion messages, do measurements, etc (know at least two (research) projects that did stuff like that based on forked LDK sample).
| } | ||
| }, | ||
| Event::FundingTxBroadcastSafe { .. } => { | ||
| // We don't use the manual broadcasting feature, so this event should never be seen. |
There was a problem hiding this comment.
Should we put an unreachable or at least a debug_assert here and in the cases below?
There was a problem hiding this comment.
Hmm, we hadn't previously done that. Its possible that in the future these events get used more broadly and I'd rather not forget to remove an assertion.
TheBlueMatt
force-pushed
the
main
branch
2 times, most recently
from
fdd3822 to
efba2f8
Compare
|
Re: custom onion message send, I'm pretty meh on it. If someone wants to use ldk-sample to do network measurements of onion messages they're gonna need both the send side and the receive side, which we hadn't implemented here. We generally have preferred to keep |
|
Also pushed a |
src/bitcoind_client.rs
Outdated
| tx_serialized); | ||
| print!("Warning, failed to broadcast a transaction, this is likely okay but may indicate an error: {}\n> ", err_str); | ||
| } | ||
| // We assume `submitpackage` is available (Bitcoin Core 25, at least). |
There was a problem hiding this comment.
Mhh, isn't it only available in general starting with v26? And wouldn't this use of it not still require v28?
There was a problem hiding this comment.
Oh, maybe, I dunno I just checked my 25 node and it at least exists. Even if its just 26 seems fine to use it here? Indeed, until 28 it may not do anything, but it also shouldn't hurt. I'll update the comment at least.
There was a problem hiding this comment.
Mh, well, would it actually work for broadcasting on mainnet, or is it a no-op until v0.28?
There was a problem hiding this comment.
Ah, it seems it will be usable just to broadcast starting with v26 (see https://bitcoincore.org/en/releases/26.0/). Good to know, might do that in lightningdevkit/ldk-node#370 from the getgo, too.
LDK has deprecated the `ChannelDetails` balance retrieval because its generally not what users want. Instead, we use the `ChainMonitor` `Balance` list.
|
Switched to LDK 0.0.125 instead. |
No description provided.