Add directory fallback behavior#1695
Conversation
This adds the ability for a backup directory to be chosen by the receiver incase of a failure on the ohttp key fetch. As well now that there is Relay and directory management the Struct is better suited as MailroomManager. Co-authored-by: Oladapo Oyindamola <111582215+0xZaddyy@users.noreply.github.com>
Coverage Report for CI Build 28387717154Coverage decreased (-0.002%) to 85.515%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
utACK f5906fe
Thank you @0xZaddyy and @benalleng. This one seems a little bit debt-y to me, but that's fine to ship payjoin-cli-1.0 by me if we're explicit about it & are looking to follow up with something like #1696
| let (directory, ohttp_keys) = loop { | ||
| let directory = self.mailroom_manager.choose_directory()?; | ||
| match self | ||
| .mailroom_manager | ||
| .unwrap_ohttp_keys_or_else_fetch_from_directory(&directory) | ||
| .await | ||
| { | ||
| Ok(keys) => break (directory, keys.ohttp_keys), | ||
| Err(e) => { | ||
| tracing::debug!("Directory {directory} failed: {e:#}"); | ||
| self.mailroom_manager.add_failed_directory(directory); | ||
| self.mailroom_manager.clear_failed_relays(); | ||
| continue; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Really seems like a function deserving its own documentation. A good time to refactor this ~tech debt out would be #1035.
I had to check, but it looks like this will fail if all directories do when choose_directory()? throws, I think. Originally I was concerned this might loop forever, but it looks like it will close.
| pub fn choose_directory(&self) -> Result<Url> { | ||
| use payjoin::bitcoin::secp256k1::rand::prelude::SliceRandom; | ||
| let directories = &self.config.v2()?.pj_directories; | ||
| let failed_directories = | ||
| self.failed_directories.lock().expect("Lock should not be poisoned"); | ||
| let remaining_directories: Vec<_> = | ||
| directories.iter().filter(|d| !failed_directories.contains(d)).cloned().collect(); | ||
|
|
||
| if remaining_directories.is_empty() { | ||
| return Err(anyhow!("No valid directories available")); | ||
| } | ||
|
|
||
| remaining_directories | ||
| .choose(&mut payjoin::bitcoin::key::rand::thread_rng()) | ||
| .cloned() | ||
| .ok_or_else(|| anyhow!("Failed to select from remaining directories")) | ||
| } |
There was a problem hiding this comment.
NBD, I duplicate first as well, just noticing: could be DRY'd up with the choose_relay function
|
@bc1cindy @benalleng |
|
So this is just the addition for the directory fallback at the session start for the receiver. Once a directory is chosen for a session it cannot be changed as the counterparty would no longer be abel to find your messages. Per request randomization was implemented in #1628 which forces a new ohhtp relay to be used per request only as a proxy for messages to the shared directory. Is that what you are asking? |
Yes, that clarifies it, thanks @benalleng I was mainly checking whether #1695 was intentionally changing the privacy guidance described in #1547, but with your explanation it sounds like these are separate concerns:
That makes sense to me. |
4 participants
This adds the ability for a backup directory to be chosen by the receiver incase of a failure on the ohttp key fetch.
As well now that there is Relay and directory management the Struct is better suited as MailroomManager.
Supersedes #1294
relates to the directory fallbacks in #1007
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.