Repository navigation
feat(crypto): add strict ECDSA validation - #7001
Federico2014 wants to merge 8 commits into
Conversation
| if (strictEcdsaValidation | ||
| && !SignUtils.isValidLength(witnessSignature.size())) { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
4bca8c9 to
8f787f0
Compare
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.
| boolean strictEcdsaValidation = CommonParameter.getInstance().isECKeyCryptoEngine() | ||
| && dynamicPropertiesStore.allowStrictEcdsaValidation(); |
There was a problem hiding this comment.
[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.
What does this PR do?
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.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:
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../gradlew testrun encountered local gRPCDEADLINE_EXCEEDEDfailures inDbLiteLevelDbTest.testToolsWithLevelDBandDbLiteLevelDbV2Test.testToolsWithLevelDBV2. The run was stopped and full-suite validation remains incomplete.Follow up
Extra details
The activation boundary differs by entry point:
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
TransactionCapsule.checkWeightnow defaults to strict mode, including its use when checking existing signatures inaddSign; 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.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.TransactionUtil.truncateSignatures(Transaction)is removed without a deprecated wrapper, breaking downstream source and binary compatibility. Migrate toTransactionUtil.validateSignatureLengths(Transaction)and handleSignatureFormatException. 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.signatureToKeyBytes(byte[], ECDSASignature),signatureToAddress(byte[], ECDSASignature), andsignatureToKey(byte[], String)are removed, breaking downstream source and binary compatibility. Use the corresponding three-argument overload and choosestrictValidationexplicitly. Other legacy overloads remain where still needed; this is not a blanket removal of legacy recovery APIs.SignUtils.signatureToAddressoverloads now wrap runtime failures asSignatureExceptionwith the original cause in both legacy and strict modes, for both crypto engines. Existing checkedSignatureExceptioninstances 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.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. UseSignUtils.isValidLength()for admission-length checks orConstant.PER_SIGN_LENGTHfor the signature size, and rebuild callers to adopt the new policy.getAllowStrictEcdsaValidation. Consumers that assume a fixed parameter list must accept this additional entry.