Attributable failures #2256
Attributable failures #2256
Conversation
|
Single hop interop testing passes. Multi hop interop testing seems to be blocked by gossip propagation issues between lnd and rust-lightning. |
|
Nice! That's not too bad, thanks for working on this. I'll dig into the crypto in a day or two. Have a number of comments on the code itself and structure, but I assume its not really worth digging into until we support both old and new errors? I'm happy to give more code-level feedback now if you prefer.
Oh? Is this some known issue on the lnd end? I'm not aware of any gossip errors. |
This doesn't surprise me. This is my first venture into Rust land. It is a bit of a switch from golang, and I need to get used to how things are done. The language is not bad though, I can see why people fall in love with it! But yes, can hold off on code-level comments for now.
Not sure if it is a known issue. I've had nagging gossip issues before when I tried to do a pathfinding benchmark. For this PR, I did apply a patch to rust-lightning somewhere to force-send node announcement always and then it worked better. Will try to come up with a reasonable repro scenario. |
|
What are you using to do the testing? I assume ldk-sample of some form? If you change the timer at https://github.com/lightningdevkit/ldk-sample/blob/main/src/main.rs#L791 it will rebroadcast a fresh node announcement more aggressively. |
|
Yes, ldk-sample. It was doing the timer alright, but somehow it got filtered out in the timer handler. |
|
Would be happy to take a look at logs. At the TRACE level we should basically be writing everything that is going out or coming in on the wire. |
|
Yes, so I did see that the announcement wasn't going out. Will continue tomorrow and get back with more data. |
|
Errr, right, so to avoid dumping tons of gossip crap that message is only really logged at a high level ( |
|
Looks like timer is at 60s. I definitely waited much longer than that, so doesn't seem to be the problem. Enabled gossip logging and saved log files. Continuing in #2259 |
|
With gossip fixed with the hint in #2259, I was able to run through a few of the multi-hop inter-op scenarios: Obviously there are more, but I think this is a good enough sanity check for now. As mentioned above, attention should go to the crypto part of this feature first. |
|
Pushed 2025 version of the code to this PR. |
lightning-invoice/src/lib.rs
Outdated
| @@ -597,12 +597,18 @@ impl InvoiceBuilder<tb::False, tb::False, tb::False, tb::False, tb::False, tb::F | |||
| /// Construct new, empty `InvoiceBuilder`. All necessary fields have to be filled first before | |||
| /// `InvoiceBuilder::build(self)` becomes available. | |||
| pub fn new(currency: Currency) -> Self { | |||
| let mut features = Bolt11InvoiceFeatures::empty(); | |||
| features.set_attributable_failures_optional(); | |||
There was a problem hiding this comment.
Generally we set features elsewhere, afaiu. Setting it here forces everyone using this to include the set features, but its possible someone builds an invoice using this logic that isn't terminating at a ChannelManager.
There was a problem hiding this comment.
Is there a different place in LDK where this feature can be set, or is it left fully up to the caller to not forget?
There was a problem hiding this comment.
The invoice builder mostly has setters that set individual flags called by invoice_utils/channelmanager::create_bolt11_invoice. eg basic_mpp.
There was a problem hiding this comment.
Will leave it out of this PR except for the definition of the bits. Until interop is completed we don't need the feature bits.
joostjager
force-pushed
the
attr-errs
branch
2 times, most recently
from
f8199d5 to
75959ae
Compare
Signal to senders that the node will return attributable failures. When the sender makes sure all path nodes supports this, they will be able to attribute every failure that may occur.
|
Ready for a first pass |
Implements lightning/bolts#1044
This PR implements the
update_fail_htlcoptionalattribution_datafield as tlv type101rather than type1until interop with other node software is achieved.Until that time, the feature bit doesn't need to be set either.