lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.64k stars 2.07k forks source link

[bug]: Blinded Path Invoice Encoding/Decoding #9121

Closed ziggie1984 closed 1 week ago

ziggie1984 commented 1 week ago

When creating a BOLT11 invoice with a blinded path I think we need to also require route-blinded besides the bolt-11-blinded-paths feature:

     "25": {
            "name": "route-blinding",
            "is_required": false,
            "is_known": true
        },
        "262": {
            "name": "bolt-11-blinded-paths",
            "is_required": true,
            "is_known": true
        }

example(regtest): lnbcrt10u1pnwkf2npp5naw8sa96a8er9944vxznk9fmy8wfc6c65eg2slx0fkewa5w39x4sdqqxqyz5vq5juqqqq3xgqqqqqxqgrqqqqqqqqqqzycqqqqqqr2xu5cqqqqqug05d7h9ap2qjda5ug8pp7c0lpdv9tut8w6ppm4p3p0pqpwphgnvpsyrpjf8af6rf4r9n2lyh5xpjmg884guld74n6lv009557aht4vx7wglnm4m66ugp3lz6dv3p30qpqllz4yu5njyzrmv46282dhajv88d5ztydg2l0m67wxtfwcqqjma6y3dp8qyqgp7welh99uj5j69jsf784kl5ehnzkwmasyvl7xxyp8hxzt8t6uteh2r75j8wj6r29dszrnglkrhc8vgnu7g2pguulhzep6k9l3w2k84x490fj67myn6hkmet4q5yc6etz6n3fw554azcvpumcrvck0yzp6d0c8t7yel46mf8yt750jdj5trwxa9yhj6n25x4297yd0vdq89jugp22rk7ehqfc8jp30mpeudlfd8r4mmxwvx96s9x49zmf6fy6ga63cdcj6n50pt9p93u5jvrfwuzk35w092fnftctsayt79a0wjx05jgy3dupm5jym3gr752dtl5rcped8ptd6973l9gdqne5aegmtyxprst0lxqhjpps5juqqqq3xgqqqqqxqgrqqqqqqqqqqzycqqqqqqq2nuwuqqqqq7v0lrh5dmdxyefe4xnashls3qhcmsfvsz869w8z398htm22464supsyrpjf8af6rf4r9n2lyh5xpjmg884guld74n6lv009557aht4vx7wga48738m6h90lk6sytnx66h3s0dj6lmla2lrjfcqjrxqwdlqhuvpvtysc9wslqtjshqth26cq9uagkxyt9eeughr22pm79jt9832phj388hf47c6jq4q8qlq8xydxekcmx2huydvj3vj9gu68cy2z4xvzfjc8s3rul8ldx6qg75pk0jd9qkvnuv79f67ay7f8l0uulqfcfqtkghutknydzeccnrds6gj79p4q3at3kc9mr74zenqsd3tundnun6fnk52gj6xh7h9tp0qm2kvwkgpxw0sxg4hy4mm8wr7lqaxe5tq2arft3ur0pg9qzzzv5z4wslldt4wh8fxg7zpes5uvppjqtwzl0utdat3gzsj46uum2909vuvztc2gk8nxfre26qq3ymdcldf2fdlr8ugjldp0t8jnhrw5d5dh2sln3jh6cmvm8f054xwwxt44t0q5juqqqq3xgqqqqqxqgrqqqqqqqqqqzycqqqqqqr2xu5cqqqqq5hj5kpvqw6fkdq432urry0s0yf3095z7uu5x88lhxvm3d5whcltypsyrpjf8af6rf4r9n2lyh5xpjmg884guld74n6lv009557aht4vx7wglshelmeang0dm4acjew2vr56mstp77wut0m4spaf8vu6hq37c3vcn803sytxff4lwdwhu2m4tc9yfp7dge432h5p4ayl9pqz3xc5r53wwd4yt8jew4sx5q4nladwlrc8gy8x7dxnv52tg4czl2wwpqy45csqqwh7jv9lea3guylhxvz2jsmjq9ew62jkc4fva4vqc6v8eshnwcj352858d3mashu6n867cdgu7z69r0pjmzy706k03w0fg56vg9sgr8cxuhnehg6rnjkjp68z4u7qeq9qyfjrp2q5rfx68mv00xvy5hm600ykyxjxpurwz6nd0dqwyvtrzqgu8gysdd47x9wadtyzgrsw4ymut8wxg800d5rlux0653g8289qkjs6a8y0xua0eax34repn2mzsqq005yv6da2lt7xk4g4x3fln6t4duj2zgc0tfelrs9p4yqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqpq9qgqy2xxu3y4p9lftn6amkkc3rfw9tma5qxx3j26mps75ydnrapn3h6s7ql84wyjh67wqeq7lqw5jksn532hryf0sv80w9854lesfc2tz5gq2r5akv

Basically also requiring feature 25 (is_required=true)

Moreover when the bolt11 invoice has no blinded path we should not set the feature in the first place currently setting the feature to is_known=true. I think an invoice is either a blinded path invoice then the destination key is just a random key (not a real destination address) or we have a normal invoice, then we should probably remove the blinded feature bits all together and also remove the blinded_path field from the invoice.

Example (regtest): lnbcrt10u1pnwkfudpp55ta7wvagad7a3g0588308uzrt752hlw84txnwtelzccvhpyxlmzsdqqcqzzsxqyz5vqsp5uvujnqquy4cjamxc5e5vg8253u5qpzyn478hkyu0kgjeh0rdgs6s9p4gqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqpqysgq7vzj4qpm5tdyvfa260d39n8yvt9q783v20jp2f5wp3c2630felmhmlhw34pxksmz6e9m403rmvpf6wk6n39g9yrwv4seev8utx9l8wcqc6vg03

Roasbeef commented 1 week ago

I think we need to also require route-blinded besides the bolt-11-blinded-paths feature:

What's the gap here? One is a dependency of the other.

Moreover when the bolt11 invoice has no blinded path we should not set the feature in the first place currently setting the feature to is_known=true.

That's the role of even/odd, if odd then clients don't break when trying to parse it. Does seem to be an oversight if we're actually still always setting it though.

ziggie1984 commented 1 week ago

What's the gap here? One is a dependency of the other.

I was wondering why we just have the is_known field set to true when blinding is active, I thought we should also require the dependency ? Setting it like this for the route_blinding feature bit (24):

"is_required": true,
"is_known": true

That's the role of even/odd, if odd then clients don't break when trying to parse it. Does seem to be an oversight if we're actually still always setting it though.

Ahh ok, I was just looking at our behaviour for AMP invoices, and there we actually don't keep the AMP feature bit when the invoice is not AMP? I think there is no real benefit including it, but maybe as you mentioned we were already thinking of fetching the blinded path via other means than the invoice itself ?

There is also no real problem in signaling it either as you mentioned. So maybe we can close this then ?

Roasbeef commented 1 week ago

I was wondering why we just have the is_known field set to true when blinding is active, I thought we should also require the dependency ?

Check out the logic: https://github.com/lightningnetwork/lnd/blob/84c91f701cab21c70c5f4292b82a8bcce9a79e63/lnrpc/invoicesrpc/utils.go#L237-L245

Is known will pretty much always be true.

There is also no real problem in signaling it either as you mentioned. So maybe we can close this then ?

Closing based on this comment.

ziggie1984 commented 4 days ago

cc @ellemouton