Merge #808: Followup to #709

0812a12 jsonrpc: don't ignore invalid params to listaddresses (Antoine Poinsot)
d533820 commands: listaddresses cleanups (Antoine Poinsot)

Pull request description:

  This addresses my latest review from #709.

ACKs for top commit:
  jp1ac4:
    ACK 0812a12

Tree-SHA512: 6f708fd2f1aa2f229a5c78a35e363870ef390513cce10fc6a5938b49e6b7ee5be9205bc4566376750e4f7eeea404709ce6d8c7a29df15b9216b8dbcf4b4fed7e

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."),
         }
     }
 }
@@ -310,34 +309,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<AddressInfo> = 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<AddressInfo> = (start_index..=index)
+        // Derive all receive and change addresses for the queried range.
+        let addresses: Result<Vec<AddressInfo>, _> = (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
@@ -353,14 +346,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.
@@ -939,14 +932,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<AddressInfo>,
 }
@@ -1115,6 +1108,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();
     }
 

src/jsonrpc/api.rs

@@ -136,21 +136,28 @@ fn list_coins(control: &DaemonControl, params: Option<Params>) -> Result<serde_j
     Ok(serde_json::json!(&res))
 }
 
+fn get_opt_u32<Q>(params: &Option<Params>, index: usize, name: &Q) -> Result<Option<u32>, Error>
+where
+    String: std::borrow::Borrow<Q>,
+    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<Params>,
 ) -> Result<serde_json::Value, Error> {
-    let start_index: Option<u32> = 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<u32> = 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(&params, 0, "start_index")?;
+    let count = get_opt_u32(&params, 1, "count")?;
 
     let res = &control.list_addresses(start_index, count)?;
     Ok(serde_json::json!(&res))

src/jsonrpc/mod.rs

@@ -164,8 +164,7 @@ impl From<commands::CommandError> 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())
             }

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
@@ -47,13 +47,29 @@ def test_listadresses(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