Attributable failures

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?

[thomash-acinq]

Implements lightning/bolts#1044

For cross compatibility, the following scenarios have been successfully tested:

• LDK generates an attributable failure and eclair decodes it
• eclair generates an attributable failure, LDK relays it, eclair decodes it
• LDK generates an attributable failure, eclair relays it, LDK relays it, eclair decodes it

It is disabled by default since the spec may not be final, and will be enabled once the spec PR has been merged to the BOLTs.

[t-bast]

Thanks, will take a look at this next week! Meanwhile, could you take a look at the interactions with trampoline and answer Joost here: lightning/bolts#836 (comment)

[t-bast]

I haven't reviewed the code yet, but before doing so I'm not seeing important tests:

• Apart from the tests in SphinxSpec, I don't see any test:
  • are there official spec test vectors? If not, we must create some, and if there are already some, it should be a dedicated unit test to make it obvious
  • can you add unit tests where this is used during payments (maybe in PaymentPacketSpec) to verify the complete encryption/decryption from failure messages?
• Have you done cross-compatibility tests? We must verify cross-compatibility before merging any of this. The PR description should detail what has been tested, against what implementation, and what works and what doesn't work

[t-bast]

Looks mostly good to me, but I think we're missing tests for the scenarios where one of the intermediate nodes drops the attribution data, or where part of the route does not support attributable failures. That's what makes attributable failures useful, allowing us to detect that at least a subset of the route is honest!

I think some of those tests should be in the official test vectors, can you discuss that with Joost to see how they could be included?

[thomash-acinq]

I've added tests checking that we are able to attribute failures even if the failing node is hiding. I'll suggest to add it to the spec.

[t-bast]

Can you add release notes since this is a new spec feature?