Add choose_relay method in RelayManager

[benalleng]

Closes #1391

This could supersede #1399

Instead of having relay selection loose state and each session spawns a new relay manager this PR instead just removes the state from RelayManager istelf by using a choose_relay method which simply randomly selects a new relay per-request. Comes from this #1399 (review).

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.

[benalleng]

@va-an if this end up being the approach we go with I think merging in the fetch_ohttp_keys functions to be methods will be similarly trivial.

[coveralls]

Coverage Report for CI Build 27210341068

Coverage decreased (-0.2%) to 85.199%

Details

• Coverage decreased (-0.2%) from the base build.
• Patch coverage: 20 uncovered changes across 3 files (41 of 61 lines covered, 67.21%).
• 18 coverage regressions across 2 files.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v2/mod.rs	37	24	64.86%
payjoin-cli/src/app/v2/ohttp.rs	19	13	68.42%
payjoin-cli/src/app/config.rs	5	4	80.0%

Coverage Regressions

18 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
payjoin-cli/src/app/v2/ohttp.rs	17	34.55%
payjoin-cli/src/app/v2/mod.rs	1	50.83%

---

Coverage Stats

Relevant Lines:	14708
Covered Lines:	12531
Line Coverage:	85.2%
Coverage Strength:	371.21 hits per line

---

💛 - Coveralls

[xstoicunicornx]

utACK 4ff6a0c

Reviewed and tested the code. Strongly prefer this solution over #1399 . Couple items of feedback that I think are fine left as follow ups if preferred.

One other follow up is that we probably want to add handling for the failed relay scenario, but its not clear to me what sorts of errors should result in the relay being added to the failed relay list. This could be added via the post_request function with something similar to this (note this does not work as is... req.url needs to be converted to Url):

    async fn post_request(&self, req: payjoin::Request) -> Result<reqwest::Response> {
        let http = http_agent(&self.config)?;
        match http
            .post(req.url)
            .header("Content-Type", req.content_type)
            .body(req.body)
            .send()
            .await
            .and_then(|r| r.error_for_status())
            .context("HTTP request failed")
        {
            res @ Ok(_) => res,
            e @ Err(_) => {
                self.relay_manager
                    .lock()
                    .expect("Lock should not be poisoned")
                    .add_failed_relay(req.url);
                e
            }
        }

[benalleng]

Curious if you like the current callsite logic or if you would prefer a post_via_relay function that handles this.

async fn post_via_relay<F, T, E>(&self, mut build: F) -> Result<(reqwest::Response, T)>                                                                       
      where                                                                        
        F: FnMut(&str) -> std::result::Result<(payjoin::Request, T), E>,         
        E: Into<anyhow::Error>,                                                  
      {                                                                            
          loop {                                                                   
              let relay =                                                          
                  self.relay_manager.lock().expect("Lock should not be poisoned").choose_relay()?;                                                                  
              let (req, ctx) = build(relay.as_str()).map_err(Into::into)?;         
              match self.post_request(req).await {                                 
                Ok(resp) => return Ok((resp, ctx)),                              
                Err(e) => {                                                      
                    tracing::debug!("Request to relay {relay} failed: {e:?}");   
                    self.relay_manager                                           
                        .lock()                                                  
                        .expect("Lock should not be poisoned")                   
                        .add_failed_relay(relay);                                
                }                                                                
            }                                                                    
        }                                                                        
    }                                                                            
 }

[xstoicunicornx]

Curious if you like the current callsite logic or if you would prefer a post_via_relay function that handles this.

I like the post_via_relay function a lot. Was actually gonna suggest something similar except as a RelayManager method but it didn't seem right. This is better.

[xstoicunicornx]

utACK be38713

Reviewed the code and ran test scripts locally, looks good. This greatly simplifies the ohttp key fetching logic and relay selection logic, while disentangling them as separate concerns. It also enforces mandatory randomness in relay selection whenever relay manager is called to provide a relay. Also love to see net negative lines :).

Just one small nit below.

[xstoicunicornx]

Suggested change

	//! otherwise selects a relay at random from the configured list,
	//! otherwise fetches the keys from the config directory using a
	//! relay selected at random from the configured list,

Nit: this comment doesn't read clearly as is

[benalleng]

Going to put this in the followup with @va-an in moving this under the RelayManager