Add simple probing API

[tnull]

This PR adds a simple API for sending payment probes and introduces the concept of probing cookies:
When we send payment probes, we generate the PaymentHash based on a probing cookie secret and a random PaymentId. This allows us to discern probes from real payments, without keeping additional state.

I also included a minor refactor to generalize PaymentParameters::max_mpp_path_count (now: max_path_count) and to make sure a maximum allowed path count of 1 results in us never trying to find an MPP path (i.e., allow_mpp = false).

[codecov-commenter]

Codecov Report

Merging #1567 (40339b9) into main (f3d5b94) will decrease coverage by 0.14%.
The diff coverage is 60.07%.

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

@@            Coverage Diff             @@
##             main    #1567      +/-   ##
==========================================
- Coverage   91.07%   90.93%   -0.15%     
==========================================
  Files          80       80              
  Lines       44128    45110     +982     
  Branches    44128    45110     +982     
==========================================
+ Hits        40190    41020     +830     
- Misses       3938     4090     +152     

Impacted Files	Coverage Δ
lightning/src/routing/scoring.rs	96.06% <12.50%> (-0.74%)	⬇️
lightning-invoice/src/payment.rs	87.76% <15.27%> (-5.15%)	⬇️
lightning/src/util/events.rs	39.25% <18.91%> (-2.74%)	⬇️
lightning/src/routing/router.rs	92.39% <65.00%> (-0.23%)	⬇️
lightning/src/ln/channelmanager.rs	84.88% <93.61%> (+0.14%)	⬆️
lightning/src/ln/payment_tests.rs	98.57% <96.70%> (-0.69%)	⬇️
lightning/src/ln/mod.rs	95.00% <100.00%> (ø)
lightning/src/chain/onchaintx.rs	93.98% <0.00%> (-0.93%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3d5b94...eb8bce0. Read the comment docs.

[tnull]

Hm, also unsure why fuzz is currently broken. I assume it is connected to the changes made to ChannelManager serialization?

[TonyGiorgio]

Cool to see probing get better support. One question on implementing this. You'd still go through the Event manager to find PaymentPathFailed events, check if they were a probe with payment_is_probe(), and then handle it appropriate?

One thing I was finding when I was doing probing is that I needed access to the error reasons to really discern the reason for the probe failure. In my fork of LDK, I had to remove the test config flags here:

rust-lightning/lightning/src/util/events.rs

Lines 380 to 383 in a600eee

	#[cfg(test)]
	error_code: Option<u16>,
	#[cfg(test)]
	error_data: Option<Vec<u8>>,

Maybe a PR to remove that could be useful here? Or maybe a better way to discern the errors.

My implementation is here if curious:

https://github.com/BitcoinDevShop/hidden-lightning-network/blob/4a016a356edbbf4764fb07ec39cbf5fe842da9e9/src/main.rs#L251-L270

[TheBlueMatt]

Its kinda awkward to make this a part of the Router interface just so that the send_probe_along_path method takes a pubkey list, we could just have that method take a Route and let the user build the route any way they want (either by getting a route or by getting a pubkey list). That seems like a strictly more general API and reduces the method set in the Router trait which is nice.

[tnull]

Hm, but isn't the interface in InvoicePayer supposed to wrap more functionality in a single method call, such as pay_invoice, pay_pubkey? If send_route_along_path should take a Route I honestly don't see it's utility over simply using send_probing_payment in ChannelManager directly, i.e., I'd just leave it out of InvoicePayer entirely?

[jkczyz]

Yeah, seems allowing arbitrary Routes could complicated the semantics, too, if say the route has more than one path.

[TheBlueMatt]

Hm, but isn't the interface in InvoicePayer supposed to wrap more functionality in a single method call, such as pay_invoice, pay_pubkey?

Sure, but by that argument we should have a probe_node method, not a probe_preselected_path method. I don't quite understand what the exact use-case is for a prober that probes a given path, selected by public keys (how did the user generate that path?) that isn't equivalently covered by a Route.

If send_route_along_path should take a Route I honestly don't see it's utility over simply using send_probing_payment in ChannelManager directly, i.e., I'd just leave it out of InvoicePayer entirely?

I understood it as something that corrects the event handling for users. That said, with the switch to dedicated probe-success-fail events, I'm not sure what the point would be as-is either. I do think we need the scorer and the router to be able to "intelligently" pick routes to probe, but that seems like a prereq here, not something in this PR.

[TheBlueMatt]

What may make sense is to scope this PR down to just the channelmanager API changes and to handle the events in the InvoicePayer like we do other payment events. Then we can do a followup that has some kind of intelligent probe-route-building API.

[tnull]

Alright. I agree that eventually we'd want to give a range of probing tools to the user, the main one probably being a method probing a particular channel rather than a full path. I'd however still argue probing a path should be part of that toolbox. That said I'm fine with landing this only providing ChannelManager's send_probe and working out the details of the API in a follow-up.

[TheBlueMatt]

Yea, I don't disagree that probing a path and a channel are useful, but I think probably the eventual end-goal should be either send_probe() or probe_node(node_id: PublicKey) - users either want to probe and have the prober handle everything (probably looking at recent payment history and finding nodes that it has had a hard time paying and probe alternative paths to those nodes) or probe trying to improve knowledge of how to reach a specific node.

[tnull]

Cool to see probing get better support. One question on implementing this. You'd still go through the Event manager to find PaymentPathFailed events, check if they were a probe with payment_is_probe(), and then handle it appropriate?

Yes, that's the idea.

One thing I was finding when I was doing probing is that I needed access to the error reasons to really discern the reason for the probe failure.

Ah, interesting. So far I though I could get around that since we have the rejected_by_dest flag derived from is_final_node (cf. https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/onion_utils.rs#L372) and would score the path more or less independently otherwise. Was there any particular case for which you had to discern by error code?

[tnull]

Rebased.

[TonyGiorgio]

Ah, interesting. So far I though I could get around that since we have the rejected_by_dest flag derived from is_final_node (cf. https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/onion_utils.rs#L372) and would score the path more or less independently otherwise. Was there any particular case for which you had to discern by error code?

So my use case is pretty non-standard, since I'm probing for unannounced channels, not balance. That said, there's a number of errors that could be returned to determine whether or not a probe was unsuccessful due to balance or some other error. For instance, fee amount or CLTV delta. However, I suppose if you just want to know whether or not a payment made it all the way through to the end node, you could probably trigger off of is_final_node. That should cover most use cases and I don't think most people care about specific error codes when using probing.

Though it would be nice to have access to error code for more advanced use cases, even if not in the simple probing api.

[jkczyz]

Yeah, seems allowing arbitrary Routes could complicated the semantics, too, if say the route has more than one path.

[tnull]

That said, there's a number of errors that could be returned to determine whether or not a probe was unsuccessful due to balance or some other error. For instance, fee amount or CLTV delta. However, I suppose if you just want to know whether or not a payment made it all the way through to the end node, you could probably trigger off of is_final_node. That should cover most use cases and I don't think most people care about specific error codes when using probing.

Though it would be nice to have access to error code for more advanced use cases, even if not in the simple probing api.

Yeah, I think the underlying assumption is that we would always create payments that would only fail by insufficient balance or at the final destination. I agree, allowing for more fine-grained / advanced control over probing would likely be a good follow up.

[tnull]

Hm, also unsure why fuzz is currently broken. I assume it is connected to the changes made to ChannelManager serialization?

Still no good idea regarding the fuzzer. @TheBlueMatt do you have a hunch?

[TheBlueMatt]

Oh, sorry, I missed that. The fuzzer test is sad cause you changed the format of what channelmanager does on startup (fetching a new block of 32 random bytes, I believe) such that it will invalidate all the existing full_stack_target tests. In general that's fine, there's no reason to change the PR in response, its mostly there to make sure we're deliberate about it. Once you do #1567 (comment) (and probably squash, since the history is gonna change anyway?) I'll update the fuzz test there and give a patch.

[tnull]

Oh, sorry, I missed that. The fuzzer test is sad cause you changed the format of what channelmanager does on startup (fetching a new block of 32 random bytes, I believe) such that it will invalidate all the existing full_stack_target tests. In general that's fine, there's no reason to change the PR in response, its mostly there to make sure we're deliberate about it. Once you do #1567 (comment) (and probably squash, since the history is gonna change anyway?) I'll update the fuzz test there and give a patch.

Ah good to know, was suspecting something like this. I assumed the comment you referenced to be addressed, since I removed the interface change here and will look into another API in a follow-up. Should I include something else here (apart from an end-to-end test)?

EDIT: Huh, seems part of the 'remove interface' changes didn't make it into the commit previously. Should be properly removed now though. Sorry!

[TheBlueMatt]

Basically LGTM, just #1567 (comment) and fuzz to go. Do you mind if this get's squashed, @jkczyz ?

[jkczyz]

Basically LGTM, just #1567 (comment) and fuzz to go. Do you mind if this get's squashed, @jkczyz ?

Sure, go ahead and squash.

[tnull]

Squashed!

[TheBlueMatt]

The following diff fixes the fuzz issue:

diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index b4ca316ed..fa0211aa2 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -393,7 +393,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
        // Adding new calls to `KeysInterface::get_secure_random_bytes` during startup can change all the
        // keys subsequently generated in this test. Rather than regenerating all the messages manually,
        // it's easier to just increment the counter here so the keys don't change.
-       keys_manager.counter.fetch_sub(1, Ordering::AcqRel);
+       keys_manager.counter.fetch_sub(2, Ordering::AcqRel);
        let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap());
        let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash(), Arc::clone(&logger)));
        let gossip_sync = Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger)));

[tnull]

Thanks for the patch, the fuzzer seems happy now (at least locally). Squashed again.

[tnull]

The following diff fixes the fuzz issue:

diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs
index b4ca316ed..fa0211aa2 100644
--- a/fuzz/src/full_stack.rs
+++ b/fuzz/src/full_stack.rs
@@ -393,7 +393,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
        // Adding new calls to `KeysInterface::get_secure_random_bytes` during startup can change all the
        // keys subsequently generated in this test. Rather than regenerating all the messages manually,
        // it's easier to just increment the counter here so the keys don't change.
-       keys_manager.counter.fetch_sub(1, Ordering::AcqRel);
+       keys_manager.counter.fetch_sub(2, Ordering::AcqRel);
        let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap());
        let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash(), Arc::clone(&logger)));
        let gossip_sync = Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger)));

Ow, I think the fuzzer now fails differently?

[TheBlueMatt]

Oops, plus:

diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs
index 615f3aad9..6e3dc4aff 100644
--- a/fuzz/src/chanmon_consistency.rs
+++ b/fuzz/src/chanmon_consistency.rs
@@ -850,6 +850,12 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out) {
                                                events::Event::PaymentClaimed { .. } => {},
                                                events::Event::PaymentPathSuccessful { .. } => {},
                                                events::Event::PaymentPathFailed { .. } => {},
+                                               events::Event::ProbeSuccessful { .. } | events::Event::ProbeFailed { .. } => {
+                                                       // Even though we don't explicitly send probes, because probes are
+                                                       // detected based on hashing the payment hash+preimage, its rather
+                                                       // trivial for the fuzzer to build payments that accidentally end up
+                                                       // looking like probes.
+                                               },
                                                events::Event::PaymentForwarded { .. } if $node == 1 => {},
                                                events::Event::PendingHTLCsForwardable { .. } => {
                                                        nodes[$node].process_pending_htlc_forwards();

[tnull]

Alright, try, try again.