Sample v0 #4

valentinewallace · 2021-03-18T16:28:46Z

TODOs

fix forwarding a payment from lnd --> lnd through this node (not sure why this doesn't work right now, even if we set announce_channel = true it results in a no_route_found error from lnd)
in cli::send_payment -- use the route hints from the invoice
get this based on persister: Expose method to read ChannelMonitors from disk rust-lightning#863

TheBlueMatt · 2021-03-19T18:20:39Z

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/

TheBlueMatt

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

README.md

src/main.rs

valentinewallace · 2021-03-30T19:11:10Z

This is good for more review!

TheBlueMatt

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

src/disk.rs

TheBlueMatt · 2021-04-01T03:50:53Z

src/main.rs

+	keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
+) {
+	let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
+	loop {


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.

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?

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.

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

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.

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.

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.

src/main.rs

devrandom · 2021-04-20T12:47:03Z

src/main.rs

+				event_notifier.clone(),
+				tcp_stream,
+			)
+			.await;


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

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.

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

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

jkczyz

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.

jkczyz · 2021-04-21T18:56:43Z

src/main.rs

+				event_notifier.clone(),
+				tcp_stream,
+			)
+			.await;


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.

jkczyz · 2021-04-21T19:16:09Z

src/main.rs

+	keys_manager: Arc<KeysManager>, payment_storage: PaymentInfoStorage, network: Network,
+) {
+	let mut pending_txs: HashMap<OutPoint, Transaction> = HashMap::new();
+	loop {


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

src/main.rs

jkczyz · 2021-04-21T19:33:09Z

Cargo.lock

@@ -0,0 +1,792 @@
+# This file is automatically @generated by Cargo.


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

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.

rustfmt.toml

jkczyz · 2021-04-21T19:37:02Z

src/main.rs

+							println!(
+							        "\nEVENT: received payment from payment hash {} of {} millisatoshis",
+							        hex_utils::hex_str(&payment_hash.0),
+							        payment.amt_msat
+						      );
+							print!("> ");


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

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

src/cli.rs

valentinewallace · 2021-05-02T21:28:22Z

src/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;


@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 · 2021-05-03T21:34:53Z

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

…onnect to chan peers on startup

We're going to split payment storage into two maps, one for inbound and one for outbound.

To prevent clashes between payment hashes

valentinewallace · 2021-05-03T22:31:33Z

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

Squashed!

Onion handler

valentinewallace added 2 commits March 18, 2021 12:13

Initial commit

d0caa25

Fee estimator WIP

a696246

TheBlueMatt reviewed Mar 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Apr 1, 2021

View reviewed changes

valentinewallace force-pushed the sample-v0 branch from 478235b to fc93106 Compare April 2, 2021 19:36

TheBlueMatt reviewed Apr 7, 2021

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Apr 7, 2021

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Apr 7, 2021

View reviewed changes

src/main.rs Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

devrandom reviewed Apr 20, 2021

View reviewed changes

jkczyz reviewed Apr 21, 2021

View reviewed changes

TheBlueMatt mentioned this pull request Apr 27, 2021

Event handling races with channelmanager storage lightningdevkit/rust-lightning#899

Closed

valentinewallace commented May 2, 2021

View reviewed changes

valentinewallace added 15 commits May 3, 2021 18:29

Initial node

1b034d6

Next draft. Features: connectpeer, listchannels, sendpayment, autorec…

224d32a

…onnect to chan peers on startup

Add readme, fix license and a few other cleanups

25a4f60

Forward HTLCs after a timeout and a few other cleanups

8091682

add rustfmt + CI + other fixups

d9e9c01

working before async refactor

ac0678c

Initial refactor to add tokio::main and be fully async

b630ab9

Post-async refactor cleanup

6781199

Update to RL master and fix PendingHTLCsForwardable

b4a5058

Remove unnecessary todo

d85a979

Read routehints from invoices when sending a payment

b17e298

Include min_final_cltv when generating invoices

0a08f4a

Move startup to separate from main for better errors

8035757

Update to latest upstream RL

0f86358

SatoshiAmount -> MillisatAmount and PaymentInfo struct

5ec1c3f

valentinewallace added 5 commits May 3, 2021 18:31

Remove HTLCDirection struct

9157f86

We're going to split payment storage into two maps, one for inbound and one for outbound.

Split payments hashmap into inbound and outbound

ed6f3c6

To prevent clashes between payment hashes

Check payment secret before succeeding payment

85e890b

Adapt to the lightning_invoice crate moving to Rust-Lightning

432106e

Update to latest RL + fix step numbers + nodeinfo + listpeers commands

a73d564

valentinewallace force-pushed the sample-v0 branch from bebb837 to a73d564 Compare May 3, 2021 22:31

jkczyz approved these changes May 4, 2021

View reviewed changes

jkczyz merged commit 6199433 into lightningdevkit:main May 4, 2021

orbitalturtle added a commit to orbitalturtle/ldk-sample that referenced this pull request Jan 17, 2024

Merge pull request lightningdevkit#4 from lndk-org/onion-handler

cdb5975

Onion handler

		@@ -0,0 +1,792 @@
		# This file is automatically @generated by Cargo.

Sample v0 #4

Sample v0 #4

Uh oh!

Conversation

valentinewallace commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Mar 19, 2021

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Mar 30, 2021

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz commented May 3, 2021

Uh oh!

valentinewallace commented May 3, 2021

Uh oh!

Uh oh!

valentinewallace commented Mar 18, 2021 •

edited

Loading