Create fallback ohttp-relay logic for payjoin-cli by benalleng · Pull Request #607 · payjoin/rust-payjoin

benalleng · 2025-03-27T21:01:57Z

The reliance on ohttp-relays can cause some distruption if they ever go offline but we did not have any examples of a fallback mechanism for choosing the relays at random and falling back to other relays in case they fail.

This adds a new RelayState to manage which relays have already been tried and ulitmately hangs on to the first successful relay that it reaches per session.

Closes #528

coveralls · 2025-03-27T21:05:43Z

Pull Request Test Coverage Report for Build 15071763691

Details

48 of 83 (57.83%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 82.135%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2.rs	33	47	70.21%
payjoin/src/io.rs	6	27	22.22%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/io.rs	1	58.26%

Totals
Change from base Build 15031183812:	-0.1%
Covered Lines:	5508
Relevant Lines:	6706

💛 - Coveralls

DanGould · 2025-03-27T22:29:26Z

Nice lookin' draft. There are some code repeats that can probably be squashed but you probably know that and are just sharing early. I suppose this fixes #528? A mention in the OP will close the issue when this merges, if so. Thanks Ben

payjoin-cli/src/app/v2.rs

DanGould

I gave this a once over as a way to stop blocking and make progress but bear in mind I didn't load the code and give it the most thorough review. Some questions:

is the http dependency new or is it already in cargo tree? If so, which version? May you please specify exactly instead of just "1"
same for rand. Do we need rand explicitly or is payjoin::bitcoin::rand available in all configurations?
How necessary are the dual code paths for _danger-local-https? To what extent may they be reduced?
Will the relay state mutex unlock ever be contentious (lots of clients during resume?) If so, we should use tokio::sync::Mutex to await instead of blocking the async fn. If not, the synchronous one is fine.

I did check through commit text for rationale but missed answers to these questions

Hope this allows us to continue the conversation even though more thorough review may be required

This is important work for reliability of payjoin clients in the wild, so we need to make sure this is shaping up to be an excellent reference once merged.

DanGould · 2025-04-10T03:33:23Z

payjoin-cli/src/app/v2.rs

+        let selected_relay = self.relay_state.lock().unwrap().get_selected_relay();
+        let ohttp_relay = match selected_relay {
+            Some(relay) => relay,
+            None => validate_relay(&self.config, self.relay_state.clone()).await?,
+        };
+


This has got to be a non-duplicated helper

DanGould · 2025-04-10T03:35:20Z

payjoin-cli/src/app/v2.rs

+    }
+}
+
+//Local relays are incapable of acting as proxies so we must opportunistically fetch keys from the config


we could also GET directly from the payjoin-directory 🤔 not sure if that's any worse or less cumbersome than ohttp_keys configuration

My only qualm about doing a direct GET request is that the local test that uses this then must depend on a good connection to the directory. This path currently only exists as our tests use dummy relays and pre-configured ohttp-keys so no changes are needed for those tests with these dual path functions

This comment should be /// not //

benalleng · 2025-04-10T17:40:00Z

Thanks for the feedback, always helpful to get some nudges in the right direction

is the http dependency new or is it already in cargo tree? If so, which version? May you please specify exactly instead of just "1"

It looks like the payjoin tree uses 1.1.0 though we also need to specify the version there as well.

same for rand. Do we need rand explicitly or is payjoin::bitcoin::rand available in all configurations?

rand can definitely be just from within payjoin, we can remove the independent dep there.

How necessary are the dual code paths for _danger-local-https? To what extent may they be reduced?

These were holdouts from v1 and I think ultimately they can be somewhat reduced though there will be some separation assuming we desire to keep the tests local and not having to actually reach out to a server.
Ultimately I think that the _danger-local-https flag here is useful for our tests as we can use local relays and directories without needing a good connection to an actual directory.

Will the relay state mutex unlock ever be contentious (lots of clients during resume?) If so, we should use tokio::sync::Mutex to await instead of blocking the async fn. If not, the synchronous one is fine.

~~I'll need to take a closer look at this.~~ See below

benalleng · 2025-05-13T20:51:43Z

Will the relay state mutex unlock ever be contentious (lots of clients during resume?) If so, we should use tokio::sync::Mutex to await instead of blocking the async fn. If not, the synchronous one is fine.

I do not think we need to use async Mutex as all of our mutex unlocking and usage is done within small blocks and our mutexes are dropped before any sort of await call is made.

benalleng · 2025-05-13T21:11:14Z

payjoin-cli/src/app/v2.rs

+                let status_code = error
+                    .to_string()
+                    .split_whitespace()
+                    .find_map(|code| http::StatusCode::from_str(code).ok())
+                    .expect("Could not find status code");


I am doing some string splits here instead of changing the payjoin io Error::UnexpectedStatusCode It still feels a bit brittle, curious about other ideas you might have beyond making InternalError public.

Making the error public is the way to go, perhaps with just an/some private variants

Will revert this logic back to 6685f85

DanGould · 2025-05-14T18:35:33Z

I think I spoke imprecisely. The Internal Error should remain private. If it's time to make certain variants that are public, but keep an Internal Variant that's private, I think that would solve the problem.

DanGould

The error handling proposed after my last comment is more complicated than I had imagined. I don't think we need a pub enum PublicError type because we already have an Error type that is public, it just needs to be a non-exhaustive enum with an Internal variant.

Since we've gone back and forth a couple of times, I took the liberty to write up a sketch of what I'm talking about. If we're not stabilizing the error and would rather kick the error can down the road this seems like the incremental step forward:

Read the last 2 commits here: https://github.com/DanGould/rust-payjoin/tree/ohttp-fallback

and specifically check out this section

https://github.com/DanGould/rust-payjoin/blob/83d8f7888ff7e2a6522d0c67028711501424246f/payjoin/src/io.rs#L65-L84

DanGould · 2025-05-15T16:09:15Z

payjoin-cli/Cargo.toml

 hyper-util = { version = "0.1", optional = true }
 log = "0.4.7"
-payjoin = { version = "0.23.0", default-features = false }
+payjoin = { path="../payjoin", default-features = false }


This change doesn't need to be here. We're already patching payjoin from crates.io with our local path in the top level Cargo.toml

rust-payjoin/Cargo.toml

Lines 5 to 6 in e2b66a2

[patch.crates-io.payjoin]

path = "payjoin"

benalleng · 2025-05-15T16:25:10Z

what msrv is #[allow(private_interfaces)] included in?
https://github.com/DanGould/rust-payjoin/actions/runs/15049802630/job/42301513021#step:5:803

This isn't included until 1.72.0.

benalleng · 2025-05-15T17:18:08Z

Since using this #[allow(public_interfaces)] + #[doc(hidden)] approach won't work with our .msrv is it ok to make the InternalError variants public as well while keeping them doc hidden like I did in c97f23f? Or should we find another approach?
I guess we could do something along the lines of just making all the variants of InternalError doc hidden except our UnexpectedStatusCode error.

DanGould · 2025-05-15T17:19:02Z

One fix would be to make the current InternalError into InternalErrorInner and then make a pub struct InternalError(InternalErrorInner).

Otherwise, #[doc(hidden)] on both the Internal Variant AND pub enum InternalError could work.

DanGould

I like how you dealt with the error now.

However we've got some .unwrap()s on locks. Shouldn't those be .expect()s?

DanGould · 2025-05-15T18:29:39Z

payjoin-cli/src/main.rs

    {
        cmd = cmd.arg(
-            Arg::new("ohttp_relay")
+            Arg::new("ohttp_relays")


How does this work? Someone passes --ohttp_relays "https://a.com" --ohttp_relays "https:://b.com"? Wouldn't we still want the singular --ohttp_relay name? Or do you expect them to be passed --ohttp_relays "https:://b.com","https://b.com ?

https://docs.rs/clap-action-command/latest/clap_action_command/fn.get_many.html

How about we pass them as one or more value like --ohttp-relays "https://pj.benalleng.com,https://pj.bobspacebkk.com" or --ohttp-relays "https://pj.benalleng.com"?

DanGould · 2025-05-15T18:31:04Z

payjoin-cli/src/app/v2.rs

+        match ohttp_keys {
+            Ok(keys) => return Ok(Some(keys)),
+            Err(error) => match error {
+                payjoin::io::Error(InternalError::UnexpectedStatusCode(_)) => {
+                    log::debug!("{error:?}");
+                    return Err(error.into());
+                }
+                _ => {
+                    log::debug!("Failed to connect to relay: {selected_relay}, {error:?}");
+                    relay_state.lock().unwrap().add_failed_relay(selected_relay);
+                }
+            },
+        }
+    }
+}


The same flatten done to the other match should probably happen here too

So that they can be matched on in downstream logic. Keep Internal variants private.

The reliance on ohttp-relays can cause some distruption if they ever go offline but we did not have any examples of a fallback mechanism for choosing the relays at random and falling back to other relays in case they fail. This adds a new RelayState to manage which relays have already been tried and ulitmately hangs on to the first successful relay that it reaches.

DanGould

tACK 152393f

This is way more reliable. I think we need to think about what errors we exactly expose publicly (re: #403 but moreso to be a good reference example) and see if we abstract the RelayState away from the main v2.rs file. We can also very likely remove some of the duplicate flags for local-https in the follow up.

benalleng force-pushed the ohttp-fallback branch 2 times, most recently from faa5d95 to b3c5707 Compare March 27, 2025 21:58

benalleng force-pushed the ohttp-fallback branch 8 times, most recently from 36376eb to 1b80380 Compare March 29, 2025 00:43

benalleng mentioned this pull request Mar 30, 2025

Is there a reason for _danger-local-https in payjoin-cli ouside of the e2e tests? #611

Closed

benalleng force-pushed the ohttp-fallback branch 5 times, most recently from ec11bd6 to 6685f85 Compare March 31, 2025 17:52

benalleng marked this pull request as ready for review March 31, 2025 19:07

benalleng force-pushed the ohttp-fallback branch from 4d9104a to 8f032e6 Compare March 31, 2025 19:09

benalleng commented Mar 31, 2025

View reviewed changes

payjoin-cli/src/app/v2.rs Outdated Show resolved Hide resolved

benalleng force-pushed the ohttp-fallback branch 2 times, most recently from c424ad6 to 001379c Compare April 3, 2025 16:53

DanGould reviewed Apr 10, 2025

View reviewed changes

benalleng force-pushed the ohttp-fallback branch 4 times, most recently from 13b6273 to a9af1e3 Compare April 10, 2025 18:21

benalleng force-pushed the ohttp-fallback branch from a9af1e3 to 1f6a728 Compare May 13, 2025 19:25

benalleng force-pushed the ohttp-fallback branch 3 times, most recently from 8bb7896 to f13ddfa Compare May 13, 2025 20:41

benalleng force-pushed the ohttp-fallback branch from f13ddfa to ef0a12f Compare May 13, 2025 20:58

benalleng commented May 13, 2025

View reviewed changes

benalleng requested a review from DanGould May 13, 2025 21:13

benalleng force-pushed the ohttp-fallback branch 2 times, most recently from b3ea7d5 to 78ee4f2 Compare May 14, 2025 02:32

benalleng force-pushed the ohttp-fallback branch from 2d9f7d5 to 1cadccf Compare May 15, 2025 02:39

DanGould requested changes May 15, 2025

View reviewed changes

benalleng force-pushed the ohttp-fallback branch 2 times, most recently from 0fdc761 to c97f23f Compare May 15, 2025 17:12

benalleng force-pushed the ohttp-fallback branch 2 times, most recently from 3c4d612 to 83abee1 Compare May 15, 2025 17:48

DanGould reviewed May 15, 2025

View reviewed changes

benalleng force-pushed the ohttp-fallback branch 5 times, most recently from 421e017 to c57927c Compare May 15, 2025 19:44

DanGould and others added 2 commits May 16, 2025 11:16

Make an io::Error variant public

9349c7e

So that they can be matched on in downstream logic. Keep Internal variants private.

benalleng force-pushed the ohttp-fallback branch from c57927c to 152393f Compare May 16, 2025 15:16

DanGould approved these changes May 16, 2025

View reviewed changes

DanGould merged commit 5b7678a into payjoin:master May 19, 2025
7 checks passed

Conversation

benalleng commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15071763691

Details

💛 - Coveralls

Uh oh!

DanGould commented Mar 27, 2025

Uh oh!

Uh oh!

DanGould left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benalleng commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould commented May 14, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benalleng commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented May 15, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benalleng commented Mar 27, 2025 •

edited

Loading

coveralls commented Mar 27, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

benalleng Apr 10, 2025 •

edited

Loading

benalleng commented Apr 10, 2025 •

edited

Loading

benalleng commented May 13, 2025 •

edited

Loading

benalleng commented May 15, 2025 •

edited

Loading

benalleng commented May 15, 2025 •

edited

Loading

benalleng May 15, 2025 •

edited

Loading