diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index 2417f246..c190083a 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -257,41 +257,39 @@ impl DaemonControl { } // Get the change address for the next derivation index. + // The spend may not have a change output, so we don't update the DB value yet. fn next_change_addr(&self, db_conn: &mut Box) -> SpendOutputAddress { let index = db_conn.change_index(); + let next_index = index + .increment() + .expect("Must not get into hardened territory"); let desc = self .config .main_descriptor .change_descriptor() - .derive(index, &self.secp); + .derive(next_index, &self.secp); let addr = desc.address(self.config.bitcoin_config.network); SpendOutputAddress { addr, info: Some(AddrInfo { - index, + index: next_index, is_change: true, }), } } - // If we detect the given address as ours, and it has a higher derivation index than our next - // derivation index, update our next derivation index to the one after the address'. - fn maybe_increase_next_deriv_index( + // If we detect the given address as ours, and it has a higher derivation index than our last + // derivation index, update our last derivation index to the given value. + fn maybe_increase_last_deriv_index( &self, db_conn: &mut Box, addr_info: &Option, ) { if let Some(AddrInfo { index, is_change }) = addr_info { - if *is_change && db_conn.change_index() <= *index { - let next_index = index - .increment() - .expect("Must not get into hardened territory"); - db_conn.set_change_index(next_index, &self.secp); - } else if !is_change && db_conn.receive_index() <= *index { - let next_index = index - .increment() - .expect("Must not get into hardened territory"); - db_conn.set_receive_index(next_index, &self.secp); + if *is_change && db_conn.change_index() < *index { + db_conn.set_change_index(*index, &self.secp); + } else if !is_change && db_conn.receive_index() < *index { + db_conn.set_receive_index(*index, &self.secp); } } } @@ -352,9 +350,9 @@ impl DaemonControl { .config .main_descriptor .receive_descriptor() - .derive(index, &self.secp) + .derive(new_index, &self.secp) .address(self.config.bitcoin_config.network); - GetAddressResult::new(address, index) + GetAddressResult::new(address, new_index) } /// Update derivation indexes @@ -435,7 +433,11 @@ impl DaemonControl { .checked_add(c) .ok_or(CommandError::InvalidDerivationIndex)? } else { - receive_index.max(change_index) + // `end_index` will not be included so add 1 in order to include the last used index. + receive_index + .max(change_index) + .checked_add(1) + .ok_or(CommandError::InvalidDerivationIndex)? }; // Derive all receive and change addresses for the queried range. @@ -667,10 +669,10 @@ impl DaemonControl { } }; for (addr, _) in destinations_checked { - self.maybe_increase_next_deriv_index(&mut db_conn, &addr.info); + self.maybe_increase_last_deriv_index(&mut db_conn, &addr.info); } if has_change { - self.maybe_increase_next_deriv_index(&mut db_conn, &change_info); + self.maybe_increase_last_deriv_index(&mut db_conn, &change_info); } Ok(CreateSpendResult::Success { @@ -1052,10 +1054,10 @@ impl DaemonControl { // In case of success, make sure to update our next derivation index if any address // used in the transaction outputs was from the future. for (addr, _) in destinations { - self.maybe_increase_next_deriv_index(&mut db_conn, &addr.info); + self.maybe_increase_last_deriv_index(&mut db_conn, &addr.info); } if has_change { - self.maybe_increase_next_deriv_index(&mut db_conn, &change_address.info); + self.maybe_increase_last_deriv_index(&mut db_conn, &change_address.info); } return Ok(CreateSpendResult::Success { @@ -1204,7 +1206,7 @@ impl DaemonControl { locktime, )?; if has_change { - self.maybe_increase_next_deriv_index(&mut db_conn, &sweep_addr_info); + self.maybe_increase_last_deriv_index(&mut db_conn, &sweep_addr_info); } Ok(CreateRecoveryResult { psbt }) @@ -1397,12 +1399,17 @@ mod tests { let ms = DummyLiana::new(DummyBitcoind::new(), DummyDatabase::new()); let control = &ms.control(); - // We can get an address + // We can get an address (it will have index 1) let addr = control.get_new_address().address; + // $ bitcoin-cli deriveaddresses "wsh(or_d(pk([aabbccdd]xpub68JJTXc1MWK8KLW4HGLXZBJknja7kDUJuFHnM424LbziEXsfkh1WQCiEjjHw4zLqSUm4rvhgyGkkuRowE9tCJSgt3TQB5J3SKAbZ2SdcKST/0/*),and_v(v:pkh([aabbccdd]xpub68JJTXc1MWK8PEQozKsRatrUHXKFNkD1Cb1BuQU9Xr5moCv87anqGyXLyUd4KpnDyZgo3gz4aN1r3NiaoweFW8UutBsBbgKHzaD5HkTkifK/0/*),older(10000))))#wx6v3mks" 1 + // [ + // "bc1q9ksrc647hx8zp2cewl8p5f487dgux3777yees8rjcx46t4daqzzqt7yga8", + // "bc1qm06ceyghltr8v5cmeckh6cquhy9nks626pahapc04xd98kwf478qwmhqew" + // ] assert_eq!( addr, bitcoin::Address::from_str( - "bc1q9ksrc647hx8zp2cewl8p5f487dgux3777yees8rjcx46t4daqzzqt7yga8" + "bc1qm06ceyghltr8v5cmeckh6cquhy9nks626pahapc04xd98kwf478qwmhqew" ) .unwrap() .assume_checked() @@ -1426,39 +1433,38 @@ mod tests { assert_eq!(list.addresses.last().unwrap().index, 6); let addr0 = control.get_new_address().address; - let addr1 = control.get_new_address().address; - let _addr2 = control.get_new_address().address; - let addr3 = control.get_new_address().address; + let _addr1 = control.get_new_address().address; + let addr2 = control.get_new_address().address; + let _addr3 = control.get_new_address().address; let addr4 = control.get_new_address().address; let list = control.list_addresses(Some(0), None).unwrap(); assert_eq!(list.addresses[0].index, 0); - assert_eq!(list.addresses[0].receive, addr0); - assert_eq!(list.addresses.last().unwrap().index, 4); + assert_eq!(list.addresses[1].receive, addr0); // first address has index 1 + assert_eq!(list.addresses.last().unwrap().index, 5); assert_eq!(list.addresses.last().unwrap().receive, addr4); let list = control.list_addresses(None, None).unwrap(); assert_eq!(list.addresses[0].index, 0); - assert_eq!(list.addresses[0].receive, addr0); - assert_eq!(list.addresses.last().unwrap().index, 4); + assert_eq!(list.addresses[1].index, 1); + assert_eq!(list.addresses[1].receive, addr0); + assert_eq!(list.addresses.last().unwrap().index, 5); assert_eq!(list.addresses.last().unwrap().receive, addr4); let list = control.list_addresses(Some(1), Some(3)).unwrap(); assert_eq!(list.addresses[0].index, 1); - assert_eq!(list.addresses[0].receive, addr1); + assert_eq!(list.addresses[0].receive, addr0); assert_eq!(list.addresses.last().unwrap().index, 3); - assert_eq!(list.addresses.last().unwrap().receive, addr3); + assert_eq!(list.addresses.last().unwrap().receive, addr2); - let addr5 = control.get_new_address().address; let list = control.list_addresses(Some(5), None).unwrap(); + assert_eq!(list.addresses.len(), 1); assert_eq!(list.addresses[0].index, 5); - assert_eq!(list.addresses[0].receive, addr5); - assert_eq!(list.addresses.last().unwrap().index, 5); - assert_eq!(list.addresses.last().unwrap().receive, addr5); + assert_eq!(list.addresses[0].receive, addr4); // We can get no address for the last unhardened index. let max_unhardened_index = 2u32.pow(31) - 1; diff --git a/lianad/src/database/mod.rs b/lianad/src/database/mod.rs index cd1bd22b..57c4a835 100644 --- a/lianad/src/database/mod.rs +++ b/lianad/src/database/mod.rs @@ -30,9 +30,9 @@ use miniscript::bitcoin::{self, bip32, psbt::Psbt, secp256k1, Address, Network, pub struct Wallet { /// Timestamp at wallet creation time. pub timestamp: u32, - /// Derivation index for the next receiving address. + /// Derivation index for the last used/revealed receiving address. pub receive_index: bip32::ChildNumber, - /// Derivation index for the next change address. + /// Derivation index for the last used/revealed change address. pub change_index: bip32::ChildNumber, /// Timestamp to start rescanning from, if any. pub rescan_timestamp: Option, @@ -73,20 +73,20 @@ pub trait DatabaseConnection { /// Update our best chain seen. fn update_tip(&mut self, tip: &BlockChainTip); - /// Get the derivation index for the next receiving address + /// Get the derivation index for the last used/revealed receiving address fn receive_index(&mut self) -> bip32::ChildNumber; - /// Set the derivation index for the next receiving address + /// Set the derivation index for the last used/revealed receiving address fn set_receive_index( &mut self, index: bip32::ChildNumber, secp: &secp256k1::Secp256k1, ); - /// Get the derivation index for the next change address + /// Get the derivation index for the last used/revealed change address fn change_index(&mut self) -> bip32::ChildNumber; - /// Set the derivation index for the next change address + /// Set the derivation index for the last used/revealed change address fn set_change_index( &mut self, index: bip32::ChildNumber, diff --git a/tests/test_rpc.py b/tests/test_rpc.py index b53523ca..c5df1bee 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -152,29 +152,35 @@ def test_update_derivation_indexes(lianad): def test_getaddress(lianad): res = lianad.rpc.getnewaddress() assert "address" in res + # The first new wallet address has index 1 + assert res["derivation_index"] == 1 # We'll get a new one at every call assert res["address"] != lianad.rpc.getnewaddress()["address"] # new address has derivation_index higher than the previous one assert lianad.rpc.getnewaddress()["derivation_index"] == res["derivation_index"] + 2 info = lianad.rpc.getinfo() - assert info["receive_index"] == res["derivation_index"] + 3 + assert info["receive_index"] == res["derivation_index"] + 2 # 3 == 1 + 2 assert info["change_index"] == 0 def test_listaddresses(lianad): - list = lianad.rpc.listaddresses(2, 5) + list1 = lianad.rpc.listaddresses(2, 5) list2 = lianad.rpc.listaddresses(start_index=2, count=5) - assert list == list2 - assert "addresses" in list - addr = list["addresses"] + assert list1 == list2 + assert "addresses" in list1 + addr = list1["addresses"] assert addr[0]["index"] == 2 assert addr[-1]["index"] == 6 - list3 = lianad.rpc.listaddresses() # start_index = 0, receive_index = 0 + list3 = ( + lianad.rpc.listaddresses() + ) # start_index = 0, receive_index = 0 (returns 1 "used" address for index 0) _ = lianad.rpc.getnewaddress() # start_index = 0, receive_index = 1 _ = lianad.rpc.getnewaddress() # start_index = 0, receive_index = 2 + # list4 returns all indexes from 0 up to last used. + # The first new address has index 1, so returned indexes are 0, 1, 2: list4 = lianad.rpc.listaddresses() - assert len(list4["addresses"]) == len(list3["addresses"]) + 2 == 2 + assert len(list4["addresses"]) == len(list3["addresses"]) + 2 == 3 list5 = lianad.rpc.listaddresses(0) assert list4 == list5 @@ -534,7 +540,13 @@ def test_create_spend(lianad, bitcoind): assert len(spend_psbt.o) == 4 assert len(spend_psbt.tx.vout) == 4 - assert lianad.rpc.getinfo()["change_index"] == 15 + # 15 new receive addresses have been generated (starting at index 1), + # so last used value is 15: + assert lianad.rpc.getinfo()["receive_index"] == 15 + # For each received coin, the change index has also been updated by the poller + # (see https://github.com/wizardsardine/liana/issues/1333), so is also 15. + # Then `createspend` will use the next index for change and update the DB value accordingly: + assert lianad.rpc.getinfo()["change_index"] == 16 # The transaction must contain the spent transaction for each input for P2WSH. But not for Taproot. # We don't make assumptions about the ordering of PSBT inputs. diff --git a/tests/test_spend.py b/tests/test_spend.py index f53520b9..c6b08f02 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -231,10 +231,21 @@ def test_send_to_self(lianad, bitcoind): lianad.rpc.getnewaddress()["address"]: 0.04, lianad.rpc.getnewaddress()["address"]: 0.05, } + info = lianad.rpc.getinfo() + # We've generated 3 receive addresses, starting at index 1, so last used is 3. + assert info["receive_index"] == 3 + # No change addresses used, so last index is 0. + assert info["change_index"] == 0 deposit_txid = bitcoind.rpc.sendmany("", destinations) bitcoind.generate_block(1, wait_for_mempool=deposit_txid) wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 3) + info = lianad.rpc.getinfo() + assert info["receive_index"] == 3 + # Change index has been updated by poller, even though none used + # (see https://github.com/wizardsardine/liana/issues/1333): + assert info["change_index"] == 3 + # Then create a send-to-self transaction (by not providing any destination) that # sweeps them all. outpoints = [c["outpoint"] for c in lianad.rpc.listcoins()["coins"]] @@ -243,6 +254,12 @@ def test_send_to_self(lianad, bitcoind): spend_psbt = PSBT.from_base64(res["psbt"]) assert len(spend_psbt.o) == len(spend_psbt.tx.vout) == 1 + info = lianad.rpc.getinfo() + # Send to self didn't use any receive addresses... + assert info["receive_index"] == 3 + # ... but it did use a new change address: + assert info["change_index"] == 4 + # Note they may ask for an impossible send-to-self. In this case we'll report missing amount. huge_feerate = 50_000 if USE_TAPROOT else 40_500 assert "missing" in lianad.rpc.createspend({}, outpoints, huge_feerate) @@ -267,16 +284,19 @@ def test_send_to_self(lianad, bitcoind): ) wait_for(lambda: len(list(unspent_coins())) == 1) - # We've used 3 receive addresses and so the DB receive index must be 3. - assert len(lianad.rpc.listaddresses()["addresses"]) == 3 + info = lianad.rpc.getinfo() + # The poller has updated the receive index based on the change index + # (see https://github.com/wizardsardine/liana/issues/1333): + assert info["receive_index"] == 4 + assert info["change_index"] == 4 # Create a new spend to the receive address with index 3. recv_addr = lianad.rpc.listaddresses(3, 1)["addresses"][0]["receive"] res = lianad.rpc.createspend( {recv_addr: 11_965_000 if USE_TAPROOT else 11_955_000}, [], 2 ) assert "psbt" in res - # Max(receive_index, change_index) is now 4: - assert len(lianad.rpc.listaddresses()["addresses"]) == 4 + # Max(receive_index, change_index) is now 4, so we return addresses 0, 1, 2, 3, 4: + assert len(lianad.rpc.listaddresses()["addresses"]) == 5 # But the spend has no change: psbt = PSBT.from_base64(res["psbt"]) assert len(psbt.o) == 1