Merge #963: commands: increment address index if current value used for new spend

bfd6ca517b1811a888f83f14d3f6900c590dc3c1 commands: increment receive index on each use (jp1ac4)
cc5e396ace8b24e7e65bc356655463a690a25091 commands: increment change index on each use (jp1ac4)
c88224ca715ccfdc6530362b960dddd54ff64610 testutils: fix change_index (jp1ac4)

Pull request description:

  This is a fix to ensure the change index in the database is incremented if a new spend is created with a change address derived from the current index, regardless of whether this new spend is broadcast or not.

  EDIT: The same fix has been applied to the receive index.

ACKs for top commit:
  darosior:
    re-ACK bfd6ca517b1811a888f83f14d3f6900c590dc3c1

Tree-SHA512: 66f88fe95f4fb518cd93cde12787f74b6db61e6a2ad6ae27ee4f134ac039df6ce03243afde321412486c435641122ddc27baace2a15a167d3bfe20e17be22597

src/commands/mod.rs

@@ -262,12 +262,12 @@ impl DaemonControl {
         addr_info: &Option<AddrInfo>,
     ) {
         if let Some(AddrInfo { index, is_change }) = addr_info {
-            if *is_change && db_conn.change_index() < *index {
+            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 {
+            } else if !is_change && db_conn.receive_index() <= *index {
                 let next_index = index
                     .increment()
                     .expect("Must not get into hardened territory");
@@ -1760,9 +1760,46 @@ mod tests {
             panic!("expect successful spend creation")
         };
         let tx_manual = psbt.unsigned_tx;
-        // Check that manual and auto selection give same outputs (including change).
-        assert_eq!(tx_auto.output, tx_manual.output);
+        // Check that manual and auto selection give same outputs (except change address).
+        assert_ne!(tx_auto.output, tx_manual.output);
+        assert_eq!(tx_auto.output.len(), tx_manual.output.len());
+        assert_eq!(tx_auto.output[0], tx_manual.output[0]);
+        assert_eq!(tx_auto.output[1].value, tx_manual.output[1].value);
+        assert_ne!(
+            tx_auto.output[1].script_pubkey,
+            tx_manual.output[1].script_pubkey
+        );
         // Check inputs are also the same. Need to sort as order is not guaranteed by `create_spend`.
+        let mut auto_input = tx_auto.clone().input;
+        let mut manual_input = tx_manual.input;
+        auto_input.sort();
+        manual_input.sort();
+        assert_eq!(auto_input, manual_input);
+
+        // Now do the same again, but this time specifying the change address to be the same
+        // as for the auto spend.
+        let change_address = bitcoin::Address::from_script(
+            tx_auto.output[1].script_pubkey.as_script(),
+            bitcoin::Network::Bitcoin,
+        )
+        .unwrap();
+        let psbt = if let CreateSpendResult::Success { psbt, .. } = control
+            .create_spend(
+                &destinations,
+                &[confirmed_op_1, confirmed_op_2],
+                1,
+                Some(change_address.as_unchecked().clone()),
+            )
+            .unwrap()
+        {
+            psbt
+        } else {
+            panic!("expect successful spend creation")
+        };
+        let tx_manual = psbt.unsigned_tx;
+        // Now the outputs of each transaction are the same.
+        assert_eq!(tx_auto.output, tx_manual.output);
+        // Check again that inputs are still the same.
         let mut auto_input = tx_auto.input;
         let mut manual_input = tx_manual.input;
         auto_input.sort();

src/testutils.rs

@@ -212,7 +212,7 @@ impl DatabaseConnection for DummyDatabase {
     }
 
     fn change_index(&mut self) -> bip32::ChildNumber {
-        self.db.read().unwrap().deposit_index
+        self.db.read().unwrap().change_index
     }
 
     fn set_change_index(

tests/test_spend.py

@@ -246,6 +246,20 @@ 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
+    # 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_955_000}, [], 1)
+    assert "psbt" in res
+    # Max(receive_index, change_index) is now 4:
+    assert len(lianad.rpc.listaddresses()["addresses"]) == 4
+    # But the spend has no change:
+    psbt = PSBT.from_base64(res["psbt"])
+    assert len(psbt.o) == 1
+    # As the spend has no change, only the receive index was incremented.
+    # Therefore, the DB receive index is now 4.
+
 
 def test_coin_selection(lianad, bitcoind):
     """We can create a spend using coin selection."""
@@ -423,10 +437,9 @@ def test_coin_selection(lianad, bitcoind):
     # Recipient details are the same for both.
     assert spend_psbt_4.tx.vout[0].nValue == psbt_manual.tx.vout[0].nValue
     assert spend_psbt_4.tx.vout[0].scriptPubKey == psbt_manual.tx.vout[0].scriptPubKey
-    # Change details are also the same
-    # (change address is same as neither transaction has been broadcast)
+    # Change amount is the same (change address will be different).
     assert spend_psbt_4.tx.vout[1].nValue == psbt_manual.tx.vout[1].nValue
-    assert spend_psbt_4.tx.vout[1].scriptPubKey == psbt_manual.tx.vout[1].scriptPubKey
+    assert spend_psbt_4.tx.vout[1].scriptPubKey != psbt_manual.tx.vout[1].scriptPubKey
 
 
 def test_coin_selection_changeless(lianad, bitcoind):