htlcswitch: attributable errors

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.

[joostjager]

This PR adds the ability for routing nodes and nodes that are the destination of a payment to generate and relay attributable errors. This capability is signaled through a new feature bit option_attributable_errors.

Sender nodes will examine the route and request attributable errors via the attributable_errors tlv field when each node on the path supports it. There is no preference for attributable error-supporting nodes (yet).

Intermediate and final nodes report the htlc hold time in their failure message payload. The sender node only logs the hold times (Hold times: 2526/1125/0). Updating a node's reputation based on the time they held an htlc is left for a follow-up pr.

Depends on lightningnetwork/lightning-onion#60

Corresponding spec PR: lightning/bolts#1044

By default, attributable errors is only enabled for intermediate and final hops. If requested by the sender through the forward onion, they'll return attributable errors. For the sender node, the user needs to opt-in to use the feature by specifying --routerrpc.attrerrors on the command line.

[joostjager]

One case to consider here is that if for some reason one of the nodes had to return InvalidOnionPayload, that node wasn't able to read the resolution format. Instead if will return an old-style error that we're not decoding properly. Not sure if we care because it should only happen in case of a bug?

[bitromortac]

Great work! I think it's nice to have everything hard-coded to not reveal any fingerprint vector (except for the signal). Also, the upgrade path looks very reasonable to me, only when a majority of the network has upgraded to attributable errors (and a full attributable error route is likely), we'll request that error type (although some senders may prefer those routers already earlier). This will give a quick transition from zero senders to many senders and therefore a larger anonymity set.
From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

[bitromortac]

Is there a reason why log.Errorf is not used?

[joostjager]

What extra formatting would you like to see added here? The error itself is already a full sentence. I followed the approach that was already taken in master:

lnd/htlcswitch/switch.go

Line 1311 in f1bca2a

log.Error(err)

[bitromortac]

This error handling seems to not belong here, right?

[joostjager]

It is pre-existing. Maybe only ErrInvalidOnionKey is ever returned here. But not sure if we want to change this in this PR?

[bitromortac]

Perhaps we could export minPaddedOnionErrorLength and insert it here. So semantically this would tell us that we have a zero message length and zero padding?

[joostjager]

We could, although in the rest of the lnd repo we generally calculate that number 260 from lnwire.FailureMessageLength and I aligned this new code with that.

Semantically we'll indeed communicate zero message with zero padding. But it really does not matter what we put here.

As a future step, we could define a new failure code in BOLT 04 for this, but it won't change much about the penalization.

[bitromortac]

What are the incentives for a router to report back honestly and to not to pass on a short error? Will this node be penalized for reporting? If the node does not get penalized for reporting, any relayer could do this to penalize their neighbor?

[joostjager]

If they pass the short error, the upstream node will replace it by a proper error and the sender will indeed penalize this node.

[bitromortac]

These flags are advertized by default, is it?

[joostjager]

Yes, they are.

[bitromortac]

Out of curiosity, why is this done, to check that all parsed fields have been processed? But it's missing that check

[joostjager]

Because right below here, the remaining records are stored as custom records. If we wouldn't delete here, the attr error record would also show up as a custom record.

[joostjager]

From a routing node's perspective, attributable errors may not be so nice if they know they are slow and could get penalized for it, but if the network should have fast payments this may be needed?

I think that ultimately they won't have a choice. At some point, I expect senders to avoid routing nodes that do not support attr errors.

[joostjager]

Added integration test.

[joostjager]

Updated lightning-onion dependency after adding updated test vector.

[joostjager]

Updated feature bit to 36/37

[joostjager]

Fallback to legacy format is probably fine. If this happens to be an attributable error route, the upstream node will substitute with an attributable error?

[thomash-acinq]

This should be added to the spec.