feat(crypto): shielded transaction API security enhancement#15
Open
Federico2014 wants to merge 1 commit intodevelopfrom
Open
feat(crypto): shielded transaction API security enhancement#15Federico2014 wants to merge 1 commit intodevelopfrom
Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughThis PR changes the default behavior of shielded transaction APIs from enabled to disabled, implements strict arithmetic overflow checking in shielded transaction builders using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
![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 13 files
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/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java">
<violation number="1" location="framework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.java:552">
P2: Update `valueBalance` before appending to the builder state. As written, an overflow throws after `spends`/`receives` has already been mutated, leaving the builder inconsistent for any later `build()` call.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Federico2014
force-pushed
the
fix/shielded-api-security
branch
from
5c2b8f4 to
ba1b665
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
framework/src/main/java/org/tron/core/config/args/Args.java (1)
719-730: Emit a startup warning when the new secure default is applied.When neither config key is present, the node now silently falls back to
false, and operators only discover the breaking change later when shielded endpoints start returningShieldedTransactionApi is not allowed. A one-time warning here would make upgrades much easier to diagnose.Suggested change
} else { PARAMETER.allowShieldedTransactionApi = false; + logger.warn("node.allowShieldedTransactionApi is unset; shielded transaction APIs are " + + "disabled by default. Set node.allowShieldedTransactionApi = true to enable them."); }🤖 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/config/args/Args.java` around lines 719 - 730, The code path in Args that sets PARAMETER.allowShieldedTransactionApi to false when neither ConfigKey.ALLOW_SHIELDED_TRANSACTION_API nor ConfigKey.NODE_FULLNODE_ALLOW_SHIELDED_TRANSACTION is present should emit a one-time startup warning; update the else branch in the Args class (where PARAMETER.allowShieldedTransactionApi is set to false) to call logger.warn (or logger.info if you prefer severity) with a clear message that the node defaulted to disallowing shielded transaction API and operators should set node.allowShieldedTransactionApi=true to re-enable it, so operators see this on upgrade rather than discovering it later at runtime.framework/src/test/java/org/tron/core/config/args/ArgsTest.java (1)
364-387: Test the config path, not just the setter.
testExplicitEnableShieldedTransactionApi()currently proves the field setter works, but the behavior that changed is inArgs.applyConfigParams(). Adding assertions fornode.allowShieldedTransactionApi=trueand the deprecated fallback key would protect the parsing and precedence logic that can actually regress.🤖 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/config/args/ArgsTest.java` around lines 364 - 387, Update testExplicitEnableShieldedTransactionApi() to exercise the config parsing path instead of just the setter: call Args.setParam(...) with a config that contains node.allowShieldedTransactionApi=true (and add a separate assertion or subtest that the deprecated fallback key allowShieldedTransactionApi=true is also recognized), then call Args.getInstance().isAllowShieldedTransactionApi() (after Args.applyConfigParams() if that’s required) and assert it is true; do not use Args.getInstance().setAllowShieldedTransactionApi(true) in this test so it verifies Args.applyConfigParams()/Args.setParam() parsing and precedence logic.framework/src/main/java/org/tron/core/Wallet.java (1)
2370-2370: Extract duplicated overflow message into a shared constant.The same literal appears in multiple methods; centralizing it reduces drift risk during future edits.
♻️ Proposed refactor
@@ private static final String SHIELDED_ID_NOT_ALLOWED = "ShieldedTransactionApi is not allowed"; + private static final String SHIELDED_AMOUNT_OVERFLOW = "shielded amount overflow"; @@ - throw new ZksnarkException("shielded amount overflow"); + throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW); @@ - throw new ZksnarkException("shielded amount overflow"); + throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW); @@ - throw new ZksnarkException("shielded amount overflow"); + throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW); @@ - throw new ZksnarkException("shielded amount overflow"); + throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW); @@ - throw new ZksnarkException("shielded amount overflow"); + throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW); @@ - throw new ZksnarkException("shielded amount overflow"); + throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW);Also applies to: 2474-2474, 3661-3661, 3733-3733, 3797-3797, 3858-3858
🤖 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/Wallet.java` at line 2370, Extract the repeated literal "shielded amount overflow" from Wallet.java into a single shared constant (for example a private static final String SHIELDED_AMOUNT_OVERFLOW_MSG) declared in the Wallet class, then replace each direct throw new ZksnarkException("shielded amount overflow") occurrence (e.g., the throws inside the methods around the existing calls at the current throw sites) with throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW_MSG); ensure all listed occurrences (the ones currently throwing ZksnarkException with that exact literal) are updated to use the new constant.framework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.java (1)
1360-1403: Restore the original API flag value instead of hardcodingtrue.At Line 1402, forcing
truecan leak state if class setup/defaults change. Snapshot and restore the pre-test value instead.♻️ Suggested change
`@Test` public void shieldedTransferValidationWorksWhenApiDisabled() { + boolean originalAllowShieldedTransactionApi = Args.getInstance().allowShieldedTransactionApi; // Disable the shielded API (this should NOT affect transaction validation) Args.getInstance().setAllowShieldedTransactionApi(false); @@ } finally { - // Restore default for other tests - Args.getInstance().setAllowShieldedTransactionApi(true); + // Restore previous value for other tests + Args.getInstance().setAllowShieldedTransactionApi(originalAllowShieldedTransactionApi); } }🤖 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/actuator/ShieldedTransferActuatorTest.java` around lines 1360 - 1403, The test currently hardcodes restoring the API flag with Args.getInstance().setAllowShieldedTransactionApi(true) which can leak state; capture the original flag before changing it (e.g. boolean originalAllow = Args.getInstance().isAllowShieldedTransactionApi()), use Args.getInstance().setAllowShieldedTransactionApi(false) for the test, and in the finally block restore the original value with Args.getInstance().setAllowShieldedTransactionApi(originalAllow) so the test leaves global state as it was; update ShieldedTransferActuatorTest around the existing setAllowShieldedTransactionApi calls and finally block to use this pattern.
🤖 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/main/java/org/tron/core/config/args/Args.java`:
- Around line 719-730: The code path in Args that sets
PARAMETER.allowShieldedTransactionApi to false when neither
ConfigKey.ALLOW_SHIELDED_TRANSACTION_API nor
ConfigKey.NODE_FULLNODE_ALLOW_SHIELDED_TRANSACTION is present should emit a
one-time startup warning; update the else branch in the Args class (where
PARAMETER.allowShieldedTransactionApi is set to false) to call logger.warn (or
logger.info if you prefer severity) with a clear message that the node defaulted
to disallowing shielded transaction API and operators should set
node.allowShieldedTransactionApi=true to re-enable it, so operators see this on
upgrade rather than discovering it later at runtime.
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Line 2370: Extract the repeated literal "shielded amount overflow" from
Wallet.java into a single shared constant (for example a private static final
String SHIELDED_AMOUNT_OVERFLOW_MSG) declared in the Wallet class, then replace
each direct throw new ZksnarkException("shielded amount overflow") occurrence
(e.g., the throws inside the methods around the existing calls at the current
throw sites) with throw new ZksnarkException(SHIELDED_AMOUNT_OVERFLOW_MSG);
ensure all listed occurrences (the ones currently throwing ZksnarkException with
that exact literal) are updated to use the new constant.
In
`@framework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.java`:
- Around line 1360-1403: The test currently hardcodes restoring the API flag
with Args.getInstance().setAllowShieldedTransactionApi(true) which can leak
state; capture the original flag before changing it (e.g. boolean originalAllow
= Args.getInstance().isAllowShieldedTransactionApi()), use
Args.getInstance().setAllowShieldedTransactionApi(false) for the test, and in
the finally block restore the original value with
Args.getInstance().setAllowShieldedTransactionApi(originalAllow) so the test
leaves global state as it was; update ShieldedTransferActuatorTest around the
existing setAllowShieldedTransactionApi calls and finally block to use this
pattern.
In `@framework/src/test/java/org/tron/core/config/args/ArgsTest.java`:
- Around line 364-387: Update testExplicitEnableShieldedTransactionApi() to
exercise the config parsing path instead of just the setter: call
Args.setParam(...) with a config that contains
node.allowShieldedTransactionApi=true (and add a separate assertion or subtest
that the deprecated fallback key allowShieldedTransactionApi=true is also
recognized), then call Args.getInstance().isAllowShieldedTransactionApi() (after
Args.applyConfigParams() if that’s required) and assert it is true; do not use
Args.getInstance().setAllowShieldedTransactionApi(true) in this test so it
verifies Args.applyConfigParams()/Args.setParam() parsing and precedence logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40c590e1-4bfb-4437-ab59-1899269c705d
📒 Files selected for processing (13)
common/src/main/java/org/tron/common/parameter/CommonParameter.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.javaframework/src/main/java/org/tron/core/zen/ZenTransactionBuilder.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.javaframework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.javaframework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.javaframework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.javasprout-verifying.key
Federico2014
force-pushed
the
fix/shielded-api-security
branch
2 times, most recently
from
0666f3c to
4512b7a
Compare
Federico2014
force-pushed
the
fix/shielded-api-security
branch
from
4512b7a to
f301e6f
Compare
Federico2014
commented
Apr 15, 2026
Owner
Author
Federico2014
left a comment
Federico2014
left a comment
There was a problem hiding this comment.
Thanks for the review. Responses to each nitpick:
-
Args.java — startup warning when default is applied: The
elsebranch means the user never configured this key at all — most nodes don't use shielded APIs, so a warning here would just be noise. Theconfig.confcomment already documents the change. Won't fix. -
ArgsTest — test config parsing path: These two tests have been merged into one in the latest commit. The current pattern (setter-based) is consistent with other tests in
ArgsTest. Won't fix. -
Wallet.java — extract duplicated overflow message: These are exception messages only, not used in logic. Keeping them inline is fine for this scope. Won't fix.
-
ShieldedTransferActuatorTest — restore original flag: Already fixed in
4512b7a— the test now saves and restores the original value viaboolean orig.
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.
What does this PR do?
Enhances the security of shielded transaction APIs by adding overflow protection, tightening default configuration, and cleaning up unused artifacts.
ZenTransactionBuilderandShieldedTRC20ParametersBuilder: replaced raw+=/-=onvalueBalancewithStrictMathWrapper.addExact()/subtractExact(), which throwArithmeticExceptionon overflow. Overflow validation is performed before mutating builder state (spends/receiveslists), ensuring the builder remains consistent if an overflow is detected.createShieldedTransaction,createShieldedTransactionWithoutSpendAuthSig,createShieldedContractParameters, andcreateShieldedContractParametersWithoutAskintry/catch ArithmeticExceptionto convert overflow toZksnarkException("shielded amount overflow")allowShieldedTransactionApifromtruetofalseto reduce exposure by default, since some shielded transaction APIs require passing private keys as parametersconfig.confwith an explicit warning advising users to only invoke these APIs locallysprout-verifying.keyfrom the repository root — the file was not referenced by any codeallowShieldedTransactionApiand properly save/restore the flagWhy are these changes required?
valueBalancewas accumulated via raw+=/-=without overflow checks, allowing crafted inputs to silently wrap aroundlongrange and produce invalid balancessprout-verifying.keywas an unreferenced artifact that added unnecessary repository bloatallowShieldedTransactionApidefault changed fromtruetofalse— nodes that previously relied on the default will find shielded transaction APIs disabled after upgrade. Fix: explicitly setallowShieldedTransactionApi = trueinconfig.conf.This PR has been tested by:
ShieldedTransferActuatorTest,ArgsTest,ShieldedTRC20BuilderTest,CreateSpendAuthSigServletTest,MerkleContainerTest./gradlew clean build -x test)Follow up
No consensus logic or on-chain state transitions affected.
Extra details