Merge #357: Batch the exchange of signatures with the watchtowers

003a308 ci: remove most of the cache across runs (Antoine Poinsot)
67f4fc2 tests/servers: update miradord to latest master (Antoine Poinsot)
662a1bb commands, sigfetcher: batch the rev signatures exchange with watchtowers (Antoine Poinsot)

Pull request description:

  As per revault/practical-revault#115.
  This greatly simplifies the implementation, too!

ACKs for top commit:
  darosior:
    self-ACK 003a308

Tree-SHA512: 96351256bd06775498133d28ec68527f7e724378e2f0c3c4c8a044f47d3429a44c5291ba8bf31cdbcc28f2a1fca51e1d8420919be9b3c853298beea573237466

Author: darosior

.cirrus.yml

@@ -28,21 +28,6 @@ task:
     folder: $CARGO_HOME/registry
     fingerprint_script: cat Cargo.lock
   before_cache_script: rm -rf $CARGO_HOME/registry/index
-  target_cache:
-    folder: target
-    fingerprint_script:
-      - rustc --version
-      - cat Cargo.lock
-  bitcoind_cache:
-    folder: $BITCOIND_DIR_NAME
-  coordinatord_cache:
-    folder: tests/servers/coordinatord
-    fingerprint_script:
-      - cd tests/servers/coordinatord && git rev-parse HEAD
-  cosignerd_cache:
-    folder: tests/servers/cosignerd
-    fingerprint_script:
-      - cd tests/servers/cosignerd && git rev-parse HEAD
 
   test_script: |
     set -xe

Cargo.lock

@@ -14,8 +14,7 @@ use crate::{
     communication::{
         announce_spend_transaction, check_spend_transaction_size, coord_share_rev_signatures,
         coordinator_status, cosigners_status, fetch_cosigs_signatures, share_unvault_signatures,
-        watchtowers_status, wts_share_emer_signatures, wts_share_second_stage_signatures,
-        CommunicationError,
+        watchtowers_status, wts_share_rev_signatures, CommunicationError,
     },
     database::{
         actions::{
@@ -503,19 +502,40 @@ impl DaemonControl {
             .expect("The database must be available");
         db_mark_securing_vault(&db_path, db_vault.id).expect("The database must be available");
 
-        // If this made the Emergency fully signed and we are a stakeholder, share
-        // it with our watchtowers.
-        let emer_db_tx = db_emer_transaction(&db_path, db_vault.id)
-            .expect("The database must be available")
+        // Now, check whether this made all revocation transactions fully signed
+        let emer_tx = db_emer_transaction(&db_path, db_vault.id)
+            .expect("Database must be available")
+            .ok_or(CommandError::Race)?;
+        let cancel_tx = db_cancel_transaction(&db_path, db_vault.id)
+            .expect("Database must be available")
             .ok_or(CommandError::Race)?;
-        if !db_vault.emer_shared && emer_db_tx.psbt.unwrap_emer().is_finalizable(secp_ctx) {
+        let unemer_tx = db_unvault_emer_transaction(&db_path, db_vault.id)
+            .expect("Database must be available")
+            .ok_or(CommandError::Race)?;
+        let all_rev_fully_signed = emer_tx
+            .psbt
+            .unwrap_emer()
+            .is_finalizable(&revaultd.secp_ctx)
+            && cancel_tx
+                .psbt
+                .unwrap_cancel()
+                .is_finalizable(&revaultd.secp_ctx)
+            && unemer_tx
+                .psbt
+                .unwrap_unvault_emer()
+                .is_finalizable(&revaultd.secp_ctx);
+
+        // If it did, share their signatures with our watchtowers
+        if all_rev_fully_signed {
             if let Some(ref watchtowers) = revaultd.watchtowers {
-                wts_share_emer_signatures(
+                wts_share_rev_signatures(
                     &revaultd.noise_secret,
                     &watchtowers,
                     db_vault.deposit_outpoint,
                     db_vault.derivation_index,
-                    &emer_db_tx,
+                    &emer_tx,
+                    &cancel_tx,
+                    &unemer_tx,
                 )?;
             }
         }
@@ -675,25 +695,6 @@ impl DaemonControl {
                 })?;
         }
 
-        // The watchtower(s) MUST have our second-stage (UnvaultEmer, Cancel) signatures
-        // before we share the Unvault one.
-        if let Some(ref watchtowers) = revaultd.watchtowers {
-            let unemer_db_tx = db_unvault_emer_transaction(&db_path, db_vault.id)
-                .expect("The database must be available")
-                .ok_or(CommandError::Race)?;
-            let cancel_db_tx = db_cancel_transaction(&db_path, db_vault.id)
-                .expect("The database must be available")
-                .ok_or(CommandError::Race)?;
-            wts_share_second_stage_signatures(
-                &revaultd.noise_secret,
-                &watchtowers,
-                db_vault.deposit_outpoint,
-                db_vault.derivation_index,
-                &cancel_db_tx,
-                &unemer_db_tx,
-            )?;
-        }
-
         // Sanity checks passed. Store it then share it.
         db_update_presigned_txs(&db_path, &db_vault, vec![unvault_db_tx.clone()], secp_ctx)
             .expect("The database must be available");

src/commands/mod.rs

@@ -87,24 +87,29 @@ impl From<revault_net::Error> for CommunicationError {
     }
 }
 
-// Send a `sig` (https://github.com/revault/practical-revault/blob/master/messages.md#sig)
+// Send a `sigs` (https://github.com/revault/practical-revault/blob/master/messages.md#sigs)
 // message to a watchtower.
-fn send_wt_sig_msg(
+fn send_wt_sigs_msg(
     transport: &mut KKTransport,
     deposit_outpoint: OutPoint,
     derivation_index: ChildNumber,
-    txid: Txid,
-    signatures: BTreeMap<secp256k1::PublicKey, secp256k1::Signature>,
+    emer_tx: &DbTransaction,
+    cancel_tx: &DbTransaction,
+    unemer_tx: &DbTransaction,
 ) -> Result<(), CommunicationError> {
-    let sig_msg = watchtower::Sig {
+    let signatures = watchtower::Signatures {
+        emergency: emer_tx.psbt.signatures(),
+        cancel: cancel_tx.psbt.signatures(),
+        unvault_emergency: unemer_tx.psbt.signatures(),
+    };
+    let sig_msg = watchtower::Sigs {
         signatures,
-        txid,
         deposit_outpoint,
         derivation_index,
     };
 
     log::debug!("Sending signatures to watchtower: '{:?}'", sig_msg);
-    let sig_result: watchtower::SigResult = transport.send_req(&sig_msg.into())?;
+    let sig_result: watchtower::SigsResult = transport.send_req(&sig_msg.into())?;
     log::debug!(
         "Got response to signatures for '{}' from watchtower: '{:?}'",
         deposit_outpoint,
@@ -149,63 +154,26 @@ pub fn send_coord_sig_msg(
     Ok(())
 }
 
-/// Send the signatures for the Emergency transaction to the Watchtower.
-/// Only sharing the Emergency signature tells the watchtower to be aware of, but
-/// not watch, the vault. Later sharing the UnvaultEmergency and Cancel signatures
-/// will trigger it to watch it.
-// FIXME: better to share all signatures early and to then have another message
-// asking permission before delegating.
-pub fn wts_share_emer_signatures(
+/// Share the revocation transactions' signatures with all our watchtowers.
+pub fn wts_share_rev_signatures(
     noise_secret: &revault_net::noise::SecretKey,
     watchtowers: &[(std::net::SocketAddr, revault_net::noise::PublicKey)],
     deposit_outpoint: OutPoint,
     derivation_index: ChildNumber,
     emer_tx: &DbTransaction,
-) -> Result<(), CommunicationError> {
-    for (wt_host, wt_noisekey) in watchtowers {
-        let mut transport = KKTransport::connect(*wt_host, noise_secret, wt_noisekey)?;
-
-        send_wt_sig_msg(
-            &mut transport,
-            deposit_outpoint,
-            derivation_index,
-            emer_tx.psbt.txid(),
-            emer_tx.psbt.signatures(),
-        )?;
-    }
-
-    Ok(())
-}
-
-/// Send the signatures for the UnvaultEmergency and Cancel transactions to all
-/// our watchtowers. This serves as a signal for watchtowers to start watching for
-/// this vault and they may therefore NACK that we delegate it (eg if they don't
-/// have enough fee reserve).
-pub fn wts_share_second_stage_signatures(
-    noise_secret: &revault_net::noise::SecretKey,
-    watchtowers: &[(std::net::SocketAddr, revault_net::noise::PublicKey)],
-    deposit_outpoint: OutPoint,
-    derivation_index: ChildNumber,
     cancel_tx: &DbTransaction,
-    unvault_emer_tx: &DbTransaction,
+    unemer_tx: &DbTransaction,
 ) -> Result<(), CommunicationError> {
     for (wt_host, wt_noisekey) in watchtowers {
         let mut transport = KKTransport::connect(*wt_host, noise_secret, wt_noisekey)?;
 
-        // The watchtower enforces a specific sequence in which to exchange transactions
-        send_wt_sig_msg(
-            &mut transport,
-            deposit_outpoint,
-            derivation_index,
-            unvault_emer_tx.psbt.txid(),
-            unvault_emer_tx.psbt.signatures(),
-        )?;
-        send_wt_sig_msg(
+        send_wt_sigs_msg(
             &mut transport,
             deposit_outpoint,
             derivation_index,
-            cancel_tx.psbt.txid(),
-            cancel_tx.psbt.signatures(),
+            emer_tx,
+            cancel_tx,
+            unemer_tx,
         )?;
     }
 

src/communication.rs

@@ -262,9 +262,9 @@ pub fn db_insert_new_unconfirmed_vault(
         tx.execute(
             "INSERT INTO vaults ( \
                 wallet_id, status, blockheight, deposit_txid, deposit_vout, amount, derivation_index, \
-                funded_at, secured_at, delegated_at, moved_at, final_txid, emer_shared \
+                funded_at, secured_at, delegated_at, moved_at, final_txid \
             ) \
-             VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, NULL, NULL, NULL, NULL, NULL, 0)",
+             VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, NULL, NULL, NULL, NULL, NULL)",
             params![
                 wallet_id,
                 VaultStatus::Unconfirmed as u32,
@@ -657,18 +657,6 @@ pub fn db_mark_activating_vault(db_path: &Path, vault_id: u32) -> Result<(), Dat
     })
 }
 
-/// Mark a vault as having its Emergency signature already shared with the watchtowers.
-pub fn db_mark_emer_shared(db_path: &Path, db_vault: &DbVault) -> Result<(), DatabaseError> {
-    db_exec(db_path, |tx| {
-        tx.execute(
-            "UPDATE vaults SET emer_shared = 1 WHERE vaults.id = (?1)",
-            params![db_vault.id],
-        )
-        .map(|_| ())
-        .map_err(DatabaseError::from)
-    })
-}
-
 // Merge the partial sigs of two transactions of the same type into the first one
 //
 // Returns true if this made the transaction "valid" (fully signed).
@@ -1431,21 +1419,6 @@ mod test {
         assert!(db_signed_emer_txs(&db_path).unwrap().is_empty());
         assert_eq!(db_signed_unemer_txs(&db_path).unwrap().len(), 1);
 
-        // Sanity check we can mark the Emergency as shared with the watchtowers
-        assert!(
-            !db_vault_by_deposit(&db_path, &db_vault.deposit_outpoint)
-                .unwrap()
-                .unwrap()
-                .emer_shared
-        );
-        db_mark_emer_shared(&db_path, &db_vault).unwrap();
-        assert!(
-            db_vault_by_deposit(&db_path, &db_vault.deposit_outpoint)
-                .unwrap()
-                .unwrap()
-                .emer_shared
-        );
-
         fs::remove_dir_all(&datadir).unwrap_or_else(|_| ());
     }
 

src/database/actions.rs

@@ -234,7 +234,6 @@ impl TryFrom<&Row<'_>> for DbVault {
         let final_txid = row
             .get::<_, Option<Vec<u8>>>(12)?
             .map(|raw_txid| encode::deserialize(&raw_txid).expect("We only store valid txids"));
-        let emer_shared: bool = row.get(13)?;
 
         Ok(DbVault {
             id,
@@ -249,7 +248,6 @@ impl TryFrom<&Row<'_>> for DbVault {
             delegated_at,
             moved_at,
             final_txid,
-            emer_shared,
         })
     }
 }
@@ -321,7 +319,7 @@ pub fn db_unvaulted_vaults(
         ],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let unvault_tx: Vec<u8> = row.get(14)?;
+            let unvault_tx: Vec<u8> = row.get(13)?;
             let unvault_tx = UnvaultTransaction::from_psbt_serialized(&unvault_tx)
                 .expect("We store it with as_psbt_serialized");
 
@@ -345,7 +343,7 @@ pub fn db_spending_vaults(
         ],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let unvault_tx: Vec<u8> = row.get(14)?;
+            let unvault_tx: Vec<u8> = row.get(13)?;
             let unvault_tx = UnvaultTransaction::from_psbt_serialized(&unvault_tx)
                 .expect("We store it with as_psbt_serialized");
 
@@ -370,7 +368,7 @@ pub fn db_canceling_vaults(
         ],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let cancel_tx: Vec<u8> = row.get(14)?;
+            let cancel_tx: Vec<u8> = row.get(13)?;
             let cancel_tx = CancelTransaction::from_psbt_serialized(&cancel_tx)
                 .expect("We store it with as_psbt_serialized");
 
@@ -395,7 +393,7 @@ pub fn db_emering_vaults(
         ],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let emer_tx: Vec<u8> = row.get(14)?;
+            let emer_tx: Vec<u8> = row.get(13)?;
             let emer_tx = EmergencyTransaction::from_psbt_serialized(&emer_tx)
                 .expect("We store it with to_psbt_serialized");
 
@@ -420,7 +418,7 @@ pub fn db_unemering_vaults(
         ],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let unemer_tx: Vec<u8> = row.get(14)?;
+            let unemer_tx: Vec<u8> = row.get(13)?;
             let unemer_tx = UnvaultEmergencyTransaction::from_psbt_serialized(&unemer_tx)
                 .expect("We store it with to_psbt_serialized");
 
@@ -626,7 +624,7 @@ pub fn db_vault_by_unvault_txid(
         params![txid.to_vec(), TransactionType::Unvault as u32],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let offset = 14;
+            let offset = 13;
 
             // FIXME: there is probably a more extensible way to implement the from()s so we don't
             // have to change all those when adding a column
@@ -669,7 +667,7 @@ pub fn db_sig_missing(
         ],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let db_tx: DbTransaction = db_tx_from_row(row, 14)?;
+            let db_tx: DbTransaction = db_tx_from_row(row, 13)?;
 
             if let Some(db_txs) = vault_map.get_mut(&db_vault) {
                 db_txs.push(db_tx);
@@ -841,7 +839,7 @@ pub fn db_vaults_from_spend(
         params![spend_txid.to_vec()],
         |row| {
             let db_vault: DbVault = row.try_into()?;
-            let txid: Txid = encode::deserialize(&row.get::<_, Vec<u8>>(14)?).expect("We store it");
+            let txid: Txid = encode::deserialize(&row.get::<_, Vec<u8>>(13)?).expect("We store it");
             db_vaults.insert(txid, db_vault);
             Ok(())
         },

src/database/interface.rs

@@ -45,8 +45,6 @@ CREATE TABLE wallets (
  * spending txid or the canceling txid out of a deposit outpoint.
  * It MUST be NOT NULL if status is 'spending', 'spent', 'canceling'
  * or 'canceled'.
- * The emer_shared field indicates wether we already shared the Emergency
- * transaction for this vault with the watchtower.
  */
 CREATE TABLE vaults (
     id INTEGER PRIMARY KEY NOT NULL,
@@ -62,7 +60,6 @@ CREATE TABLE vaults (
     delegated_at INTEGER,
     moved_at INTEGER,
     final_txid BLOB,
-    emer_shared BOOLEAN NOT NULL CHECK (emer_shared IN (0,1)),
     FOREIGN KEY (wallet_id) REFERENCES wallets (id)
         ON UPDATE RESTRICT
         ON DELETE RESTRICT
@@ -151,7 +148,6 @@ pub struct DbVault {
     pub delegated_at: Option<u32>,
     pub moved_at: Option<u32>,
     pub final_txid: Option<Txid>,
-    pub emer_shared: bool,
 }
 
 // FIXME: naming it "db transaction" was ambiguous..