feat(crypto): add proposal-gated ECDSA validation - #49
Open
Federico2014 wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds fork-gated strict ECDSA validation. It stores activation state through a proposal, enforces 65-byte signatures and recovery bounds, applies strict checks to blocks and transactions, and invalidates cached verification states after activation. ChangesStrict ECDSA validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProposalService
participant DynamicPropertiesStore
participant Manager
participant TransactionCapsule
participant ECKey
ProposalService->>DynamicPropertiesStore: save strict validation value
Manager->>DynamicPropertiesStore: detect activation at maintenance boundary
Manager->>TransactionCapsule: clear verified state
TransactionCapsule->>ECKey: validate signatures in strict mode
ECKey-->>TransactionCapsule: accept or reject recovered signature
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/main/java/org/tron/core/db/Manager.java`:
- Around line 1924-1930: Update the transaction verification cache used by
pushTransaction and validateSignature to bind each cached result to the strict
ECDSA activation state or validation epoch, and revalidate whenever it does not
match the current state. Ensure an in-flight legacy validation cannot restore a
cache entry usable after activation, while preserving normal cache reuse within
the same state. Add an overlap test covering legacy validation blocked until
activation completes.
In `@framework/src/test/java/org/tron/core/WalletMockTest.java`:
- Around line 212-217: Gate strict signature-length validation on the shared
EC-key and dynamic-property activation condition. In
framework/src/test/java/org/tron/core/WalletMockTest.java:212-217, explicitly
test strict validation disabled and enabled, preserving legacy handling before
activation and expecting SIGERROR after activation; update
Wallet.broadcastTransaction accordingly. In
framework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java:405-421,
add the same off/on coverage, preserving legacy P2P handling before activation
and expecting BAD_TRX after activation; gate the P2P validation path with the
same condition.
🪄 Autofix
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 Plus
Run ID: 14f8d709-6824-4aa8-aed3-17684f6258b4
📒 Files selected for processing (20)
actuator/src/main/java/org/tron/core/utils/ProposalUtil.javachainbase/src/main/java/org/tron/core/capsule/BlockCapsule.javachainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.javachainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.javacommon/src/main/java/org/tron/core/Constant.javacommon/src/main/java/org/tron/core/config/Parameter.javacrypto/src/main/java/org/tron/common/crypto/ECKey.javacrypto/src/main/java/org/tron/common/crypto/SignUtils.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/consensus/ProposalService.javaframework/src/main/java/org/tron/core/db/Manager.javaframework/src/test/java/org/tron/common/crypto/ECKeyTest.javaframework/src/test/java/org/tron/core/WalletMockTest.javaframework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.javaframework/src/test/java/org/tron/core/capsule/BlockCapsuleTest.javaframework/src/test/java/org/tron/core/capsule/TransactionCapsuleTest.javaframework/src/test/java/org/tron/core/db/ManagerMockTest.javaframework/src/test/java/org/tron/core/db/ManagerTest.javaframework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.javaframework/src/test/java/org/tron/core/services/ProposalServiceTest.java
💤 Files with no reviewable changes (1)
- common/src/main/java/org/tron/core/Constant.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 20 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Federico2014
force-pushed
the
feature/strict-ecdsa-validation
branch
from
August 5, 2026 10:07
307fb5c to
f0483b0
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This was referenced Aug 6, 2026
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Federico2014
force-pushed
the
feature/strict-ecdsa-validation
branch
from
August 13, 2026 09:56
4112998 to
f2addb0
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.
What does this PR do?
This PR adds proposal 99 (
ALLOW_STRICT_ECDSA_VALIDATION), gated by java-tron block versionVERSION_4_8_3, to activate strict recoverable secp256k1 ECDSA validation for transaction, block-witness, and TVM recovery paths.Strict validation:
1 <= r, s < nbefore curve work;BigIntegers.modOddInversefor valid inverses;r/scomponents at the public recovery boundary.Existing public
ECKeyrecovery overloads remain legacy-compatible and delegate with strict validation disabled; new overloads allow strict validation to be selected explicitly.Fresh RPC and P2P transaction admission requires exactly 65-byte signatures independently of proposal activation. Relay handshake recovery is also always strict. Inbound block sanitization canonicalizes an overlong witness signature to a detached 65-byte copy, preventing the response object from retaining the oversized backing storage. Batch transaction verification validates all signature lengths before submitting any asynchronous verification task.
When proposal 99 becomes active, cached positive transaction-verification results are invalidated across pending, re-push, popped, and pushing queues. Cache reuse requires an explicitly verified capsule with matching signatures, and per-transaction synchronization prevents an in-flight legacy validation from restoring a stale positive result.
Why are these changes required?
Trailing signature bytes create non-canonical encodings and unnecessary network, memory, and storage costs without changing the recovered signer. Malformed components should also be rejected before expensive elliptic-curve work or asynchronous task submission.
A range-valid signature can recover the secp256k1 point at infinity. That value is not a valid public key, but the legacy path can encode it as
{0x00}and derive a deterministic address from an empty public-key payload. Strict recovery rejects this result at the common recovery boundary.Proposal-gated consensus rules preserve historical transaction, block, and VM behavior, while independent admission checks reject malformed fresh input early. Cache invalidation prevents a legacy verification result from bypassing the post-activation rules.
This PR has been tested by:
ECKeyTestManagerSignaturePreValidationTestSanitizeUnknownFieldsTest./gradlew :framework:checkstyleMain./gradlew :framework:checkstyleTestCoverage includes activation boundaries, legacy and strict signature lengths, scalar and recovery-ID bounds, high-S compatibility, modular-inverse conformance, null components, point-at-infinity recovery, witness-signature sanitization, pre-submission length validation, VM behavior, and verification-cache transitions.
Follow up
Confirm the numeric fork version assigned to
VERSION_4_8_3, complete the TIP review, and coordinate the release and proposal activation schedule before enabling proposal 99.Extra details
Related TIP draft: Federico2014/tips#2
Compatibility:
ECKeypublic overloads retain legacy defaults.27..34remain supported.Consensus upgrade: all block-producing and validating nodes must run a release containing this change before proposal 99 is activated.
Cross-module impact: crypto recovery, VM execution, chainbase state, proposal processing, admission validation, block sanitization, and transaction-cache lifecycle share one activation boundary and must be deployed together.