Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Sample v0 by valentinewallace · Pull Request #4 · lightningdevkit/ldk-sample · GitHub
Skip to content
This repository was archived by the owner on Aug 3, 2026. It is now read-only.

Sample v0 - #4

Merged
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0
May 4, 2021
Merged

Sample v0#4
jkczyz merged 22 commits into
lightningdevkit:mainfrom
valentinewallace:sample-v0

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

TODOs

@TheBlueMatt

Copy link
Copy Markdown
Contributor

nit: there is a bunch of inconsistent whitespace here, does it make since to use rustfmt (do you like its formatting in this crate?) or, if not, it might be useful to have this in vimrc (depending on your terminal settings):

au BufEnter * hi Tab ctermbg=black
au BufEnter * syntax match Tab /\t/

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really awesome work. A few comments about how we interact with tokio to start.

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

This is good for more review!

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great, I didn't do a careful review, but glanced over most of it.

Comment threadsrc/disk.rs Outdated
Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I think we should move the loop to the background-processor crate. We already spawn a background thread there that does waiting on the ChannelManager until we do things to it, I think that's fine for use as a fetch-events loop, too. It would require another parameter in the form of the ChainMonitor, but I think that's OK. Also, technically, we must process all pending events before writing the ChannelManager - otherwise we have races where we may have gotten an event, written the manager to disk without it, and then crashed before handling it.

Sadly, this is made more complicated by our desire to be async here. We'll probably want to allow the event-handling function to by async, and then call https://docs.rs/futures/0.3.13/futures/executor/fn.block_on.html on whatever is returned. I think this would mean we can't use #[tokio::main] (because we can't block on tokio net futures in another executor) but need a global tokio::Runtime, need to call Runtime::enter() at the top of the event-handling function and call tokio::spawn and return the tokio::JoinHandle as a future. (I think you're allowed to do this, but would need to check if you can poll a tokio::JoinHandle in an outside executor.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could we also assume that backgroundprocessor::start() is called from within a tokio runtime?

In any case, are you fine to push this to follow-up? I can open an issue in rust-lightning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, a followup is fine, but I don't think calling ::start() from within a runtime fixes the issue (nor should we assume it if we don't have to) - it spawns a new thread directly, and that thread isn't in the runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just implemented an async version of background-processor in https://gitlab.com/lightning-signer/lnrod/-/merge_requests/10/diffs . Perhaps we want to add that to the original crate under a tokio feature gate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Wasn't sure if our use of Condvar in ChannelManager would be a problem, but I think it's ok to mix non-tokio sync code.

@TheBlueMatt What do you think? Looks like @devrandom's change could be DRY-ed up and included without much repetition.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the use of tokio::task::spawn_blocking allows standard blocking code to be spawned inside a tokio runtime. It uses a separate thread that isn't responsible for other things. The Condvar waiting is inside that thread.

Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this waits until the connection is closed which is not what you want here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think anything in a tokio::spawn makes progress regardless, since the threadpool implicitly awaits it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that wasn't clear. You would need another spawn on the setup_inbound to fire it off.

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Very well organized and easy to read through. Thanks for spending so much effort on making this guide-worthy. 😄

Left mostly minor comments. Still tinkering a bit on a node abstraction, but that can come as a follow-up.

Comment threadsrc/main.rs Outdated
event_notifier.clone(),
tcp_stream,
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the future need to be held onto in this case? If so, we could push them into a Vec. Our lightning-net-tokio doesn't do so, but that may be an oversight.

Comment threadsrc/main.rs
keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
) {
let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be worth making an async version of background-processor? Or does that require modifying the guts of ChannelManager as well?

Comment threadsrc/main.rs
Comment threadsrc/main.rs Outdated
Comment threadCargo.lock
@@ -0,0 +1,792 @@
# This file is automatically @generated by Cargo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be included because this is a binary? I vaguely recall that being the case but just want to make sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say you want this to protect against dependency attacks by locking to hashes. i.e. supposedly you have reviewed all the locked dependencies and only need to re-review if this file gets modified.

Comment threadrustfmt.toml Outdated
Comment threadsrc/main.rs
Comment on lines +161 to +175
println!(
"\nEVENT: received payment from payment hash {} of {} millisatoshis",
hex_utils::hex_str(&payment_hash.0),
payment.amt_msat
);
print!("> ");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to log these? The user may be typing something when these are printed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be better. Down to address in follow-up.

Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs Outdated
Comment threadsrc/cli.rs
let peer_mgr = peer_manager.clone();
let event_ntfns = event_notifier.clone();
tokio::spawn(async move {
lightning_net_tokio::setup_outbound(peer_mgr, event_ntfns, pubkey, stream).await;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt any idea why outbound connections to RL nodes isn't working? the node ID never shows up in get_peer_node_ids. Outbound connections to lnd work.

@jkczyz

Copy link
Copy Markdown
Contributor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

@valentinewallace

Copy link
Copy Markdown
ContributorAuthor

Could you squash any fixup commits? Feel free to squash everything in one commit if that makes the most sense.

Squashed!

@jkczyz
jkczyz merged commit 6199433 into lightningdevkit:mainMay 4, 2021
orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@valentinewallace@TheBlueMatt@jkczyz@devrandom