Require payment secrets and track them in ChannelManager #893

TheBlueMatt · 2021-04-23T04:48:12Z

This should massively reduce confusion and foot-guns around our PaymentReceived event handling.

Still a few TODOs and some docs to write, but this is basically there. Next step would be to add utilities for invoices to use the new APIs exposed here.

valentinewallace · 2021-04-23T16:30:48Z

lightning/src/ln/channelmanager.rs

+	/// in excess of the current block height. This should match the value set in the invoice.
+	///
+	/// May panic if invoice_expiry_delta_blocks is greater than one year of blocks (52,560).
+	pub fn get_payment_secret_preimage(&self, value_msat: Option<u64>, invoice_expiry_delta_blocks: u32) -> (PaymentHash, PaymentSecret) {


This is roughly the top-level API I was picturing:

impl Invoice { pub fn from_channelmanager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>(channelmanager: &ChannelManager<Signer, M, T, K, F, L>, our_node_privkey: SecretKey, amt_msat: u64, preimage: PaymentPreimage, payment_secret: PaymentSecret, description: String, network: Network) -> Result<Invoice, CreationError>

And in my mind, this function^ then calls a new method channel_manager.invoice_generated(preimage, secret, amt, cltv) which then stores the info.

So I guess that makes me a bit confused where get_payment_secret_preimage and get_payment_secret fit into the equation. Maybe giving example usage in the docs would help?

FYI @jkczyz I think I told you I was creating this API^ but ended up passing the buck to Matt yesterday, who then created this PR.

I think we should (a) drop the node_privkey parameter and expose it from ChannelManager, (b) make amount_msat Optional (so that people can do 0-value invoices), though I guess they could pass 0?, (d) drop the preimage and secret parameters.

Then that method would call chanman.get_payment_secret_preimage(amount, DEFAULT_EXPIRY) and use the payment secret and hash provided by that method, then calling chanman.get_node_key when signing.

Ah okay... So when does the user have an opportunity to store the payment preimage/secret? (I know we're currently not dropping them on expiry, but I presume that we plan to.)

Could I help by writing that utility based off this work? Or is someone already working on it?

Discussed offline, I have a half-finished utility so gonna update features and then PR the utility based on this PR.

So when does the user have an opportunity to store the payment preimage/secret?

I thought the goal was that they wouldn't have to? :) But, generally, if the user wants to store it, they can do it after calling get_payment_secret_preimage or, if they prefer to generate their own preimage, before calling get_payment_secret.

codecov · 2021-04-23T18:01:20Z

Codecov Report

Merging #893 (b7bc634) into main (affefb6) will increase coverage by 0.03%.
The diff coverage is 93.76%.

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

@@            Coverage Diff             @@
##             main     #893      +/-   ##
==========================================
+ Coverage   90.39%   90.43%   +0.03%     
==========================================
  Files          57       57              
  Lines       29353    29501     +148     
==========================================
+ Hits        26535    26679     +144     
- Misses       2818     2822       +4

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`97.06% <ø> (+0.04%)`	⬆️
lightning/src/util/events.rs	`17.27% <0.00%> (-1.00%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.42% <89.43%> (+0.27%)`	⬆️
lightning-persister/src/lib.rs	`94.06% <100.00%> (ø)`
lightning/src/chain/chainmonitor.rs	`95.52% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.64% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/features.rs	`98.81% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`95.06% <100.00%> (-0.03%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.80% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/reorg_tests.rs	`99.62% <100.00%> (ø)`
... and 4 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 affefb6...615ef7d. Read the comment docs.

TheBlueMatt · 2021-04-24T00:03:24Z

Aside from some minor issues noted in the last commit and some new tests which are required, this should be good for review! Sorry about the delay, I thought it was closer than it was yesterday.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-04-25T02:18:04Z

lightning/src/ln/channelmanager.rs

+	/// Gets a payment secret and payment hash for use in an invoice given to a third party wishing
+	/// to pay us.


I wonder if these two methods are trait-worthy. It may have (1) a single method for creating an inbound payment and generating/storing the preimage as it sees fit (rather than two methods) and (2) a method for retrieving this data including the preimage.

Or is there a reason why PaymentReceived should not contain a preimage that was generated elsewhere?

Hmm, what exactly did you have in mind? A trait that ChannelManager implements? Or a trait for fetching payment preimages against a user implementation of some trait (presumably with some default implementation)?

In general I don't mind doing this via some external trait, but I do worry that we'd be adding a chunk of complexity to make users serialize yet another object that stores payment preimages (which is already the case, but I figured we could drop that requirement).

Decided to keep the interface as-is for now after offline discussion on Slack. For posterity: https://lightningdevkit.slack.com/archives/CTBLT3CAU/p1619364792017100

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-04-27T01:07:06Z

Addressed outstanding comments and issues, as well as added appropriate testing. Should be good to go.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-04-27T01:10:14Z

lightning/src/ln/channelmanager.rs

+											if total_value >= msgs::MAX_VALUE_MSAT || total_value > payment_data.total_msat {
+												log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
+													log_bytes!(payment_hash.0), total_value, payment_data.total_msat);


Is running over ok as per create_inbound_payment_for_hash's docs?

No - running over inbound_payment.get().min_value_msat (which comes from create_inbound_payment_for_hash) is fine, but running over payment_data.total_msat (which comes from the HTLC sender) is not. The second is controlled by the spec.

valentinewallace

This looks pretty much good to me!

I wasn't sure what you meant by this on Slack: "actually handling preimages could use some thinking to see if you can figure a way to distinguish between "node knows preimage" and "node doesn't know preimage" if you only have a payment hash but not the payment secret -- is this in process_pending_htlc_forwards?

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-04-27T18:43:05Z

Pushed a set of changes to swap out the expiration height for time - invoices use time not height, so it was incongruous. Of course time is a bit more awkward since we only extract time from block headers, but it should work alright and doesn't need to be perfect.

valentinewallace · 2021-04-27T21:09:21Z

lightning/src/ln/channelmanager.rs

+
+	/// Storage for PaymentSecrets and any requirements on future inbound payments before we will
+	/// expose them to users via a PaymentReceived event. HTLCs which do not meet the requirements
+	/// here are failed when we process them as pending-forwardable-HTLCs, and entries are removed


nit: could note that entries are also removed when they expire

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-04-27T21:29:38Z

lightning/src/ln/channelmanager.rs

+	/// expose them to users via a PaymentReceived event. HTLCs which do not meet the requirements
+	/// here are failed when we process them as pending-forwardable-HTLCs, and entries are removed
+	/// after we generate a PaymentReceived upon receipt of all MPP parts.
+	/// Locked *after* channel_state.


Should this be enforced somehow, ideally?

Yes! I don't know that Rust has a built-in way to do this :(. I suppose we could add our own lock-order detection, and maybe we should when we move to per-channel locks, but its probably a bit out of scope here :/.

valentinewallace · 2021-04-27T22:43:32Z

lightning/src/ln/channelmanager.rs

-				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
-				if (is_mpp && !valid_mpp) || (!is_mpp && (htlc.value < expected_amount || htlc.value > expected_amount * 2)) {
+				if !valid_mpp {
+					if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }


Don't think this check: htlc.value > expected_amount * 2 was migrated anywhere, is it needed?

It only really applies for non-MPP - pre-MPP "don't overpay massively" was one (kinda poor) heuristic suggested in the spec to ignore sends from people other than the invoice-recipient. Once we have the payment secret, we don't really care too much, and the sender (who proved their knowledge of the payment preimage) sends also the specific amount we should expect, which we enforce is identical.

ariard · 2021-04-27T23:36:26Z

lightning/src/ln/channelmanager.rs

@@ -3321,6 +3377,100 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			self.finish_force_close_channel(failure);
 		}
 	}
+
+	fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
+		assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106


Small thing but not tested AFAICT:

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 9a52828b..b2d4e3b6 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3379,7 +3379,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana } fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> { - assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106 + //assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106 let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes());

ariard · 2021-04-27T23:39:29Z

lightning/src/ln/channelmanager.rs

+	fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
+		assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106
+
+		let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes());


Super likely we'll need to replace with a new keysinterface method for AMP/PTLC support where you want to enforce a cryptographic relation between the payment path identifier and the packet redeeming secret. But that's maybe too far ahead and we'll need to tweak this API anyway...

Shorter term, is this API keysend-proof where the payment_secret is actually provided by the payer ?

Super likely we'll need to replace with a new keysinterface method for AMP/PTLC support where you want to enforce a cryptographic relation between the payment path identifier and the packet redeeming secret.

Hmm, I wasn't under the impression it would matter? The only relation that matters for the payment secret for invoice-initiated payments is that it came from the invoice to authenticate the sender. I kinda assumed PTLC stuff will only impact the payment hash-payment preimage relation, but maybe I'm missing something obvious?

Shorter term, is this API keysend-proof

IIUC, keysend implies reading the payment preimage from the final hop onion TLV, which you can't do with either the current or the new API. We need to have a completely separate HTLC-processing path for HTLCs identifying themselves as keysend.

Hmm, I wasn't under the impression it would matter?

After reading again the documentation available on PTLC (https://github.com/ElementsProject/scriptless-scripts/blob/master/md/multi-hop-locks.md) I think you're right and we might still need a payment_secret in the future to associate adaptor secret to final pointlock otherwise you might to create all adaptor sigs combinations you know about, which might be a DoS vector...

We need to have a completely separate HTLC-processing path for HTLCs identifying themselves as keysend.

Right and it needs to start as deep as adding a new msgs::OnionHopDataFormat and corresponding processing in decode_update_add_htlc_onion

ariard · 2021-04-28T00:05:47Z

lightning/src/ln/channelmanager.rs

+					// never fail a payment too early.
+					// Note that we assume that received blocks have reasonably up-to-date
+					// timestamps.
+					expiry_time: self.last_node_announcement_serial.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200,


Can a counterparty replay announcement signatures to prevent expiration of our pending payments ?

In internal_announcement_signatures, we generate a BroadcastChannelAnnouncement if signatures validation is successful. But we don't seem to prevent replay.

Following BroadcastChannelAnnouncement documentation it says :

Note that after doing so, you very likely (unless you did so very recently) want to call ChannelManager::broadcast_node_announcement...

If implemented automatically, this call to broadcast_node_announcement will increase our self.last_node_announcement_serial by one.

In best_block_updated, we'll keep the max between current last_node_announcement_serial and header time. If old serial is superior to header time we'll never expire payments ?

I think potential manipulation of last_node_announcement_serial is already there before this PR ? Maybe we should upper bound on some assumptions on block variance. Like no more than 300 updates per-block interval.

Hmm, good point, I hadn't caught the docs suggesting that (I'd only looked at the explicit uses of last_node_announcement_serial. If we feel strongly, we should probably just add a second variable which only tracks header times rather than bothering mixing two different uses in the same variable. As for the general case around last_node_announcement_serial, do we actually care if we end up with some super high serial number for our node announcements? Last I checked the spec indicated that's allowed but I may be wrong now.

As for the general case around last_node_announcement_serial, do we actually care if we end up with some super high serial number for our node announcements? Last I checked the spec indicated that's allowed but I may be wrong now.

Yes i think we don't care about a super high serial number for our node announcements, unless other implementations have some custom upper bounds or discard any further announcements about some rate. Though to be honest, accidental or malicious partitions from the rest of the gossip layer are still ranking as a low-priority for me. I was more concerned with the payment reception interference here.

Thanks for adding highest_seen_timestamp.

ariard · 2021-04-28T00:14:19Z

lightning/src/ln/channelmanager.rs

+					// announcement. As long as we aren't creating a new node announcement more
+					// often than once per second, it should never be more than two hours in the
+					// future. Thus, we add two hours here as a buffer to ensure we absolutely
+					// never fail a payment too early.


Expiration is gateway on header time superior to old_serial so the risk is more never expiring a never-received inbound payment ?

Also I don't think the relation between "once per second" and "more than two hours in the future" is really clear. The problem is last_announcement_serial getting forever in advance of the block time, i.e if you generate more than 600 updates per-expected block interval. Of course your header time can still jump to 2 hours in the future per-consensus rules and swallow back the delay...and then your last_announcement_serial jumping ahead in the time race again. Not clearly easy to explain and reasoning on.

Hmm, do you have a concrete suggestion to improve it? I'll go ahead and drop the reliance on last_node_announcement_serial here and just add a new field to track highest-header, which simplifies it some, but I'm not sure how to further simplify it.

TheBlueMatt · 2021-04-28T16:37:35Z

I wasn't sure what you meant by this on Slack: "actually handling preimages could use some thinking to see if you can figure a way to distinguish between "node knows preimage" and "node doesn't know preimage" if you only have a payment hash but not the payment secret -- is this in process_pending_htlc_forwards?

Correct, yes - if a third-party which does not know the payment secret can distinguish between "knows preimage" and "doesn't know preimage" in our handling of HTLCs anywhere, then we're vulnerable to some probing attacks. I believe this code correctly handles both "payment secret doesn't match" and "we don't know the preimage" identically, making such probing impossible.

valentinewallace

LGTM, fine to leave anything further to follow-up

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-04-28T17:10:46Z

lightning/src/ln/channelmanager.rs

+	/// Note that we use block header time to time-out pending inbound payments (with some margin
+	/// to compensate for the inaccuracy of block header timestamps). Thus, in practice we will
+	/// accept a payment and generate a [`PaymentReceived`] event for some time after the expiry.
+	/// If you need exact expiry semantics, you should enforce them upon receipt of


Maybe we could give a hint for how users should compute this number (i.e., should it be at least the highest cltv_expiry_delta of their route hints, if that makes sense?)

Hmm, good point. I'm not sure what it should be - the limit is unrelated to the cltv_expiry, which is relative to the block height at the time the invoice is paid/received, but the timeout is relative to when the invoice is generated. Its really up to the user for how long they want the invoice to still be valid. Maybe we can better capture that requirement in the docs (or mention the default of 2 hours) - do you have any suggestions?

Ah, I see. I think it may have just been my misunderstanding, hmm. I think mentioning the default would be helpful.

Pushed a fixup commit, let me know if its good and I'll squash + merge.

jkczyz

Overall looks good. I'm sure we can iterate on the API once we get a better sense of the use cases.

TheBlueMatt · 2021-04-28T19:18:53Z

Squashed the fixup commits so CI should hopefully pass now. The fuzz CI failure here is #902.

This adds support for tracking payment secrets and (optionally) payment preimages in ChannelManager. This potentially makes client implementations much simper as they don't have to have external payment preimage tracking. This doesn't yet use such tracking anywhere.

In order to reduce code movement in the next commit, this commit simply tweaks get_payment_preimage_hash!() and related functions in functional tests to return a payment secret. Further, we ensure that we always call get_payment_preimage_hash!() with the node which will ultimately receive the payment.

This prepares us for requiring payment_secrets for all received payments, by demonstrating test changes work even prior to the new requirement. In order to avoid needing to pipe payment secrets through to additional places in the claim logic and then removing that infrastructure once payment secrets are required, we use the new payment secret storage in ChannelManager to look up the payment secret for any given pament hash in claim and fail-back functions. This part of the diff is reverted in the next commit.

Our current PaymentReceived API is incredibly easy to mis-use - the "obvious" way to implement a client is to always call `ChannelManager::claim_funds` in response to a `PaymentReceived` event. However, users are *required* to check the payment secret and value against the expected values before claiming in order to avoid a number of potentially funds-losing attacks. Instead, if we rely on payment secrets being pre-registered with the ChannelManager before we receive HTLCs for a payment we can simply check the payment secrets and never generate `PaymentReceived` events if they do not match. Further, when the user knows the value to expect in advance, we can have them register it as well, allowing us to check it for them. Other implementations already require payment secrets for inbound payments, so this shouldn't materially lose compatibility.

This allows users to store metadata about an invoice at invoice-generation time and then index into that storage with a general-purpose id when they call `get_payment_secret`. They will then be provided the same index when the payment has been received.

Like the payment_secret parameter, this paramter has been the source of much confusion, so we just drop it. Users should prefer to do this check when registering the payment secret instead of at claim-time.

For users who get PaymentPreimages via `get_payment_secret_preimage`, they need to provide the PaymentPreimage back in `claim_funds` but they aren't actually given the preimage anywhere. This commit gives users the PaymentPreimage in the `PaymentReceived` event.

TheBlueMatt · 2021-04-28T19:43:55Z

Only diff is docs, gonna merge once all the non-fuzz CI passes (fuzz will have to wait for 902):

$ git diff-tree -U1 7f320f546 615ef7d6f
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 752452414..31529343d 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3460,2 +3460,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// in excess of the current time. This should roughly match the expiry time set in the invoice.
+	/// After this many seconds, we will remove the inbound payment, resulting in any attempts to
+	/// pay the invoice failing. The BOLT spec suggests 7,200 secs as a default validity time for
+	/// invoices when no timeout is set.
+	///
 	/// Note that we use block header time to time-out pending inbound payments (with some margin

ariard

Post-merge Code Review ACK 615ef7d

ariard · 2021-04-28T22:04:53Z

lightning/src/ln/channelmanager.rs

+/// Minimum CLTV difference between the current block height and received inbound payments.
+/// Invoices generated for payment to us must set their `min_final_cltv_expiry` field to at least
+/// this value.
+pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER;


FYI, we might scale up those delays in function of HTLC value and counterparty risk in the future. I believe one of the issue down the road is a weird dependency where the invoice also has to commit to a specific r field to force inbound receiving through the private channel with the corresponding scaled-on-purpose MIN_FINAL_CLTV_EXPIRY.

Right, that would make sense. We'll need a new API then either way cause presumably we'll want some kind of configurability then as well.

ariard · 2021-04-28T22:39:45Z

lightning/src/ln/channelmanager.rs

+	fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
+		assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106
+
+		let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes());


Hmm, I wasn't under the impression it would matter?

After reading again the documentation available on PTLC (https://github.com/ElementsProject/scriptless-scripts/blob/master/md/multi-hop-locks.md) I think you're right and we might still need a payment_secret in the future to associate adaptor secret to final pointlock otherwise you might to create all adaptor sigs combinations you know about, which might be a DoS vector...

We need to have a completely separate HTLC-processing path for HTLCs identifying themselves as keysend.

Right and it needs to start as deep as adding a new msgs::OnionHopDataFormat and corresponding processing in decode_update_add_htlc_onion

ariard · 2021-04-28T23:03:16Z

lightning/src/ln/channelmanager.rs

+	/// [`create_inbound_payment`]: Self::create_inbound_payment
+	/// [`PaymentReceived`]: events::Event::PaymentReceived
+	/// [`PaymentReceived::user_payment_id`]: events::Event::PaymentReceived::user_payment_id
+	pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {


Good with the API, only concern is a limit pending_inbound_payments(MAX_PENDING_PAYMENTS=10000). Otherwise if your payment logic generates invoices on third-party demand and call create_inbound_payment_for_hash in consequence you have a DoS vector ? Even if I don't know how much we should care about unsafe use of the API this one sounds pretty easy to mitigate...

Hmmm, that's definitely a good question. I don't really know that that's a requirement at our layer - certainly some users (especially those on a server providing access to inbound payments via an API) may want a huge limit, and others a very small one. Presumably you also want rate-limiting by the client, not just a strict limit. We can certainly mention it in the docs, is that sufficient?

Mentioned in #905.

TheBlueMatt force-pushed the 2021-04-features-chanman branch from 9af57bb to 466d9c2 Compare April 23, 2021 04:48

valentinewallace reviewed Apr 23, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-04-features-chanman branch 3 times, most recently from e0b1aa1 to 0520b5f Compare April 23, 2021 17:41

TheBlueMatt force-pushed the 2021-04-features-chanman branch from 0520b5f to fa34660 Compare April 23, 2021 19:05

valentinewallace mentioned this pull request Apr 23, 2021

Invoice chanman utility #895

Merged

TheBlueMatt force-pushed the 2021-04-features-chanman branch 4 times, most recently from 2285bf9 to 2890f76 Compare April 24, 2021 00:03

TheBlueMatt force-pushed the 2021-04-features-chanman branch 2 times, most recently from 8da02a4 to fd22dc1 Compare April 24, 2021 21:53

jkczyz reviewed Apr 25, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-04-features-chanman branch 2 times, most recently from 2073ce3 to 5abd7e1 Compare April 26, 2021 23:13

TheBlueMatt marked this pull request as ready for review April 27, 2021 01:06

TheBlueMatt force-pushed the 2021-04-features-chanman branch from e395c19 to 13d7c97 Compare April 27, 2021 01:06

jkczyz reviewed Apr 27, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-04-features-chanman branch 2 times, most recently from 7716d34 to 0487e74 Compare April 27, 2021 02:27

valentinewallace reviewed Apr 27, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2021-04-features-chanman branch from 0487e74 to 0e9c55e Compare April 27, 2021 18:42

TheBlueMatt force-pushed the 2021-04-features-chanman branch from 0e9c55e to 78cbc38 Compare April 27, 2021 18:46

valentinewallace reviewed Apr 27, 2021

View reviewed changes

Set payment_secret to required in features flags

3180c43

TheBlueMatt force-pushed the 2021-04-features-chanman branch from 78cbc38 to cb6d4d4 Compare April 27, 2021 23:41

ariard reviewed Apr 28, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-04-features-chanman branch 2 times, most recently from 6be94e7 to 7f320f5 Compare April 28, 2021 15:21

valentinewallace approved these changes Apr 28, 2021

View reviewed changes

jkczyz approved these changes Apr 28, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-04-features-chanman branch 2 times, most recently from 2b917bd to 259e6d0 Compare April 28, 2021 19:00

TheBlueMatt added 16 commits April 28, 2021 15:30

DRY the get_route_and_payment_hash!() macro duplicated in tests

7d8dc7a

Use known InvoiceFeatures for routing in tests

73a3bb3

[fuzz] Always use PaymentSecrets in chanmon_consistency

533a003

Drop dead code for handling non-MPP payments in claim_funds

25e4f3e

Drop now-useless PaymentSecret parameters when claiming/failing-back

5a14048

Drop the amount parameter to claim_funds

5e96811

Like the payment_secret parameter, this paramter has been the source of much confusion, so we just drop it. Users should prefer to do this check when registering the payment secret instead of at claim-time.

Make the PaymentSecret in PaymentReceived events non-Optional

ecaeddc

Fail PendingInboundPayments after their expiry time is reached

f9a6cb2

Add some simple tests of payment secret tracking

fd0ebcf

Add a const and docs for the min min_final_cltv_expiry we allow

615ef7d

TheBlueMatt force-pushed the 2021-04-features-chanman branch from b7bc634 to 615ef7d Compare April 28, 2021 19:30

TheBlueMatt merged commit a8bc8fb into lightningdevkit:main Apr 28, 2021

ariard reviewed Apr 28, 2021

View reviewed changes

		/// Gets a payment secret and payment hash for use in an invoice given to a third party wishing
		/// to pay us.

Require payment secrets and track them in ChannelManager #893

Require payment secrets and track them in ChannelManager #893

Uh oh!

Conversation

TheBlueMatt commented Apr 23, 2021

Uh oh!

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

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 Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Apr 24, 2021

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!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Apr 27, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt commented Apr 27, 2021

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!

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

valentinewallace Apr 23, 2021 •

edited

Loading

jkczyz Apr 23, 2021 •

edited

Loading

valentinewallace Apr 23, 2021 •

edited

Loading

codecov bot commented Apr 23, 2021 •

edited

Loading