Skip to content

feat(crypto): add strict ECDSA validation - #7001

Open
Federico2014 wants to merge 8 commits into
tronprotocol:release_v4.8.3from
Federico2014:feature/strict-ecdsa-validation-v4.8.3
Open

Federico2014 wants to merge 8 commits into
tronprotocol:release_v4.8.3from
Federico2014:feature/strict-ecdsa-validation-v4.8.3

Conversation

@Federico2014

@Federico2014 Federico2014 commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

  • Add proposal 100 (ALLOW_STRICT_ECDSA_VALIDATION), gated by block version 39, to enable strict ECDSA recovery for transaction consensus verification, witness verification, and the affected TVM signature precompiles. Strict recovery checks scalar bounds and recovery inputs and rejects point-at-infinity public keys.
  • Require exactly 65 bytes per signature at transaction admission and auxiliary-query entry points. Auxiliary queries and fast-forward HELLO authentication also use strict ECDSA recovery upon upgrade, independently of proposal activation.
  • Discard P2P transactions individually if any signature is not exactly 65 bytes, before normal or smart-contract task submission. Continue processing other transactions in the batch, clean up received request entries, and avoid disconnecting or marking the peer as bad solely for signature length. Preserve duplicate-ID, unsolicited-transaction, and missing-contract checks.
  • Trim witness-signature trailing bytes during P2P block sanitization, including historical synchronization, while checking the original block size before trimming.
  • Invalidate queued transaction verification caches on activation and require verified pending transactions before reusing their results.
  • Handle failed address recovery explicitly in ValidateMultiSign, normalize component-based SignUtils recovery failures to SignatureException, remove unused ECKey overloads, and use explicit strict recovery in local signing and the transaction test utility.

Why are these changes required?

Malformed signatures and unused trailing bytes can produce inconsistent validation or unnecessary storage and response costs. The proposal controls consensus recovery changes so that legacy recovery remains available before activation. Separate admission, query, and HELLO policies take effect upon software upgrade and therefore require client and peer migration before deployment.

Per-transaction P2P rejection prevents one padded transaction from blocking the rest of a mixed batch or disconnecting an otherwise compatible peer. Rejected transactions are not repaired or forwarded, so clients must still migrate to 65-byte signatures.

Related TIP: tronprotocol/tips#935

This PR has been tested by:

  • Network tests (org.tron.core.net.*): 108 passed, including 11 TransactionsMsgHandler tests. Coverage includes signature-length boundaries, mixed batches with rejected transactions first/middle/last, multisign transactions, normal and smart-contract dispatch, unchanged accepted payloads, request cleanup, no peer penalty solely for signature length, and preserved protocol checks. Framework main/test Checkstyle passed after this change.
  • Focused signature-query and Wallet/TransactionUtil tests: 33 passed.
  • ValidateMultiSign and BatchValidateSign test classes: 13 passed; all 6 ValidateMultiSign tests passed again after adding the null check.
  • Recovery and transaction-utility regression tests: 47 passed across SignUtilsTest, ECKeyTest, BouncyCastleTest, and TransactionUtilsTest. After the final overload cleanup, all 26 ECKeyTest tests and framework test Checkstyle passed again.
  • Framework main/test and plugin main Checkstyle checks passed during the implementation.
  • A full-suite ./gradlew test run encountered local gRPC DEADLINE_EXCEEDED failures in DbLiteLevelDbTest.testToolsWithLevelDB and DbLiteLevelDbV2Test.testToolsWithLevelDBV2. The run was stopped and full-suite validation remains incomplete.

Follow up

  • Add maintenance-boundary integration coverage for proposal activation and pending-transaction revalidation.
  • Add handler-level coverage for HELLO and padded witness signatures through normal block ingress and historical synchronization.
  • Complete affected-client measurements, migration notices, and mixed-version rollout validation before deployment, including end-to-end propagation and per-transaction P2P rejection under both proposal states.
  • Complete a full test-suite run without early termination.

Extra details

The activation boundary differs by entry point:

Entry point Before proposal activation After proposal activation
RPC broadcast and P2P transaction admission Exactly 65 bytes on the upgraded node; legacy ECDSA recovery Exactly 65 bytes; strict ECDSA recovery
getTransactionSignWeight / getTransactionApprovedList Exactly 65 bytes; strict ECDSA recovery on upgrade Same
Fast-forward HELLO authentication Exactly 65 bytes; strict ECDSA recovery on upgrade Same
ECDSA transaction verification in blocks Legacy length/recovery behavior Exactly 65 bytes; strict recovery
P2P witness verification, including historical sync Trim trailing bytes to 65 first, then legacy recovery Trim trailing bytes to 65 first, then strict recovery
Affected TVM signature precompiles Legacy recovery Strict recovery

The 65-byte admission/query/HELLO length policy applies to both ECKey and SM2 configurations. Strict ECDSA recovery does not change the SM2 recovery algorithm. TVM precompiles retain their existing input formats; the table describes their recovery mode rather than a uniform 65-byte ABI requirement. Strict ECDSA recovery does not introduce a low-S requirement.

Compatibility

  • Transaction clients and relay peers: Upgraded admission entry points reject non-65-byte signatures immediately, including the formerly accepted 66–68-byte range. RPC broadcast returns SIGERROR for invalid lengths. P2P ingress first applies existing batch-level checks for duplicate IDs, unsolicited transactions, and missing contracts. Once these pass, request entries for all received transactions are removed, including transactions subsequently rejected for signature length; unrelated outstanding requests remain. Any transaction containing a non-65-byte signature is then discarded before either dispatch path, while other transactions continue through normal admission and queue handling. Length failure alone does not cause a BAD_TX disconnect or bad-peer marking; existing protocol-error handling remains applicable. Transaction signatures are never truncated. Padded transactions still cannot propagate through upgraded nodes, so client migration remains required.
  • Auxiliary queries and signature-weight helpers: Both getTransactionSignWeight and getTransactionApprovedList validate original signature lengths after the existing signature-count limit check and before hashing or recovery. Length-error responses use SIGNATURE_FORMAT_ERROR and omit the transaction extension instead of rewriting signatures. They also reject ECDSA signatures that fail strict recovery before proposal activation. The existing four-argument TransactionCapsule.checkWeight now defaults to strict mode, including its use when checking existing signatures in addSign; downstream callers that intentionally require legacy recovery must use the five-argument overload with an explicit mode. Query validation can therefore be stricter than pre-activation consensus validation.
  • Fast-forward peers: HELLO authentication requires exactly 65 bytes and strict ECDSA recovery upon upgrade. The standard java-tron HELLO signing path meets these requirements; custom peers producing padded signatures or signatures rejected by strict recovery must migrate before upgrading.
  • Block archives and RPC consumers: P2P sanitization trims only witness-signature trailing bytes and preserves the retained 65-byte prefix. A padded witness signature can still pass ingress, even after activation, if that prefix is valid; transaction signatures are not truncated because their complete bytes contribute to txTrieRoot, even though txID excludes signatures. The block ID and signed raw-header digest remain unchanged, but the complete serialized block bytes stored or returned by RPC can differ between an older stored copy and a newly synchronized copy of the same block. Existing database records are not bulk-rewritten. Archive/index consumers comparing complete protobuf bytes must account for this normalization and use the block ID for identity.
  • Removed transaction utility: TransactionUtil.truncateSignatures(Transaction) is removed without a deprecated wrapper, breaking downstream source and binary compatibility. Migrate to TransactionUtil.validateSignatureLengths(Transaction) and handle SignatureFormatException. The replacement returns no rewritten transaction and does not truncate signatures; clients must supply 65-byte signatures. Existing binaries invoking the removed method must be updated and rebuilt.
  • Removed ECKey overloads: signatureToKeyBytes(byte[], ECDSASignature), signatureToAddress(byte[], ECDSASignature), and signatureToKey(byte[], String) are removed, breaking downstream source and binary compatibility. Use the corresponding three-argument overload and choose strictValidation explicitly. Other legacy overloads remain where still needed; this is not a blanket removal of legacy recovery APIs.
  • Recovery exception behavior: The component-based SignUtils.signatureToAddress overloads now wrap runtime failures as SignatureException with the original cause in both legacy and strict modes, for both crypto engines. Existing checked SignatureException instances pass through that component-based wrapper. Callers relying on an unchecked exception type must adjust their handling; direct ECKey calls retain their own exception behavior.
  • Removed length constant: Constant.MAX_PER_SIGN_LENGTH (68) is removed without a deprecated alias, breaking downstream source compatibility on recompilation; reflective lookup also fails. Ordinary compiled Java references inline the old value and retain it after upgrading. Use SignUtils.isValidLength() for admission-length checks or Constant.PER_SIGN_LENGTH for the signature size, and rebuild callers to adopt the new policy.
  • Chain-parameter response: getChainParameters adds getAllowStrictEcdsaValidation. Consumers that assume a fixed parameter list must accept this additional entry.

Comment thread actuator/src/main/java/org/tron/core/utils/TransactionUtil.java
Comment thread actuator/src/main/java/org/tron/core/utils/TransactionUtil.java
Comment thread common/src/main/java/org/tron/core/Constant.java
Comment thread crypto/src/main/java/org/tron/common/crypto/SignUtils.java Outdated
Comment thread chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java
Comment on lines +193 to +194
if (strictEcdsaValidation
&& !SignUtils.isValidLength(witnessSignature.size())) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] Please clarify whether strict block-signature validation applies to the raw P2P wire input or to the canonicalized block representation.

BlockMsgHandler calls sanitize() before validBlock(), so a P2P block carrying a witness signature longer than 65 bytes is truncated first and may still be accepted under strict validation as long as its first 65 bytes are valid. In contrast, directly validating the same unsanitized BlockCapsule rejects it.

If strict mode is intended to reject non-canonical wire blocks, please validate the original signature length before sanitization. If canonicalization on ingress is intentional for historical compatibility, please keep the current order, document this behavior, and add a handler-level test demonstrating that padded P2P input is normalized, validated, stored, and rebroadcast with a 65-byte signature.

The current unit tests separately demonstrate that:

  • an unsanitized padded block is rejected in strict mode;
  • sanitize() truncates the signature while preserving the block ID.

However, there is currently no test covering the full BlockMsgHandler path to define the actual combined semantics of these two behaviors. This is the most valuable test to add for this part of the PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@3for The ordering is intentional and addresses two issues discussed in the TIP thread.

First, as explained here, a peer can append trailing bytes to a historical block’s witness signature without changing its block ID or signed digest. Legacy recovery ignores those bytes, so governance-activated strict validation alone cannot prevent padded historical representations from being accepted. Trimming avoids retaining unnecessary data in storage, propagation, and block-query responses.

Second, as discussed here, applying the same trimming under legacy and strict modes removes the witness-signature length difference between branches with different activation states. A node with a strict Head should not reject an otherwise valid legacy-branch candidate solely because its witness signature contains trailing padding. This addresses the length difference; it does not resolve the separate infinity-recovery or Head-state permission mismatch cases.

The intended behavior, both before and after activation, is to:

  • Apply the existing block-size limit before sanitization.
  • Reject witness signatures shorter than 65 bytes.
  • Independently copy the first 65 bytes of longer signatures before validation.
  • Validate the retained prefix under the applicable rules and use the trimmed representation for storage, forwarding, and API output.

Strict validation therefore applies to the canonicalized witness signature. This exception does not apply to transaction signatures.

I agree that the separate unit tests do not establish the combined ingress behavior. I’ll clarify this boundary in the TIP and add handler-level coverage for padded input, canonicalized storage and forwarding, and rejection of short signatures or invalid retained prefixes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This clarifies that strict validation applies to canonicalized block witness signatures. Please align the TIP and add handler-level tests under both proposal states, covering canonical storage/forwarding and rejection of short signatures or invalid retained prefixes. Transaction signatures in received blocks, including historical blocks, must not be truncated.

@Federico2014
Federico2014 force-pushed the feature/strict-ecdsa-validation-v4.8.3 branch from 4bca8c9 to 8f787f0 Compare October 8, 2026 14:56
Merge the latest release_v4.8.3 updates while preserving TVM storage optimization at proposal 99 and fork version 38. Move strict ECDSA validation to proposal 100 and fork version 39, retain both governance flags, and verify independent activation and VM snapshot restoration.
@github-actions
github-actions Bot requested a review from 3for October 9, 2026 03:03
Comment on lines +191 to +192
boolean strictEcdsaValidation = CommonParameter.getInstance().isECKeyCryptoEngine()
&& dynamicPropertiesStore.allowStrictEcdsaValidation();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] Revalidate the witness signature after the parent-block state is established

BlockMsgHandler.processBlock() verifies the witness signature before acquiring blockLock, so the proposal activation state may differ from the state when the block is actually applied.

For example, block B may pass verification under the old rules while its parent block A is still being processed. After A activates strict signature validation, B can be applied without revalidation in Manager.processBlock(), potentially causing inconsistent block acceptance between broadcast and synchronization paths.

Suggested fix: Revalidate the witness signature at the beginning of Manager.processBlock(), where the lock is already held and the parent-block state is established:

if (!block.generatedByMyself
    && !block.validateSignature(getDynamicPropertiesStore(), getAccountStore())) {
  throw new ValidateSignatureException(
      "block " + block.getNum() + " signature invalid");
}

Please also add a regression test covering B passing preliminary verification under the old rules but being rejected after A activates strict validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants