identifysparkcoins crash fix

[levonpetrosyan93]

fixes the issue #1704

[coderabbitai]

Walkthrough

The identifysparkcoins function in the RPC wallet module now includes defensive error handling. When GetTransaction fails to retrieve a transaction, the code throws a runtime_error instead of proceeding with a null transaction object.

Changes

Cohort / File(s)	Summary
Error handling in transaction retrieval src/wallet/rpcwallet.cpp	Added guard clause to identifysparkcoins that throws runtime_error when GetTransaction fails, preventing null transaction objects from being processed

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

• Single guard clause addition with straightforward error message
• No logic complexity or multi-file interactions
• Defensive programming improvement with minimal scope

Poem

🐰 A guard so fine, a check so true,
When transactions flee, we catch them too!
No null shall pass without our call,
With error thrown, we catch them all. ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only states 'fixes the issue #1704' without following the required template structure, missing both mandatory 'PR intention' section and optional 'Code changes brief' section that details what the PR does and changes made.	Expand the description to include the mandatory 'PR intention' section explaining what issue #1704 is, what the fix does, and optionally describe architectural changes made to handle the missing transaction error case.
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 (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'identifysparkcoins crash fix' directly references the function being modified and describes the fix as addressing a crash, which aligns with the actual change of adding error handling to prevent crashes from missing transactions.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch identify_fix

---

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

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/wallet/rpcwallet.cpp (1)

4187-4192: Similar vulnerability: unchecked GetTransaction call.

The getsparkcoinaddr function has the same pattern as the bug that was just fixed in identifysparkcoins. If GetTransaction fails and returns false, tx will be uninitialized, causing a potential crash when dereferenced.

Apply this diff to add the same defensive check:

     uint256 txHash;
     txHash.SetHex(request.params[0].get_str());
 
     CTransactionRef tx;
     uint256 hashBlock;
-    GetTransaction(txHash, tx,  Params().GetConsensus(), hashBlock);
+    if (!GetTransaction(txHash, tx, Params().GetConsensus(), hashBlock))
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, 
+                "Transaction not found for the given txHash");

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fecd68a and 7ed9c9e.

📒 Files selected for processing (1)

• src/wallet/rpcwallet.cpp (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/wallet/rpcwallet.cpp (2)

src/validation.cpp (2)

• GetTransaction (1677-1730)
• GetTransaction (1677-1677)

src/chainparams.cpp (4)

• Params (1348-1351)
• Params (1348-1348)
• Params (1353-1365)
• Params (1353-1353)

⏰ 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: build-mac-cmake
• GitHub Check: build-guix-x86_64-linux-gnu
• GitHub Check: build-mac-cmake
• GitHub Check: build-guix-x86_64-apple-darwin
• GitHub Check: build-guix-x86_64-linux-gnu
• GitHub Check: build-guix-aarch64-linux-gnu
• GitHub Check: build-guix-arm64-apple-darwin
• GitHub Check: build-guix-x86_64-w64-mingw32
• GitHub Check: build-windows-cmake
• GitHub Check: build-linux-cmake

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Use JSONRPCError for consistency with other RPC error handling.

The defensive check correctly prevents the crash, but should throw JSONRPCError instead of std::runtime_error to be consistent with error handling patterns used throughout this RPC module (see lines 2188, 4812, etc.).

Apply this diff:

     if (!GetTransaction(txHash, tx,  Params().GetConsensus(), hashBlock))
-        throw std::runtime_error(
-                "Transaction is not found by the given txHash.");
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, 
+                "Transaction not found for the given txHash");

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (!GetTransaction(txHash, tx, Params().GetConsensus(), hashBlock))
	throw std::runtime_error(
	"Transaction is not found by the given txHash.");
	if (!GetTransaction(txHash, tx, Params().GetConsensus(), hashBlock))
	throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY,
	"Transaction not found for the given txHash");

🤖 Prompt for AI Agents

In src/wallet/rpcwallet.cpp around lines 4150 to 4152, replace the thrown
std::runtime_error with a JSONRPCError to match the RPC module's error handling;
throw a JSONRPCError with an appropriate RPC error code (e.g.
RPC_INVALID_ADDRESS_OR_KEY or RPC_MISC_ERROR) and the existing message
"Transaction is not found by the given txHash." so callers receive a proper RPC
error object instead of a raw exception.

[aleflm]

LGTM. Thank you

[justanwar]

Tested confirmed working.