Implement the UtxoSource interface for REST/RPC clients #2248

TheBlueMatt · 2023-04-30T01:17:44Z

In LDK, we expect users operating nodes on the public network to
implement the UtxoSource interface in order to validate the
gossip they receive from the network.

Sadly, because the DoS attack of flooding a node's gossip store
isn't a common issue, and because we do not provide an
implementation off-the-shelf to make doing so easily, many of our
downstream users do not have a UtxoSource implementation.

In order to change that, here we implement an async UtxoSource
in the lightning-block-sync crate, providing one for users who
sync the chain from Bitcoin Core's RPC or REST interfaces.

codecov-commenter · 2023-04-30T03:07:17Z

Codecov Report

Patch coverage: 74.50% and project coverage change: +0.10% 🎉

Comparison is base (685f266) 90.33% compared to head (a5cb5e3) 90.44%.
Report is 131 commits behind head on main.

❗ Current head a5cb5e3 differs from pull request most recent head b691d4d. Consider uploading reports for the commit b691d4d to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2248      +/-   ##
==========================================
+ Coverage   90.33%   90.44%   +0.10%     
==========================================
  Files         106      106              
  Lines       55732    56698     +966     
  Branches    55732    56698     +966     
==========================================
+ Hits        50347    51279     +932     
- Misses       5385     5419      +34

Files Changed	Coverage Δ
lightning-block-sync/src/lib.rs	`93.82% <ø> (ø)`
lightning-block-sync/src/convert.rs	`87.79% <41.66%> (-2.82%)`	⬇️
lightning-block-sync/src/rest.rs	`70.96% <79.31%> (+3.78%)`	⬆️
lightning-block-sync/src/rpc.rs	`78.08% <84.21%> (+0.84%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`88.98% <100.00%> (+0.06%)`	⬆️
lightning/src/routing/gossip.rs	`89.73% <100.00%> (ø)`
lightning/src/routing/router.rs	`93.63% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightning-block-sync/src/gossip.rs

dunxen

Just a first pass for docs. Will look at caching a bit more in depth still today.

lightning-block-sync/src/rpc.rs

lightning-block-sync/src/gossip.rs

tnull · 2023-06-12T09:03:49Z

Unfortunately, this seems to be in need of a rebase.

TheBlueMatt · 2023-06-12T17:01:05Z

Rebased (just needed a use update).

alecchendev

First pass, generally things make sense. Correct me if I'm wrong, but the goal here is more to provide an implementation of UtxoLookup right? And implementing UtxoSource makes it easier to do so since you can just provide that to GossipVerifier instead of implementing UtxoLookup directly? The PR description confused me a little initially since it's mainly talking about UtxoSource

lightning-block-sync/src/convert.rs

lightning-block-sync/src/gossip.rs

alecchendev · 2023-07-11T19:24:46Z

lightning-block-sync/src/gossip.rs

+		let pm = Arc::clone(&self.peer_manager);
+		self.spawn.spawn(async move {
+			let res = Self::retrieve_utxo(source, block_cache, short_channel_id).await;
+			fut.resolve(gossiper.network_graph(), &*gossiper, res);
+			pm.process_events();


Why is it necessary to call PeerManager::process_events here?

process_events has to be called cause we now have a pending outbound message waiting to be sent (or possibly we are ready to process more messages from the peer's socket, which we stopped reading from cause we were busy). If we don't call it, the message will go out when we next hit a timer tick in the background processor (or some other regular event calls it for us) but this may be a while.

lightning-block-sync/src/gossip.rs

TheBlueMatt · 2023-07-24T21:54:11Z

First pass, generally things make sense. Correct me if I'm wrong, but the goal here is more to provide an implementation of UtxoLookup right? And implementing UtxoSource makes it easier to do so since you can just provide that to GossipVerifier instead of implementing UtxoLookup directly? The PR description confused me a little initially since it's mainly talking about UtxoSource

Right, a user could implement UtxoSource themselves, or, more likely, they'll use the new ones built into lightning-block-sync which do UTXO queries against Bitcoin Core's API.

alecchendev

Looking pretty good I think!

alecchendev · 2023-07-27T22:47:53Z

lightning-block-sync/src/gossip.rs

+				let recent_blocks = block_cache.lock().unwrap();
+				for (height, block) in recent_blocks.iter() {
+					if *height == block_height {
+						process_block!(block);
+						break 'tx_found;
+					}
+				}


Making a note that since we don't move the block to the end of the queue here, we may pop off a block even though it's been used recently - probably not a big deal, but just something that popped up

Yea, I think its fine. Making it LRU is possible, but we'd have to shift the array, which isn't bad but is kinda annoying. Also, I don't think the behavior of "LRFirstUsed" is all that bad, either, really.

lightning-block-sync/src/gossip.rs

This is actually a valid response in some cases, at least for the `gettxout` command, where `null` is returned if no corresponding UTXO was found, but the command otherwise succeeded.

TheBlueMatt · 2023-07-28T19:05:28Z

Rebased with a trivial syntax fix to hopefully fix CI.

TheBlueMatt · 2023-08-19T18:04:13Z

Squashed fixups without changes.

wpaulino

LGTM, but will try to test this with the sample before approving

lightning-block-sync/src/convert.rs

lightning-block-sync/src/gossip.rs

lightning/src/ln/functional_test_utils.rs

lightning-block-sync/src/gossip.rs

TheBlueMatt · 2023-08-23T03:43:04Z

LGTM, but will try to test this with the sample before approving

I've been running this on my sample node on mainnet since basically when this PR was open, do you want a patch or do you want to try to do it to see how hard the API is to use?

wpaulino · 2023-08-23T21:21:44Z

Feel free to squash but CI needs a small fix.

TheBlueMatt · 2023-08-23T21:39:14Z

Pushed incorporating the required fix:

$ git diff-tree -U1 71452c03 189c1fbe
diff --git a/lightning-block-sync/src/convert.rs b/lightning-block-sync/src/convert.rs
index e452cb807..bf9e95776 100644
--- a/lightning-block-sync/src/convert.rs
+++ b/lightning-block-sync/src/convert.rs
@@ -52,4 +52,4 @@ impl TryInto<BlockHash> for BinaryResponse {
 		BlockHash::from_slice(&self.0).map_err(|_|
-			Err(std::io::Error::new(std::io::ErrorKind::InvalidData, "bad block hash length"))
-		)?
+			std::io::Error::new(std::io::ErrorKind::InvalidData, "bad block hash length")
+		)
 	}

In LDK, we expect users operating nodes on the public network to implement the `UtxoSource` interface in order to validate the gossip they receive from the network. Sadly, because the DoS attack of flooding a node's gossip store isn't a common issue, and because we do not provide an implementation off-the-shelf to make doing so easily, many of our downstream users do not have a `UtxoSource` implementation. In order to change that, here we implement an async `UtxoSource` in the `lightning-block-sync` crate, providing one for users who sync the chain from Bitcoin Core's RPC or REST interfaces.

Because a `UtxoLookup` implementation is likely to need a reference to the `PeerManager` which contains a reference to the `P2PGossipSync`, it is likely to be impossible to get a mutable reference to the `P2PGossipSync` by the time we want to add a `UtxoLookup` without a ton of boilerplate and trait wrapping. Instead, we simply place the `UtxoLookup` in a `RwLock`, allowing us to modify it without a mutable self reference. The lifetime bounds updates in tests required in this commit are entirely unclear to me, but do allow tests to continue building, so somehow make rustc happier.

The BOLT spec mandates that channels not be announced until they have at least six confirmations. This is important to enforce not because we particularly care about any specific DoS concerns, but because if we do not we may have to handle reorgs of channel funding transactions which change their SCID or have conflicting SCIDs.

wpaulino · 2023-08-24T20:00:15Z

lightning-block-sync/src/gossip.rs

+
+			let block_hash = source.get_block_hash_by_height(block_height).await
+				.map_err(|_| UtxoLookupError::UnknownTx)?;
+			let block_data = source.get_block(&block_hash).await


We could end up fetching the block multiple times here since we're not holding the lock, but probably not a big deal since we're already ok with the bandwidth costs associated with P2P sync and validation.

Yea, and we can't hold the lock across an await point. It does suck, I agree, but storing and poll'ing other futures sounded hard 🤷

tnull

LGTM

tnull · 2023-08-25T11:22:09Z

lightning-block-sync/src/gossip.rs

+///
+/// If the `tokio` feature is enabled, this is implemented on `TokioSpawner` struct which
+/// delegates to `tokio::spawn()`.
+pub trait FutureSpawner : Send + Sync + 'static {


non-blocking nit: Technically this does not spawn futures but tasks. :P

One day we may be able to replace that with an std trait: rust-lang/wg-async#283

non-blocking nit: Technically this does not spawn futures but tasks. :P

bleh. if you think its gonna confuse anyone ill change it but i doubt it otherwise.

One day we may be able to replace that with an std trait: rust-lang/wg-async#283

Yea, rust async is such an MVP...sadly its gotten all hung up on deciding big hairy decisions so never really got past MVP :(

TheBlueMatt mentioned this pull request Apr 30, 2023

Trivial PeerManager cleanups #2249

Merged

TheBlueMatt force-pushed the 2023-04-gossip-check branch 3 times, most recently from aaff5be to 2edc4f4 Compare April 30, 2023 01:39

TheBlueMatt force-pushed the 2023-04-gossip-check branch from c241108 to 7c51581 Compare April 30, 2023 03:58

TheBlueMatt mentioned this pull request Apr 30, 2023

Server considers incomplete data as complete initial sync lightningdevkit/rapid-gossip-sync-server#28

Closed

TheBlueMatt commented May 1, 2023

View reviewed changes

lightning-block-sync/src/gossip.rs Outdated Show resolved Hide resolved

wpaulino self-requested a review May 2, 2023 21:57

TheBlueMatt added the Seeking Code Review label May 16, 2023

dunxen reviewed Jun 5, 2023

View reviewed changes

lightning-block-sync/src/rpc.rs Outdated Show resolved Hide resolved

lightning-block-sync/src/gossip.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2023-04-gossip-check branch from 7c51581 to 69ad06e Compare June 5, 2023 17:24

TheBlueMatt force-pushed the 2023-04-gossip-check branch from 69ad06e to 0bef89d Compare June 12, 2023 17:00

alecchendev reviewed Jul 11, 2023

View reviewed changes

alecchendev reviewed Jul 19, 2023

View reviewed changes

lightning-block-sync/src/gossip.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2023-04-gossip-check branch from 0bef89d to 8cf0271 Compare July 24, 2023 22:11

alecchendev reviewed Jul 27, 2023

View reviewed changes

Accept RPC responses with a null result

8164cb9

This is actually a valid response in some cases, at least for the `gettxout` command, where `null` is returned if no corresponding UTXO was found, but the command otherwise succeeded.

TheBlueMatt force-pushed the 2023-04-gossip-check branch from d626657 to 80aab12 Compare July 28, 2023 19:05

TheBlueMatt force-pushed the 2023-04-gossip-check branch 2 times, most recently from 117532c to a5cb5e3 Compare July 28, 2023 19:08

TheBlueMatt force-pushed the 2023-04-gossip-check branch from a5cb5e3 to da20a14 Compare August 19, 2023 18:04

wpaulino reviewed Aug 23, 2023

View reviewed changes

lightning-block-sync/src/convert.rs Outdated Show resolved Hide resolved

lightning-block-sync/src/gossip.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_test_utils.rs Show resolved Hide resolved

lightning-block-sync/src/gossip.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2023-04-gossip-check branch from da20a14 to 71452c0 Compare August 23, 2023 03:42

TheBlueMatt force-pushed the 2023-04-gossip-check branch from 71452c0 to b691d4d Compare August 23, 2023 21:38

TheBlueMatt added 4 commits August 23, 2023 21:48

Add a simple naive block cache in gossip sync lookups

3482fce

TheBlueMatt force-pushed the 2023-04-gossip-check branch from b691d4d to 189c1fb Compare August 23, 2023 21:48

wpaulino approved these changes Aug 24, 2023

View reviewed changes

tnull approved these changes Aug 25, 2023

View reviewed changes

tnull merged commit 3dffe54 into lightningdevkit:main Aug 25, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the UtxoSource interface for REST/RPC clients #2248

Implement the UtxoSource interface for REST/RPC clients #2248

TheBlueMatt commented Apr 30, 2023

codecov-commenter commented Apr 30, 2023 •

edited

dunxen left a comment

tnull commented Jun 12, 2023

TheBlueMatt commented Jun 12, 2023

alecchendev left a comment

alecchendev Jul 11, 2023

TheBlueMatt Jul 24, 2023

TheBlueMatt commented Jul 24, 2023

alecchendev left a comment

alecchendev Jul 27, 2023

TheBlueMatt Jul 28, 2023

TheBlueMatt commented Jul 28, 2023 •

edited

TheBlueMatt commented Aug 19, 2023

wpaulino left a comment

TheBlueMatt commented Aug 23, 2023

wpaulino commented Aug 23, 2023

TheBlueMatt commented Aug 23, 2023 •

edited

wpaulino Aug 24, 2023

TheBlueMatt Aug 24, 2023

tnull left a comment

tnull Aug 25, 2023

TheBlueMatt Aug 25, 2023

Implement the UtxoSource interface for REST/RPC clients #2248

Implement the UtxoSource interface for REST/RPC clients #2248

Conversation

TheBlueMatt commented Apr 30, 2023

codecov-commenter commented Apr 30, 2023 • edited

Codecov Report

dunxen left a comment

Choose a reason for hiding this comment

tnull commented Jun 12, 2023

TheBlueMatt commented Jun 12, 2023

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 24, 2023

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 28, 2023 • edited

TheBlueMatt commented Aug 19, 2023

wpaulino left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 23, 2023

wpaulino commented Aug 23, 2023

TheBlueMatt commented Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 30, 2023 •

edited

TheBlueMatt commented Jul 28, 2023 •

edited

TheBlueMatt commented Aug 23, 2023 •

edited