nik/liana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge #808: Followup to #709

Browse Source 
0812a1216690514a0cf5ee6d80dbc6366fd91d43 jsonrpc: don't ignore invalid params to listaddresses (Antoine Poinsot)
d5338201d292ccf8d87849966c2f93efdfa0ccda commands: listaddresses cleanups (Antoine Poinsot)

Pull request description:

  This addresses my latest review from #709.

ACKs for top commit:
  jp1ac4:
    ACK 0812a12166

Tree-SHA512: 6f708fd2f1aa2f229a5c78a35e363870ef390513cce10fc6a5938b49e6b7ee5be9205bc4566376750e4f7eeea404709ce6d8c7a29df15b9216b8dbcf4b4fed7e
This commit is contained in:
Antoine Poinsot 2023-11-13 11:55:49 +01:00
commit 44f5a85b43
No known key found for this signature in database
GPG Key ID: E13FC145CD3F4304
4 changed files with 131 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

127
src/commands/mod.rs
View File

@ -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 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();
if start_index > (2u32.pow(31) - 1) {
return Err(CommandError::InvalidAddressIndex);
}
// 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 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)?;
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();
}

29
src/jsonrpc/api.rs
View File

@ -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))

3
src/jsonrpc/mod.rs
View File

@ -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())
}

22
tests/test_rpc.py
View File

@ -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