Merge #20612: [0.21] final rc3 backports

48134a09adef3b5302cdd6e95500db404c9ac961 doc: Update wallet database installation guide for macOS (Hennadii Stepanov)
f51e1cb2917bbd7b0966a7ad688e04fc3ce02ccf build: Use Homebrew's sqlite package if it is available (Hennadii Stepanov)
48f8929aade118469cb0014e78a15b4e71fdd17d build, refactor: Check that Homebrew's qt5 package is actually installed (Hennadii Stepanov)
96124a204193ed114ca9594df7d5151206990e91 build: Check that Homebrew's berkeley-db4 package is actually installed (Hennadii Stepanov)
61e316e66168be593fcc90b90217062fa9d993dc Don't set BDB flags when configuring without (Jonas Schnelli)
ce13b99020df8d46a9b594add3c49e38d4601b42 Add regression test for incorrect decoding (Pieter Wuille)
1caa32e3f2a74cd5700a4afe8ecf650f9020fb5c Improve heuristic hex transaction decoding (Pieter Wuille)
0d3c140c4db051fb33c2935ad9536f0f4aa2a8c5 test: add coverage for passing fee rate as a string (Jon Atack)
06c84232b310e6196c814894537ad935d773fe98 wallet, bugfix: allow send to take string fee rate values (Jon Atack)
bead93547067e4b62b44fba335f1d4697119c2d7 Send and require SENDADDRV2 before VERACK (Pieter Wuille)
9e806887a8f9ef63431b28d7dfd0470aa663dd02 Don't send 'sendaddrv2' to pre-70016 software (Pieter Wuille)

Pull request description:

ACKs for top commit:
  laanwj:
    ACK 48134a09adef3b5302cdd6e95500db404c9ac961

Tree-SHA512: 92f1199b96ab7775f88e882ec7fedf43118a4b8452d1c8d0b1cf072d8de153bbb601c7381bc1c5c80c93803c6f9942d54646e9c74e3a6703ce13854fb383fd5e

3 files changed, 71 insertions, 18 deletions

diff --git a/src/core_read.cpp b/src/core_read.cpp
index 121e62457c..fc02b1b647 100644
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -119,31 +119,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;
 }
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c649cf7757..98e3d90c2d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2364,10 +2364,16 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
             m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY));
         }
 
-        m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK));
-
         // Signal ADDRv2 support (BIP155).
-        m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2));
+        if (greatest_common_version >= 70016) {
+            // BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some
+            // implementations reject messages they don't know. As a courtesy, don't send
+            // it to nodes with a version before 70016, as no software is known to support
+            // BIP155 that doesn't announce at least that protocol version number.
+            m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::SENDADDRV2));
+        }
+
+        m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::VERACK));
 
         pfrom.nServices = nServices;
         pfrom.SetAddrLocal(addrMe);
@@ -2540,6 +2546,17 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
         return;
     }
 
+    if (msg_type == NetMsgType::SENDADDRV2) {
+        if (pfrom.fSuccessfullyConnected) {
+            // Disconnect peers that send SENDADDRV2 message after VERACK; this
+            // must be negotiated between VERSION and VERACK.
+            pfrom.fDisconnect = true;
+            return;
+        }
+        pfrom.m_wants_addrv2 = true;
+        return;
+    }
+
     if (!pfrom.fSuccessfullyConnected) {
         LogPrint(BCLog::NET, "Unsupported message \"%s\" prior to verack from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
         return;
@@ -2607,11 +2624,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
         return;
     }
 
-    if (msg_type == NetMsgType::SENDADDRV2) {
-        pfrom.m_wants_addrv2 = true;
-        return;
-    }
-
     if (msg_type == NetMsgType::SENDHEADERS) {
         LOCK(cs_main);
         State(pfrom.GetId())->fPreferHeaders = true;
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 9ba10e6c39..e8c3ae888d 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -4083,7 +4083,7 @@ static RPCHelpMan send()
                 UniValueType(), // outputs (ARR or OBJ, checked later)
                 UniValue::VNUM, // conf_target
                 UniValue::VSTR, // estimate_mode
-                UniValue::VNUM, // fee_rate
+                UniValueType(), // fee_rate, will be checked by AmountFromValue() in SetFeeEstimateMode()
                 UniValue::VOBJ, // options
                 }, true
             );