-
Notifications
You must be signed in to change notification settings - Fork 107
[WIP] Add AS-aware relay selection #1514
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
32491b4
07354b8
5910eb6
76a480e
76ba401
acf609f
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,4 +1,10 @@ | ||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| use std::fmt; | ||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| use std::net::IpAddr; | ||
| use std::path::PathBuf; | ||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| use std::sync::Arc; | ||
|
|
||
| use anyhow::Result; | ||
| use config::builder::DefaultState; | ||
|
|
@@ -29,24 +35,102 @@ pub struct V1Config { | |
| pub pj_endpoint: Url, | ||
| } | ||
|
|
||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| #[derive(Clone)] | ||
| pub struct LoadedAsmap { | ||
| map: Arc<::asmap::Asmap>, | ||
| } | ||
|
|
||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| impl LoadedAsmap { | ||
| pub fn lookup(&self, ip: IpAddr) -> u32 { self.map.lookup(ip) } | ||
|
|
||
| pub fn as_bytes(&self) -> &[u8] { self.map.as_bytes() } | ||
| } | ||
|
|
||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| impl<'de> Deserialize<'de> for LoadedAsmap { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let path = PathBuf::deserialize(deserializer)?; | ||
| let map = ::asmap::Asmap::from_file(&path).map_err(|e| { | ||
| serde::de::Error::custom(format!( | ||
| "Failed to load v2.asmap.asmap_file {}: {e}", | ||
| path.display() | ||
| )) | ||
| })?; | ||
| Ok(LoadedAsmap { map: Arc::new(map) }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| impl fmt::Debug for LoadedAsmap { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("LoadedAsmap").field("bytes", &self.as_bytes().len()).finish() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| #[derive(Debug, Clone, Deserialize)] | ||
| pub struct AsmapConfig { | ||
| #[serde(rename = "asmap_file")] | ||
| pub asmap: LoadedAsmap, | ||
|
Comment on lines
+77
to
+79
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. a configured, per-integration asmap file is a client fingerprint two ways: (1) only some integrations load one, so the AS-aware ones stand out (uses-it-or-not), and (2) selection is deterministic from the asmap, so even those that do load one diverge unless they're on the exact same snapshot. could the lib ship one bundled snapshot per version instead, so every integration on a given version has the same file with nothing to fetch? it's ~1.5 MB, and asmap-data updates ~monthly (irregular, gaps up to ~2 months), but relay ASNs are stable so across versions keeping everyone consistent https://github.com/bitcoin-core/asmap-data wdyt?
Contributor
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. No answer for this yet , this might require more discussion will get back to you |
||
| #[serde(default)] | ||
| pub user_public_ips: Vec<IpAddr>, | ||
| #[serde(default)] | ||
| pub user_asns: Vec<u32>, | ||
|
Comment on lines
+80
to
+83
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.
requiring the user's own ASN/IP makes AS-aware mode operator-only, a NAT'd wallet can't supply it without an external lookup, which is a privacy leak of its own #919 lists four AS-overlap risks:
of the three that selection can address, two don't need your own AS:
both use IPs you resolve via DNS anyway to connect, like Bitcoin Core's asmap, which diversifies its peers by AS (anti-eclipse) and never learns its own ASN; you bucket the servers, not yourself.
so, if we drop this and keep just:
that way both use only server ASes, zero input, so AS-aware selection can be on by default for everyone, which removes the uses-it-or-not fingerprint, default and zero-input, instead of operator-only. makes sense? |
||
| } | ||
|
|
||
| #[cfg(all(feature = "v2", feature = "asmap"))] | ||
| impl AsmapConfig { | ||
| fn validate(&self) -> Result<(), ConfigError> { | ||
| if self.user_public_ips.is_empty() && self.user_asns.is_empty() { | ||
| return Err(ConfigError::Message( | ||
| "v2.asmap requires at least one of user_public_ips or user_asns".into(), | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "v2")] | ||
| #[derive(Debug, Clone, Deserialize)] | ||
| pub struct V2Config { | ||
| #[serde(deserialize_with = "deserialize_ohttp_keys_from_path")] | ||
| pub ohttp_keys: Option<payjoin::OhttpKeys>, | ||
| pub ohttp_relays: Vec<Url>, | ||
| pub pj_directories: Vec<Url>, | ||
| #[cfg(feature = "asmap")] | ||
| #[serde(default)] | ||
| pub asmap: Option<AsmapConfig>, | ||
| } | ||
|
|
||
| #[cfg(feature = "v2")] | ||
| impl V2Config { | ||
| fn validate(&self) -> Result<(), ConfigError> { | ||
| if self.pj_directories.is_empty() { | ||
| return Err(ConfigError::Message( | ||
| "At least one v2 trusted directory is required".to_owned(), | ||
| )); | ||
| } | ||
|
|
||
| #[cfg(feature = "asmap")] | ||
| if let Some(asmap) = &self.asmap { | ||
| asmap.validate()?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[allow(clippy::large_enum_variant)] | ||
| #[derive(Debug, Clone, Deserialize)] | ||
| #[serde(tag = "version")] | ||
| #[derive(Debug, Clone)] | ||
| pub enum VersionConfig { | ||
| #[cfg(feature = "v1")] | ||
| #[serde(rename = "v1")] | ||
| V1(V1Config), | ||
| #[cfg(feature = "v2")] | ||
| #[serde(rename = "v2")] | ||
| V2(V2Config), | ||
| } | ||
|
|
||
|
|
@@ -206,7 +290,7 @@ impl Config { | |
| Version::Two => { | ||
| #[cfg(feature = "v2")] | ||
| { | ||
| match built_config.get::<V2Config>("v2") { | ||
| match load_v2_config(&built_config) { | ||
| Ok(v2) => { | ||
| if v2.ohttp_relays.len() < 2 { | ||
| tracing::warn!( | ||
|
|
@@ -402,25 +486,45 @@ fn handle_subcommands(config: Builder, cli: &Cli) -> Result<Builder, ConfigError | |
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "v2")] | ||
| fn load_v2_config(built_config: &config::Config) -> Result<V2Config, ConfigError> { | ||
| #[cfg(not(feature = "asmap"))] | ||
| if built_config.get_table("v2.asmap").is_ok() { | ||
| return Err(ConfigError::Message( | ||
| "This build does not include ASMap support. Recompile with --features asmap".to_owned(), | ||
| )); | ||
| } | ||
|
|
||
| let v2 = built_config.get::<V2Config>("v2")?; | ||
| v2.validate()?; | ||
| Ok(v2) | ||
| } | ||
|
|
||
| #[cfg(feature = "v2")] | ||
| fn deserialize_ohttp_keys_from_path<'de, D>( | ||
| deserializer: D, | ||
| ) -> Result<Option<payjoin::OhttpKeys>, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let path_str: Option<String> = Option::deserialize(deserializer)?; | ||
|
|
||
| match path_str { | ||
| let path: Option<PathBuf> = Option::deserialize(deserializer)?; | ||
| match path { | ||
| None => Ok(None), | ||
| Some(path) => std::fs::read(path) | ||
| .map_err(|e| serde::de::Error::custom(format!("Failed to read ohttp_keys file: {e}"))) | ||
| .and_then(|bytes| { | ||
| payjoin::OhttpKeys::decode(&bytes).map_err(|e| { | ||
| serde::de::Error::custom(format!("Failed to decode ohttp keys: {e}")) | ||
| }) | ||
| }) | ||
| .map(Some), | ||
| Some(path) => { | ||
| let bytes = std::fs::read(&path).map_err(|e| { | ||
| serde::de::Error::custom(format!( | ||
| "Failed to read ohttp_keys file {}: {e}", | ||
| path.display() | ||
| )) | ||
| })?; | ||
| let keys = payjoin::OhttpKeys::decode(&bytes).map_err(|e| { | ||
| serde::de::Error::custom(format!( | ||
| "Failed to decode ohttp keys from {}: {e}", | ||
| path.display() | ||
| )) | ||
| })?; | ||
| Ok(Some(keys)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
would be good to know what the team think about adding this dep to parse the binary, especially if we are moving to the payjoin crate
is it a good idea or would it be better to have our own parser? #1452 (comment)
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.
I did check out the library also saw a contibutor to asmap bitcoin core library give it a star but more eyes on the library would be nice