Offers: bolt12 updated for latest draft.

[rustyrussell]

In some places this is a minimal implementation, and it's still experimental, but it Works For Me!

First three patches make sure we track the latest route blinding spec.

The offers spec changed so much that I'm afraid "rewrite" is more appropriate, particularly bolt12: import the latest spec, update to fit. which is pretty hard to split further. Sorry.

[cdecker]

Got up to f7e350e, after the dreaded monster commit, but then my eyes glazed over.

One thing I noticed is the splitting of the onion code, and I was wondering if it has any actual impact on compilation time, or binary size in the end, that'd warrant the big code move.

[rustyrussell]

Got up to f7e350e, after the dreaded monster commit, but then my eyes glazed over.

One thing I noticed is the splitting of the onion code, and I was wondering if it has any actual impact on compilation time, or binary size in the end, that'd warrant the big code move.

I split it because I ended up dragging in too much, IIRC. OTOH the blinding code was developed in pieces, but it may want further rationalization.

The onion code has multiple files which I find confusing (sphinx, onion, all the blinded* parts) which may want a cleanup now we have removed legacy onion support.

[rustyrussell]

Updated rust build, and fixed the missing API update noted by @cdecker.

[cdecker]

Gotcha, I was thinking there might be a performance reason to do so that I was unaware, but I understand the desire of a cleaner separation, given the onion and sphinx code is about the earliest code I wrote, and didn't have a good idea on how to structure things yet 😋

[cdecker]

Squashed fixups, and eyes glazed over, but looks ok.

ACK cb83675

[cdecker]

There are some missing schemas for the new RPC methods, that would have helped interpret the meaning of the new RPC methods too. Worth following up with a cleanup PR.

[rustyrussell]

Trivial rebase on master (weird CI issues?) and remove final commit (enhances cppcheck, but now we need more annotations...)

[cdecker]

Rebased, merging as soon as CI is happy.

ACK 0f329ff

[cdecker]

Seems I was celebrating prematurely. There seems to be a use of uninitialized memory that reliably kills the htlc_accepted hook:

Valgrind error file: valgrind-errors.68291
==68291== Conditional jump or move depends on uninitialised value(s)
==68291==    at 0x179003: htlc_accepted_hook_serialize (peer_htlcs.c:1080)
==68291==    by 0x18714C: plugin_hook_call_next (plugin_hook.c:242)
==68291==    by 0x187333: plugin_hook_call_ (plugin_hook.c:279)
==68291==    by 0x1795B8: plugin_hook_call_htlc_accepted (peer_htlcs.c:1195)
==68291==    by 0x179E92: peer_accepted_htlc (peer_htlcs.c:1407)
==68291==    by 0x17D26B: peer_got_revoke (peer_htlcs.c:2412)
==68291==    by 0x1307F9: channel_msg (channel_control.c:535)
==68291==    by 0x18AF2C: sd_msg_read (subd.c:557)
==68291==    by 0x2A600A: next_plan (io.c:59)
==68291==    by 0x2A6C12: do_plan (io.c:407)
==68291==    by 0x2A6C54: io_ready (io.c:417)
==68291==    by 0x2A8F47: io_loop (poll.c:453)
==68291== 
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:htlc_accepted_hook_serialize
   fun:plugin_hook_call_next
   fun:plugin_hook_call_
   fun:plugin_hook_call_htlc_accepted
   fun:peer_accepted_htlc
   fun:peer_got_revoke
   fun:channel_msg
   fun:sd_msg_read
   fun:next_plan
   fun:do_plan
   fun:io_ready
   fun:io_loop
}

This refers to the check here that checks if p->payload->forward_node_id is not NULL, which it is, and the non-valgrind nodes then crash one line below when we try to encode the value as a JSON pubkey.

Most likely it is just a missing initialization of the forward_node_id in the hook payload creation, but I'll let @rustyrussell do that since he has more context here.

Thanks, addressed!

[rustyrussell]

Seems I was celebrating prematurely. There seems to be a use of uninitialized memory that reliably kills the htlc_accepted hook:

Valgrind error file: valgrind-errors.68291
==68291== Conditional jump or move depends on uninitialised value(s)
==68291==    at 0x179003: htlc_accepted_hook_serialize (peer_htlcs.c:1080)
==68291==    by 0x18714C: plugin_hook_call_next (plugin_hook.c:242)
==68291==    by 0x187333: plugin_hook_call_ (plugin_hook.c:279)
==68291==    by 0x1795B8: plugin_hook_call_htlc_accepted (peer_htlcs.c:1195)
==68291==    by 0x179E92: peer_accepted_htlc (peer_htlcs.c:1407)
==68291==    by 0x17D26B: peer_got_revoke (peer_htlcs.c:2412)
==68291==    by 0x1307F9: channel_msg (channel_control.c:535)
==68291==    by 0x18AF2C: sd_msg_read (subd.c:557)
==68291==    by 0x2A600A: next_plan (io.c:59)
==68291==    by 0x2A6C12: do_plan (io.c:407)
==68291==    by 0x2A6C54: io_ready (io.c:417)
==68291==    by 0x2A8F47: io_loop (poll.c:453)
==68291== 
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:htlc_accepted_hook_serialize
   fun:plugin_hook_call_next
   fun:plugin_hook_call_
   fun:plugin_hook_call_htlc_accepted
   fun:peer_accepted_htlc
   fun:peer_got_revoke
   fun:channel_msg
   fun:sd_msg_read
   fun:next_plan
   fun:do_plan
   fun:io_ready
   fun:io_loop
}

This refers to the check here that checks if p->payload->forward_node_id is not NULL, which it is, and the non-valgrind nodes then crash one line below when we try to encode the value as a JSON pubkey.

Most likely it is just a missing initialization of the forward_node_id in the hook payload creation, but I'll let @rustyrussell do that since he has more context here.

Fixed, thanks!

[rustyrussell]

Fixed another uninitialized variable in debug logging too.

[cdecker]

Thanks for the fix. It now just has one failure where the error does not match the expectation:

        with pytest.raises(RpcError, match="INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS.*'erring_node': '{}'".format(l3.info['id'])):
>           l1.rpc.pay(inv2['invoice'])

Fails with this error instead:

>           raise RpcError(method, payload, resp['error'])
E           pyln.client.lightning.RpcError: RPC call failed: method: pay, payload: {'bolt11': 'lni1qqgf42den6cwzacntjjz66uvn7tkjq3qqc3xu3s3rg94nj40zfsy866mhu5vxne6tcej5878k2mneuvgjy8ssqgppg8hx6twvakx2tt4wdjjqar9wd6pvggrt543ryklhgf5uy89gzr46dnwhj9ux5744fmxhqxqjzeecwja3pw4qgqxyfhyvyg6pdvu4tcjvpp7kkal9rp57wj7xv4pl3ajku70rzy3pavzzq3gfvfwydjehktxec99tanr7tvvgt9g8gjcyxuggevt3nvus9p43jsfsq6a9vge9ha6zd8ppe2qsawnvm4u30p484d2we4cpsyskwwr5hvgt5pw5drhhvv8dgrzk6ee99nlnvddejuagzs5jaez6x3tqvjh4c8r0lqpqd5l2v3fz9pmzd733zrj3phe5vakljw5awn8g6zm0wv02ulre4gujqpjzgrcaqpg8w0e3xxahn35w6n2m7p6u3zuumxyhvlzp7a58gvcys9gpa7fg90x9vgucqjcee6yspfs9u8l5gwqqqqqqqqqqqqqqqzsqqqqqqqqqqqqr5jt9hav2gqqqqqq5szxx6wdj75zqjz9pdj24nt2ecjl776nyrxxpqq6y8ay3lk33j7duxmff2vz2fax4gqsrvppqdwjkyvjm7apxnssu4qgwhfkd67ghs6n6k48v6uqczgt88p6tky9muzq9qr62gcx0wwn55q86s594u9zd6x07snpkj2zwfv85gg2q7m5e050vmxewmp4k6qt6rtrhun2kphkdwzla6wgycc5uspqwtaeh98qf9s'}, error: {'code': 203, 'message': 'failed: WIRE_INVALID_ONION_BLINDING (reply from remote)', 'id': 6, 'failcode': 49176, 'failcodename': 'WIRE_INVALID_ONION_BLINDING', 'bolt12': 'lni1qqgf42den6cwzacntjjz66uvn7tkjq3qqc3xu3s3rg94nj40zfsy866mhu5vxne6tcej5878k2mneuvgjy8ssqgppg8hx6twvakx2tt4wdjjqar9wd6pvggrt543ryklhgf5uy89gzr46dnwhj9ux5744fmxhqxqjzeecwja3pw4qgqxyfhyvyg6pdvu4tcjvpp7kkal9rp57wj7xv4pl3ajku70rzy3pavzzq3gfvfwydjehktxec99tanr7tvvgt9g8gjcyxuggevt3nvus9p43jsfsq6a9vge9ha6zd8ppe2qsawnvm4u30p484d2we4cpsyskwwr5hvgt5pw5drhhvv8dgrzk6ee99nlnvddejuagzs5jaez6x3tqvjh4c8r0lqpqd5l2v3fz9pmzd733zrj3phe5vakljw5awn8g6zm0wv02ulre4gujqpjzgrcaqpg8w0e3xxahn35w6n2m7p6u3zuumxyhvlzp7a58gvcys9gpa7fg90x9vgucqjcee6yspfs9u8l5gwqqqqqqqqqqqqqqqzsqqqqqqqqqqqqr5jt9hav2gqqqqqq5szxx6wdj75zqjz9pdj24nt2ecjl776nyrxxpqq6y8ay3lk33j7duxmff2vz2fax4gqsrvppqdwjkyvjm7apxnssu4qgwhfkd67ghs6n6k48v6uqczgt88p6tky9muzq9qr62gcx0wwn55q86s594u9zd6x07snpkj2zwfv85gg2q7m5e050vmxewmp4k6qt6rtrhun2kphkdwzla6wgycc5uspqwtaeh98qf9s', 'raw_message': 'c0185fbe64cb00087d99ba487ce474cb3efaa18ad05412c0ffe004607df0a4ddc051', 'created_at': 1667878297, 'destination': '035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d', 'payment_hash': '48450b64aacd6ace25ff7b5320cc60801a21fa48fed18cbcde1b694a982527a6', 'status': 'failed', 'amount_msat': 1msat, 'amount_sent_msat': 0msat, 'erring_index': 2, 'erring_node': '035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d'}

So we expect an unknown payment, but get WIRE_INVALID_ONION_BLINDING. @rustyrussell can you confirm that the expected error has changed and we should be expecting the invalid onion blinding instead?

[rustyrussell]

Fixed test properly in correct place in series, which lead me to find a place where we'd broken it in another way for one patch.

Now there's a "disable checks" patch in there, which is reverted two patches later. But better than breaking bisect...

[cdecker]

Thanks @rustyrussell, git range-diff is here.

ACK 83cfc5f

[cdecker]

Rebased on top of the other merged PRs.

ACK 6c25666