-
Notifications
You must be signed in to change notification settings - Fork 93
Payjoin-cli should cache ohttp-keys for re-use #1035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,16 @@ | ||
| use std::fs; | ||
| use std::path::PathBuf; | ||
| use std::sync::{Arc, Mutex}; | ||
| use std::time::{Duration, SystemTime}; | ||
|
|
||
| use anyhow::{anyhow, Result}; | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use super::Config; | ||
|
|
||
| // 6 months | ||
| const CACHE_DURATION: Duration = Duration::from_secs(6 * 30 * 24 * 60 * 60); | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct RelayManager { | ||
| selected_relay: Option<url::Url>, | ||
|
|
@@ -39,7 +46,6 @@ pub(crate) async fn unwrap_ohttp_keys_or_else_fetch( | |
| } else { | ||
| println!("Bootstrapping private network transport over Oblivious HTTP"); | ||
| let fetched_keys = fetch_ohttp_keys(config, directory, relay_manager).await?; | ||
|
|
||
| Ok(fetched_keys) | ||
| } | ||
| } | ||
|
|
@@ -75,6 +81,17 @@ async fn fetch_ohttp_keys( | |
| .expect("Lock should not be poisoned") | ||
| .set_selected_relay(selected_relay.clone()); | ||
|
|
||
| // try cache for this selected relay first | ||
| 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 { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| return Ok(ValidatedOhttpKeys { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. returning short circuits here which removes the need to read from the cache a second time for writeback |
||
| ohttp_keys: cached.keys, | ||
| relay_url: cached.relay_url, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| let ohttp_keys = { | ||
| #[cfg(feature = "_manual-tls")] | ||
| { | ||
|
|
@@ -99,8 +116,17 @@ async fn fetch_ohttp_keys( | |
| }; | ||
|
|
||
| match ohttp_keys { | ||
| Ok(keys) => | ||
| 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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this reads from the cache a second time unnecessarily |
||
| if let Err(e) = cache_ohttp_keys(&keys, &selected_relay) { | ||
| tracing::debug!( | ||
| "Failed to cache OHTTP keys for relay {selected_relay}: {e:?}" | ||
| ); | ||
| } | ||
| } | ||
| return Ok(ValidatedOhttpKeys { ohttp_keys: keys, relay_url: selected_relay }); | ||
| } | ||
| Err(payjoin::io::Error::UnexpectedStatusCode(e)) => { | ||
| return Err(payjoin::io::Error::UnexpectedStatusCode(e).into()); | ||
| } | ||
|
|
@@ -114,3 +140,49 @@ async fn fetch_ohttp_keys( | |
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Serialize, Deserialize, Debug)] | ||
| struct CachedOhttpKeys { | ||
| keys: payjoin::OhttpKeys, | ||
| relay_url: url::Url, | ||
| fetched_at: u64, | ||
| } | ||
|
|
||
| fn get_cache_file(relay_url: &url::Url) -> PathBuf { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are cached keys not being persisted in the database?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
this is very important |
||
| dirs::cache_dir() | ||
| .unwrap() | ||
| .join("payjoin-cli") | ||
| .join(relay_url.host_str().unwrap()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| .join("ohttp-keys.json") | ||
| } | ||
|
|
||
| fn read_cached_ohttp_keys(relay_url: &url::Url) -> Option<CachedOhttpKeys> { | ||
| let cache_file = get_cache_file(relay_url); | ||
| if !cache_file.exists() { | ||
| return None; | ||
| } | ||
| let data = fs::read_to_string(cache_file).ok().unwrap(); | ||
| serde_json::from_str(&data).ok() | ||
| } | ||
|
|
||
| fn cache_ohttp_keys(ohttp_keys: &payjoin::OhttpKeys, relay_url: &url::Url) -> Result<()> { | ||
| let cached = CachedOhttpKeys { | ||
| keys: ohttp_keys.clone(), | ||
| relay_url: relay_url.clone(), | ||
| fetched_at: SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap().as_secs(), | ||
| }; | ||
|
|
||
| let serialized = serde_json::to_string(&cached)?; | ||
| let path = get_cache_file(relay_url); | ||
| fs::create_dir_all(path.parent().unwrap())?; | ||
| fs::write(path, serialized)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn is_expired(cached_keys: &CachedOhttpKeys) -> bool { | ||
| let now = SystemTime::now() | ||
| .duration_since(SystemTime::UNIX_EPOCH) | ||
| .unwrap_or(Duration::ZERO) | ||
| .as_secs(); | ||
| now.saturating_sub(cached_keys.fetched_at) > CACHE_DURATION.as_secs() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?