Skip to content

tests: fix test_funding_v2_cancel_race flake - #9523

Open
daywalker90 wants to merge 1 commit into
ElementsProject:masterfrom
daywalker90:fix-test_funding_v2_cancel_race-flake
Open

daywalker90 wants to merge 1 commit into
ElementsProject:masterfrom
daywalker90:fix-test_funding_v2_cancel_race-flake

Conversation

@daywalker90

Copy link
Copy Markdown
Collaborator

openchannel_update is idempotent: once the commitments are secured a repeat call returns the existing inflight, so two racing openchannel_update calls can both legitimately succeed and the "only up to one should succeed" assertion no longer holds.

Keep the original intent (catching commands that fight over the shared command pointer and hang) by relying on result(TIMEOUT) raising TimeoutError, and replace the count check with the stronger invariant that no two distinct secured PSBTs (i.e. two commitments) were made.

Changelog-None

openchannel_update is idempotent: once the commitments are secured a
repeat call returns the existing inflight, so two racing
openchannel_update calls can both legitimately succeed and the "only up
to one should succeed" assertion no longer holds.

Keep the original intent (catching commands that fight over the shared
command pointer and hang) by relying on result(TIMEOUT) raising
TimeoutError, and replace the count check with the stronger invariant
that no two distinct secured PSBTs (i.e. two commitments) were made.

Changelog-None

@Andezion Andezion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

secured.add(result["psbt"]) compares the full serialized PSBT string for equality. But inflight->funding_psbt is mutated in place shortly after commitments are secured - once tx-signatures are exchanged, psbt_finalize(inflight->funding_psbt) is called (at file lightningd/dual_open_control.c at line 1809, reached from handle_peer_tx_sigs_sent, which fires immediately after the commit_send_ack round trip that follows handle_commit_ready). This adds final witness/scriptSig data to the same wally_psbt object that build_commit_response() serializes??

i mean we have call1 - gets its response from handle_commit_received right when commitments become secured -> serializes the unfinalized PSBT, and call2 arrives late enough (thread scheduling / socket jitter under 100 concurrent executor.submit calls) that by the time json_openchannel_update runs for it, psbt_finalize has already mutated the inflights PSBT -> serializes a different?, finalized? byte string for the same commitment? So both have commitments_secured == True, so both go into secured, and len(secured) <= 1 fails - even though there was only ever one commitment?

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.

2 participants