Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Part 1: offers cleanups #7454

Merged

Conversation

rustyrussell
Copy link
Contributor

In updating and enhancing bolt12 to be a production-quality implementation, I found these miscellaneous nits which needed fixing.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a8eaedc

Copy link
Contributor

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a8eaedc

Nice set of tidy-ing up!

@vincenzopalazzo
Copy link
Contributor

===End Flaky Test Report===
=========================== short test summary info ============================
FAILED tests/test_pay.py::test_fetchinvoice_disconnected_reply - pyln.client.lightning.RpcError: RPC call failed: method: fetchinvoice, payload: {'offer': 'lno1qgsqvgnwgcg35z6ee2h3yczraddm72xrfua9uve2rlrm9deu7xyfzrcgqyzs5fr5v4ehghmxv46xx6rfdemx76trv40kg6tnvdhkumn9vd6x2ezlwfjhqmrezcssxhftzxfdlwsnfcgw2sy8t5mxa0ytcdfat2nkdwqvpy9nnsa9mzza', 'dev_reply_path': ['0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518', '022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59']}, error: {'code': 1005, 'message': 'Timeout waiting for response'}
ERROR tests/test_connection.py::test_feerate_stress - ValueError: 
Node errors:
 - lightningd-2: had BROKEN messages
Global errors:
======= 1 failed, 687 passed, 61 skipped, 1 error in 1706.82s (0:28:26) ========

We did since v23.05.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This can't happen because we go the self-pay path in this case, but
once we fix that for bolt12, this can be reached.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It doesn't work but at least now it doesn't crash!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This was when we handled pre-TLV onions where the first byte was 0.  We haven't
done that for a while: you can tell, because process_onionpacket doesn't use
the parameter at all!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We don't use the payment_secret in bolt12, but in onion_decode() we
do put the path_secret (if of correct length) into payment_secret.  Not
realizing this confused me, so document that, and make sure we insist
on it being present for bolt12.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I noticed we were missing this.  Move logging up a level so it's easier to
spot the omission.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Only sphinx internally uses the hmac field: it's actually a general descriptor
of onion contents, which we can use elsewhere.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
A second user is coming.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…r connect_disable.

It's always the same string, so simplify the interface.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Document and enforce the --experimental-anchors deprecation, which was somehow missed in v24.02

Changelog-Deprecated: Config: the --experimental-anchors option is ignored (on by default since v24.02).
We never call `listconfigs` in our tests with `experimental-offers` enabled,
so we didn't notice that the schema is wrong: it does not expect the
"plugin" field in 'fetchinvoice-noconnect'.

The next patch folds the fetchinvoice plugin into the offers plugin,
which is enabled even if `experimental-offers` isn't (for `decode`),
so we notice it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the part-1-offers-cleanups branch from a8eaedc to 1fb6cb3 Compare July 9, 2024 12:13
@rustyrussell
Copy link
Contributor Author

Trivial rebase on master, for flake fix.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1fb6cb3

@vincenzopalazzo vincenzopalazzo enabled auto-merge (rebase) July 9, 2024 12:51
@vincenzopalazzo vincenzopalazzo merged commit 7f71e63 into ElementsProject:master Jul 9, 2024
31 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants