Payjoin-cli should cache ohttp-keys for re-use by zealsham · Pull Request #1035 · payjoin/rust-payjoin

zealsham · 2025-09-03T00:10:05Z

This pr adds the functionality of ohttp-keys catching to payjoin-cli , ohttp-keys should not be fetched each time and a cached key should be use. Keys expire in 6 months .

To do

it shoudnt still use cached key when the relay-url isnt even present in config or as command-line flag
should handle a case of non-expired key but key being rejected by relay (will be handled in different PR)
get rid of unwrap calls that can panic (unhandled)
should not use the cached keys if --ohttp-keys flag is present cli command
it should be possible to cache multiple ohttp-keys based on relay-url (maybe in subsequent pr ?)

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 · 2025-09-03T00:13:31Z

Pull Request Test Coverage Report for Build 23672911589

Details

47 of 50 (94.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 84.165%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/ohttp.rs	47	50	94.0%

Totals
Change from base Build 23607944477:	0.04%
Covered Lines:	10694
Relevant Lines:	12706

💛 - Coveralls

zealsham · 2025-09-26T13:01:35Z

payjoin-cli/src/app/v2/ohttp.rs

    relay_manager: Arc<Mutex<RelayManager>>,
 ) -> Result<ValidatedOhttpKeys> {
+    println!("before first some");
    if let Some(ohttp_keys) = config.v2()?.ohttp_keys.clone() {


oops! , println statements i used for debugging got commited accidentally , cleaning it up

arminsabouri · 2025-11-24T16:35:33Z

@zealsham I am going to mark this as draft. Do you mind cleaning up the commit messages and history and re-opening. Thank you.

zealsham · 2025-11-25T18:40:53Z

@arminsabouri the PR is ready

arminsabouri · 2025-12-01T12:55:22Z

payjoin-cli/src/app/v2/ohttp.rs


+        // try cache for this selected relay first
+        if let Some(cached) = read_cached_ohttp_keys(&selected_relay) {
+            println!("using Cached keys  for relay: {selected_relay}");


Can we use the logger instead of println?

arminsabouri · 2025-12-01T12:55:30Z

payjoin-cli/src/app/v2/ohttp.rs


+        // try cache for this selected relay first
+        if let Some(cached) = read_cached_ohttp_keys(&selected_relay) {
+            println!("using Cached keys  for relay: {selected_relay}");


Suggested change

println!("using Cached keys for relay: {selected_relay}");

println!("using Cached keys for relay: {selected_relay}");

arminsabouri · 2025-12-01T12:57:11Z

payjoin-cli/src/app/v2/ohttp.rs

+        // try cache for this selected relay first
+        if let Some(cached) = read_cached_ohttp_keys(&selected_relay) {
+            println!("using Cached keys  for relay: {selected_relay}");
+            if !is_expired(&cached) && cached.relay_url == selected_relay {


Why would read_cached_ohttp_keys return an expired or keys for a different relay? Perphaps read_cached_ohttp_keys should return ValidateOhttpKeys?

Thier is a possiblity of the cached keys being expired by the time we try to use. so it checks that the keys being rend hasn't elapsed that duration before using it

if the relay_url were the key the comparison would not be necessary, but instead it's just the host part of this key. i think that only causes collisions when different ports are used, see other comment

arminsabouri · 2025-12-01T13:16:42Z

payjoin-cli/src/app/v2/ohttp.rs

+    fetched_at: u64,
+}
+
+fn get_cache_file(relay_url: &url::Url) -> PathBuf {


Why are cached keys not being persisted in the database?

They are persisted to file system and then get replaced with new keys if the cached one expires. i think it's pretty more straight forward than adding it to the db. on the file system keys are stored per-relay, each relay can have one key cache at a time

we already use sqlite so why not have a table with relay URL as the key and pubkey as the value? no need for serde, key mangling, or an additional bit of mutable storage to remember exists

Another note: keys correspond to directories, not relays. Might just be a mistype but it's a concrete distinction I felt necessary to repeat for certainty.

Another note: keys correspond to directories, not relays. Might just be a mistype but it's a concrete distinction I felt necessary to repeat for certainty.

this is very important

This pr adds the functionality of ohttp-keys catching to payjoin-cli , ohttp-keys should not be fetched each time and a cached key should be use. Keys expire in 6 months .

nothingmuch

i think the cache TTL being hard coded to 6 months is a bit problematic. if a directory's keys are lost, this may render its URL useless for that period of time, giving operators no recourse.

instead we should probably use standard HTTP cache control headers, which would allow an operator with high confidence in their deployment to set more aggressive caching parameters.

rfc 9458 says (sections 5.2, 6.4) that a server must generate a response with a 422 status in response to an invalid message, which is what the server sees if it can't decapsulate because the client is using a stale pubkey.

i'm not sure what the implementation and/or testing for that is right now, but we should handle that by fetching the ohttp key and comparing it, erroring if there is an inconsistency but allowing opting in to just overwrite the old key with the new key (i.e. making such errors into warnings)

nothingmuch · 2026-03-28T00:45:48Z

payjoin-cli/src/app/v2/ohttp.rs

+// 6 months
+const CACHE_DURATION: Duration = Duration::from_secs(6 * 30 * 24 * 60 * 60);


this is a pretty long duration to hard code. it seems more appropriate to allow OHTTP gateways to control this by using the cache control mechanisms defined in HTTP, so that operators can determine their own policies?

nothingmuch · 2026-03-28T00:53:23Z

payjoin-cli/src/app/v2/ohttp.rs

+    dirs::cache_dir()
+        .unwrap()
+        .join("payjoin-cli")
+        .join(relay_url.host_str().unwrap())


why not store relay_url in full so two gateways on the same host don't collide?

the only collision that can happen in practice given that we set things up to be on the top level of a single vhost, is the same host with different ports

nothingmuch · 2026-03-28T00:56:46Z

payjoin-cli/src/app/v2/ohttp.rs

-                return Ok(ValidatedOhttpKeys { ohttp_keys: keys, relay_url: selected_relay }),
+            Ok(keys) => {
+                // Cache the keys if they are not already cached for this relay
+                if read_cached_ohttp_keys(&selected_relay).is_none() {


this reads from the cache a second time unnecessarily

nothingmuch · 2026-03-28T00:58:18Z

payjoin-cli/src/app/v2/ohttp.rs

+        // try cache for this selected relay first
+        if let Some(cached) = read_cached_ohttp_keys(&selected_relay) {
+            println!("using Cached keys  for relay: {selected_relay}");
+            if !is_expired(&cached) && cached.relay_url == selected_relay {


if the relay_url were the key the comparison would not be necessary, but instead it's just the host part of this key. i think that only causes collisions when different ports are used, see other comment

nothingmuch · 2026-03-28T00:59:15Z

payjoin-cli/src/app/v2/ohttp.rs

+        if let Some(cached) = read_cached_ohttp_keys(&selected_relay) {
+            tracing::info!("using Cached keys for relay: {selected_relay}");
+            if !is_expired(&cached) && cached.relay_url == selected_relay {
+                return Ok(ValidatedOhttpKeys {


returning short circuits here which removes the need to read from the cache a second time for writeback

nothingmuch · 2026-03-28T01:16:46Z

see also the key consistency reference of rfc 9458 which discusses strategies for shared caching, which i think we can include in the mailroom services... that might be sufficient motivation to not use a reqwest middleware for caching.

nothingmuch · 2026-03-29T15:26:46Z

as Dan pointed out, this should be keyed the directory url, not relay url, everywhere i wrote relay_url i was confused

zealsham marked this pull request as draft September 3, 2025 00:10

DanGould added payjoin-cli enhancement New feature or request labels Sep 3, 2025

zealsham force-pushed the cache-ohttp-key branch from 27c936f to 8512a28 Compare September 8, 2025 23:42

zealsham changed the title ~~Payjoin-cli should cache ohttp-keys for re-use~~ WIP: Payjoin-cli should cache ohttp-keys for re-use Sep 14, 2025

zealsham force-pushed the cache-ohttp-key branch from 0b94117 to 3744b03 Compare September 24, 2025 13:41

zealsham changed the title ~~WIP: Payjoin-cli should cache ohttp-keys for re-use~~ Payjoin-cli should cache ohttp-keys for re-use Sep 24, 2025

zealsham marked this pull request as ready for review September 24, 2025 13:47

zealsham commented Sep 26, 2025

View reviewed changes

zealsham force-pushed the cache-ohttp-key branch from 3744b03 to 0fe2229 Compare September 26, 2025 13:06

arminsabouri marked this pull request as draft November 24, 2025 16:35

zealsham force-pushed the cache-ohttp-key branch 2 times, most recently from 971c8d3 to 2198e3f Compare November 25, 2025 17:31

zealsham marked this pull request as ready for review November 25, 2025 18:40

arminsabouri reviewed Dec 1, 2025

View reviewed changes

spacebear21 mentioned this pull request Mar 7, 2026

Ensure random relay selection with fixed ohttp-keys config #1390

Merged

2 tasks

bc1cindy mentioned this pull request Mar 11, 2026

Should ohttp-relay selection be done on a per session basis in the cli #1385

Open

zealsham added 3 commits March 27, 2026 13:35

Payjoin-cli should cache ohttp-keys for re-use

575c6d1

This pr adds the functionality of ohttp-keys catching to payjoin-cli , ohttp-keys should not be fetched each time and a cached key should be use. Keys expire in 6 months .

fix some stuffs

ff612fd

fix cache failing

2677446

zealsham force-pushed the cache-ohttp-key branch from 2198e3f to 2677446 Compare March 28, 2026 00:14

zealsham requested a review from arminsabouri March 28, 2026 00:27

nothingmuch suggested changes Mar 28, 2026

View reviewed changes

	println!("using Cached keys for relay: {selected_relay}");
	println!("using Cached keys for relay: {selected_relay}");

		// 6 months
		const CACHE_DURATION: Duration = Duration::from_secs(6 * 30 * 24 * 60 * 60);

Conversation

zealsham commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To do

Pull Request Checklist

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 23672911589

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Nov 24, 2025

Uh oh!

zealsham commented Nov 25, 2025

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

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!

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!

nothingmuch commented Mar 28, 2026

Uh oh!

nothingmuch commented Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zealsham commented Sep 3, 2025 •

edited

Loading

coveralls commented Sep 3, 2025 •

edited

Loading