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

.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

src/commands/mod.rs

@@ -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/communication.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/database/actions.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/interface.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/schema.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..