Issue 789. Avoid the duplication of websocket messages that failed that test

[aaronmcdaid]

The test ensures that the state goes from UNPAID -> PAID -> ISSUE

Sometimes though, there is duplication and the test fails: UNPAID -> UNPAID -> PAID -> PAID -> ISSUE, reported in Issue #789

After the invoice has been paid, the wallet calls wallet.mint to convert that paid invoice into issued proofs.

Every state transition calls the callback and wallet.mint was being called on every state transition.

In this PR, the call to wallet.mint is made just once - just when it's PAID - instead of three times

[aaronmcdaid]

A question about NUT-17 that seems to be relevant for this test:

Does NUT-17 allow duplication, i.e. a state message to be sent to the websocket even if the state hasn't changed? The text isn't very clear to me, implying that only changes are recorded:

of the current state of the subscribed objects and whenever there is an update for the wallet's subscriptions

[KvngMikey]

A question about NUT-17 that seems to be relevant for this test:

Does NUT-17 allow duplication, i.e. a state message to be sent to the websocket even if the state hasn't changed? The text isn't very clear to me, implying that only changes are recorded:

of the current state of the subscribed objects and whenever there is an update for the wallet's subscriptions

hi @aaronmcdaid ,

NUT-17 wording: “… current state … whenever there is an update …”

That phrase does not guarantee deduplication, it only says the mint must push the new state when it changes.
A retransmission of the same state is therefore not forbidden, and most websocket implementations will resend on reconnect or heartbeat.
So the test must tolerate duplicates the fix in this PR (gate on state == PAID + >= 3 assertion) is exactly the right approach.

thanks for the quick fix.

[KvngMikey]

hi @callebtc @gudnuf

I too hit the same flake while setting up a local dev environment.
The test failed ~60 % of the time with assert 5 == 3 (or 4 == 3) because the callback fired multiple paid notifications and each one tried to mint, triggering “Mint quote already issued”.

I've implemented the changes in this PR, ran the loop below 20 times and all green:
for i in {1..20}; do poetry run pytest tests/wallet/test_wallet_subscription.py::test_wallet_subscription_mint -v done
and there are no more duplicate mint() calls; only the first paid message triggers the mint.

The assertion change to >= 3 is safe: we still enforce the three expected states (unpaid, paid, issued) in order, but tolerate harmless retransmissions, no regressions spotted; happy to approve.

[callebtc]

@aaronmcdaid @KvngMikey that's amazing thank you! Finally someone found the issue! You don't know how grateful I am for this.

[aaronmcdaid]

Is there anything I can do to advance this PR, or is it good as-is? I see some of the test pipelines timed-out after six hours

The assertion change to >= 3 is safe: we still enforce the three expected states (unpaid, paid, issued) in order, but tolerate harmless retransmissions, no regressions spotted; happy to approve.

Currently, the test still has ==3:

    assert len(msg_stack) == 3
    assert msg_stack[0].payload["state"] == MintQuoteState.unpaid.value
    assert msg_stack[1].payload["state"] == MintQuoteState.paid.value
    assert msg_stack[2].payload["state"] == MintQuoteState.issued.value

To be tolerant to duplicates, how about the following?

    assert len(msg_stack) >= 3
    # First state is UNPAID
    assert msg_stack[0].payload["state"] == MintQuoteState.unpaid.value
    # There must be at least one PAID state
    assert any(m.payload["state"] == MintQuoteState.paid.value for m in msg_stack)
    # Last state is ISSUED
    assert msg_stack[-1].payload["state"] == MintQuoteState.issued.value

[callebtc]

how about the following?

Thanks! Yes, sounds good to me, I hope it solves the issue with the timed-out pipelines

[aaronmcdaid]

I've pushed that change here just a few seconds ago, to allow duplicates and to test more directly what we want (first==UNPAID, last==ISSUED, with PAID appearing at least once in between)

[lollerfirst]

This PR needs to be rebased on main as there are some new tests