Allow duplicate-payment_hash HTLCs for HTLC forwards by TheBlueMatt · Pull Request #167 · lightningdevkit/rust-lightning

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

TheBlueMatt · 2018-09-11T19:28:04Z

This is required by BOLT 2 to ensure that no attacker can simply
relay every public node a duplicate-payment_hash HTLC for each HTLC
it receives to deduce where an HTLC came from.

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

This further simplifies the payment transition state a bit, so
hopefully at least we got some readability out of all of this.

This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

ariard · 2018-09-11T23:21:32Z

 	/// the remote side hasn't yet revoked their previous state, which we need them to do before we
 	/// accept this HTLC. Implies AwaitingRemoteRevoke.
 	/// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
 	/// a remote revoke_and_ack on a previous state before we can do so.


It wasn't a change but maybe a clearer comment on this one or AwaitingRemoteRevokeToAnnounce could be better, "which we need them to do before we apply its HTLC on our commitment tx" ? To signal that sending a HTLC in commitment_signed it's the same that applying it on our commitment tx, that we only do when we received remote revoke_and_ack

Hmm, I'm not 100% sure what you're suggesting here, but as its not a change I'm open to a new PR to change it.

ariard · 2018-09-12T01:04:27Z

+/// We hold various information about HTLC relay in the HTLC objects in Channel itself:
+///
+/// Upon receipt of an HTLC from a peer, we'll give it a PendingHTLCStatus indicating if it should
+/// Forward the HTLC with information it will give back to us when it does so, or if it should Fail


ariard · 2018-09-12T02:01:02Z

 }
- chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))
+ chan.update_fail_malformed_htlc(&msg, HTLCFailReason::Reason { failure_code: msg.failure_code, data: Vec::new() }).map_err(|e| MsgHandleErrInternal::from_maybe_close(e))?;
+ Ok(())


why not give back error here ? we can do it with HTLCSource internally now, right ?

We can't fail backwards until revoke_and_ack tells us it is safe to do so, receiving an update_fail_htlc/update_fail_malformed_htlc is really only an indication from the remote side that they intend to fail the HTLC, its really once they've revoked their previous state that we consider it done.

ariard · 2018-09-12T02:21:38Z

- session_priv: SecretKey,
-	},
-	/// Used for channel rebalancing
-	CycledRoute {


I find HTLCSource cleaner but os there edge cases that do we want to prevent with CycledRoute to us ? Not at first glance to me but still wandering..

I hope not? We can't tell the difference between a payment that went around the network and came back through us (unless we are the source) and two payments that are independant and is just an attempt to deanonymize a payment.

ariard · 2018-09-12T02:24:36Z

utACK, have review the whole, nothing shocks me!

This is required by BOLT 2 to ensure that no attacker can simply relay every public node a duplicate-payment_hash HTLC for each HTLC it receives to deduce where an HTLC came from. Note that this makes the claim logic much less incentive-compatible as we will not claim all available HTLCs with the same payment_hash even if we know the preimage! This is OK because, most likely, any attackers trying to map the network will use small-value payments and, hopefully, we will move away from constant hashes across an entire payment at some point in the near future. This further simplifies the payment transition state a bit, so hopefully at least we got some readability out of all of this

yuntai · 2018-10-12T04:47:00Z

Note that this makes the claim logic much less incentive-compatible
as we will not claim all available HTLCs with the same payment_hash
even if we know the preimage! This is OK because, most likely, any
attackers trying to map the network will use small-value payments
and, hopefully, we will move away from constant hashes across an
entire payment at some point in the near future.

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

TheBlueMatt · 2018-10-29T21:25:43Z

Duplicate claims will only happen when the recipient of a payment doesn't care about her privacy (and is also greedy) and issue multiple claims, so no reason to protect her while sacrificing our incentive? (also to cost less to the attackers)

No, any intermediate node can create a duplicate payment, which we would then claim. eg if Alice pays Eve through Bob, Charlie and Dave, Bob may send a payment with the same hash through a number of nodes on the network, including Dave, and if Dave claims all pending HTLCs which have the same hash when Eve gives Dave the preimage, then Bob will learn that the payment Alice sent went through Dave, which they shouldn't be able to learn due to the onion encryption.

yuntai · 2018-10-30T09:16:03Z

Thanks, very clear!

Split HTLC tracking into separate Inbound/Outbound types
3b49837
This isnt as simplifying as I'd hoped, but still increases compile-time checking, which is nice, and removes one of two panic!()s.

TheBlueMatt mentioned this pull request Sep 11, 2018
Do not claim/reject all inbound HTLCs at once based on preimage/hash #168
Closed

TheBlueMatt force-pushed the 2018-09-dup-htlc branch 2 times, most recently from d0330df to e847e65Compare September 11, 2018 21:16

ariard reviewed Sep 11, 2018
View reviewed changes

ariard reviewed Sep 12, 2018
View reviewed changes

TheBlueMatt added 3 commits September 12, 2018 11:15

Avoid cross-test statics in ChannelManager network tests
6b3cc8b

Add simple test for duplicate-payment_hash HTLC tracking
0a65255

TheBlueMatt force-pushed the 2018-09-dup-htlc branch from e847e65 to 0a65255Compare September 12, 2018 15:21

TheBlueMatt merged commit f27c74b into lightningdevkit:masterSep 12, 2018

TheBlueMatt mentioned this pull request Oct 10, 2018
Ready for Review : Handle resolution in case of channel going onchain, pass state backward offchain #198
Closed