-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consolidates building a node tutorial #156
Consolidates building a node tutorial #156
Conversation
✅ Deploy Preview for lightningdevkit ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Concept ACK |
e81534c
to
2a05a55
Compare
2a05a55
to
c597f31
Compare
c597f31
to
bf123ea
Compare
8cda8b0
to
95ca4ce
Compare
|
||
There are a few dependencies needed to get this working. Let's walk through setting up each one so we can plug them into our `ChannelManager`. | ||
|
||
### Initialize the `FeeEstimator` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should provide an example using BDK's new fee estimator API.
docs/tutorials/building-a-node-with-ldk/setting-up-a-channel-manager.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick scroll through and annotated what stuck out to me. Feel free to disregard if many of the things are still WIP.
docs/tutorials/building-a-node-with-ldk/setting-up-a-channel-manager.md
Outdated
Show resolved
Hide resolved
docs/tutorials/building-a-node-with-ldk/setting-up-a-channel-manager.md
Outdated
Show resolved
Hide resolved
docs/tutorials/building-a-node-with-ldk/setting-up-a-channel-manager.md
Outdated
Show resolved
Hide resolved
docs/tutorials/building-a-node-with-ldk/setting-up-a-channel-manager.md
Outdated
Show resolved
Hide resolved
de5e256
to
4bd2cde
Compare
4bd2cde
to
c1fef0b
Compare
52c58df
to
f8f7404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a ~full high-level pass.
While it's nice to have a single flow, I'm not fully convinced that a unified tutorial improves readability. I think at times it can be confusing where the APIs or implementation behavior of the different languages diverge slightly.
3. Payments & Routing, ability to create and pay invoices. | ||
|
||
To make the above work we also need to setup a series of supporting modules, including: | ||
1. A [`FeeEstimator`](https://docs.rs/lightning/*/lightning/chain/chaininterface/trait.FeeEstimator.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice if each of these list items would hold a short description giving some context what the respective modules are used/required for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would mostly be repeating what is in the Rust docs though so I'd rather just lean on that.
docs/tutorials/building-a-node-with-ldk/setting-up-a-peer-manager.md
Outdated
Show resolved
Hide resolved
docs/tutorials/building-a-node-with-ldk/setting-up-a-peer-manager.md
Outdated
Show resolved
Hide resolved
docs/tutorials/building-a-node-with-ldk/setting-up-a-peer-manager.md
Outdated
Show resolved
Hide resolved
docs/tutorials/building-a-node-with-ldk/setting-up-a-peer-manager.md
Outdated
Show resolved
Hide resolved
Event::PaymentFailed { payment_hash, rejected_by_dest } => { | ||
// Handle failed payment | ||
} | ||
Event::FundingGenerationReady { .. } => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to at least mention PaymentClaimable
,PendingHTLCsForwardable
, SpendableOutputs
here, as they really need to be implemented. Possibly also BumpTransaction
if users use Anchors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if they were mentioned in context like - https://deploy-preview-156--lightningdevkit.netlify.app/tutorials/building-a-node-with-ldk/opening-a-channel/
a7fa74b
to
fe72a99
Compare
6c08290
to
5e80f4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a ~full high-level pass.
While it's nice to have a single flow, I'm not fully convinced that a unified tutorial improves readability. I think at times it can be confusing where the APIs or implementation behavior of the different languages diverge slightly.
I have similar concerns. Perhaps separate pages should be dedicated to components where the interfaces diverge (i.e., pull each of those components out of the ChannelManager
setup guide).
With that in mind, it seems there's so much needed in terms of step-by-step instructions to set up ChannelManager
. But a good number of the examples are just skeleton implementations of traits or simply demonstrate how to pass parameters to preexisting implementations. Whether here or later, maybe we should take an opportunity to pare back that portion of the guide to have fewer and more consolidated examples. The previous suggestion may help as well on cutting down the size.
```rust | ||
let network_graph_path = format!("{}/network_graph", ldk_data_dir.clone()); | ||
let network_graph = Arc::new(disk::read_network(Path::new(&network_graph_path), args.network, logger.clone())); | ||
|
||
let scorer_path = format!("{}/scorer", ldk_data_dir.clone()); | ||
let scorer = Arc::new(RwLock::new(disk::read_scorer( | ||
Path::new(&scorer_path), | ||
Arc::clone(&network_graph), | ||
Arc::clone(&logger), | ||
))); | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using utilities from the sample, which aren't available in LDK. Similarly above for gossip sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of keeping the code snippets short maybe it's ok?
|
||
</CodeSwitcher> | ||
|
||
References: [Rust `Event` docs](https://docs.rs/lightning/0.0.114/lightning/util/events/enum.Event.html), [Java/Kotlin `Event` bindings](https://github.com/lightningdevkit/ldk-garbagecollected/blob/main/src/main/java/org/ldk/structs/Event.java) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to use *
or latest
instead of version numbers. Same elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 39e95b4
```rust | ||
use lightning::util::events::{Event}; | ||
|
||
// In the event handler passed to BackgroundProcessor::start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like BackgroundProcessor
was removed from the guide. We'll want to have a section on it as it drives everything. Probably somewhere in the introduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 43b12ca
1. A [`PeerManager`](https://docs.rs/lightning/*/lightning/ln/peer_handler/struct.PeerManager.html), for establishing TCP/IP connections to other nodes on the lightning network. | ||
2. A [`ChannelManager`](https://docs.rs/lightning/*/lightning/ln/channelmanager/struct.ChannelManager.html), to open and close channels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to switch the order of these two given the order they are covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 31f155f
To add a `ChannelManager` to your application, run: | ||
|
||
<CodeSwitcher :languages="{rust:'Rust', kotlin:'Kotlin', swift:'Swift'}"> | ||
<template v-slot:rust> | ||
|
||
```rust | ||
use lightning::ln::channelmanager; | ||
|
||
let channel_manager = ChannelManager::new( | ||
&fee_estimator, | ||
&chain_monitor, | ||
&broadcaster, | ||
&router, | ||
&logger, | ||
&entropy_source, | ||
&node_signer, | ||
&signer_provider, | ||
user_config, | ||
chain_params, | ||
current_timestamp | ||
); | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This alone isn't very valuable and requires that prerequisites that follow. Additionally, it is repeated in greater detail later on under "Initialize the ChannelManager
".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the ChannelManager
is so "involved" I find it useful to start with what one might end up with as it helps shape the mental model of a critical piece of the API. I feel like when you see this the next logical question is well how do I set up a FeeEstimator
etc arguably it's already covered in the intro. Slight rewording d26a8e8
2. Tell LDK what your best known block header and height is. | ||
3. Call `channel_manager_constructor.chain_sync_completed(..)` to complete the initial sync process. | ||
|
||
**References:** [Rust `Confirm` docs](https://docs.rs/lightning/*/lightning/chain/trait.Confirm.html), [Rust `Listen` docs](https://docs.rs/lightning/*/lightning/chain/trait.Listen.html), [Rust `lightning_block_sync` module docs](https://docs.rs/lightning-block-sync/*/lightning_block_sync/), [Rust `lightning_transaction_sync` module docs](https://docs.rs/lightning-transaction-sync/*/lightning_transaction_sync/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/module/crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5787ac6
**Full Blocks or BIP 157/158** | ||
|
||
If you are connecting full blocks or using BIP 157/158, then it is recommended to use | ||
LDK's `lightning_block_sync` sample module as in the example above: the high-level steps that must be done for both `ChannelManager` and each `ChannelMonitor` are as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/lightning_block_sync
sample module/lightning-block-sync
crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 74a2f49
&fee_estimator, | ||
&chain_monitor, | ||
&broadcaster, | ||
&router, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most recent version of LDK has ChannelManager
take Router
, so this is correct. But a later section with a similar examples is using an older version of LDK that does not take a Router
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8f276a0
0f797b1
to
63d3ead
Compare
I feel like it is quite a common pattern/layout in SDK's that offer multiple languages, some examples include.
The structure is supposed to reflect someone who never built an LDK node before and wants to build the simplest node that can perform the basic functions. More detailed/advanced examples can follow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another pass.
Let's start by looking at the core components we'll need to make this node work for the tasks we outlined above. | ||
|
||
1. A [`ChannelManager`](https://docs.rs/lightning/*/lightning/ln/channelmanager/struct.ChannelManager.html), to open and close channels. | ||
2. A [`PeerManager`](https://docs.rs/lightning/*/lightning/ln/peer_handler/struct.PeerManager.html), for establishing TCP/IP connections to other nodes on the lightning network. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Technically it's not the PeerManager
, but the network stack, i.e., lightning-net-tokio
that establishes the connections. The PeerManager
just keeps track of them and the associated peer state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated d41fecb
|
||
</CodeSwitcher> | ||
|
||
**Implementation notes:** this struct is not required if you are providing your own routes. It will be used internally in `ChannelManager` to build a `NetworkGraph`. Other networking options are: `LDKNetwork_Bitcoin`, `LDKNetwork_Regtest` and `LDKNetwork_Testnet` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The networking options here are really bindings specific types, won't work in Rust as described. Also not sure what 'other' is referring to, and Signet
seems missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in eb7ca72
let router = DefaultRouter::new( | ||
network_graph.clone(), | ||
logger.clone(), | ||
keys_manager.get_secure_random_bytes(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think by now it takes an EntropySource
(i.e., KeysManager
reference) directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet is from ldk-node
and ldk-sample
. I don't see an example with EntropySource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think this was part of the bindings fixes for the last release, i.e., they have not 'officially' released on the Rust side yet. But probably makes sense to already update to the upcoming API versions here to not have the guide immediately outdated.
Co-authored-by: Elias Rohrer <[email protected]>
Co-authored-by: Elias Rohrer <[email protected]>
Co-authored-by: Elias Rohrer <[email protected]>
Co-authored-by: Elias Rohrer <[email protected]>
This uses the new
CodeSwitcher
so we can have oneBuilding a node tutorial
but have code examples for multiple languages in one place. For instance, clicking the "Java" tab changes all the code examples on the page to be Java. This PR includes Rust, Java, Kotlin. Swift + JavaScript/TypeScript to followIt also reworks the content structure to be more in line with how we currently explain LDK.
TO-DO
Includes minor changes to the navigation layout.
Preview