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

Quick #3010 followups #3311

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

Few minor things I noticed in the second-pass review.

`creates_and_pays_for_offer_with_retry` intends to check that we
re-send a BOLT 12 `invoice_request` in response to a
`message_received` call, but doesn't actually test that there were
no messages in the outbound buffer after the initial send, which we
do here.
While `message_received` purports to be called on every message,
prior to the message, doing so on `Init` messages means we have to
call `message_received` while holding the per-peer mutex, which
can cause some lock contention.

Instead, here, we call `message_received` after processing `Init`
messages (which is probably more useful anyway - the peer isn't
really "connected" until we've processed the `Init` messages),
allowing us to call it unlocked.
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.

LGTM

@@ -1100,6 +1100,7 @@ fn creates_and_pays_for_offer_with_retry() {
expect_recent_payment!(bob, RecentPaymentDetails::AwaitingInvoice, payment_id);

let _lost_onion_message = bob.onion_messenger.next_onion_message_for_peer(alice_id).unwrap();
assert!(bob.onion_messegner.next_onion_message_for_peer(alice_id).is_none())
Copy link
Contributor

Choose a reason for hiding this comment

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

onion_messenger typo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants