Keysend #967

valentinewallace · 2021-06-23T19:16:33Z

Based on #964

~~06/23/2021: Seeking concept ACKs for the API before adding tests~~

Re this Slack discussion, implement keysend to private nodes

codecov · 2021-06-23T19:33:56Z

Codecov Report

Merging #967 (6dd6289) into main (d37b1dd) will decrease coverage by 0.03%.
The diff coverage is 88.09%.

@@            Coverage Diff             @@
##             main     #967      +/-   ##
==========================================
- Coverage   90.78%   90.75%   -0.04%     
==========================================
  Files          60       60              
  Lines       30926    31322     +396     
==========================================
+ Hits        28076    28426     +350     
- Misses       2850     2896      +46

Impacted Files	Coverage Δ
lightning/src/util/events.rs	`15.13% <0.00%> (-2.04%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.62% <81.48%> (-0.33%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.80% <92.85%> (+0.94%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.33% <93.75%> (-0.11%)`	⬇️
lightning/src/ln/features.rs	`99.70% <100.00%> (+<0.01%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`94.90% <100.00%> (+0.03%)`	⬆️
lightning/src/ln/msgs.rs	`88.69% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`97.11% <100.00%> (ø)`
lightning/src/ln/onion_utils.rs	`94.91% <100.00%> (+0.01%)`	⬆️
lightning/src/routing/router.rs	`95.94% <100.00%> (+0.01%)`	⬆️
... and 2 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 d37b1dd...6dd6289. Read the comment docs.

TheBlueMatt

Concept ACK the (public) api changes.

lightning/src/ln/channelmanager.rs

lightning/src/ln/msgs.rs

jkczyz

Concept ACK

lightning/src/ln/channelmanager.rs

lightning/src/ln/peer_handler.rs

lightning/src/ln/channelmanager.rs

lightning/src/util/events.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt

Structure all looks good IMO, one request for more tests and a comment or two.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-07-05T14:45:04Z

lightning/src/ln/channelmanager.rs

 impl_writeable_tlv_based!(ClaimableHTLC, {
 	(0, prev_hop, required),
 	(2, value, required),
-	(4, payment_data, required),
+	// XXX what to do here?
+	(4, claim_type, required),


Hmm, right, so to make this compatible we need to pull the impl_writeable_tlv_based apart and implement manually - if ClaimableType is Invoice we'll write the 4-type payment_data and if its Spontaneous we'll write an 8-type preimage. Its not ideal and we could just cheat here if we want, but IMO its not a ton of work here.

I think I did this

jkczyz · 2021-07-12T23:53:37Z

lightning/src/ln/msgs.rs

 				if let Some(final_data) = payment_data {
 					if final_data.total_msat > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); }
 				}
 				encode_varint_length_prefixed_tlv!(w, {
 					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward), required),
 					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value), required),
-					(8, payment_data, option)
+					(8, payment_data, option),
+					(5482373484, keysend_preimage, option)


nit: could you use a trailing comma so the blame layer for future additions don't affect this line?

The macro gives me an error when I do this :/

lightning/src/util/events.rs

jkczyz · 2021-07-13T00:13:08Z

lightning/src/util/events.rs

-						(2, payment_secret, required),
+						(2, payment_secret, option),


Just to check my understanding, going from required to optional is ok, but going from optional to required would necessitate increasing the version number?

I think there's ways to make both directions work depending on if you're willing to add a special case in the code for backwards-compatibility? @TheBlueMatt would have to confirm

I think going from option to required is never allowed. If we move from option to required, and an old version "wrote" None, we'll fail to deserialize data written by an old node, which is not allowed unless we are deliberately breaking reading old versions (presumably after several sequential versions that always wrote Some so that you have to be reading very old data to fail).

jkczyz · 2021-07-13T00:16:41Z

lightning/src/util/events.rs

 						(8, payment_preimage, option),
 					});
+					let purpose = match payment_secret {


If payment_secret were optional in InvoicePayment, would we be able to distinguish what the purpose is? In other words, should we encode this as an enum instead? Or would that break compatibility?

In other words, should we encode this as an enum instead?

Could you clarify roughly what enum is being suggested? Didn't quite follow..

PaymentPurpose enum using impl_writeable_tlv_based_enum.

Believe that'd break compatibility

lightning/src/ln/channelmanager.rs

jkczyz · 2021-07-13T02:20:01Z

lightning/src/ln/channelmanager.rs

+impl Readable for ClaimableHTLC {
+	fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
+		init_tlv_field_var!(prev_hop, required);
+		init_tlv_field_var!(value, required);
+		init_tlv_field_var!(payment_data, required);
+		init_tlv_field_var!(cltv_expiry, required);
+		init_tlv_field_var!(keysend_preimage, option);
+		read_tlv_fields!
+		(reader,
+		 {
+		   (0, prev_hop, required), (2, value, required),
+		   (4, payment_data, required), (6, cltv_expiry, required),
+			 (8, keysend_preimage, option)
+		 });
+		let claim_type = match keysend_preimage {
+			Some(p) => {
+				let data: msgs::FinalOnionHopData = payment_data.0.unwrap();
+				if data.payment_secret != PaymentSecret([0; 32]) || data.total_msat != 0 {
+					return Err(DecodeError::InvalidValue)
+				}
+				ClaimableType::Spontaneous(p)
+			},
+			None => ClaimableType::Invoice(payment_data.0.unwrap()),
+		};
+		Ok(Self {
+			prev_hop: init_tlv_based_struct_field!(prev_hop, required),
+			value: init_tlv_based_struct_field!(value, required),
+			claim_type,
+			cltv_expiry: init_tlv_based_struct_field!(cltv_expiry, required),
+		})
+	}
+}


Seems like as we change our data format more, the more complex our (de)serialization logic becomes as we can no longer use impl_writeable_tlv_based. I'm wondering if it would be any better to bump the version but support reading older versions. Then maybe you could condition on the version read and use a macro for implementing each version we'd like to support? I might be missing something about this, though.

Hmm, I dont think bumping the version helps a ton, then we'd just have two copies of the deserialization logic - one for the old objects, and one for the new, and we'd have to switch between them based on the version. That just results in a code blowup that's almost equivalent, I think.

I guess then the versions would at least be fairly well delineated. Otherwise, if we refactor further there is more cognitive overhead in figuring out the various cases. But maybe at that point we'd just decided to bump versions. I don't feel strongly on this, but just want to point out the cost.

That said, if our version is only in topic-level structs (is it?) then we'd have to pass it through. So maybe not very feasible / clean?

Right, I have a feeling sooner or later we'll adapt our read trait to pass a version number. I haven't bothered yet, but it may be required eventually. I agree we should be careful about growing bloat in read functions, maybe it makes sense to have a "we only support things written by versions as old as one year ago"-style policy.

jkczyz · 2021-07-13T02:22:00Z

lightning/src/ln/channelmanager.rs

-impl_writeable_tlv_based!(ClaimableHTLC, {
-	(0, prev_hop, required),
-	(2, value, required),
-	(4, payment_data, required),
-	(6, cltv_expiry, required),
-});
+impl Writeable for ClaimableHTLC {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
+		let payment_data = match &self.claim_type {
+			ClaimableType::Invoice(data) => data.clone(),
+			_ => msgs::FinalOnionHopData {
+				payment_secret: PaymentSecret([0; 32]),
+				total_msat: 0,
+			}
+		};
+		let keysend_preimage = match self.claim_type {
+			ClaimableType::Invoice(_) => None,
+			ClaimableType::Spontaneous(preimage) => Some(preimage.clone()),
+		};
+		write_tlv_fields!
+		(writer,
+		 {
+		   (0, self.prev_hop, required), (2, self.value, required),
+		   (4, payment_data, required), (6, self.cltv_expiry, required),
+			 (8, keysend_preimage, option),
+		 });
+		Ok(())
+	}


Do we need to do this because we introduced an enum where there wasn't one before? Would it make sense to bump the version and serialize enums using the macro for doing so?

Yeah: #967 (comment)

Would it make sense to bump the version and serialize enums using the macro for doing so?

Is this the same idea as in this comment? I'm pretty eager to get this PR out the door to unblock sphinx, but would love to get @TheBlueMatt's input on this idea since it does sound potentially nicer

Yeah, similar concern.

We can't "just bump the version" since we want to at least try (I think) to be compatible where old versions can read new serialized data. Because some users will be synchronizing state across multiple applications that may upgrade at different times, I think we should generally try to at least have our data be readable by the previous version as long as you aren't using new features.

lightning/src/ln/msgs.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-07-13T19:51:41Z

lightning/src/util/events.rs

-						(2, payment_secret, required),
+						(2, payment_secret, option),


I think going from option to required is never allowed. If we move from option to required, and an old version "wrote" None, we'll fail to deserialize data written by an old node, which is not allowed unless we are deliberately breaking reading old versions (presumably after several sequential versions that always wrote Some so that you have to be reading very old data to fail).

lightning/src/ln/channelmanager.rs

ariard · 2021-07-15T13:51:07Z

lightning/src/ln/channelmanager.rs

+			// final_expiry_too_soon
+			// We have to have some headroom to broadcast on chain if we have the preimage, so make sure we have at least
+			// HTLC_FAIL_BACK_BUFFER blocks to go.
+			// Also, ensure that, in the case of an unknown payment hash, our payment logic has enough time to fail the HTLC backward


I think this comment could be clearer saying "an unknown preimage for the received payment hash" ?

I don't think this is true that we trigger a channel closure for an inbound HTLC of which the payment hash is unknown. See ChannelMonitor::should_broadcast_holder_commitment_txn, if the htlc is !htlc_outbound and we don't know a self.payment_preimages.contains_key(&htlc.payment_hash) so it doesn't matter for our payment logic has enough time to fail the HTLC backward ?

I think this comment could be clearer saying "an unknown preimage for the received payment hash" ?

Added this!

ariard · 2021-07-15T14:05:52Z

lightning/src/ln/channelmanager.rs

@@ -1863,6 +1886,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}

+	/// Send a spontaneous payment, which is a payment that does not require the recipient to have


What if the recipient did generate an invoice with a given payment hash and signals even payment_secret feature bit ? If the recipient is a LDK node, I think it'll fail the keysend reception as we don't allow payment_secret + keysend_preimage ?

Maybe a bit edgy but point to be cleared up with keysend specification ? Like invoice reception implies MUST NOT pay with keysend?

Right, I think the behavior here is right, we should expect a "real" payment, the spec should mention this.

ariard · 2021-07-15T14:31:48Z

lightning/src/ln/channelmanager.rs

+														});
+													},
+													hash_map::Entry::Occupied(_) => {
+														log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} for a duplicative payment hash", log_bytes!(payment_hash.0));


IIUC this case handling, if we've a valid (payment_secret, payment_hash) pair in self.pending_inbound_payments and we receive a latter keysend with the same hash, we fail the keysend instead of claiming ?

If you're an intermediary hop Mallory and you relay HTLC 1 hash X to final hop Bob, I think this let Mallory send a "final-receiver probing" keysend hash X to Bob and observe the early failure (PendingHTLCsForwardable vs PaymentReceived processing times diff) ? If this is correct, I'm not sure if it binds well the requirement laid out above ("Further, we must not expose whether we have any other HTLCs associated with the same payment_hash pending or not')...

Grr good catch. Going one step up the stack, I wonder if we're gonna have users screw this up reliably too. If you're a user and doing accounting based on the payment_hash when you issue invoices, if you receive a keysend I can see users looking up the payment hash in their accounting database. I wonder if we should avoid exposing the keysend payment hash/preimage entirely. I guess some users may want them for other reasons, but we should go to great pains to point out in documentation that you MUST NEVER EVER co-mingle payment hash lookups between keysend and non-keysend. Maybe at least we should name the field differently to call it "keysend preimage" instead of "payment preimage"?

Can we avoid to return payment hash to avoid mangle with the accounting database ? I can foresee users willingly to know the preimage if it has any semantic sense like a pay-to-publish, privacy-preserving satoshi place. But within the keysend model I would say the payment hash should be ignored by the receiver there is no hash validation operation to realize against a database.

And if the receiver wants to prove delivery they can still manually hash the preimage. We can even return a prefixed preimage by concatening a "Lightning keysend" ?

You might even have worst collision, where a low-value keysend trigger the release of a virtual/physical good by your business logic, substituting to a regular high-value payment...?

Right, disregard, I had made that comment before I saw your suggestion at #967 (comment)

ariard · 2021-07-15T14:43:56Z

lightning/src/ln/channelmanager.rs

@@ -2304,15 +2380,27 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 											let mut total_value = 0;
 											let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
 												.or_insert(Vec::new());
+											if htlcs.len() == 1 {
+												if let OnionPayload::Spontaneous(_) = htlcs[0].onion_payload {
+													log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we already had an existing keysend HTLC with the same payment hash", log_bytes!(payment_hash.0));


Well I've a doubt if this doesn't introduce a weird payment censorship attack. Let's say an attacker observe that a payment invoice hash X has been issued by Alice. From now on, it will repeatedly send keysend X to Alice aiming to provoke a collision with honest HTLC X, thus triggering a reject.

Is the concern exposed sound ? I don't think the attacker even has to be on the honest HTLC payment path...

Yea, but its solved by #967 (comment) as well, no?

ariard · 2021-07-15T14:53:38Z

lightning/src/ln/channelmanager.rs

+							incoming_cltv_expiry: msg.cltv_expiry,
+						}
+					} else if let Some(payment_preimage) = keysend_preimage {
+						PendingHTLCRouting::ReceiveKeysend {


What do you think about checking early that keysend preimage == HTLC hash and isn't pure junk, returning 0x4000|22 if it is ? May help with probing concerns expressed after.

Ah! Yea, if we do that then we don't have to completely rewrite this whole PR to avoid the issues you noted at #967 (comment)

Yes i think that's okay to solve the issues. I had a concern with MPP payment where a preimage revealed could be replay as a probing keysend as long as the whole eMPP isn't settled but in fact we won't start the MPP settlement phase until all chunks have been received. And when we do so we remove the entry from pending_inbound_payments and as such the collision ?

I think it doesn't matter either way - basically we would be using the payment preimage the same way we use the payment secret today - to authenticate that the sender of a given HTLC is the one authorized to send for that payment hash. We reliably check this authentication immediately upon HTLC receipt both for MPP and regular payments, and will just have to also do it for keysend.

What do you think about checking early that keysend preimage == HTLC hash and isn't pure junk, returning 0x4000|22 if it is ? May help with probing concerns expressed after.

Added this check! Thanks for the thoughtful review Antoine

lightning/src/ln/channelmanager.rs

ariard · 2021-07-19T19:40:12Z

lightning/src/ln/channelmanager.rs

+												log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we didn't have a corresponding inbound payment.", log_bytes!(payment_hash.0));
+												fail_htlc!(claimable_htlc);
+											},
+											OnionPayload::Spontaneous(preimage) => {


Just to be clear, this doesn't prevent honest keysend replay, where a buggy/hazardous payer sends twice a keysend with the same hash, even with different payment paths, and as such opens the payment to be claimed in-flight by an intermediary node ? Note, I think this is already a risk for any hashlock-based LN payment (variant of wormhole attacks...)

I would say we even care less that w.r.t to regular payments as we keysend the preimage doesn't have the proof-of-payment property plead most of the time ?

Hmm, yes it does seem like keysend requires more thought than normal payments for replay situations. I don't think it would open up buggy clients to in-flight claims by an intermediate node though, because wouldn't the buggy client run into issues once they started "replaying" the whole update_add dance, preventing the HTLC from being successfully added to the commitment tx to begin with?

I mean the HTLC would appear under a new htlc_id. I'm thinking a situation similar to duplicate_htlc_test/test_duplicate_htlc_different_direction_onchain. Though nevermind, we don't prevent double-spend already for normal payments, see comment around send_payment.

lightning/src/ln/channelmanager.rs

ariard

See my points about payment fault-injection and the lowered cost of hash-collision in our ChannelMonitor.

I wonder if we shouldn't introduce a no_keysend_receive config setting. Previously, we have a cleaner separation between routing and payment endpoint, and a routing hop no eager to receive payments could have just not issue invoices at all. I think this is blurred by keysend, and you might have weird interactions to care about with potential higher application logic. E.g a third-party replaying hash trying to provoke undefined behaviors. Likely going to be worst if we implement MPP keysend.

I think we can address this config setting question in a follow-up, otherwise I believe the PR to be pretty mature. Great work overall!

ariard · 2021-07-22T21:46:51Z

lightning/src/util/events.rs

+	},
+	/// Because this is a spontaneous payment, the payer generated their own preimage rather than us
+	/// (the payee) providing a preimage.
+	SpontaneousPayment(PaymentPreimage),


I think we're bouncing between mentions of "spontaenous" or "keysend" payments across this PR to designate the same feature. LDK-user wise, maybe we should bind to a consistent naming ?

@jkczyz I was thinking that people might want to specifically send either a keysend spontaneous payment or a non-keysend spontaneous payment if they know that the payee only supports one or the other. So, I'm somewhat in favor of switching all Spontaneous variable names to Keysend (including s/send_spontaneous_payment/send_keysend_payment). Thoughts?

My understanding was that Keysend is a specific type of spontaneous payment that is the predecessor to spontaneous AMP payments. They both seem either unintuitive or somewhat overloaded names and also are implementation details from a user's perspective.

On the receiving side (Event::PaymentReceived), does the user need to know what implementation was used to make the spontaneous payment? If not, I'd argue we shouldn't expose the implementation detail in our public API.

On the sending side (ChannelManager), how does the user know if the receiver accepts spontaneous payments and using which implementation? If via feature flags, could we chose which implementation to use based on the flags rather than making the user choose?

On the processing side, can these be generalized in any way?

I don't have definitive answers to these, but I'd imagine we could at least keep the first two generic (i.e., "spontaneous") and if needed make the third specific to the implementation.

w/r/t:

On the receiving side (Event:: PaymentReceived), does the user need to know what implementation was used to make the spontaneous payment?
If via feature flags, could we chose which implementation to use based on the flags rather than making the user choose?

Not sure, pinged Sphinx for their thoughts last night. Hopefully they get back to me soonish

how does the user know if the receiver accepts spontaneous payments and using which implementation?

It's a little annoying right now... C-Lightning and Eclair advertise feature bit 55 if they support keysend, but iiuc lnd doesn't advertise any feature bits for keysend. So, right now you know if the receiver doesn't support keysend if they fail back the payment, but there's no good way to tell if they do support keysend.

lightning/src/ln/channelmanager.rs

ariard · 2021-07-22T23:38:16Z

lightning/src/ln/channelmanager.rs

+											if let OnionPayload::Invoice(ref data) = claimable_htlc.onion_payload {
+												data.clone()
+											} else {
+												log_trace!(self.logger, "Failing new keysend HTLC with payment_hash {} because we already have an inbound payment with the same payment hash", log_bytes!(payment_hash.0));


Do you have concerns of fault-injection attacks against honest routing hops introduced with our keysend implementation ?

Let's say you have the following topology : A -> B -> C -> D -> E.
A is the payer, it draws a payment path to the payee E, going through B, C and D.
B receives a keysend X and from HTLC traffic A's habits assumes it's a keysend.
B knows a payment secret for C, which is also a merchant node and deliver on-demand.
B sends a regular payment X to C then A's keysend X.
C receives in-order the regular payment X then A's keysend X, provoking the failure of the keysend.
C returns an error to A as the source of the payment path failure.
A downgrades C's routing reputation and doesn't select it for future payment paths.

C returns an error to A as the source of the payment path failure.

C Should only fail if the payment is to C. It should (maybe could use a test for this) still happily forward an HTLC with the same payment hash onwards.

Ah you're right, the relayed keysend payment shouldn't appear twice in pending_inbound_payments we won't generate a PaymentReceived.

ariard · 2021-07-22T23:53:41Z

lightning/src/ln/channelmanager.rs

+			// we have at least HTLC_FAIL_BACK_BUFFER blocks to go.
+			// Also, ensure that, in the case of an unknown preimage for the received payment hash, our
+			// payment logic has enough time to fail the HTLC backward before our onchain logic triggers a
+			// channel closure (see HTLC_FAIL_BACK_BUFFER rationale).


I think keysend make it a bit easier to exploit our hash-collision in ChannelMonitor.payment_preimages.
Let's say you have the following topology : M1 -- A -- B -- M2
M1 and M2 are collusioning attackers.
M1 sends a keysend to A, populating its payment_preimages with H (ChannelManager::claim_funds_from_hop())
M1 sends a regular payment H to M2 via A and B
M2 breaks the chan, and pin the commitment, B will never update_fail_htlc to A before the forward payment is settled (is_resolving_htlc_output)
B goes onchain on the AB link as there is a pending inbound HTLC for which it knows the preimage.
M1 and M2 succeed to close a channel, of which they're not a counterparty
Attack can be made economically rational by batching, if B has neighboors C, D, E, ..., malicious HTLCs on their channels can be routed and made pending on the one M2's commitment
Previously, this attack would have require interactivity with B, like paying an invoice for each malicious HTLC

Ultimately, I don't think keysend is faultive, it's a known defect of our ChannelMonitor logic, so I don't believe there is anything to do on this PR...

it's a known defect of our ChannelMonitor logic

If the attack starts with "M2 prevents a channel-close transaction from going onchain because of pinning" I don't think we have a bug, lightning has a bug :). There isn't anything we can do about this short of fixing Bitcoin Core.

TheBlueMatt · 2021-07-27T19:14:01Z

Needs rebase.

This doesn't yet use the field, but it will be used in upcoming commits.

C-Lightning requires us to advertise this feature before they'll attempt a keysend payment to us.

This will allow keysend tests to assert that the PaymentReceived payment preimage is as expected in upcoming commits.

and use it to make more keysend tests

Clearer phrasing

ariard · 2021-07-27T22:56:11Z

Code Review ACK 6dd6289, good on my side!

Thanks to adding coverage and can't think of of other weirdness. W.r.t to naming we can fix it with follow-ups once we have feedbacks from users.

valentinewallace · 2021-07-27T22:58:29Z

Code Review ACK 6dd6289, good on my side!

Thanks to adding coverage and can't think of of other weirdness. W.r.t to naming we can fix it with follow-ups once we have feedbacks from users.

Thanks, I agree, we can bikeshed on naming in follow-up

TheBlueMatt

Looks good mod one comment. Gonna go ahead and merge so we can move forward, but would be good to follow up.

TheBlueMatt · 2021-07-28T00:25:35Z

lightning/src/ln/channelmanager.rs

+	/// never reach the recipient.
+	///
+	/// Similar to regular payments, you MUST NOT reuse a `payment_preimage` value. See
+	/// [`send_payment`] for more information about the risks of duplicate preimage usage.


This also needs to mention that you should see send_payment for more details on the return value. Further, this needs to mention that the Route must only have at most one path. Finally, we need to actually check that, but not here - as far as I can see we don't currently require that non-payment-secret-containing sends don't contain more than one path, which we should do in send_payment_internal.

jkczyz · 2021-07-28T03:22:59Z

Thanks, I agree, we can bikeshed on naming in follow-up

SGTM

valentinewallace force-pushed the 2021-06-keysend branch from 9c80f5f to 93a3533 Compare June 23, 2021 19:20

TheBlueMatt reviewed Jun 23, 2021

View reviewed changes

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

ariard reviewed Jun 23, 2021

View reviewed changes

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

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

jkczyz reviewed Jun 24, 2021

View reviewed changes

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

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

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

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

valentinewallace force-pushed the 2021-06-keysend branch 4 times, most recently from a4fe071 to b825069 Compare June 25, 2021 21:58

valentinewallace marked this pull request as ready for review June 25, 2021 22:03

valentinewallace force-pushed the 2021-06-keysend branch from b825069 to de9d7f7 Compare June 25, 2021 23:44

TheBlueMatt reviewed Jun 26, 2021

View reviewed changes

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

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

valentinewallace force-pushed the 2021-06-keysend branch 6 times, most recently from cf23fbf to 08a1cb3 Compare July 2, 2021 18:40

valentinewallace commented Jul 2, 2021

View reviewed changes

lightning/src/util/events.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jul 3, 2021

View reviewed changes

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

valentinewallace force-pushed the 2021-06-keysend branch 2 times, most recently from 2cdf5fb to a003460 Compare July 4, 2021 22:00

TheBlueMatt reviewed Jul 5, 2021

View reviewed changes

valentinewallace force-pushed the 2021-06-keysend branch 3 times, most recently from af5ea24 to 85950c0 Compare July 10, 2021 22:06

TheBlueMatt linked an issue Jul 12, 2021 that may be closed by this pull request

Spontaneous/keysend payments #505

Closed

jkczyz reviewed Jul 13, 2021

View reviewed changes

TheBlueMatt reviewed Jul 13, 2021

View reviewed changes

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

valentinewallace force-pushed the 2021-06-keysend branch from 85950c0 to 0ec10bd Compare July 14, 2021 23:19

ariard reviewed Jul 15, 2021

View reviewed changes

valentinewallace force-pushed the 2021-06-keysend branch from 0ec10bd to b6d7d55 Compare July 16, 2021 23:43

ariard reviewed Jul 19, 2021

View reviewed changes

valentinewallace force-pushed the 2021-06-keysend branch from b6d7d55 to d724a68 Compare July 19, 2021 22:46

ariard reviewed Jul 23, 2021

View reviewed changes

valentinewallace force-pushed the 2021-06-keysend branch 2 times, most recently from e008dad to 4fdcda5 Compare July 23, 2021 22:21

valentinewallace added 11 commits July 27, 2021 15:15

Parse keysend TLV field in onion.

99b0e7e

This doesn't yet use the field, but it will be used in upcoming commits.

Advertise keysend feature

288b93b

C-Lightning requires us to advertise this feature before they'll attempt a keysend payment to us.

Fix indentation in decode_update_add_htlc_onion

a8d5c6e

Add PendingHTLCRouting variant for receiving keysend payments

f60a65f

Refactor PaymentReceived event for keysend receives

d1e8d9c

Implement receiving keysend payments

5a42be0

test utils: add optional PaymentPreimage param to pass_along_path

d32052f

This will allow keysend tests to assert that the PaymentReceived payment preimage is as expected in upcoming commits.

Implement sending keysend payments (to public nodes)

2d94401

Implement utilities for keysending to private nodes

0328be3

tests: make PaymentSecret optional in pass_along path

47bcc18

and use it to make more keysend tests

Clarify decode_update_add_htlc_onion comment

6dd6289

Clearer phrasing

valentinewallace force-pushed the 2021-06-keysend branch from 4fdcda5 to 6dd6289 Compare July 27, 2021 19:18

TheBlueMatt approved these changes Jul 28, 2021

View reviewed changes

TheBlueMatt merged commit 0b4079d into lightningdevkit:main Jul 28, 2021

valentinewallace mentioned this pull request Jul 28, 2021

Add config option for rejecting keysends #1017

Closed

TheBlueMatt mentioned this pull request Jul 31, 2021

Reject Keysend/no-PaymentSecret sends with multiple paths #1026

Closed

valentinewallace mentioned this pull request Aug 6, 2021

Update keysend docs + add payment secret requirement for MPP #1038

Merged

Keysend #967

Keysend #967

Conversation

valentinewallace commented Jun 23, 2021 • edited Loading

codecov bot commented Jun 23, 2021 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valentinewallace Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

jkczyz Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt commented Jul 27, 2021

ariard commented Jul 27, 2021

valentinewallace commented Jul 27, 2021

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jul 28, 2021

valentinewallace commented Jun 23, 2021 •

edited

Loading

codecov bot commented Jun 23, 2021 •

edited

Loading

valentinewallace Jul 23, 2021 •

edited

Loading

jkczyz Jul 26, 2021 •

edited

Loading

TheBlueMatt Jul 23, 2021 •

edited

Loading