qa: add retransmit metro test to qa.testnet - #4156
Conversation
f842731 to
a75ee13
Compare
nikw9944
left a comment
There was a problem hiding this comment.
One finding beyond what's already in review.
The "and nothing else" subscriber assertion compares against User.Subscribers, which is the user account's complete multicast subscription list — not the groups the shred feed seat granted. Any unrelated group the QA client holds trips it, which is exactly the rocksteady / rebop-eu failure in the PR description. The deleted test had this right: retransmit group present, leader group absent.
The rejection payment now runs after wait_for_open_phase. The program is closed for the last grace-period slots of every epoch, so a payment made before the wait fails for the phase and never carries the onboarding message the test asserts on. The settlement flow is inlined into the test. It had one caller after the retransmit-only test went away, and the hook indirection hid the branch. The onboarding flag is read outside every subtest, so a -run filter can no longer skip the read and take the closest-device path in silence. The price assertion is back as --retransmit-price, checked against the price the program charges rather than the CLI quote. TestProgramConfigFlags tables all four program config flag accessors, including bit 7 for retransmit-only onboarding. ClosestRetransmitOnlyDevice and ClosestNonRetransmitOnlyDevice now share one selector taking the membership it wants.
A host running -multi-tunnel holds an IBRL user at the same client IP, and GetServiceabilityUser returns whichever account comes first. Reading that one leaves the subscriber list empty and the group check reports missing groups.
nikw9944
left a comment
There was a problem hiding this comment.
Re-reviewed at b32b8151. The user-type half of the previous finding is verified fixed — GetMulticastServiceabilityUser filters on UserTypeMulticast, so an IBRL user at the same client IP can no longer be read. The other reviewer's findings are all addressed in cb3ec334: the rejection payment moved after wait_for_open_phase, the harness is inlined with the flag read hoisted out of every subtest, the selectors share one predicate, TestProgramConfigFlags passes (its 0x7c case discriminates bit 7), and the CHANGELOG prefix is fixed.
Two things left, neither blocking:
- The exact-set subscriber assertion is unchanged. The rationale for keeping it is reasonable and it is your call, but it rests on config outside this repo and a live-network observation, so it stands as unverified here. Failures should name group codes rather than raw pubkeys, since the strict form will fire again on any oracle lag.
--retransmit-pricedefaults to0, which asserts nothing — the old flag defaulted to10. Until the paired infra change passes it, the enforced path goes green with the discounted price unchecked, silently.
elitegreg
left a comment
There was a problem hiding this comment.
Approving. The review feedback is addressed: the rejection step now runs after wait_for_open_phase, the flag is read outside every subtest, the selector dedup and the flag-bit table test are in, and GetMulticastServiceabilityUser fixes the multi-tunnel user ambiguity. go vet -tags qa ./e2e/... and the sdk/shreds/go tests are clean.
Two follow-ups filed rather than held against this PR:
- #4157 — the retransmit-only price and group assertions are gated on the ProgramConfig enforcement bit, but they only depend on a metro being flagged in MetroHistory, so an environment with flagged metros and enforcement off gets no coverage.
- #4158 — the stale oracle snapshot you traced the
rebop-euextra to sounds like a product bug worth confirming.
Remember this needs the malbeclabs/infra change landing alongside it: -leader-group-code / -retransmit-group-code are gone and an undefined flag fails the whole test binary.
A failure printed raw pubkeys, which is what made the first report of this check unreadable. One read of the program data now maps every group pubkey to its code, and the check labels both the extras and the subscribed list from it. That read also replaces the per-code lookups, each of which fetched the whole program data again. The seat-price assertion logs at Info when --retransmit-price is unset, so a run cannot look like it checked the price when it did not.
nikw9944
left a comment
There was a problem hiding this comment.
Both findings from the last round are verified fixed in 72e5802b.
Failures now name groups by code: MulticastGroupCodes reads the program data once and the label closure is applied to both the extras and the subscribed list, falling back to the pubkey for an unknown group. That single read also replaces the per-code lookups, each of which refetched the whole program data. And the enforced path logs at Info with the observed price when --retransmit-price is unset, so a run can no longer look like it checked the price. Keeping the default at 0 rather than 10 is the right call given Frankfurt costs 36.
One note carried forward, no action needed: the exact-set assertion still rests on config outside this repo, so it stays unverified here rather than disputed.
Summary of Changes
TestQA_MulticastSettlement.e2e/qa_shred_settlement_test.go.since it only contained one setup method that I moved into theTestQA_MulticastSettlementfile.edge-solana-retranse2e/qa_retransmit_only_settlement_test.go.TestQA_RetransmitOnlySettlementdrove the same positive path, so the reworked test covers it now. Its device selector, its metro-code formatter and its group assertion moved into the multicast test file.-retransmit-group-codes, which replaces-leader-group-codeand-retransmit-group-code. The check requires the seat's groups to equal that list, so any extra group fails it.sdk/shreds/go/state.gogainsIsRetransmitOnlyOnboardingEnforced, which reads bit 7 of the program config flags.e2e/internal/qa/client_settlement.gogains that same read, plusClosestNonRetransmitOnlyDevicefor the device the rejection step pays on.malbeclabs/infrapasses-retransmit-group-codes=edge-solana-retransinqa.testnet.ymland deletesqa.retransmits.testnet.yml. Merge both together, or the hourly job runs the group check without the codes it needs.