From ee10692b23fd60b8503f1d867ddd2f8a351708cb Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Thu, 11 Jul 2024 18:06:51 -0400 Subject: [PATCH] Fix handling of output distribution We prior didn't handle how the output distribution only starts after a specific block. --- .../monero/rpc/simple-request/tests/tests.rs | 4 +- coins/monero/rpc/src/lib.rs | 88 ++++++++++++------- coins/monero/wallet/src/decoys.rs | 5 +- 3 files changed, 64 insertions(+), 33 deletions(-) diff --git a/coins/monero/rpc/simple-request/tests/tests.rs b/coins/monero/rpc/simple-request/tests/tests.rs index a44e05f7..f0671e19 100644 --- a/coins/monero/rpc/simple-request/tests/tests.rs +++ b/coins/monero/rpc/simple-request/tests/tests.rs @@ -69,9 +69,9 @@ async fn test_decoy_rpc() { .unwrap(); // Test get_output_distribution - // It's documented to take two inclusive block numbers + // Our documentation for our Rust fn defines it as taking two block numbers { - let distribution_len = rpc.get_output_distribution_len().await.unwrap(); + let distribution_len = rpc.get_output_distribution_end_height().await.unwrap(); assert_eq!(distribution_len, rpc.get_height().await.unwrap()); rpc.get_output_distribution(0 ..= distribution_len).await.unwrap_err(); diff --git a/coins/monero/rpc/src/lib.rs b/coins/monero/rpc/src/lib.rs index b37bcace..ad2c3239 100644 --- a/coins/monero/rpc/src/lib.rs +++ b/coins/monero/rpc/src/lib.rs @@ -168,20 +168,20 @@ impl FeePriority { } } -#[derive(Deserialize, Debug)] +#[derive(Debug, Deserialize)] struct EmptyResponse {} -#[derive(Deserialize, Debug)] +#[derive(Debug, Deserialize)] struct JsonRpcResponse { result: T, } -#[derive(Deserialize, Debug)] +#[derive(Debug, Deserialize)] struct TransactionResponse { tx_hash: String, as_hex: String, pruned_as_hex: String, } -#[derive(Deserialize, Debug)] +#[derive(Debug, Deserialize)] struct TransactionsResponse { #[serde(default)] missed_tx: Vec, @@ -189,7 +189,7 @@ struct TransactionsResponse { } /// The response to an output query. -#[derive(Deserialize, Debug)] +#[derive(Debug, Deserialize)] pub struct OutputResponse { /// The height of the block this output was added to the chain in. pub height: usize, @@ -282,12 +282,12 @@ pub trait Rpc: Sync + Clone + Debug { /// /// This is specifically the major version within the most recent block header. async fn get_hardfork_version(&self) -> Result { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct HeaderResponse { major_version: u8, } - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct LastHeaderResponse { block_header: HeaderResponse, } @@ -306,7 +306,7 @@ pub trait Rpc: Sync + Clone + Debug { /// The height is defined as the amount of blocks on the blockchain. For a blockchain with only /// its genesis block, the height will be 1. async fn get_height(&self) -> Result { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct HeightResponse { height: usize, } @@ -398,11 +398,11 @@ pub trait Rpc: Sync + Clone + Debug { /// `number` is the block's zero-indexed position on the blockchain (`0` for the genesis block, /// `height - 1` for the latest block). async fn get_block_hash(&self, number: usize) -> Result<[u8; 32], RpcError> { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct BlockHeaderResponse { hash: String, } - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct BlockHeaderByHeightResponse { block_header: BlockHeaderResponse, } @@ -416,7 +416,7 @@ pub trait Rpc: Sync + Clone + Debug { /// /// The received block will be hashed in order to verify the correct block was returned. async fn get_block(&self, hash: [u8; 32]) -> Result { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct BlockResponse { blob: String, } @@ -437,7 +437,7 @@ pub trait Rpc: Sync + Clone + Debug { /// `number` is the block's zero-indexed position on the blockchain (`0` for the genesis block, /// `height - 1` for the latest block). async fn get_block_by_number(&self, number: usize) -> Result { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct BlockResponse { blob: String, } @@ -499,7 +499,7 @@ pub trait Rpc: Sync + Clone + Debug { /// This MUST NOT be expected to be deterministic in any way. // TODO: Take a sanity check argument async fn get_fee_rate(&self, priority: FeePriority) -> Result { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct FeeResponse { status: String, fees: Option>, @@ -555,7 +555,7 @@ pub trait Rpc: Sync + Clone + Debug { /// Publish a transaction. async fn publish_transaction(&self, tx: &Transaction) -> Result<(), RpcError> { #[allow(dead_code)] - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct SendRawResponse { status: String, double_spend: bool, @@ -798,22 +798,23 @@ pub trait Rpc: Sync + Clone + Debug { } } -/// A trait for any object which can be used to select decoys. +/// A trait for any object which can be used to select RingCT decoys. /// -/// An implementation is provided for any satisfier of `Rpc`. The benefit of this trait is the -/// ability to select decoys off of a locally stored output distribution, preventing potential -/// attacks a remote node can perform. +/// An implementation is provided for any satisfier of `Rpc`. It is not recommended to use an `Rpc` +/// object to satisfy this. This should be satisfied by a local store of the output distribution, +/// both for performance and to prevent potential attacks a remote node can perform. #[async_trait] pub trait DecoyRpc: Sync + Clone + Debug { - /// Get the length of the output distribution. + /// Get the height the output distribution ends at. /// /// This is equivalent to the hight of the blockchain it's for. This is intended to be cheaper /// than fetching the entire output distribution. - async fn get_output_distribution_len(&self) -> Result; + async fn get_output_distribution_end_height(&self) -> Result; - /// Get the output distribution. + /// Get the RingCT (zero-amount) output distribution. /// - /// `range` is in terms of block numbers. + /// `range` is in terms of block numbers. The result may be smaller than the requested range if + /// the range starts before RingCT outputs were created on-chain. async fn get_output_distribution( &self, range: impl Send + RangeBounds, @@ -843,7 +844,7 @@ pub trait DecoyRpc: Sync + Clone + Debug { #[async_trait] impl DecoyRpc for R { - async fn get_output_distribution_len(&self) -> Result { + async fn get_output_distribution_end_height(&self) -> Result { ::get_height(self).await } @@ -851,14 +852,17 @@ impl DecoyRpc for R { &self, range: impl Send + RangeBounds, ) -> Result, RpcError> { - #[derive(Deserialize, Debug)] + #[derive(Default, Debug, Deserialize)] struct Distribution { distribution: Vec, + // A blockchain with just its genesis block has a height of 1 + start_height: usize, } - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct Distributions { distributions: [Distribution; 1], + status: String, } let from = match range.start_bound() { @@ -889,15 +893,39 @@ impl DecoyRpc for R { "binary": false, "amounts": [0], "cumulative": true, + // These are actually block numbers, not heights "from_height": from, "to_height": if zero_zero_case { 1 } else { to }, })), ) .await?; - let mut distributions = distributions.distributions; - let mut distribution = core::mem::take(&mut distributions[0].distribution); - let expected_len = if zero_zero_case { 2 } else { (to - from) + 1 }; + if distributions.status != "OK" { + Err(RpcError::ConnectionError( + "node couldn't service this request for the output distribution".to_string(), + ))?; + } + + let mut distributions = distributions.distributions; + let Distribution { start_height, mut distribution } = core::mem::take(&mut distributions[0]); + // start_height is also actually a block number, and it should be at least `from` + // It may be after depending on when these outputs first appeared on the blockchain + // Unfortunately, we can't validate without a binary search to find the RingCT activation block + // and an iterative search from there, so we solely sanity check it + if start_height < from { + Err(RpcError::InvalidNode(format!( + "requested distribution from {from} and got from {start_height}" + )))?; + } + // It shouldn't be after `to` though + if start_height > to { + Err(RpcError::InvalidNode(format!( + "requested distribution to {to} and got from {start_height}" + )))?; + } + + let expected_len = if zero_zero_case { 2 } else { (to - start_height) + 1 }; + // Yet this is actually a height if expected_len != distribution.len() { Err(RpcError::InvalidNode(format!( "distribution length ({}) wasn't of the requested length ({})", @@ -905,7 +933,7 @@ impl DecoyRpc for R { expected_len )))?; } - // Requesting 0, 0 returns the distribution for the entire chain + // Requesting to = 0 returns the distribution for the entire chain // We work-around this by requesting 0, 1 (yielding two blocks), then popping the second block if zero_zero_case { distribution.pop(); @@ -914,7 +942,7 @@ impl DecoyRpc for R { } async fn get_outs(&self, indexes: &[u64]) -> Result, RpcError> { - #[derive(Deserialize, Debug)] + #[derive(Debug, Deserialize)] struct OutsResponse { status: String, outs: Vec, diff --git a/coins/monero/wallet/src/decoys.rs b/coins/monero/wallet/src/decoys.rs index 5867b23b..7abd3ce2 100644 --- a/coins/monero/wallet/src/decoys.rs +++ b/coins/monero/wallet/src/decoys.rs @@ -33,7 +33,7 @@ async fn select_n( if height < DEFAULT_LOCK_WINDOW { Err(RpcError::InternalError("not enough blocks to select decoys".to_string()))?; } - if height > rpc.get_output_distribution_len().await? { + if height > rpc.get_output_distribution_end_height().await? { Err(RpcError::InternalError( "decoys being requested from blocks this node doesn't have".to_string(), ))?; @@ -41,6 +41,9 @@ async fn select_n( // Get the distribution let distribution = rpc.get_output_distribution(.. height).await?; + if distribution.len() < DEFAULT_LOCK_WINDOW { + Err(RpcError::InternalError("not enough blocks to select decoys".to_string()))?; + } let highest_output_exclusive_bound = distribution[distribution.len() - DEFAULT_LOCK_WINDOW]; // This assumes that each miner TX had one output (as sane) and checks we have sufficient // outputs even when excluding them (due to their own timelock requirements)