Split resume session relays

[benalleng]

Closes #1391

This commit ensures that each resumed payjoin-cli session is using a
separate instance of the RelayManager to then check the ohttp connection
independently. This fixes a bug where resuming would converge all
existing sessions to one ohttp relay.

Also, the incompatible_msrv allow seems not needed anymore?

Big pickle wrote most of this code. The free open weight models aren't too shabby now either. Though it did want to recreate the entire config instead of just making the app mutable. I thought this seemed a bit easier to parse as its clear everything else is the same.

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[coveralls]

Coverage Report for CI Build 24364263264

Coverage increased (+0.02%) to 84.36%

Details

• Coverage increased (+0.02%) from the base build.
• Patch coverage: 16 uncovered changes across 2 files (48 of 64 lines covered, 75.0%).
• 2 coverage regressions across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v2/mod.rs	56	42	75.0%
payjoin-cli/src/app/v2/ohttp.rs	8	6	75.0%

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin-cli/src/app/v2/mod.rs	2	55.37%

---

Coverage Stats

Relevant Lines:	12820
Covered Lines:	10815
Line Coverage:	84.36%
Coverage Strength:	411.86 hits per line

---

💛 - Coveralls

[arminsabouri]

CAck cf8b0b7

big pickle? Cool.
Should we / do we already ensure that there is > 2 ohttp relays ?

[spacebear21]

This PR also includes the commit from #1390, is it meant to supersede that PR? I left a comment there that I think we should address holistically (i.e. is ohttp_keys "cache" config field relevant at all since we refetch every time anyways?)

[benalleng]

I should add blocked as this simply builds on top of #1390

This PR would have simply needed to also add the commit from #1390 as the ohttp_keys have been "cached" by that point so relay selection is skipped and resume would just choose ohttp_relay[0] no matter what. #1390 (comment)

[bc1cindy]

tACK 5062488

minimal fix, each resumed session gets a fresh RelayManager, isolating relay state between sessions

but I think removing state from RelayManager and moving randomization into the struct as a method could simplify the design further, as commented in #1385 (comment)

[codaMW]

Reviewed the fix, giving each resumed session a fresh RelayManager instance is a clean, minimal solution that correctly isolates relay state between sessions.

One open question from the discussion: @arminsabouri asked whether we should ensure there are >1 ohttp relays configured before attempting randomization. If ohttp_relays has only one entry, each session will always select the same relay regardless of this fix, the privacy benefit only kicks in with multiple relays. Is there a place we should surface a warning or enforce a minimum?

Also noting the 50% coverage on changed lines flagged by Coveralls, would a test asserting that two resumed sessions receive different relay instances be feasible here?

[arminsabouri]

This seems a bit off to me. Can add a getter to god app struct which randomizes which relay is use instead of cloning the app and mutating?

Maybe as simple as:

pub(crate) fn relay_manager (&self) -> RelayManager {
    RelayManager::new()
}

[arminsabouri]

Reviewed the fix, giving each resumed session a fresh RelayManager instance is a clean, minimal solution that correctly isolates relay state between sessions.

One open question from the discussion: @arminsabouri asked whether we should ensure there are >1 ohttp relays configured before attempting randomization. If ohttp_relays has only one entry, each session will always select the same relay regardless of this fix, the privacy benefit only kicks in with multiple relays. Is there a place we should surface a warning or enforce a minimum?
Also noting the 50% coverage on changed lines flagged by Coveralls, would a test asserting that two resumed sessions receive different relay instances be feasible here?

Thanks for resurfacing that. Would a warning string be enough? Eitherway would like to get this PR through. This can be follow up work

[arminsabouri]

Why do we need to pass a mut reference if we can get a relay manager from self.relay_manager() ?
Not requesting a change, just a question

[benalleng]

No you're right the current behavior would I think just return back to caching the relay when it finds one that works during any long polling No actually I think I am getting mixed up, I'm jumping ahead towards per-request logic. I don't think the logic fundamentally changes though I think its unnecessary to bubble it up to these calls instead of right before unwrap_ohttp_keys_or_else_fetch