fix: remove incorrect request generation with bad serialization hash by UdjinM6 · Pull Request #6902 · dashpay/dash

UdjinM6 · 2025-10-19T17:15:40Z

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)

github-actions · 2025-10-19T17:16:09Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-10-19T17:19:44Z

Walkthrough

The templated GenInputLockRequestId was replaced with a concrete overload uint256 GenInputLockRequestId(const COutPoint&). Call sites that previously passed CTxIn now pass in.prevout (handled in GetIdsFromLockable with a branch for T == CTxIn). The generated ID is produced by serializing the pair (INPUTLOCK_REQUESTID_PREFIX, outpoint). Related signing code (TrySignInputLocks, TrySignInstantSendLock) was updated to use the outpoint-based ID. Unit tests were added to validate determinism, uniqueness, serialization format, and edge cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "fix: remove incorrect request generation with bad serialization hash" directly aligns with the primary change in the changeset. The raw summary confirms that the changes replace a generic template `GenInputLockRequestId<T>` with a concrete `COutPoint` overload, removing the ability to generate request IDs using `CTxIn`. This is exactly what the title describes as "removing incorrect request generation" with "bad serialization hash." The title is concise, clear, and specific enough that a developer scanning history would understand this addresses the request ID generation logic.
Description Check	✅ Passed	The PR description clearly states the issue being fixed: "We should not be generating the hash using a CTxIn, only a COutPoint." This issue statement is directly related to the changeset, which replaces the generic template accepting any type with a concrete overload accepting only `COutPoint`, and removes `CTxIn` support. Although the "What was done?" and "How Has This Been Tested?" sections are left blank, the issue statement is specific and directly corresponds to the changes, meeting the requirement that the description be "related in some way to the changeset" regardless of detail level.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/instantsend/lock.cpp (1)

41-49: Consider adding result caching if called frequently.

If these functions are called repeatedly with the same inputs, consider implementing result caching using unordered_lru_cache as mentioned in the coding guidelines. This would improve performance by avoiding redundant hash computations.

Based on coding guidelines.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 764893d and 53f59c0adf681e8179dcd9d4a6496f80191105e9.

📒 Files selected for processing (2)

src/instantsend/lock.cpp (1 hunks)
src/instantsend/lock.h (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/instantsend/lock.cpp
src/instantsend/lock.h

🧬 Code graph analysis (1)

src/instantsend/lock.h (1)

src/instantsend/lock.cpp (4)

GenInputLockRequestId (41-44)

GenInputLockRequestId (41-41)

GenInputLockRequestId (46-49)

GenInputLockRequestId (46-46)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: mac-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: Lint / Run linters

🔇 Additional comments (4)

src/instantsend/lock.h (2)

17-17: LGTM!

The forward declaration for CTxIn is necessary and correct since the function declaration at line 45 requires it.

44-45: All call sites work correctly with the new overloads and consistency is properly ensured.

The function overloads at all four call sites (instantsend.cpp:47, signing.cpp:114, signing.cpp:298, signing.cpp:347) correctly resolve to the appropriate overload—COutPoint calls use overload 1, CTxIn calls use overload 2. Both implementations produce identical hashes when txin.prevout == outpoint, fixing the consistency issue.

Regarding "cache it properly": the GenInputLockRequestId functions are lightweight hash operations that don't require result caching. The phrase likely refers to using the consistent request IDs as cache keys elsewhere in the codebase (as seen in src/instantsend/db.h:42 and similar patterns).

src/instantsend/lock.cpp (2)

41-44: LGTM!

The implementation correctly serializes the prefix and outpoint to produce a consistent request ID.

46-49: Correct use of txin.prevout ensures hash consistency.

The critical fix here is using txin.prevout instead of serializing the entire CTxIn object. This ensures that calling GenInputLockRequestId with a COutPoint or with a CTxIn that references the same input produces identical hashes, fixing the cache consistency issue mentioned in the PR title.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 53f59c0adf681e8179dcd9d4a6496f80191105e9 and c6e16240653c9ba061430275999a1708fcee8024.

📒 Files selected for processing (1)

src/instantsend/lock.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/instantsend/lock.cpp

🪛 GitHub Actions: Clang Diff Format Check

src/instantsend/lock.cpp

[error] 1-1: Clang format differences detected. Run the clang-format-diff.py pipeline step to fix formatting changes; the process exited with code 1.

🔇 Additional comments (2)

src/instantsend/lock.cpp (2)

41-44: LGTM! Correct base implementation for COutPoint hashing.

The implementation properly serializes the prefix and outpoint together to generate a consistent hash.

46-49: Excellent fix! Ensures consistent hashing across both input types.

By delegating to the COutPoint overload via txin.prevout, this guarantees that both CTxIn and COutPoint produce identical hashes when referring to the same outpoint. This resolves the issue where the previous template implementation would serialize the entire CTxIn object instead of just its prevout field.

src/instantsend/lock.cpp

We should not be generating the hash using a CTxIn, only a COutPoint

kwvg

Consider f43be22 as there's only one call where we pass a CTxIn and this would be more explicit in conveying the constraint

coderabbitai

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/instantsend/lock.cpp (1)
41-43: Clang-format issue flagged.

As noted in previous review comments, this file has formatting differences detected by the pipeline. Please run the clang-format-diff tool before merging:
git diff -U0 develop...HEAD | ./contrib/devtools/clang-format-diff.py -p1 -i -v

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f43be22 and 58f671c.

📒 Files selected for processing (2)

src/instantsend/lock.cpp (1 hunks)
src/instantsend/lock.h (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/instantsend/lock.cpp
src/instantsend/lock.h

🧬 Code graph analysis (2)

src/instantsend/lock.cpp (2)

src/rpc/rawtransaction.cpp (1)

outpoint (485-485)

src/wallet/wallet.cpp (1)

outpoint (1065-1065)

src/instantsend/lock.h (1)

src/instantsend/lock.cpp (2)

GenInputLockRequestId (41-44)

GenInputLockRequestId (41-41)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: mac-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: Lint / Run linters

🔇 Additional comments (1)

src/instantsend/lock.h (1)

43-43: LGTM: Signature correctly restricts input to COutPoint.

The change from a template to a concrete overload ensures only COutPoint can be passed, preventing the previous bug where CTxIn (including scriptSig and nSequence) was serialized into the hash.

src/instantsend/lock.cpp

kwvg

utACK 58f671c

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 58f671c and 5a878bc83e634ae3be49bf3e57154429d1d6be42.

📒 Files selected for processing (2)

src/instantsend/instantsend.cpp (1 hunks)
src/test/evo_islock_tests.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/test/evo_islock_tests.cpp
src/instantsend/instantsend.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/evo_islock_tests.cpp

🧬 Code graph analysis (2)

src/test/evo_islock_tests.cpp (1)

src/instantsend/lock.cpp (2)

GenInputLockRequestId (41-44)

GenInputLockRequestId (41-41)

src/instantsend/instantsend.cpp (1)

src/instantsend/lock.cpp (2)

GenInputLockRequestId (41-44)

GenInputLockRequestId (41-41)

🪛 GitHub Actions: Clang Diff Format Check

src/test/evo_islock_tests.cpp

[error] 162-162: Clang format differences found by clang-format-diff.py; Process completed with exit code 1.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: linux64_nowallet-build / Build source
GitHub Check: mac-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: Lint / Run linters

🔇 Additional comments (6)

src/instantsend/instantsend.cpp (1)

47-53: LGTM! Critical fix correctly implemented.

The branching logic ensures that only the COutPoint (hash and index) is used to generate the request ID, regardless of whether a CTxIn or COutPoint is passed. This correctly fixes the issue where scriptSig and nSequence from CTxIn were incorrectly influencing the hash, causing different transactions spending the same UTXO to generate different request IDs.

src/test/evo_islock_tests.cpp (5)

110-125: LGTM! Determinism test is solid.

This test correctly validates that identical outpoints produce consistent, non-null request IDs.

127-145: LGTM! Uniqueness test covers key scenarios.

This test correctly validates that different outpoints (varying by hash or index) produce distinct request IDs.

147-176: Excellent! Critical test validates the core fix.

This test directly validates the PR objective by confirming that scriptSig and nSequence from CTxIn do not influence the request ID—only the COutPoint (prevout) matters. This ensures different transactions spending the same UTXO generate identical request IDs, which is essential for InstantSend locking.

178-193: LGTM! Serialization format verification is thorough.

This test ensures the request ID is computed using the correct serialization format: SerializeHash(pair("inlock", outpoint)), matching the implementation in src/instantsend/lock.cpp.

195-210: LGTM! Edge case coverage is appropriate.

This test validates that boundary conditions (null hash, max index) produce valid and distinct request IDs.

src/test/evo_islock_tests.cpp

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/test/evo_islock_tests.cpp (1)

162-162: Fix formatting issue flagged by clang-format.

This line has a clang-format discrepancy as previously noted. Run clang-format --style=file -i src/test/evo_islock_tests.cpp to resolve.

🧹 Nitpick comments (1)

src/test/evo_islock_tests.cpp (1)
178-193: Consider using INPUTLOCK_REQUESTID_PREFIX constant.

Line 187 hardcodes "inlock" instead of using the INPUTLOCK_REQUESTID_PREFIX constant from the implementation. Using the constant ensures the test stays in sync with the implementation if the prefix changes.

Apply this diff:
-    const uint256 expectedHash = ::SerializeHash(std::make_pair(std::string_view("inlock"), outpoint));
+    const uint256 expectedHash = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, outpoint));
Note: You may need to add using instantsend::INPUTLOCK_REQUESTID_PREFIX; or use the fully qualified name if the constant is not in scope.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a878bc83e634ae3be49bf3e57154429d1d6be42 and a8506ec.

📒 Files selected for processing (1)

src/test/evo_islock_tests.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/test/evo_islock_tests.cpp

src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests

Files:

src/test/evo_islock_tests.cpp

🧬 Code graph analysis (1)

src/test/evo_islock_tests.cpp (1)

src/instantsend/lock.cpp (2)

GenInputLockRequestId (41-44)

GenInputLockRequestId (41-41)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: mac-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: Lint / Run linters

🔇 Additional comments (4)

src/test/evo_islock_tests.cpp (4)

110-125: Good basic test coverage.

This test appropriately validates determinism and non-null output for identical outpoints.

127-145: Excellent uniqueness validation.

The test properly validates that different outpoints (varying by hash or index) produce distinct request IDs.

147-176: Critical test that validates the fix.

This test directly addresses the bug that was fixed by ensuring only the COutPoint (prevout) influences the request ID, not the scriptSig or nSequence fields from CTxIn. Excellent validation.

195-210: Good edge case coverage.

The test appropriately validates behavior for boundary conditions (null hash and max index), ensuring both produce valid, distinct request IDs.

knst · 2025-10-20T13:25:46Z

src/instantsend/instantsend.cpp

+        } else if constexpr (std::is_same_v<T, CTxIn>) {
+            ret.emplace(instantsend::GenInputLockRequestId(in.prevout));
+        } else {
+            assert(false);


it should be static_assert here, because type is known on compilation time here

Is the assert necessary? requires already constrains types, more restrictive than open ended template.

PastaPastaPasta

utACK a8506ec

kwvg

utACK a8506ec

knst

LGTM overall, see nit about static_assert

UdjinM6 added this to the 23 milestone Oct 19, 2025

UdjinM6 changed the title ~~fix: GenInputLockRequestId should cache it properly for CTxIn/COutPoint to get the same result in both cases~~ fix: GenInputLockRequestId should hash it properly for CTxIn/COutPoint to get the same result in both cases Oct 19, 2025

UdjinM6 force-pushed the fix_inlock_id branch from 0776153 to 53f59c0 Compare October 19, 2025 17:16

coderabbitai bot reviewed Oct 19, 2025

View reviewed changes

UdjinM6 force-pushed the fix_inlock_id branch from c6e1624 to 135bb5b Compare October 19, 2025 18:10

coderabbitai bot reviewed Oct 19, 2025

View reviewed changes

src/instantsend/lock.cpp Outdated Show resolved Hide resolved

UdjinM6 requested review from PastaPastaPasta and kwvg October 19, 2025 19:48

fix: remove incorrect request generation with bad serialization hash

f43be22

We should not be generating the hash using a CTxIn, only a COutPoint

kwvg reviewed Oct 19, 2025

View reviewed changes

UdjinM6 force-pushed the fix_inlock_id branch from 135bb5b to f43be22 Compare October 20, 2025 03:02

UdjinM6 changed the title ~~fix: GenInputLockRequestId should hash it properly for CTxIn/COutPoint to get the same result in both cases~~ fix: remove incorrect request generation with bad serialization hash Oct 20, 2025

refactor: trivial s/val/outpoint/

58f671c

coderabbitai bot reviewed Oct 20, 2025

View reviewed changes

src/instantsend/lock.cpp Show resolved Hide resolved

kwvg previously approved these changes Oct 20, 2025

View reviewed changes

fix: trivial safety belt

5456354

UdjinM6 dismissed kwvg’s stale review via 5a878bc October 20, 2025 11:37

coderabbitai bot reviewed Oct 20, 2025

View reviewed changes

src/test/evo_islock_tests.cpp Show resolved Hide resolved

test: add tests for inlock id generation

a8506ec

UdjinM6 force-pushed the fix_inlock_id branch from 5a878bc to a8506ec Compare October 20, 2025 11:42

coderabbitai bot reviewed Oct 20, 2025

View reviewed changes

knst reviewed Oct 20, 2025

View reviewed changes

PastaPastaPasta approved these changes Oct 20, 2025

View reviewed changes

kwvg self-requested a review October 20, 2025 15:51

kwvg approved these changes Oct 20, 2025

View reviewed changes

knst approved these changes Oct 20, 2025

View reviewed changes

PastaPastaPasta merged commit ee41b18 into dashpay:develop Oct 20, 2025
35 of 39 checks passed

Conversation

UdjinM6 commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwvg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

knst Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

kwvg Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

UdjinM6 commented Oct 19, 2025 •

edited

Loading

github-actions bot commented Oct 19, 2025 •

edited

Loading

coderabbitai bot commented Oct 19, 2025 •

edited

Loading

kwvg left a comment •

edited

Loading