Clarify safety of Scanner::block_number and KeyGen::keys

processor/src/key_gen.rs

@@ -134,7 +134,11 @@ impl<C: Coin, D: Db> KeyGen<C, D> {
     key: &<C::Curve as Ciphersuite>::G,
   ) -> (ThresholdKeys<Ristretto>, ThresholdKeys<C::Curve>) {
     // This is safe, despite not having a txn, since it's a static value
-    // At worst, it's not set when it's expected to be set, yet that should be handled contextually
+    // The only concern is it may not be set when expected, or it may be set unexpectedly
+    // Since this unwraps, it being unset when expected to be set will cause a panic
+    // The only other concern is if it's set when it's not safe to use
+    // The keys are only written on confirmation, and the transaction writing them is atomic to
+    // every associated operation
     KeyGenDb::<C, D>::keys(&self.db, key)
   }
 

processor/src/main.rs

@@ -183,6 +183,7 @@ async fn sign_plans<C: Coin, D: Db>(
 
   let mut block_hash = <C::Block as Block<C>>::Id::default();
   block_hash.as_mut().copy_from_slice(&context.coin_latest_finalized_block.0);
+  // block_number call is safe since it unwraps
   let block_number = substrate_mutable
     .scanner
     .block_number(&block_hash)
@@ -229,26 +230,26 @@ async fn handle_coordinator_msg<D: Db, C: Coin, Co: Coordinator>(
     let mut needed_hash = <C::Block as Block<C>>::Id::default();
     needed_hash.as_mut().copy_from_slice(&block_hash.0);
 
-    let block_number;
-    loop {
+    let block_number = loop {
       // Ensure our scanner has scanned this block, which means our daemon has this block at
       // a sufficient depth
-      let Some(block_number_inner) = scanner.block_number(&needed_hash).await else {
-      warn!(
-        "node is desynced. we haven't scanned {} which should happen after {} confirms",
-        hex::encode(&needed_hash),
-        C::CONFIRMATIONS,
-      );
-      sleep(Duration::from_secs(10)).await;
-      continue;
+      // The block_number may be set even if scanning isn't complete
+      let Some(block_number) = scanner.block_number(&needed_hash).await else {
+        warn!(
+          "node is desynced. we haven't scanned {} which should happen after {} confirms",
+          hex::encode(&needed_hash),
+          C::CONFIRMATIONS,
+        );
+        sleep(Duration::from_secs(10)).await;
+        continue;
+      };
+      break block_number;
     };
-      block_number = block_number_inner;
-      break;
-    }
 
     // While the scanner has cemented this block, that doesn't mean it's been scanned for all
     // keys
     // ram_scanned will return the lowest scanned block number out of all keys
+    // This is a safe call which fulfills the unfulfilled safety requirements from the prior call
     while scanner.ram_scanned().await < block_number {
       sleep(Duration::from_secs(1)).await;
     }
@@ -313,6 +314,7 @@ async fn handle_coordinator_msg<D: Db, C: Coin, Co: Coordinator>(
 
           let mut activation_block_hash = <C::Block as Block<C>>::Id::default();
           activation_block_hash.as_mut().copy_from_slice(&activation_block.0);
+          // This block_number call is safe since it unwraps
           let activation_number = substrate_mutable
             .scanner
             .block_number(&activation_block_hash)

processor/src/scanner.rs

@@ -288,9 +288,10 @@ impl<C: Coin, D: Db> ScannerHandle<C, D> {
     scanner.keys.push(key);
   }
 
+  // This perform a database read which isn't safe with regards to if the value is set or not
+  // It may be set, when it isn't expected to be set, or not set, when it is expected to be set
+  // Since the value is static, if it's set, it's correctly set
   pub async fn block_number(&self, id: &<C::Block as Block<C>>::Id) -> Option<usize> {
-    // This is safe, despite not having a txn, since it's a static value
-    // At worst, it's not set when it's expected to be set, yet that should be handled contextually
     ScannerDb::<C, D>::block_number(&self.scanner.read().await.db, id)
   }
 
@@ -405,6 +406,9 @@ impl<C: Coin, D: Db> Scanner<C, D> {
             let block_id = block.id();
 
             // These block calls are safe, despite not having a txn, since they're static values
+            // only written to/read by this thread
+            // There's also no error caused by them being unexpectedly written (if the commit is
+            // made and then the processor suddenly reboots)
             if let Some(id) = ScannerDb::<C, D>::block(&scanner.db, i) {
               if id != block_id {
                 panic!("reorg'd from finalized {} to {}", hex::encode(id), hex::encode(block_id));