fix(crypto): harden shielded transaction safety and input validation#10
Open
Federico2014 wants to merge 1 commit intodevelopfrom
Open
fix(crypto): harden shielded transaction safety and input validation#10Federico2014 wants to merge 1 commit intodevelopfrom
Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNarrowed zkSNARK exception handling, treat librustzcash init failures as errors, enforce explicit failure return values for verifications, simplify Sapling task callables, add strict byte-length validation and early INVALID_ARGUMENT responses for shielded RPCs and TRC20 servlet, and update tests with deterministic keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PrecompiledContracts
participant ExecutorService
participant SaplingTask
participant JLibrustzcash
Client->>PrecompiledContracts: submit VerifyTransferProof request
PrecompiledContracts->>ExecutorService: submit SaplingTask instances
ExecutorService->>SaplingTask: run task (concurrently)
SaplingTask->>JLibrustzcash: call verification function
JLibrustzcash-->>SaplingTask: return verification result
SaplingTask-->>ExecutorService: complete future
ExecutorService-->>PrecompiledContracts: signal via CountDownLatch
alt timeout occurs
PrecompiledContracts->>ExecutorService: cancel all futures
PrecompiledContracts-->>Client: return failure (empty bytes / false)
else all tasks complete
PrecompiledContracts-->>Client: aggregate results and return success/failure bytes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/src/main/java/org/tron/core/services/RpcApiService.java (1)
663-681:⚠️ Potential issue | 🟠 MajorReturn immediately after
onError()in theWalletSolidityApiscan handlers.The gRPC Java
StreamObservercontract specifies thatonCompleted()andonError()are mutually exclusive terminal events—both may be called at most once and, if called, must be the last method invoked on that observer. These methods currently fall through toonCompleted()after catching exceptions and callingonError(), violating this contract. Addreturn;after eachonError(...)call, matching the pattern already used in the correspondingWalletApiimplementations.Also applies to: 685-707, 711-728, 742-772, 776-801
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/services/RpcApiService.java` around lines 663 - 681, The RPC handlers (e.g., RpcApiService.scanNoteByIvk) call responseObserver.onError(...) but then continue and call responseObserver.onCompleted(), violating the StreamObserver contract; update each scan handler to return immediately after any responseObserver.onError(...) call (add a plain return; after the onError call), including the ones that catch exceptions and the ivk length validation branch, so that onCompleted() is never invoked after onError; identify locations by method names like scanNoteByIvk and the use of getRunTimeException to locate all similar handlers and apply the same fix.
🧹 Nitpick comments (1)
framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java (1)
73-86: Add the same key/address length checks on the HTTP GET path.
doGet()now forwardsivk/ak/nk/shielded_TRC20_contract_addressstraight towallet.scanShieldedTRC20NotesByIvk(...), whileframework/src/main/java/org/tron/core/services/RpcApiService.java:742-761rejects malformed 32-byte/21-byte inputs before reaching the wallet. Reusing that guard here would keep GET from entering the zk path with bad inputs and make transport behavior consistent.Possible direction
- GrpcAPI.DecryptNotesTRC20 notes = wallet + byte[] contractAddressBytes = ByteArray.fromHexString(contractAddress); + byte[] ivkBytes = ByteArray.fromHexString(ivk); + byte[] akBytes = ByteArray.fromHexString(ak); + byte[] nkBytes = ByteArray.fromHexString(nk); + if (ivkBytes.length != 32 || akBytes.length != 32 || nkBytes.length != 32) { + throw new IllegalArgumentException("ivk, ak, nk must each be 32 bytes"); + } + if (contractAddressBytes.length != 21) { + throw new IllegalArgumentException("contractAddress must be 21 bytes"); + } + + GrpcAPI.DecryptNotesTRC20 notes = wallet .scanShieldedTRC20NotesByIvk(startNum, endNum, - ByteArray.fromHexString(contractAddress), ByteArray.fromHexString(ivk), - ByteArray.fromHexString(ak), ByteArray.fromHexString(nk), + contractAddressBytes, ivkBytes, akBytes, nkBytes, builder.getEventsList());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java` around lines 73 - 86, In ScanShieldedTRC20NotesByIvkServlet.doGet(), add the same input length validation used in RpcApiService for shielded_TRC20_contract_address (21-byte) and ivk/ak/nk (32-byte) before calling wallet.scanShieldedTRC20NotesByIvk; verify the hex strings decode to the expected byte lengths (or that their hex length equals 42/64 chars respectively) and return the same error/response path on failure so malformed inputs are rejected early and behavior matches RpcApiService.java; perform these checks immediately after reading request parameters and before building IvkDecryptTRC20Parameters or invoking scanShieldedTRC20NotesByIvk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@framework/src/main/java/org/tron/core/services/RpcApiService.java`:
- Around line 663-681: The RPC handlers (e.g., RpcApiService.scanNoteByIvk) call
responseObserver.onError(...) but then continue and call
responseObserver.onCompleted(), violating the StreamObserver contract; update
each scan handler to return immediately after any responseObserver.onError(...)
call (add a plain return; after the onError call), including the ones that catch
exceptions and the ivk length validation branch, so that onCompleted() is never
invoked after onError; identify locations by method names like scanNoteByIvk and
the use of getRunTimeException to locate all similar handlers and apply the same
fix.
---
Nitpick comments:
In
`@framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java`:
- Around line 73-86: In ScanShieldedTRC20NotesByIvkServlet.doGet(), add the same
input length validation used in RpcApiService for
shielded_TRC20_contract_address (21-byte) and ivk/ak/nk (32-byte) before calling
wallet.scanShieldedTRC20NotesByIvk; verify the hex strings decode to the
expected byte lengths (or that their hex length equals 42/64 chars respectively)
and return the same error/response path on failure so malformed inputs are
rejected early and behavior matches RpcApiService.java; perform these checks
immediately after reading request parameters and before building
IvkDecryptTRC20Parameters or invoking scanShieldedTRC20NotesByIvk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9330320d-b583-4579-8e1e-4c07de4e4aff
📒 Files selected for processing (5)
actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.javachainbase/src/main/java/org/tron/common/zksnark/JLibrustzcash.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/main/java/org/tron/core/zen/ZenTransactionBuilder.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java (1)
545-553: Add explicit negative-path coverage for length validation.These inputs now cover the valid path, but there’s still no assertion here that invalid lengths are rejected. Adding one failure-case test would better lock in the new safety behavior.
[tags]
Proposed test addition
+ `@Test` + public void testScanShieldedTRC20NotesByIvkRejectsInvalidLengths() { + IvkDecryptTRC20Parameters invalid = IvkDecryptTRC20Parameters.newBuilder() + .setStartBlockIndex(1) + .setEndBlockIndex(10) + .setIvk(ByteString.copyFrom(new byte[31])) + .setAk(ByteString.copyFrom(new byte[32])) + .setNk(ByteString.copyFrom(new byte[32])) + .setShieldedTRC20ContractAddress(ByteString.copyFrom(new byte[20])) + .build(); + + try { + blockingStubFull.scanShieldedTRC20NotesByIvk(invalid); + Assert.fail("Expected invalid-length request to be rejected"); + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains("INVALID_ARGUMENT")); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java` around lines 545 - 553, Add a negative-path unit test in RpcApiServicesTest that verifies IvkDecryptTRC20Parameters rejects invalid-length fields: build parameter instances where ivk/ak/nk are not 32 bytes (e.g., 31 or 33 bytes) and shieldedTRC20ContractAddress is not 21 bytes, then assert the expected validation failure/exception when calling the code under test (use the same IvkDecryptTRC20Parameters builder pattern shown with dummyKey32/dummyAddr21 but with wrong-length byte arrays) so the test covers explicit length-validation failure cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java`:
- Around line 545-553: Add a negative-path unit test in RpcApiServicesTest that
verifies IvkDecryptTRC20Parameters rejects invalid-length fields: build
parameter instances where ivk/ak/nk are not 32 bytes (e.g., 31 or 33 bytes) and
shieldedTRC20ContractAddress is not 21 bytes, then assert the expected
validation failure/exception when calling the code under test (use the same
IvkDecryptTRC20Parameters builder pattern shown with dummyKey32/dummyAddr21 but
with wrong-length byte arrays) so the test covers explicit length-validation
failure cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88fb39c1-d14b-4fbd-9dc4-f4d06f3b0250
📒 Files selected for processing (3)
framework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java
- framework/src/main/java/org/tron/core/services/RpcApiService.java
Owner
Author
|
Both findings from the initial review are addressed in 305e199:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java:587">
P2: Assert the gRPC status code directly instead of matching exception message text, which is brittle and not API-stable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java (1)
610-626: Add direct tests for invalid TRC20 contractAddress length.Current TRC20 negative tests only exercise bad key-length branches. Add dedicated invalid
shieldedTRC20ContractAddresslength tests so that validation path is explicitly locked.Suggested follow-up tests
+ `@Test` + public void testScanShieldedTRC20NotesByIvkRejectsInvalidContractAddressLength() { + IvkDecryptTRC20Parameters message = IvkDecryptTRC20Parameters.newBuilder() + .setStartBlockIndex(1) + .setEndBlockIndex(10) + .setIvk(ByteString.copyFrom(new byte[32])) + .setAk(ByteString.copyFrom(new byte[32])) + .setNk(ByteString.copyFrom(new byte[32])) + .setShieldedTRC20ContractAddress(ByteString.copyFrom(new byte[20])) + .build(); + try { + blockingStubFull.scanShieldedTRC20NotesByIvk(message); + Assert.fail("Expected INVALID_ARGUMENT"); + } catch (StatusRuntimeException e) { + Assert.assertEquals(Status.Code.INVALID_ARGUMENT, e.getStatus().getCode()); + } + } + + `@Test` + public void testScanShieldedTRC20NotesByOvkRejectsInvalidContractAddressLength() { + OvkDecryptTRC20Parameters message = OvkDecryptTRC20Parameters.newBuilder() + .setStartBlockIndex(1) + .setEndBlockIndex(10) + .setOvk(ByteString.copyFrom(new byte[32])) + .setShieldedTRC20ContractAddress(ByteString.copyFrom(new byte[20])) + .build(); + try { + blockingStubFull.scanShieldedTRC20NotesByOvk(message); + Assert.fail("Expected INVALID_ARGUMENT"); + } catch (StatusRuntimeException e) { + Assert.assertEquals(Status.Code.INVALID_ARGUMENT, e.getStatus().getCode()); + } + }Also applies to: 628-642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java` around lines 610 - 626, Add dedicated negative tests in RpcApiServicesTest to assert that passing an invalid-length shieldedTRC20ContractAddress yields INVALID_ARGUMENT: for the existing testScanShieldedTRC20NotesByIvkRejectsInvalidLength create/modify the IvkDecryptTRC20Parameters construction to setShieldedTRC20ContractAddress(ByteString.copyFrom(new byte[21])) (or any length != expected 20) and call blockingStubFull.scanShieldedTRC20NotesByIvk, catching StatusRuntimeException and asserting Status.Code.INVALID_ARGUMENT; do the same for the companion test that targets the OVK path (the test around lines 628-642) using the corresponding OvkDecryptTRC20Parameters and blockingStubFull.scanShieldedTRC20NotesByOvk to lock the validation path for contract address length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java`:
- Around line 10-13: Reorder the gRPC import block so it matches the project's
Checkstyle ordering: group and alphabetize the gRPC imports
(io.grpc.ManagedChannel, io.grpc.ManagedChannelBuilder, io.grpc.Status,
io.grpc.StatusRuntimeException) in the import section; replace the current order
with the alphabetized sequence so Checkstyle passes.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java`:
- Around line 610-626: Add dedicated negative tests in RpcApiServicesTest to
assert that passing an invalid-length shieldedTRC20ContractAddress yields
INVALID_ARGUMENT: for the existing
testScanShieldedTRC20NotesByIvkRejectsInvalidLength create/modify the
IvkDecryptTRC20Parameters construction to
setShieldedTRC20ContractAddress(ByteString.copyFrom(new byte[21])) (or any
length != expected 20) and call blockingStubFull.scanShieldedTRC20NotesByIvk,
catching StatusRuntimeException and asserting Status.Code.INVALID_ARGUMENT; do
the same for the companion test that targets the OVK path (the test around lines
628-642) using the corresponding OvkDecryptTRC20Parameters and
blockingStubFull.scanShieldedTRC20NotesByOvk to lock the validation path for
contract address length.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a76ecd2a-e205-4d2c-bd33-db15c52dc065
📒 Files selected for processing (1)
framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
Outdated
Show resolved
Hide resolved
Federico2014
force-pushed
the
fix/shielded-transaction-safety
branch
from
b2fff12 to
ac26b51
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/main/java/org/tron/core/zen/ZenTransactionBuilder.java`:
- Around line 140-142: After calling
JLibrustzcash.librustzcashSaplingProvingCtxInit() in
ShieldedTRC20ParametersBuilder (where ctx is assigned), add the same defensive
check used in ZenTransactionBuilder: if ctx == 0 then throw a
ZksnarkException("Failed to initialize proving context"); this ensures
ShieldedTRC20ParametersBuilder (the method that calls
JLibrustzcash.librustzcashSaplingProvingCtxInit()) validates the native context
before proceeding and avoids undefined behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea189945-c312-4305-8a30-18b113cbc3ad
📒 Files selected for processing (6)
actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.javachainbase/src/main/java/org/tron/common/zksnark/JLibrustzcash.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/main/java/org/tron/core/zen/ZenTransactionBuilder.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
✅ Files skipped from review due to trivial changes (2)
- chainbase/src/main/java/org/tron/common/zksnark/JLibrustzcash.java
- framework/src/main/java/org/tron/core/services/RpcApiService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/test/java/org/tron/core/services/RpcApiServicesTest.java
- framework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.java
Federico2014
force-pushed
the
fix/shielded-transaction-safety
branch
from
ac26b51 to
305c6e4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
VerifyTransferProof—countDownLatch.await()result was never checked, futures now cancelled on timeoutVerifyMintProof,VerifyBurnProof, andZenTransactionBuilderUNCOMMITTEDstatic initializer fail-fast instead of silently swallowingZksnarkExceptioninsertLeaves()and precompiled contract exception paths to returnPair.of(false, EMPTY_BYTE_ARRAY)to distinguish execution failure from verification failurecatch (ZksnarkException e) { throw e; }blocks inSaplingCheckSpendTask,SaplingCheckOutputTask,SaplingCheckBingdingSig, andZenTransactionBuilderJLibrustzcash.INSTANCEfinalScanShieldedTRC20NotesByIvkServlet.doGet()missingeventsparameter parsingTest plan
./gradlew clean build -x testpassesINVALID_ARGUMENTScanShieldedTRC20NotesByIvkServletGET witheventsparameter works consistently with POSTSummary by cubic
Hardens shielded transaction safety and input validation across gRPC/HTTP and precompiles. Adds fail‑fast checks, proper completion semantics, and clear INVALID_ARGUMENT errors to prevent silent failures.
Bug Fixes
VerifyTransferProof: enforce latch timeout and cancel futures on timeout.ivk/ovk/ak/nk(32 bytes) andcontractAddress(21 bytes); return INVALID_ARGUMENT; callonCompleted()only on success.ScanShieldedTRC20NotesByIvkGET: parseeventsquery; validate key and address lengths.Hardening
VerifyMintProof,VerifyBurnProof,ZenTransactionBuilder, andShieldedTRC20ParametersBuilder.Pair.of(false, EMPTY_BYTE_ARRAY)to distinguish from verification failure.UNCOMMITTEDstatic init now throws onZksnarkException; remove redundant rethrows; makeJLibrustzcash.INSTANCEfinal.Written for commit 305c6e4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Tests