Merge #20595: Improve heuristic hex transaction decoding

0f949cde3dff15170db7930b0f7345ff995c267d Add regression test for incorrect decoding (Pieter Wuille)
39c42c442044aef611d03ee7053d2dd6df63deb7 Improve heuristic hex transaction decoding (Pieter Wuille)

Pull request description:

  The current hex tx decoding logic will refuse to decode valid extended-encoded transactions if the result fails the heuristic sanity check, even when the legacy-encoding fails. Fix this.

  Fixes #20579

ACKs for top commit:
  achow101:
    Code review ACK 0f949cde3dff15170db7930b0f7345ff995c267d
  jonatack:
    Tested ACK 0f949cde3dff15170db7930b0f7345ff995c267d
  laanwj:
    Code review ACK 0f949cde3dff15170db7930b0f7345ff995c267d

Tree-SHA512: bd6dc80d824eb9a87026a623be910cac92173f8ce1c8b040c2246348c3cf0c6d64bcc40127b859e5e4da1efe88cf02a6945f7ebb91079799395145cb09d9c7a5

1 files changed, 50 insertions, 9 deletions

diff --git a/src/core_read.cpp b/src/core_read.cpp
index a2eebbd528..7687a86185 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -126,31 +126,72 @@ static bool CheckTxScriptsSanity(const CMutableTransaction& tx)
 
 static bool DecodeTx(CMutableTransaction& tx, const std::vector<unsigned char>& tx_data, bool try_no_witness, bool try_witness)
 {
+    // General strategy:
+    // - Decode both with extended serialization (which interprets the 0x0001 tag as a marker for
+    //   the presense of witnesses) and with legacy serialization (which interprets the tag as a
+    //   0-input 1-output incomplete transaction).
+    //   - Restricted by try_no_witness (which disables legacy if false) and try_witness (which
+    //     disables extended if false).
+    //   - Ignore serializations that do not fully consume the hex string.
+    // - If neither succeeds, fail.
+    // - If only one succeeds, return that one.
+    // - If both decode attempts succeed:
+    //   - If only one passes the CheckTxScriptsSanity check, return that one.
+    //   - If neither or both pass CheckTxScriptsSanity, return the extended one.
+
+    CMutableTransaction tx_extended, tx_legacy;
+    bool ok_extended = false, ok_legacy = false;
+
+    // Try decoding with extended serialization support, and remember if the result successfully
+    // consumes the entire input.
     if (try_witness) {
         CDataStream ssData(tx_data, SER_NETWORK, PROTOCOL_VERSION);
         try {
-            ssData >> tx;
-            // If transaction looks sane, we don't try other mode even if requested
-            if (ssData.empty() && (!try_no_witness || CheckTxScriptsSanity(tx))) {
-                return true;
-            }
+            ssData >> tx_extended;
+            if (ssData.empty()) ok_extended = true;
         } catch (const std::exception&) {
             // Fall through.
         }
     }
 
+    // Optimization: if extended decoding succeeded and the result passes CheckTxScriptsSanity,
+    // don't bother decoding the other way.
+    if (ok_extended && CheckTxScriptsSanity(tx_extended)) {
+        tx = std::move(tx_extended);
+        return true;
+    }
+
+    // Try decoding with legacy serialization, and remember if the result successfully consumes the entire input.
     if (try_no_witness) {
         CDataStream ssData(tx_data, SER_NETWORK, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS);
         try {
-            ssData >> tx;
-            if (ssData.empty()) {
-                return true;
-            }
+            ssData >> tx_legacy;
+            if (ssData.empty()) ok_legacy = true;
         } catch (const std::exception&) {
             // Fall through.
         }
     }
 
+    // If legacy decoding succeeded and passes CheckTxScriptsSanity, that's our answer, as we know
+    // at this point that extended decoding either failed or doesn't pass the sanity check.
+    if (ok_legacy && CheckTxScriptsSanity(tx_legacy)) {
+        tx = std::move(tx_legacy);
+        return true;
+    }
+
+    // If extended decoding succeeded, and neither decoding passes sanity, return the extended one.
+    if (ok_extended) {
+        tx = std::move(tx_extended);
+        return true;
+    }
+
+    // If legacy decoding succeeded and extended didn't, return the legacy one.
+    if (ok_legacy) {
+        tx = std::move(tx_legacy);
+        return true;
+    }
+
+    // If none succeeded, we failed.
     return false;
 }