Merge rust-bitcoin/rust-bitcoin#897: Check for SIGHASH_SINGLE bug in writer fn

83dda74ecb Check for SIGHASH_SINGLE bug in writer fn (Tobin Harding)

Pull request description:

  Recently we moved the logic for checking for the SIGHASH_SINGLE bug to
  the `signature_hash()` function. Although this left users of the
  `encode_signing_data_to()` function without correct handling of the bug
  there is not much else we can do but alert users to this behaviour.

  Add documentation to highlight the behaviour of `encdoe_signing_data_to`
  in regards to the sighash single bug. Requires updating docs for
  `signature_hash` also.

  Please note, uses non-conventional markdown header `# Warning`.

  Closes: #817

ACKs for top commit:
  sanket1729:
    ACK 83dda74ecb. This is much cleaner
  dr-orlovsky:
    ACK 83dda74ecb
  apoelstra:
    ACK 83dda74ecb

Tree-SHA512: 1263b06ddfbb05a293c80e7dbf6f87eac5922c501e7db1c1d26d41d3ea0172c6b7a44afc0b1843b06e78985d3ecf70a3a3feb2515d535a7413685aed0a338c64

src/blockdata/transaction.rs

@@ -323,12 +323,16 @@ impl Transaction {
     /// this is the general (and hard) part.
     ///
     /// The `sighash_type` supports an arbitrary `u32` value, instead of just [`EcdsaSigHashType`],
-    /// because internally 4 bytes are being hashed, even though only lowest byte is appended to
+    /// because internally 4 bytes are being hashed, even though only the lowest byte is appended to
     /// signature in a transaction.
     ///
-    /// **Warning**: This does NOT attempt to support OP_CODESEPARATOR. In general this would
-    /// require evaluating `script_pubkey` to determine which separators get evaluated and which
-    /// don't, which we don't have the information to determine.
+    /// # Warning
+    ///
+    /// - Does NOT attempt to support OP_CODESEPARATOR. In general this would require evaluating
+    /// `script_pubkey` to determine which separators get evaluated and which don't, which we don't
+    /// have the information to determine.
+    /// - Does NOT handle the sighash single bug, you should either handle that manually or use
+    /// [`Self::signature_hash()`] instead.
     ///
     /// # Panics
     ///
@@ -343,6 +347,15 @@ impl Transaction {
         let sighash_type: u32 = sighash_type.into();
         assert!(input_index < self.input.len());  // Panic on OOB
 
+        if self.is_invalid_use_of_sighash_single(sighash_type, input_index) {
+            // We cannot correctly handle the SIGHASH_SINGLE bug here because usage of this function
+            // will result in the data written to the writer being hashed, however the correct
+            // handling of the SIGHASH_SINGLE bug is to return the 'one array' - either implement
+            // this behaviour manually or use `signature_hash()`.
+            writer.write(b"[not a transaction] SIGHASH_SINGLE bug")?;
+            return Ok(())
+        }
+
         let (sighash, anyone_can_pay) = EcdsaSigHashType::from_u32_consensus(sighash_type).split_anyonecanpay_flag();
 
         // Build tx to sign
@@ -393,7 +406,27 @@ impl Transaction {
 
     /// Computes a signature hash for a given input index with a given sighash flag.
     ///
-    /// Please refer [`Self::encode_signing_data_to`] for full documentation of this method.
+    /// To actually produce a scriptSig, this hash needs to be run through an ECDSA signer, the
+    /// [`EcdsaSigHashType`] appended to the resulting sig, and a script written around this, but
+    /// this is the general (and hard) part.
+    ///
+    /// The `sighash_type` supports an arbitrary `u32` value, instead of just [`EcdsaSigHashType`],
+    /// because internally 4 bytes are being hashed, even though only the lowest byte is appended to
+    /// signature in a transaction.
+    ///
+    /// This function correctly handles the sighash single bug by returning the 'one array'. The
+    /// sighash single bug becomes exploitable when one tries to sign a transaction with
+    /// `SIGHASH_SINGLE` and there is not a corresponding output with the same index as the input.
+    ///
+    /// # Warning
+    ///
+    /// Does NOT attempt to support OP_CODESEPARATOR. In general this would require evaluating
+    /// `script_pubkey` to determine which separators get evaluated and which don't, which we don't
+    /// have the information to determine.
+    ///
+    /// # Panics
+    ///
+    /// If `input_index` is out of bounds (greater than or equal to `self.input.len()`).
     pub fn signature_hash(
         &self,
         input_index: usize,
@@ -410,9 +443,6 @@ impl Transaction {
         SigHash::from_engine(engine)
     }
 
-    /// The SIGHASH_SINGLE bug becomes exploitable when one tries to sign a transaction with
-    /// SIGHASH_SINGLE and there is not a corresponding output transaction with the same index as
-    /// the input transaction.
     fn is_invalid_use_of_sighash_single(&self, sighash: u32, input_index: usize) -> bool {
         let ty = EcdsaSigHashType::from_u32_consensus(sighash);
         ty == EcdsaSigHashType::Single && input_index >= self.output.len()