Merge #6902: fix: remove incorrect request generation with bad serialization hash

a8506ec test: add tests for `inlock` id generation (UdjinM6)
5456354 fix: trivial safety belt (UdjinM6)
58f671c refactor: trivial s/val/outpoint/ (UdjinM6)
f43be22 fix: remove incorrect request generation with bad serialization hash (Kittywhiskers Van Gogh)

Pull request description:

  ## Issue being fixed or feature implemented
  We should not be generating the hash using a CTxIn, only a COutPoint

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK a8506ec
  kwvg:
    utACK a8506ec

Tree-SHA512: ec3f8eb6700496ad42c0935435c5acae28e3616ebcde8f1d4c6f8e445524f7134451cbda76f3bed2809b422484d8eb6a0c4b02674b1668e0215b069aabcc8982

Author: PastaPastaPasta

src/instantsend/instantsend.cpp

@@ -44,7 +44,13 @@ Uint256HashSet GetIdsFromLockable(const std::vector<T>& vec)
     if (vec.empty()) return ret;
     ret.reserve(vec.size());
     for (const auto& in : vec) {
-        ret.emplace(instantsend::GenInputLockRequestId(in));
+        if constexpr (std::is_same_v<T, COutPoint>) {
+            ret.emplace(instantsend::GenInputLockRequestId(in));
+        } else if constexpr (std::is_same_v<T, CTxIn>) {
+            ret.emplace(instantsend::GenInputLockRequestId(in.prevout));
+        } else {
+            assert(false);
+        }
     }
     return ret;
 }

src/instantsend/lock.cpp

@@ -38,11 +38,8 @@ bool InstantSendLock::TriviallyValid() const
     return inputs_set.size() == inputs.size();
 }
 
-template <typename T>
-uint256 GenInputLockRequestId(const T& val)
+uint256 GenInputLockRequestId(const COutPoint& outpoint)
 {
-    return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
+    return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, outpoint));
 }
-template uint256 GenInputLockRequestId(const COutPoint& val);
-template uint256 GenInputLockRequestId(const CTxIn& val);
 } // namespace instantsend

src/instantsend/lock.h

@@ -40,8 +40,7 @@ struct InstantSendLock {
     bool TriviallyValid() const;
 };
 
-template <typename T>
-uint256 GenInputLockRequestId(const T& val);
+uint256 GenInputLockRequestId(const COutPoint& outpoint);
 
 using InstantSendLockPtr = std::shared_ptr<InstantSendLock>;
 } // namespace instantsend

src/instantsend/signing.cpp

@@ -295,7 +295,7 @@ bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroact
 
     size_t alreadyVotedCount = 0;
     for (const auto& in : tx.vin) {
-        auto id = GenInputLockRequestId(in);
+        auto id = GenInputLockRequestId(in.prevout);
         ids.emplace_back(id);
 
         uint256 otherTxHash;
@@ -344,7 +344,7 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx)
     const auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend;
 
     for (const auto& in : tx.vin) {
-        auto id = GenInputLockRequestId(in);
+        auto id = GenInputLockRequestId(in.prevout);
         if (!m_sigman.HasRecoveredSig(llmqType, id, tx.GetHash())) {
             return;
         }

src/test/evo_islock_tests.cpp

@@ -107,4 +107,106 @@ BOOST_AUTO_TEST_CASE(deserialize_instantlock_from_realdata2)
     BOOST_CHECK_EQUAL(islock.sig.Get().ToString(), expectedSignatureStr);
 }
 
+BOOST_AUTO_TEST_CASE(geninputlockrequestid_basic)
+{
+    // Test that GenInputLockRequestId generates consistent hashes for the same outpoint
+    const uint256 txHash = uint256S("0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+    const uint32_t outputIndex = 5;
+
+    COutPoint outpoint1(txHash, outputIndex);
+    COutPoint outpoint2(txHash, outputIndex);
+
+    // Same outpoints should produce identical request IDs
+    const uint256 requestId1 = instantsend::GenInputLockRequestId(outpoint1);
+    const uint256 requestId2 = instantsend::GenInputLockRequestId(outpoint2);
+
+    BOOST_CHECK(requestId1 == requestId2);
+    BOOST_CHECK(!requestId1.IsNull());
+}
+
+BOOST_AUTO_TEST_CASE(geninputlockrequestid_different_outpoints)
+{
+    // Test that different outpoints produce different request IDs
+    const uint256 txHash1 = uint256S("0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+    const uint256 txHash2 = uint256S("0xfedcba0987654321fedcba0987654321fedcba0987654321fedcba0987654321");
+
+    COutPoint outpoint1(txHash1, 0);
+    COutPoint outpoint2(txHash2, 0);
+    COutPoint outpoint3(txHash1, 1); // Same hash, different index
+
+    const uint256 requestId1 = instantsend::GenInputLockRequestId(outpoint1);
+    const uint256 requestId2 = instantsend::GenInputLockRequestId(outpoint2);
+    const uint256 requestId3 = instantsend::GenInputLockRequestId(outpoint3);
+
+    // All should be different
+    BOOST_CHECK(requestId1 != requestId2);
+    BOOST_CHECK(requestId1 != requestId3);
+    BOOST_CHECK(requestId2 != requestId3);
+}
+
+BOOST_AUTO_TEST_CASE(geninputlockrequestid_only_outpoint_matters)
+{
+    // Critical test: Verify that only the COutPoint is hashed, not scriptSig or nSequence
+    // This validates the fix where CTxIn was incorrectly used before
+    const uint256 txHash = uint256S("0xabcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890");
+    const uint32_t outputIndex = 3;
+
+    COutPoint outpoint(txHash, outputIndex);
+
+    // Create two CTxIn objects with the same prevout but different scriptSig and nSequence
+    CTxIn txin1;
+    txin1.prevout = outpoint;
+    txin1.scriptSig = CScript() << OP_1 << OP_2;
+    txin1.nSequence = 0xFFFFFFFF;
+
+    CTxIn txin2;
+    txin2.prevout = outpoint;
+    txin2.scriptSig = CScript() << OP_3 << OP_4 << OP_5; // Different scriptSig
+    txin2.nSequence = 0x12345678;                        // Different nSequence
+
+    // The request IDs should be identical because they share the same prevout (COutPoint)
+    const uint256 requestId1 = instantsend::GenInputLockRequestId(txin1.prevout);
+    const uint256 requestId2 = instantsend::GenInputLockRequestId(txin2.prevout);
+
+    BOOST_CHECK(requestId1 == requestId2);
+
+    // Also verify against the direct outpoint
+    const uint256 requestId3 = instantsend::GenInputLockRequestId(outpoint);
+    BOOST_CHECK(requestId1 == requestId3);
+}
+
+BOOST_AUTO_TEST_CASE(geninputlockrequestid_serialization_format)
+{
+    // Test that the serialization format is: SerializeHash(pair("inlock", outpoint))
+    const uint256 txHash = uint256S("0x0000000000000000000000000000000000000000000000000000000000000001");
+    const uint32_t outputIndex = 0;
+
+    COutPoint outpoint(txHash, outputIndex);
+
+    // Calculate the expected hash manually
+    const uint256 expectedHash = ::SerializeHash(std::make_pair(std::string_view("inlock"), outpoint));
+
+    // Get the actual hash from the function
+    const uint256 actualHash = instantsend::GenInputLockRequestId(outpoint);
+
+    BOOST_CHECK(actualHash == expectedHash);
+}
+
+BOOST_AUTO_TEST_CASE(geninputlockrequestid_edge_cases)
+{
+    // Test edge cases: null hash, max index
+    COutPoint nullOutpoint(uint256(), 0);
+    COutPoint maxIndexOutpoint(uint256::ONE, COutPoint::NULL_INDEX);
+
+    const uint256 nullRequestId = instantsend::GenInputLockRequestId(nullOutpoint);
+    const uint256 maxIndexRequestId = instantsend::GenInputLockRequestId(maxIndexOutpoint);
+
+    // Both should produce valid (non-null) request IDs
+    BOOST_CHECK(!nullRequestId.IsNull());
+    BOOST_CHECK(!maxIndexRequestId.IsNull());
+
+    // And they should be different from each other
+    BOOST_CHECK(nullRequestId != maxIndexRequestId);
+}
+
 BOOST_AUTO_TEST_SUITE_END()