Add core RelaySelector

[bc1cindy]

add payjoin::relay::RelaySelector, a reusable OHTTP relay-selection primitive (uniform-random over non-failed relays, with mark_failed failover), so integrators share one policy instead of each diverging.

non-breaking: io::fetch_ohttp_keys keeps its single-relay signature; selection and failover live in the caller, which reuses one long-lived RelaySelector (the cli's MailroomManager). SelectContext is the extension point for the AS-aware selection.

follow-up referenced in #1695 (review) by @DanGould; directory selection stays in the manager; durable tracking is #1697

part of #1694 / part of #1586

disclosure: co-authored by Claude

[coveralls]

Coverage Report for CI Build 29109211191

Coverage increased (+0.07%) to 86.072%

Details

• Coverage increased (+0.07%) from the base build.
• Patch coverage: 2 uncovered changes across 1 file (86 of 88 lines covered, 97.73%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v2/ohttp.rs	12	10	83.33%
Total (2 files)	88	86	97.73%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	15731
Covered Lines:	13540
Line Coverage:	86.07%
Coverage Strength:	347.22 hits per line

---

💛 - Coveralls

[zealsham]

Cack

one question, once a relay is marked as failed, its impossible to unfail except a total restart, Should we have time window for retryability ?

[bc1cindy]

thanks for the review! @zealsham

one question, once a relay is marked as failed, its impossible to unfail except a total restart, Should we have time window for retryability ?

RelaySelector only tracks failures in memory per operation. I opened #1697 for the failure-lifecycle policy, which covers exactly this "path back (re-test recovery)" question

keeping it out of this PR to stay focused on the selection refactor

[bc1cindy]

rebased onto master, integrated with #1695: relay selection moved out of MailroomManager into the lib RelaySelector

[xstoicunicornx]

Strong cACK, it is a good idea to modularize the relay selection so it is reusable by implementers and makes it easier to enforce good relay selection practices.

However, have some large pieces of feedback that affect the overall implementation and some questions on the design decisions.

[xstoicunicornx]

What is the intended purpose of this? It doesn't seem to be used.

[bc1cindy]

it's the extension point for AS-aware selection (#919/#1514): those fill it with request kind / time window / receiver key as private fields, so select gains them without a sig change

empty today = uniform-random (which is why it looks unused); expanded the doc to say this

[xstoicunicornx]

Since the AS-aware selection should be optional once it is implemented I think a better strategy would be to just create a new RelaySelector::select_as_aware method for AS-aware selection and not prematurely worry about it here. This would prevent callers who opt not to use AS-aware selection to not have deal with this extraneous struct, would prevent needing any sig change in the future (without the need for #[non-exhaustive]), and also potentially allow for feature gating AS-aware selection.

[bc1cindy]

thank you! @xstoicunicornx

ready for re-review!

[bc1cindy]

it's the extension point for AS-aware selection (#919/#1514): those fill it with request kind / time window / receiver key as private fields, so select gains them without a sig change

empty today = uniform-random (which is why it looks unused); expanded the doc to say this

[xstoicunicornx]

Couple pieces of feedback.

[xstoicunicornx]

Since the AS-aware selection should be optional once it is implemented I think a better strategy would be to just create a new RelaySelector::select_as_aware method for AS-aware selection and not prematurely worry about it here. This would prevent callers who opt not to use AS-aware selection to not have deal with this extraneous struct, would prevent needing any sig change in the future (without the need for #[non-exhaustive]), and also potentially allow for feature gating AS-aware selection.

[bc1cindy]

Since the AS-aware selection should be optional once it is implemented I think a better strategy would be to just create a new RelaySelector::select_as_aware method for AS-aware selection and not prematurely worry about it here. This would prevent callers who opt not to use AS-aware selection to not have deal with this extraneous struct, would prevent needing any sig change in the future (without the need for #[non-exhaustive]), and also potentially allow for feature gating AS-aware selection.

the reason to keep this in a single path is anti-fingerprinting: #919 explicitly calls out that divergent relay-selection behavior across wallets is itself a client fingerprint. A separate select_as_aware + feature-gated API would institutionalize an "asmap vs non-asmap" split, the exact fingerprint this is meant to reduce. (#919 even contemplates shipping it enabled by default.)

I think the cleaner split is: feature-gate the asmap data/dependency (that's the real weight, and why #1514's PoC gates it), but keep a single select() that transparently does AS-aware ordering when asmap data is present and falls back to uniform-random otherwise, so call sites converge and never fork. SelectContext is the vehicle for the per-request inputs the #919 algorithm needs (POST/POLL, time window, receiver key), which is why it's there now even though empty.

makes sense?

[Mshehu5]

Since the AS-aware selection should be optional once it is implemented I think a better strategy would be to just create a new RelaySelector::select_as_aware method for AS-aware selection and not prematurely worry about it here. This would prevent callers who opt not to use AS-aware selection to not have deal with this extraneous struct, would prevent needing any sig change in the future (without the need for #[non-exhaustive]), and also potentially allow for feature gating AS-aware selection.

the reason to keep this in a single path is anti-fingerprinting: #919 explicitly calls out that divergent relay-selection behavior across wallets is itself a client fingerprint. A separate select_as_aware + feature-gated API would institutionalize an "asmap vs non-asmap" split, the exact fingerprint this is meant to reduce. (#919 even contemplates shipping it enabled by default.)

I think the cleaner split is: feature-gate the asmap data/dependency (that's the real weight, and why #1514's PoC gates it), but keep a single select() that transparently does AS-aware ordering when asmap data is present and falls back to uniform-random otherwise, so call sites converge and never fork. SelectContext is the vehicle for the per-request inputs the #919 algorithm needs (POST/POLL, time window, receiver key), which is why it's there now even though empty.

makes sense?

IMO the main reason #1514 is taking a gated approach is because the ASMap library being used is https://github.com/0xB10C/asmap, and this implementation is in Rust.
Some implementations use other languages such as Dart and from what I can see there is currently no ASMap library available in Dart. Maybe one way to accommodate this would be to create an FFI layer for the existing Rust ASMap project so other implementations can use it (but other options maybe available too which I might not know ).

If that is done making ASMap-aware relay selection the default would make more sense since the main requirements would simply be access to the library and the ASMap data from https://github.com/bitcoin-core/asmap-data.

[bc1cindy]

IMO the main reason #1514 is taking a gated approach is because the ASMap library being used is https://github.com/0xB10C/asmap, and this implementation is in Rust. Some implementations use other languages such as Dart and from what I can see there is currently no ASMap library available in Dart. One way to accommodate this would be to create an FFI layer for the existing Rust ASMap project so other implementations can use it.

If that is done making ASMap-aware relay selection the default would make more sense since the main requirements would simply be access to the library and the ASMap data from https://github.com/bitcoin-core/asmap-data.

agreed, so it's a dependency-availability gate, not an opt-in one. Keeps the case for a single select() (gate the asmap data/dep)

FFI for 0xB10C/asmap + making it default is follow-up, this PR just keeps the primitive in the right shape for it.

[xstoicunicornx]

I understand the argument, but I need to understand more about how SelectContext is intended to be used. So let me ask some questions.

Are callers expected to only use methods on SelectContext to create the different context instances?
Is SelectContext::random() intended to retain the same function signature?
Would the asmap context need a new SelectContext::as_random(params...) method?

[bc1cindy]

I understand the argument, but I need to understand more about how SelectContext is intended to be used. So let me ask some questions.

Are callers expected to only use methods on SelectContext to create the different context instances? Is SelectContext::random() intended to retain the same function signature? Would the asmap context need a new SelectContext::as_random(params...) method?

yes to all three, that's the intended contract:

1. construction only via methods. SelectContext is #[non_exhaustive] (and gains private fields when asmap lands), so callers can't struct-literal it across the crate boundary, they go through constructors (random() today). That's the mechanism that lets it gain fields without a breaking change.

2. random() keeps its signature. It's the unconstrained → uniform-random context and the fallback; AS-aware selection doesn't touch it. Callers who never use asmap keep passing SelectContext::random() unchanged.

3. asmap = a new constructor, not a new select. The AS-aware context is a dedicated constructor (name TBD, e.g. SelectContext::as_aware(request_kind, time_window, receiver_key), the per-request inputs asmap should be used for relay selection #919 needs), filling the private fields. select() reads them and does AS-aware ordering when present, uniform otherwise:

// today
selector.select(&SelectContext::random(), rng)

// later
selector.select(&SelectContext::as_aware(...), rng)

same select(), one path, asmap slots in as a constructor + select() reading new fields, with no select sig change.

[xstoicunicornx]

So what is the advantage of this approach, where SelectContext carries the different methods for each select context, vs one where RelaySelector carries the different methods for each select context? While it does eliminate the need to touch the RelaySelector::select(...) call site specifically, it feels like its actually just moving that complexity to the SelectContext method call site rather than eliminating it?

[bc1cindy]

So what is the advantage of this approach, where SelectContext carries the different methods for each select context, vs one where RelaySelector carries the different methods for each select context? While it does eliminate the need to touch the RelaySelector::select(...) call site specifically, it feels like its actually just moving that complexity to the SelectContext method call site rather than eliminating it?

the advantage isn't eliminating a call site, it's that the method-per-context approach (select_as_aware on RelaySelector) converges to SelectContext. adding select_as_aware(kind, window, key, …) once is non-breaking, but each further AS input breaks its signature; the non-breaking fix is to pass a struct, and that struct is SelectContext. SelectContext just commits to that shape up front, and keeps one select (one failover path) instead of two entry points.

that said, AS-aware can be added later as a second method without breaking, and the context is empty while it's still WIP, so no need to reserve it now. I'll drop it and ship select(rng) for the #1586 convergence and introduce SelectContext later, with the concrete #1514 inputs.

thank you!