From d5338201d292ccf8d87849966c2f93efdfa0ccda Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 11 Nov 2023 13:59:06 +0100 Subject: [PATCH 1/2] commands: listaddresses cleanups Introduce a single error enum variant. Avoid underflows. Clarify and comment the logic. --- src/commands/mod.rs | 129 ++++++++++++++++++++++++++++++++------------ src/jsonrpc/mod.rs | 3 +- tests/test_rpc.py | 2 +- 3 files changed, 96 insertions(+), 38 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index f5ab4210b..15ebb4811 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -72,8 +72,8 @@ pub enum CommandError { /// An error that might occur in the racy rescan triggering logic. RescanTrigger(String), RecoveryNotAvailable, - InvalidAddressCount, - InvalidAddressIndex, + /// Overflowing or unhardened derivation index. + InvalidDerivationIndex, } impl fmt::Display for CommandError { @@ -138,8 +138,7 @@ impl fmt::Display for CommandError { f, "No coin currently spendable through this timelocked recovery path." ), - Self::InvalidAddressCount => write!(f, "Invalid address count, should be under 2^31-1"), - Self::InvalidAddressIndex => write!(f, "Invalid address index, should be under 2^31-1"), + Self::InvalidDerivationIndex => write!(f, "Unhardened or overflowing BIP32 derivation index."), } } } @@ -303,34 +302,28 @@ impl DaemonControl { let receive_index: u32 = db_conn.receive_index().into(); let change_index: u32 = db_conn.change_index().into(); - let start_index = start_index.unwrap_or(0); - - if start_index > (2u32.pow(31) - 1) { - return Err(CommandError::InvalidAddressIndex); - } - - let count = count.unwrap_or_else(|| receive_index.max(change_index) - start_index); - - if count == 0 { - let out: Vec = Vec::new(); - return Ok(ListAddressesResult::new(out)); - } - - let index = start_index - .checked_add(count) - .and_then(|index| index.checked_sub(1)) - .and_then(|index| { - if index > (2u32.pow(31) - 1) { - None - } else { - Some(index) - } - }) - .ok_or(CommandError::InvalidAddressCount)?; + // If a start index isn't provided, we derive from index 0. Make sure the provided index is + // unhardened. + let start_index = bip32::ChildNumber::from_normal_idx(start_index.unwrap_or(0)) + .map_err(|_| CommandError::InvalidDerivationIndex)?; + let start_index_u32: u32 = start_index.into(); + + // Derive the end index (ie, the first index to not be returned) from the count of + // addresses to provide. If no count was passed, use the next derivation index between + // change and receive as end index. + let end_index = if let Some(c) = count { + start_index_u32 + .checked_add(c) + .ok_or(CommandError::InvalidDerivationIndex)? + } else { + receive_index.max(change_index) + }; - let addresses: Vec = (start_index..=index) + // Derive all receive and change addresses for the queried range. + let addresses: Result, _> = (start_index_u32..end_index) .map(|index| { - let child = bip32::ChildNumber::from_normal_idx(index).expect("Cannot fail here"); + let child = bip32::ChildNumber::from_normal_idx(index) + .map_err(|_| CommandError::InvalidDerivationIndex)?; let receive = self .config @@ -346,14 +339,14 @@ impl DaemonControl { .derive(child, &self.secp) .address(self.config.bitcoin_config.network); - AddressInfo { + Ok(AddressInfo { index, receive, change, - } + }) }) .collect(); - Ok(ListAddressesResult::new(addresses)) + Ok(ListAddressesResult::new(addresses?)) } /// Get a list of all known coins, optionally by status and/or outpoint. @@ -927,14 +920,14 @@ impl GetAddressResult { } } -#[derive(Debug, Clone, Serialize)] +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] pub struct AddressInfo { index: u32, receive: bitcoin::Address, change: bitcoin::Address, } -#[derive(Debug, Clone, Serialize)] +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] pub struct ListAddressesResult { addresses: Vec, } @@ -1103,6 +1096,72 @@ mod tests { assert_eq!(list.addresses.last().unwrap().index, 5); assert_eq!(list.addresses.last().unwrap().receive, addr5); + // We can get no address for the last unhardened index. + let max_unhardened_index = 2u32.pow(31) - 1; + let res = control + .list_addresses(Some(max_unhardened_index), Some(0)) + .unwrap(); + // This is equivalent to not passing a count. + assert_eq!( + res, + control + .list_addresses(Some(max_unhardened_index), None) + .unwrap() + ); + // We can also get the one last unhardened index. + control + .list_addresses(Some(max_unhardened_index), Some(1)) + .unwrap(); + // However we can't get into hardened territory. + assert_eq!( + control + .list_addresses(Some(max_unhardened_index), Some(2)) + .unwrap_err(), + CommandError::InvalidDerivationIndex + ); + + // We also can't pass a hardened start index. + let first_hardened_index = max_unhardened_index + 1; + assert_eq!( + control + .list_addresses(Some(first_hardened_index), None) + .unwrap_err(), + CommandError::InvalidDerivationIndex + ); + assert_eq!( + control + .list_addresses(Some(first_hardened_index), Some(0)) + .unwrap_err(), + CommandError::InvalidDerivationIndex + ); + assert_eq!( + control + .list_addresses(Some(first_hardened_index), Some(1)) + .unwrap_err(), + CommandError::InvalidDerivationIndex + ); + + // Much less so overflow. + assert_eq!( + control.list_addresses(Some(u32::MAX), None).unwrap_err(), + CommandError::InvalidDerivationIndex + ); + assert_eq!( + control.list_addresses(Some(u32::MAX), Some(0)).unwrap_err(), + CommandError::InvalidDerivationIndex + ); + assert_eq!( + control.list_addresses(Some(u32::MAX), Some(1)).unwrap_err(), + CommandError::InvalidDerivationIndex + ); + + // We won't crash if we pass a start index larger than the next derivation index without + // passing a count. (ie no underflow.) + let next_deriv_index = list.addresses.last().unwrap().index + 1; + control + .list_addresses(Some(next_deriv_index + 1), None) + .unwrap(); + ms.shutdown(); } diff --git a/src/jsonrpc/mod.rs b/src/jsonrpc/mod.rs index f9bb9e785..78fbb9d14 100644 --- a/src/jsonrpc/mod.rs +++ b/src/jsonrpc/mod.rs @@ -164,8 +164,7 @@ impl From for Error { | commands::CommandError::SpendFinalization(..) | commands::CommandError::InsaneRescanTimestamp(..) | commands::CommandError::AlreadyRescanning - | commands::CommandError::InvalidAddressCount - | commands::CommandError::InvalidAddressIndex + | commands::CommandError::InvalidDerivationIndex | commands::CommandError::RecoveryNotAvailable => { Error::new(ErrorCode::InvalidParams, e.to_string()) } diff --git a/tests/test_rpc.py b/tests/test_rpc.py index f01027498..4c0ff40b2 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -37,7 +37,7 @@ def test_getaddress(lianad): assert res["address"] != lianad.rpc.getnewaddress()["address"] -def test_listadresses(lianad): +def test_listaddresses(lianad): list = lianad.rpc.listaddresses(2, 5) list2 = lianad.rpc.listaddresses(start_index=2, count=5) assert list == list2 From 0812a1216690514a0cf5ee6d80dbc6366fd91d43 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sat, 11 Nov 2023 14:17:55 +0100 Subject: [PATCH 2/2] jsonrpc: don't ignore invalid params to listaddresses --- src/jsonrpc/api.rs | 29 ++++++++++++++++++----------- tests/test_rpc.py | 20 ++++++++++++++++++-- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/jsonrpc/api.rs b/src/jsonrpc/api.rs index 67e527e89..cfc91838e 100644 --- a/src/jsonrpc/api.rs +++ b/src/jsonrpc/api.rs @@ -136,21 +136,28 @@ fn list_coins(control: &DaemonControl, params: Option) -> Result(params: &Option, index: usize, name: &Q) -> Result, Error> +where + String: std::borrow::Borrow, + Q: ?Sized + Ord + Eq + std::hash::Hash + std::fmt::Display, +{ + Ok( + if let Some(i) = params.as_ref().and_then(|p| p.get(index, name)) { + Some(i.as_u64().and_then(|i| i.try_into().ok()).ok_or_else(|| { + Error::invalid_params(format!("Invalid value for '{}': {}", name, i)) + })?) + } else { + None + }, + ) +} + fn list_addresses( control: &DaemonControl, params: Option, ) -> Result { - let start_index: Option = params - .as_ref() - .and_then(|p| p.get(0, "start_index")) - .and_then(|i| i.as_u64()) - .and_then(|i| i.try_into().ok()); - - let count: Option = params - .as_ref() - .and_then(|p| p.get(1, "count")) - .and_then(|c| c.as_u64()) - .and_then(|i| i.try_into().ok()); + let start_index = get_opt_u32(¶ms, 0, "start_index")?; + let count = get_opt_u32(¶ms, 1, "count")?; let res = &control.list_addresses(start_index, count)?; Ok(serde_json::json!(&res)) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 4c0ff40b2..8d2b82479 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -47,13 +47,29 @@ def test_listaddresses(lianad): assert addr[-1]["index"] == 6 list3 = lianad.rpc.listaddresses() # start_index = 0, receive_index = 0 - _ = lianad.rpc.getnewaddress() # start_index = 0, receive_index = 1 - _ = lianad.rpc.getnewaddress() # start_index = 0, receive_index = 2 + _ = lianad.rpc.getnewaddress() # start_index = 0, receive_index = 1 + _ = lianad.rpc.getnewaddress() # start_index = 0, receive_index = 2 list4 = lianad.rpc.listaddresses() assert len(list4["addresses"]) == len(list3["addresses"]) + 2 == 2 list5 = lianad.rpc.listaddresses(0) assert list4 == list5 + # Will explicitly error on invalid start_index. + with pytest.raises( + RpcError, + match=re.escape( + "Invalid params: Invalid value for \\'start_index\\': \"blabla\"" + ), + ): + lianad.rpc.listaddresses("blabla", None) + + # Will explicitly error on invalid count. + with pytest.raises( + RpcError, + match=re.escape("Invalid params: Invalid value for \\'count\\': \"blb\""), + ): + lianad.rpc.listaddresses(0, "blb") + def test_listcoins(lianad, bitcoind): # Initially empty