Merge #578: Fix the tracking of immature coinbase deposits

097d5e7 qa: adapt the migration functional test to also support 1.0 (Antoine Poinsot)
289f658 qa: functional test createspend refuses immature outpoints (Antoine Poinsot)
95dd3e5 qa: fix and improve the coinbase deposit functional test (Antoine Poinsot)
6b82894 bitcoin: track maturity of coinbase deposits (Antoine Poinsot)
6ab6161 commands: expose whether a coin is immature in listcoins (Antoine Poinsot)
fd71712 commands: don't create spends with immature coins (Antoine Poinsot)
26add29 database: record whether a coin comes from an immature coinbase (Antoine Poinsot)

Pull request description:

  #567 uncovered that we were actually not tracking coinbase deposits correctly. In fact we would most likely miss them all in any real situation. This is because we would filter out immature coinbase deposits from the result of `listsinceblock` and not consider them newly received coins. But they would be confirmed, and as the chain moves forward we'd not scan this range anymore even once they've become mature.

  This PR fixes it by the simplest possible manner: record immature coinbase deposits as unconfirmed and only mark them as confirmed once they've become mature. This is a bit clumsy, but should be fine for the number of users that would receive payouts from coinbase transactions (ie most likely 0). Also, we don't accurately update coinbase coins on reorg. This is unnecessary as it'd be very unlikely that a mature coinbase would become immature and if there is a 100 blocks reorg that would invalidate it altogether we'd have bigger problems.

  Fixes #567.
  ~~Still as draft as i want to go over this one more time before asking for review, as this if pretty intricate and touches core parts of our codebase.~~

ACKs for top commit:
  darosior:
    self-ACK 097d5e7. Didn't have the chance to re-review it but getting it in is best at this time. Edouard will still have a look post-merge.

Tree-SHA512: 4a5f0fb7561af1d4c51dcba26bc20ef5e7a8b2c730547f762782f75c1f28c26e2a577573aa514db8927c1eeb601685071e9eb85c11537621de58c75824435b05

doc/API.md

@@ -96,6 +96,7 @@ This command does not take any parameter for now.
 | `outpoint`     | string        | Transaction id and output index of this coin.                                                                      |
 | `block_height` | int or null   | Block height the transaction was confirmed at, or `null`.                                                          |
 | `spend_info`   | object        | Information about the transaction spending this coin. See [Spending transaction info](#spending_transaction_info). |
+| `is_immature`  | bool          | Whether this coin was created by a coinbase transaction that is still immature.                                    |
 
 
 ##### Spending transaction info

src/bitcoin/d/mod.rs

@@ -1111,6 +1111,7 @@ pub struct LSBlockEntry {
     pub block_height: Option<i32>,
     pub address: bitcoin::Address<address::NetworkUnchecked>,
     pub parent_descs: Vec<descriptor::Descriptor<descriptor::DescriptorPublicKey>>,
+    pub is_immature: bool,
 }
 
 impl From<&Json> for LSBlockEntry {
@@ -1156,12 +1157,19 @@ impl From<&Json> for LSBlockEntry {
             })
             .expect("bitcoind can't give invalid descriptors");
 
+        let is_immature = json
+            .get("category")
+            .and_then(Json::as_str)
+            .expect("must be present")
+            == "immature";
+
         LSBlockEntry {
             outpoint,
             amount,
             block_height,
             address,
             parent_descs,
+            is_immature,
         }
     }
 }
@@ -1189,7 +1197,7 @@ impl From<Json> for LSBlockRes {
                     .get("category")
                     .and_then(Json::as_str)
                     .expect("must be present");
-                if category == "receive" || category == "generate" {
+                if ["receive", "generate", "immature"].contains(&category) {
                     let lsb_entry: LSBlockEntry = j.into();
                     Some(lsb_entry)
                 } else {
@@ -1207,6 +1215,8 @@ pub struct GetTxRes {
     pub conflicting_txs: Vec<bitcoin::Txid>,
     pub block: Option<Block>,
     pub tx: bitcoin::Transaction,
+    pub is_coinbase: bool,
+    pub confirmations: i32,
 }
 
 impl From<Json> for GetTxRes {
@@ -1244,10 +1254,21 @@ impl From<Json> for GetTxRes {
         let bytes = Vec::from_hex(hex).expect("bitcoind returned a wrong transaction format");
         let tx: bitcoin::Transaction = bitcoin::consensus::encode::deserialize(&bytes)
             .expect("bitcoind returned a wrong transaction format");
+        let is_coinbase = json
+            .get("generated")
+            .and_then(Json::as_bool)
+            .unwrap_or(false);
+        let confirmations = json
+            .get("confirmations")
+            .and_then(Json::as_i64)
+            .expect("Must be present in the response") as i32;
+
         GetTxRes {
             conflicting_txs: conflicting_txs.unwrap_or_default(),
             block,
             tx,
+            is_coinbase,
+            confirmations,
         }
     }
 }

src/bitcoin/mod.rs

@@ -14,6 +14,8 @@ use std::{fmt, sync};
 
 use miniscript::bitcoin::{self, address};
 
+const COINBASE_MATURITY: i32 = 100;
+
 /// Information about a block
 #[derive(Debug, Clone, Eq, PartialEq, Copy)]
 pub struct Block {
@@ -146,6 +148,7 @@ impl BitcoinInterface for d::BitcoinD {
                     block_height,
                     address,
                     parent_descs,
+                    is_immature,
                 } = entry;
                 if parent_descs
                     .iter()
@@ -156,6 +159,7 @@ impl BitcoinInterface for d::BitcoinD {
                         amount,
                         block_height,
                         address,
+                        is_immature,
                     })
                 } else {
                     None
@@ -184,6 +188,11 @@ impl BitcoinInterface for d::BitcoinD {
 
             // If the transaction was confirmed, mark the coin as such.
             if let Some(block) = res.block {
+                // Do not mark immature coinbase deposits as confirmed until they become mature.
+                if res.is_coinbase && res.confirmations < COINBASE_MATURITY {
+                    log::debug!("Coin at '{}' comes from an immature coinbase transaction with {} confirmations. Not marking it as confirmed for now.", op, res.confirmations);
+                    continue;
+                }
                 confirmed.push((*op, block.height, block.time));
                 continue;
             }
@@ -409,4 +418,5 @@ pub struct UTxO {
     pub amount: bitcoin::Amount,
     pub block_height: Option<i32>,
     pub address: bitcoin::Address<address::NetworkUnchecked>,
+    pub is_immature: bool,
 }

src/bitcoin/poller/looper.rs

@@ -24,6 +24,8 @@ struct UpdatedCoins {
 // or spent.
 // NOTE: A coin may be updated multiple times at once. That is, a coin may be received, confirmed,
 // and spent in a single poll.
+// NOTE: Coinbase transaction deposits are very much an afterthought here. We treat them as
+// unconfirmed until the CB tx matures.
 fn update_coins(
     bit: &impl BitcoinInterface,
     db_conn: &mut Box<dyn DatabaseConnection>,
@@ -42,6 +44,7 @@ fn update_coins(
             outpoint,
             amount,
             address,
+            is_immature,
             ..
         } = utxo;
         // We can only really treat them if we know the derivation index that was used.
@@ -66,6 +69,7 @@ fn update_coins(
             if !curr_coins.contains_key(&utxo.outpoint) {
                 let coin = Coin {
                     outpoint,
+                    is_immature,
                     amount,
                     derivation_index,
                     is_change,

src/commands/mod.rs

@@ -50,6 +50,7 @@ pub enum CommandError {
     InvalidFeerate(/* sats/vb */ u64),
     UnknownOutpoint(bitcoin::OutPoint),
     AlreadySpent(bitcoin::OutPoint),
+    ImmatureCoinbase(bitcoin::OutPoint),
     Address(bitcoin::address::Error),
     InvalidOutputValue(bitcoin::Amount),
     InsufficientFunds(
@@ -77,6 +78,7 @@ impl fmt::Display for CommandError {
             Self::NoOutpoint => write!(f, "No provided outpoint. Need at least one."),
             Self::InvalidFeerate(sats_vb) => write!(f, "Invalid feerate: {} sats/vb.", sats_vb),
             Self::AlreadySpent(op) => write!(f, "Coin at '{}' is already spent.", op),
+            Self::ImmatureCoinbase(op) => write!(f, "Coin at '{}' is from an immature coinbase transaction.", op),
             Self::UnknownOutpoint(op) => write!(f, "Unknown outpoint '{}'.", op),
             Self::Address(e) => write!(
                 f,
@@ -298,6 +300,7 @@ impl DaemonControl {
                     block_info,
                     spend_txid,
                     spend_block,
+                    is_immature,
                     ..
                 } = coin;
                 let spend_info = spend_txid.map(|txid| LCSpendInfo {
@@ -310,6 +313,7 @@ impl DaemonControl {
                     outpoint,
                     block_height,
                     spend_info,
+                    is_immature,
                 }
             })
             .collect();
@@ -349,6 +353,10 @@ impl DaemonControl {
             if coin.is_spent() {
                 return Err(CommandError::AlreadySpent(*op));
             }
+            if coin.is_immature {
+                return Err(CommandError::ImmatureCoinbase(*op));
+            }
+
             // Fetch the transaction that created it if necessary
             if !spent_txs.contains_key(op) {
                 let tx = self
@@ -840,6 +848,8 @@ pub struct ListCoinsEntry {
     pub block_height: Option<i32>,
     /// Information about the transaction spending this coin.
     pub spend_info: Option<LCSpendInfo>,
+    /// Whether this coin was created by a coinbase transaction that is still immature.
+    pub is_immature: bool,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -976,6 +986,7 @@ mod tests {
         let mut db_conn = control.db().lock().unwrap().connection();
         db_conn.new_unspent_coins(&[Coin {
             outpoint: dummy_op,
+            is_immature: false,
             block_info: None,
             amount: bitcoin::Amount::from_sat(100_000),
             derivation_index: bip32::ChildNumber::from(13),
@@ -1081,6 +1092,7 @@ mod tests {
         };
         db_conn.new_unspent_coins(&[Coin {
             outpoint: dummy_op_dup,
+            is_immature: false,
             block_info: None,
             amount: bitcoin::Amount::from_sat(400_000),
             derivation_index: bip32::ChildNumber::from(42),
@@ -1095,6 +1107,26 @@ mod tests {
             )))
         );
 
+        // Can't create a transaction that spends an immature coinbase deposit.
+        let imma_op = bitcoin::OutPoint::from_str(
+            "4753a1d74c0af8dd0a0f3b763c14faf3bd9ed03cbdf33337a074fb0e9f6c7810:0",
+        )
+        .unwrap();
+        db_conn.new_unspent_coins(&[Coin {
+            outpoint: imma_op,
+            is_immature: true,
+            block_info: None,
+            amount: bitcoin::Amount::from_sat(100_000),
+            derivation_index: bip32::ChildNumber::from(13),
+            is_change: false,
+            spend_txid: None,
+            spend_block: None,
+        }]);
+        assert_eq!(
+            control.create_spend(&destinations, &[imma_op], 1_001),
+            Err(CommandError::ImmatureCoinbase(imma_op))
+        );
+
         ms.shutdown();
     }
 
@@ -1127,6 +1159,7 @@ mod tests {
         db_conn.new_unspent_coins(&[
             Coin {
                 outpoint: dummy_op_a,
+                is_immature: false,
                 block_info: None,
                 amount: bitcoin::Amount::from_sat(100_000),
                 derivation_index: bip32::ChildNumber::from(13),
@@ -1136,6 +1169,7 @@ mod tests {
             },
             Coin {
                 outpoint: dummy_op_b,
+                is_immature: false,
                 block_info: None,
                 amount: bitcoin::Amount::from_sat(115_680),
                 derivation_index: bip32::ChildNumber::from(34),
@@ -1303,6 +1337,7 @@ mod tests {
             // Deposit 1
             Coin {
                 is_change: false,
+                is_immature: false,
                 outpoint: OutPoint {
                     txid: deposit1.txid(),
                     vout: 0,
@@ -1316,6 +1351,7 @@ mod tests {
             // Deposit 2
             Coin {
                 is_change: false,
+                is_immature: false,
                 outpoint: OutPoint {
                     txid: deposit2.txid(),
                     vout: 0,
@@ -1329,6 +1365,7 @@ mod tests {
             // This coin is a change output.
             Coin {
                 is_change: true,
+                is_immature: false,
                 outpoint: OutPoint::new(spend_tx.txid(), 1),
                 block_info: Some(BlockInfo { height: 3, time: 3 }),
                 spend_block: None,
@@ -1339,6 +1376,7 @@ mod tests {
             // Deposit 3
             Coin {
                 is_change: false,
+                is_immature: false,
                 outpoint: OutPoint {
                     txid: deposit3.txid(),
                     vout: 0,

src/database/mod.rs

@@ -91,6 +91,9 @@ pub trait DatabaseConnection {
     fn remove_coins(&mut self, coins: &[bitcoin::OutPoint]);
 
     /// Mark a set of coins as being confirmed at a specified height and block time.
+    /// NOTE: if the coin comes from an immature coinbase transaction, this will mark it as mature.
+    /// Immature coinbase deposits must not be confirmed before they are 100 blocks deep in the
+    /// chain.
     fn confirm_coins(&mut self, outpoints: &[(bitcoin::OutPoint, i32, u32)]);
 
     /// Mark a set of coins as being spent by a specified txid of a pending transaction.
@@ -281,6 +284,7 @@ impl From<DbBlockInfo> for BlockInfo {
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct Coin {
     pub outpoint: bitcoin::OutPoint,
+    pub is_immature: bool,
     pub block_info: Option<BlockInfo>,
     pub amount: bitcoin::Amount,
     pub derivation_index: bip32::ChildNumber,
@@ -293,6 +297,7 @@ impl std::convert::From<DbCoin> for Coin {
     fn from(db_coin: DbCoin) -> Coin {
         let DbCoin {
             outpoint,
+            is_immature,
             block_info,
             amount,
             derivation_index,
@@ -303,6 +308,7 @@ impl std::convert::From<DbCoin> for Coin {
         } = db_coin;
         Coin {
             outpoint,
+            is_immature,
             block_info: block_info.map(BlockInfo::from),
             amount,
             derivation_index,