feat(crypto): add post-quantum signature support#26
Open
Federico2014 wants to merge 50 commits into
Open
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:
📝 WalkthroughWalkthroughAdds FN-DSA-512 post-quantum signatures end-to-end: proto, crypto (FNDSA512), PQ scheme registry, VM precompiles (0x16/0x17/0x18), block/tx PQ auth, witness/miner plumbing, governance flag, config wiring, bandwidth accounting, manager signing, and extensive tests and demo programs. ChangesPost-Quantum Signature Core Implementation
Configuration, Governance, and Feature Control
Witness Management and Consensus Configuration
Block Authentication with PQ Signatures
Transaction Authentication with PQ Signatures
FN-DSA-512 Precompiled Contracts
Supporting Infrastructure and Tests / Demos
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java (2)
200-223: 💤 Low valueMutual-exclusivity gate looks right; null-checks on protobuf getters are redundant.
The hasLegacy / hasPq routing and the "exactly-one" enforcement are the correct policy for V2 launch.
Minor cleanup:
header.getPqAuthSig()andpqAuthSig.getSignature()are protobuf3 message/scalar getters — neither can returnnull, so the!= nullchecks at lines 205–207 are dead. Either drop them, or (preferred) gate onheader.hasPqAuthSig()so an explicitly-set-but-emptypq_auth_sigdoesn't silently fall through as "no PQ":♻️ Suggested simplification
- BlockHeader header = block.getBlockHeader(); - boolean hasLegacy = !header.getWitnessSignature().isEmpty(); - PQAuthSig pqAuthSig = header.getPqAuthSig(); - boolean hasPq = pqAuthSig != null - && pqAuthSig.getSignature() != null - && !pqAuthSig.getSignature().isEmpty(); + BlockHeader header = block.getBlockHeader(); + boolean hasLegacy = !header.getWitnessSignature().isEmpty(); + boolean hasPq = header.hasPqAuthSig() + && !header.getPqAuthSig().getSignature().isEmpty(); + PQAuthSig pqAuthSig = header.getPqAuthSig();🤖 Prompt for 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. In `@chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java` around lines 200 - 223, The null checks on protobuf getters in validateSignature are redundant; update the PQ branch to use header.hasPqAuthSig() and the existing signature emptiness check instead of testing for != null: replace the pqAuthSig != null && pqAuthSig.getSignature() != null && !pqAuthSig.getSignature().isEmpty() logic with a hasPq condition that calls header.hasPqAuthSig() && !header.getPqAuthSig().getSignature().isEmpty(), keeping the rest of validateSignature (and calls to validatePQSignature/validateLegacySignature) unchanged so an explicitly-set-but-empty pq_auth_sig is treated as “no PQ.”
225-242: 💤 Low valuePre-existing NPE risk in legacy path, surfaced by the refactor.
accountStore.get(witnessAccountAddress)can returnnullwhen the witness account is missing from the store; the chained.getWitnessPermissionAddress()will then NPE rather than surface as aValidateSignatureException. The PQ path (lines 262–270) is correctly defensive about this; the legacy path is not. This is pre-existing behavior carried over by the extract, so I'm not blocking on it, but worth a small guard while you're already touching this method.🤖 Prompt for 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. In `@chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java` around lines 225 - 242, The legacy path in validateLegacySignature calls accountStore.get(witnessAccountAddress).getWitnessPermissionAddress() which can NPE if the account is missing; change it to first fetch the account into a local (e.g., AccountCapsule acct = accountStore.get(witnessAccountAddress)), check acct for null, and if null throw a ValidateSignatureException (or return false per the PQ path behavior) before calling acct.getWitnessPermissionAddress(); this ensures validateLegacySignature (and its SignatureException catch) handles missing accounts safely.framework/src/test/java/org/tron/core/capsule/BlockCapsulePQTest.java (1)
24-194: ⚡ Quick winSolid coverage of the new branches.
Activation gating, mutual exclusivity, the PQ-only happy path, tampered-signature, and signer-mismatch are all exercised. Test isolation is fine because each test re-sets
saveAllowFnDsa512.One coverage gap to consider once the scheme-normalization issue I raised on
BlockCapsule.validatePQSignatureis fixed: a test that builds aPQAuthSigwithout callingsetScheme(FN_DSA_512)(so the field stays at the proto3 defaultUNKNOWN_PQ_SCHEME) and asserts that the block still validates after activation. That is the exact path the registry's wire-default doc claims to support, and it's currently uncovered.🤖 Prompt for 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. In `@framework/src/test/java/org/tron/core/capsule/BlockCapsulePQTest.java` around lines 24 - 194, Add a test that verifies a PQAuthSig with the proto3 default scheme (i.e., do NOT call setScheme on PQAuthSig) is accepted after activation: in BlockCapsulePQTest create a witness bound to pqAddress, call dbManager.getDynamicPropertiesStore().saveAllowFnDsa512(1L) and saveAllowMultiSign(1L), build an unsigned block, compute digest, construct a PQAuthSig using buildPQAuthSig-like data but omitting setScheme (so scheme stays UNKNOWN_PQ_SCHEME), attach it to the block via setPqAuthSig and assert block.validateSignature(...) returns true; place the test near the existing pqOnlyAccepted test and reference BlockCapsule.validatePQSignature, PQAuthSig, buildUnsignedBlock and signPQ to locate helpers.chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.java (1)
661-695: 💤 Low valueGating order looks right; one minor diagnostic gap.
The
pqCount > 0 && !isAnyPqSchemeAllowed()short-circuit before per-witness checks is the correct activation gate, and the combinedlegacyCount + pqCountcap againstgetTotalSignNum()is consistent with how legacy multi-sig is bounded.Minor:
logSlowSigVerify(line 705) only logsthis.transaction.getSignatureCount(). Now that PQ verifies are on the same hot path and are typically the slow side (Falcon verify is more expensive than secp recovery), it'd be useful to also loggetPqAuthSigCount()so the slow-path log lets you see which side is dominating.♻️ Optional log-line tweak
void logSlowSigVerify(long startNs) { long costMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNs); if (costMs > SLOW_SIG_VERIFY_MS) { - logger.warn("slow verify: txId={}, sigCount={}, cost={} ms", - getTransactionId(), this.transaction.getSignatureCount(), costMs); + logger.warn("slow verify: txId={}, sigCount={}, pqSigCount={}, cost={} ms", + getTransactionId(), + this.transaction.getSignatureCount(), + this.transaction.getPqAuthSigCount(), + costMs); } }🤖 Prompt for 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. In `@chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.java` around lines 661 - 695, The slow-signature logging currently only records this.transaction.getSignatureCount(); update the slow-path logging to include post-quantum counts by passing or reading this.transaction.getPqAuthSigCount() into the log; specifically modify logSlowSigVerify (or its call site in validateSignature flow) to accept/emit both getSignatureCount() and getPqAuthSigCount() (or have logSlowSigVerify read both from the transaction) so the slow-path message shows legacy and PQ signature counts for easier diagnosis.framework/src/test/java/org/tron/common/crypto/pqc/SignatureSchemeBenchmarkTest.java (1)
22-36: ⚡ Quick winExclude this benchmark from regular CI runs.
This is a
@Testwith no assertions that performs ~1500 ECKey ops + 1500 FN-DSA ops + warmup on every CI run, dumping results to stdout. It will lengthen every test run without ever failing the build. Consider gating it behind@Ignore(run on demand), a JUnit category, or a separate Gradle source set / task so it stays out of the default test pipeline.Two secondary notes worth calling out in the class doc / output header:
- The ECDSA "verify" leg measures signature recovery (
ECKey.signatureToAddress), not direct verification. That is the path TRON actually uses, but readers of the table may misread it as a like-for-likeverifycomparison with FN-DSA.WARMUP = 20is well under the HotSpot C2 compile threshold (~10k); even withITERATIONS = 500the early measurements include interpreted/profile-only runs. JMH (or at least a much larger warmup) would yield more meaningful numbers if these results are ever quoted.🤖 Prompt for 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. In `@framework/src/test/java/org/tron/common/crypto/pqc/SignatureSchemeBenchmarkTest.java` around lines 22 - 36, This test method benchmarkAllSchemes in SignatureSchemeBenchmarkTest should be excluded from regular CI: annotate the method (or class) with `@Ignore` or move it into a non-default test category/Gradle source set or task so it does not run in the default pipeline; additionally update the test/class Javadoc and the printed header to state that the ECDSA "verify" column reflects signature recovery via ECKey.signatureToAddress (not a direct verify) and either increase WARMUP significantly or switch this benchmark to a proper JMH benchmark (or document why current WARMUP/ITERATIONS are insufficient) so readers understand the warmup/measurement limitations.
🤖 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 `@actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java`:
- Around line 2441-2444: The getEnergyForData method in PrecompiledContracts
(used by the 0x16 VerifyFnDsa precompile) returns 2500 which is inconsistent
with ValidateMultiSignPQ.PQ_ENERGY_PER_SIGN and
BatchValidateSignPQ.ENERGY_PER_SIGN (both 15000); update the energy unit so all
three precompiles use the same per-Falcon-512 verify cost: change
getEnergyForData in PrecompiledContracts for VerifyFnDsa to return 15000 (or
alternatively change the two PQ constants to 2500 if you intend the lower cost),
and if you choose 15000 also update the test expectation in FnDsaPrecompileTest
(assert value at the assertion currently checking 2500) to assert 15000.
In `@chainbase/src/main/java/org/tron/core/db/BandwidthProcessor.java`:
- Around line 143-153: The current size calculation in BandwidthProcessor
computes pqAuthSigBytes by summing aw.getSerializedSize(), which misses protobuf
wire framing; instead clear the auth/signature repeated fields on the
transaction builder and measure the payload directly: in the block around
sigOverhead and createAccountBytesSize (in BandwidthProcessor), remove the loop
that sums aw.getSerializedSize(), do not try to subtract a separately computed
pqAuthSigBytes, and replace it by building a copy with the PQ auth and signature
fields cleared (e.g., call
clearPqAuthSigList()/clearPqAuthSig()/clearSignatures/clearRet as appropriate on
trx.getInstance().toBuilder()) and use getSerializedSize() on that cleared
builder to compute createAccountBytesSize so the full wire overhead is included
correctly.
In `@crypto/src/main/java/org/tron/common/crypto/pqc/FNDSA.java`:
- Around line 76-87: The constructor FNDSA(byte[] privateKey, byte[] publicKey)
currently only checks lengths and can accept mismatched halves; update the
constructor to derive the public half from the provided privateKey (using the
class's public-key derivation routine — e.g., the method used elsewhere to
compute a public key from a private key) and compare it to the supplied
publicKey, throwing IllegalArgumentException if they differ; keep the length
checks, clone both arrays on success, and apply the same validation to the other
constructor overload referenced around lines 96-107 so sign(), getPublicKey(),
and getAddress() cannot operate on inconsistent keypairs.
In `@crypto/src/main/java/org/tron/common/crypto/pqc/PQSignature.java`:
- Around line 42-49: The default validatePrivateKey method rejects keys whose
length differs from getPrivateKeyLength(); update it to accept the FN-DSA
extended key form used for local witness config by allowing either the standard
length or the extended f‖g‖F‖h length when getScheme() identifies FN-DSA.
Concretely: keep the null check, but when checking length, allow
privateKey.length == getPrivateKeyLength() OR (getScheme().equals("FN-DSA") &&
privateKey.length == <extendedLength>) so only FN-DSA accepts the larger format;
otherwise throw the same IllegalArgumentException. Ensure you reference
validatePrivateKey, getPrivateKeyLength and getScheme in the change.
In `@framework/src/main/java/org/tron/core/db/Manager.java`:
- Around line 1762-1814: signBlockCapsule currently falls back to
blockCapsule.sign(miner.getPrivateKey()) even when resolveWitnessScheme(miner)
returns null and the miner is PQ-only (no ECDSA key), causing an NPE; update
signBlockCapsule to: call resolveWitnessScheme(miner) once, remove the redundant
PQSchemeRegistry.contains(scheme) check, and if scheme==null then only call
blockCapsule.sign(...) when miner.getPrivateKey() is non-null, otherwise throw a
clear IllegalStateException indicating the miner is configured for PQ scheme X
but has no local signing material; also amend the message in signWitnessAuth
(the throw at pq key null) to reference miner configuration (e.g. "miner has
scheme X set but PQ key material is missing") to distinguish config vs on-chain
permission issues.
In
`@framework/src/test/java/org/tron/common/runtime/vm/BatchValidateSignPQTest.java`:
- Around line 303-310: The run() helper is unconditionally overwriting callers'
per-test CPU-time budget by calling contract.setVmShouldEndInUs(now +
5_000_000L); modify run() (and the duplicate at lines 239-259) so it does not
clobber an existing vmShouldEndInUs set by callers (e.g. atMaxSize16_setsAllBits
and asyncPath_*); either remove the setVmShouldEndInUs call entirely from run(),
or change it to only call contract.setVmShouldEndInUs(...) when the contract has
no budget set (check for a default/sentinel value via an existing getter or add
a hasVmShouldEndInUs/getVmShouldEndInUs and only set when that indicates unset),
ensuring callers’ long timeouts are preserved.
In `@framework/src/test/java/org/tron/core/BandwidthProcessorTest.java`:
- Around line 908-914: Replace the hard-coded byte lengths with the FNDSA
constants and fix the off-by-one public-key size: allocate fakePub using
FNDSA.PUBLIC_KEY_LENGTH (not 897) and fakeSig using FNDSA.SIGNATURE_LENGTH
(instead of 752 literal), updating the occurrences that build Protocol.PQAuthSig
in BandwidthProcessorTest (the blocks creating fakeSig/fakePub and calling
.setPublicKey/.setSignature); apply the same replacement to the second
occurrence around the 970–976 block so both tests use the correct constants and
no longer overstate the witness size.
- Around line 886-939: Test relies on the PQ scheme gate but never enables the
ALLOW_FN_DSA_512 flag; update pqPQAuthWitnessBytesSubtractedInCreateAccountCap
(and the sibling test at 941-999) to explicitly enable the proposal gate on the
dynamic properties store before building the transaction (e.g. call the dynamic
properties method that sets ALLOW_FN_DSA_512 to true via
chainBaseManager.getDynamicPropertiesStore(), then run the test, and finally
restore/clear the flag in the finally block); reference the test method name
pqPQAuthWitnessBytesSubtractedInCreateAccountCap and BandwidthProcessor so you
enable the flag before processor.consume(...) and clean it up afterwards.
---
Nitpick comments:
In `@chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java`:
- Around line 200-223: The null checks on protobuf getters in validateSignature
are redundant; update the PQ branch to use header.hasPqAuthSig() and the
existing signature emptiness check instead of testing for != null: replace the
pqAuthSig != null && pqAuthSig.getSignature() != null &&
!pqAuthSig.getSignature().isEmpty() logic with a hasPq condition that calls
header.hasPqAuthSig() && !header.getPqAuthSig().getSignature().isEmpty(),
keeping the rest of validateSignature (and calls to
validatePQSignature/validateLegacySignature) unchanged so an
explicitly-set-but-empty pq_auth_sig is treated as “no PQ.”
- Around line 225-242: The legacy path in validateLegacySignature calls
accountStore.get(witnessAccountAddress).getWitnessPermissionAddress() which can
NPE if the account is missing; change it to first fetch the account into a local
(e.g., AccountCapsule acct = accountStore.get(witnessAccountAddress)), check
acct for null, and if null throw a ValidateSignatureException (or return false
per the PQ path behavior) before calling acct.getWitnessPermissionAddress();
this ensures validateLegacySignature (and its SignatureException catch) handles
missing accounts safely.
In `@chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.java`:
- Around line 661-695: The slow-signature logging currently only records
this.transaction.getSignatureCount(); update the slow-path logging to include
post-quantum counts by passing or reading this.transaction.getPqAuthSigCount()
into the log; specifically modify logSlowSigVerify (or its call site in
validateSignature flow) to accept/emit both getSignatureCount() and
getPqAuthSigCount() (or have logSlowSigVerify read both from the transaction) so
the slow-path message shows legacy and PQ signature counts for easier diagnosis.
In
`@framework/src/test/java/org/tron/common/crypto/pqc/SignatureSchemeBenchmarkTest.java`:
- Around line 22-36: This test method benchmarkAllSchemes in
SignatureSchemeBenchmarkTest should be excluded from regular CI: annotate the
method (or class) with `@Ignore` or move it into a non-default test
category/Gradle source set or task so it does not run in the default pipeline;
additionally update the test/class Javadoc and the printed header to state that
the ECDSA "verify" column reflects signature recovery via
ECKey.signatureToAddress (not a direct verify) and either increase WARMUP
significantly or switch this benchmark to a proper JMH benchmark (or document
why current WARMUP/ITERATIONS are insufficient) so readers understand the
warmup/measurement limitations.
In `@framework/src/test/java/org/tron/core/capsule/BlockCapsulePQTest.java`:
- Around line 24-194: Add a test that verifies a PQAuthSig with the proto3
default scheme (i.e., do NOT call setScheme on PQAuthSig) is accepted after
activation: in BlockCapsulePQTest create a witness bound to pqAddress, call
dbManager.getDynamicPropertiesStore().saveAllowFnDsa512(1L) and
saveAllowMultiSign(1L), build an unsigned block, compute digest, construct a
PQAuthSig using buildPQAuthSig-like data but omitting setScheme (so scheme stays
UNKNOWN_PQ_SCHEME), attach it to the block via setPqAuthSig and assert
block.validateSignature(...) returns true; place the test near the existing
pqOnlyAccepted test and reference BlockCapsule.validatePQSignature, PQAuthSig,
buildUnsignedBlock and signPQ to locate helpers.
🪄 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: ff20c54c-d8ae-4c46-9fd8-65fa2a421335
📒 Files selected for processing (48)
actuator/src/main/java/org/tron/core/actuator/AccountPermissionUpdateActuator.javaactuator/src/main/java/org/tron/core/utils/ProposalUtil.javaactuator/src/main/java/org/tron/core/vm/PrecompiledContracts.javaactuator/src/main/java/org/tron/core/vm/config/ConfigLoader.javachainbase/src/main/java/org/tron/common/utils/LocalWitnesses.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/db/BandwidthProcessor.javachainbase/src/main/java/org/tron/core/store/DynamicPropertiesStore.javacommon/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/core/Constant.javacommon/src/main/java/org/tron/core/vm/config/VMConfig.javaconsensus/src/main/java/org/tron/consensus/base/Param.javacrypto/src/main/java/org/tron/common/crypto/pqc/FNDSA.javacrypto/src/main/java/org/tron/common/crypto/pqc/PQSchemeRegistry.javacrypto/src/main/java/org/tron/common/crypto/pqc/PQSignature.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/config/args/ConfigKey.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/consensus/ConsensusService.javaframework/src/main/java/org/tron/core/consensus/ProposalService.javaframework/src/main/java/org/tron/core/db/Manager.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/crypto/pqc/FNDSAKatTest.javaframework/src/test/java/org/tron/common/crypto/pqc/FNDSATest.javaframework/src/test/java/org/tron/common/crypto/pqc/PQSchemeRegistryTest.javaframework/src/test/java/org/tron/common/crypto/pqc/PQSignatureDefaultsTest.javaframework/src/test/java/org/tron/common/crypto/pqc/SignatureSchemeBenchmarkTest.javaframework/src/test/java/org/tron/common/crypto/pqc/program/PQClient.javaframework/src/test/java/org/tron/common/crypto/pqc/program/PQFullNode.javaframework/src/test/java/org/tron/common/crypto/pqc/program/PQWitnessNode.javaframework/src/test/java/org/tron/common/runtime/vm/BatchValidateSignPQTest.javaframework/src/test/java/org/tron/common/runtime/vm/FnDsaPrecompileTest.javaframework/src/test/java/org/tron/common/runtime/vm/ValidateMultiSignPQTest.javaframework/src/test/java/org/tron/common/utils/LocalWitnessesTest.javaframework/src/test/java/org/tron/core/BandwidthProcessorTest.javaframework/src/test/java/org/tron/core/actuator/AccountPermissionUpdateActuatorTest.javaframework/src/test/java/org/tron/core/actuator/CreateAccountActuatorTest.javaframework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.javaframework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.javaframework/src/test/java/org/tron/core/capsule/BlockCapsulePQTest.javaframework/src/test/java/org/tron/core/capsule/TransactionCapsuleTest.javaframework/src/test/java/org/tron/core/exception/TronErrorTest.javaframework/src/test/java/org/tron/core/services/ProposalServiceTest.javaframework/src/test/java/org/tron/core/services/http/UtilTest.javaframework/src/test/resources/config-test.confprotocol/src/main/protos/core/Tron.proto
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 48 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/resources/config.conf">
<violation number="1" location="framework/src/main/resources/config.conf:821">
P2: The added sample committee key name is incorrect (`allowMlDsa`) and does not match the implemented gate (`allowFnDsa512`), which can mislead operators into setting a non-functional config.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/Args.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/Args.java:994">
P2: Uppercase `"0X"` prefix is silently passed through (not stripped) and later produces a confusing length-mismatch error. Add an explicit check and rejection for the `"0X"` prefix to fail early with a clear message.
(Based on your team's feedback about case-sensitive 0x prefix handling for hex inputs.) [FEEDBACK_USED]</violation>
</file>
<file name="framework/src/main/java/org/tron/core/db/Manager.java">
<violation number="1" location="framework/src/main/java/org/tron/core/db/Manager.java:1767">
P1: NullPointerException when a PQ-only witness falls back to ECDSA signing. If `resolveWitnessScheme` returns null (e.g., the `ALLOW_FN_DSA_512` proposal is not yet activated), a PQ-only miner (whose ECDSA `privateKey` is `null`) will trigger an NPE in `blockCapsule.sign(null)`. Add a null guard and throw a descriptive error instead of silently falling through.</violation>
</file>
<file name="framework/src/test/java/org/tron/common/crypto/pqc/program/PQClient.java">
<violation number="1" location="framework/src/test/java/org/tron/common/crypto/pqc/program/PQClient.java:141">
P2: Use the configured hash function here instead of hardcoding SHA-256; otherwise the demo breaks on non-ECKey crypto-engine configurations.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
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/config/args/Args.java`:
- Around line 962-1004: The code unconditionally accepts
ConfigKey.LOCAL_WITNESS_PQ_KEYS and initializes PQ-only witnesses
(WitnessInitializer.initFromPQOnly) even when FN-DSA-512 is not enabled; gate
this path by checking the same activation flag used at runtime
(PARAMETER.allowFnDsa512 or the existing activation check used elsewhere) before
parsing pqEntries and constructing pqPrivateKeys/pqPublicKeys, and if the flag
is false throw a TronError (ErrCode.WITNESS_INIT) rejecting
LOCAL_WITNESS_PQ_KEYS so PQ-only config is not accepted pre-activation.
- Around line 991-992: The hex prefix check only strips lowercase "0x", so
update the logic in Args.java where 'hex' and 'stripped' are computed to also
accept uppercase "0X" (e.g., use a case-insensitive check or call
hex.toLowerCase().startsWith("0x") before substring) so that 'stripped'
correctly removes either "0x" or "0X" prior to the length check against
'extHexLen'; keep the rest of the length validation unchanged.
🪄 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: 74440a04-fb7e-4561-85f2-139fbb210ac3
📒 Files selected for processing (4)
actuator/src/main/java/org/tron/core/utils/ProposalUtil.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java
There was a problem hiding this comment.
9 issues found across 48 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="protocol/src/main/protos/core/Tron.proto">
<violation number="1" location="protocol/src/main/protos/core/Tron.proto:261">
P2: The PQ address-derivation comment uses the wrong hash slice (`[0:20]`). Implementation and tests use the rightmost 20 bytes (`[12..32]`), so this spec line can cause incompatible address derivation.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/BandwidthProcessorTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/BandwidthProcessorTest.java:909">
P3: The test fixture uses an invalid FN_DSA_512 public key length (897 instead of 896), which makes the new PQ bandwidth tests use non-protocol input.</violation>
</file>
<file name="chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.java">
<violation number="1" location="chainbase/src/main/java/org/tron/core/capsule/TransactionCapsule.java:662">
P2: Shielded-transfer transactions from shielded addresses do not reject or validate `pq_auth_sig[]`. This bypasses the intended no-transparent-signature rule and skips the PQ activation gate for that path.</violation>
</file>
<file name="framework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java">
<violation number="1" location="framework/src/test/java/org/tron/core/actuator/utils/ProposalUtilTest.java:812">
P2: Initialize `forkUtils` in this test before using `getManager()`. Without explicit init, the test depends on another test's side effects and can fail when run in isolation/order changes.</violation>
</file>
<file name="chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java">
<violation number="1" location="chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java:205">
P2: Mutual-exclusion validation is bypassable because `pq_auth_sig` is considered present only when its `signature` bytes are non-empty.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/consensus/ConsensusService.java">
<violation number="1" location="framework/src/main/java/org/tron/core/consensus/ConsensusService.java:106">
P1: PQ-only miner initialization leaves `privateKey` null, but the signing path falls back to legacy signing when PQ is not activated, which can break block production at runtime.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java:141">
P2: Reject `localWitnessAccountAddress` when multiple PQ keypairs are configured; otherwise the config is accepted but ignored at runtime.</violation>
</file>
<file name="actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java">
<violation number="1" location="actuator/src/main/java/org/tron/core/vm/PrecompiledContracts.java:2560">
P0: ECDSA deduplication is signature-specific instead of address-specific, so one signer can be counted multiple times with different valid signatures and bypass multi-sign threshold intent.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/Args.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/Args.java:962">
P1: Gate `localwitness_pq_keys` initialization behind `ALLOW_FN_DSA_512`; otherwise a PQ-only witness can be configured before activation and later hit legacy signing fallback with no ECDSA private key.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Sunny6889
reviewed
May 11, 2026
Sunny6889
reviewed
May 11, 2026
Sunny6889
reviewed
May 11, 2026
Sunny6889
reviewed
May 11, 2026
Sunny6889
reviewed
May 11, 2026
Sunny6889
reviewed
May 11, 2026
Sunny6889
reviewed
May 11, 2026
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
1 issue found across 22 files (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/common/runtime/vm/ValidateMultiFnDsa512Test.java">
<violation number="1" location="framework/src/test/java/org/tron/common/runtime/vm/ValidateMultiFnDsa512Test.java:287">
P1: Energy expectation is underpriced: this assertion accepts PQ multisig cost as 2000 instead of the specified 15000, masking a costly pricing regression.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Federico2014
force-pushed
the
pqc-falcon512
branch
from
fce7819 to
fee23f6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Federico2014
force-pushed
the
pqc-falcon512
branch
from
fee23f6 to
7067a00
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Federico2014
force-pushed
the
pqc-falcon512
branch
from
7067a00 to
c32015b
Compare
There was a problem hiding this comment.
1 issue found across 13 files (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="consensus/src/main/java/org/tron/consensus/pbft/message/PbftMessage.java">
<violation number="1" location="consensus/src/main/java/org/tron/consensus/pbft/message/PbftMessage.java:128">
P1: PQ PBFT signing clears `signature`, but commit aggregation still consumes only `getSignature()`, so PQ commit signatures are lost from PBFT commit proofs.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
There was a problem hiding this comment.
1 issue found across 6 files (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/main/java/org/tron/core/net/messagehandler/PbftDataSyncHandler.java">
<violation number="1" location="framework/src/main/java/org/tron/core/net/messagehandler/PbftDataSyncHandler.java:138">
P0: PBFT quorum is counted by unique signature entries instead of unique SR addresses, so repeated valid PQ signatures from one SR can be counted as multiple votes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Sunny6889
reviewed
May 14, 2026
Sunny6889
reviewed
May 14, 2026
There was a problem hiding this comment.
1 issue found across 23 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1. generalize pq signature length predicate across schemes 2. drop fndsa512 private-key length override 3. correct 0x16 fn-dsa slot abi comment 4. document precompile return-semantics convention 5. clarify mixed-mode localwitnessaddress error message 6. guard relayservice pq branch against empty keypair list 7. gate pq miner on its specific scheme flag
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Federico2014
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Federico2014
force-pushed
the
pqc-falcon512
branch
from
0dd8bd4 to
f7f0c62
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Federico2014
force-pushed
the
pqc-falcon512
branch
from
f7f0c62 to
ae3df1b
Compare
Federico2014
force-pushed
the
pqc-falcon512
branch
from
ae3df1b to
1d6068d
Compare
…tron into pqc-falcon512 # Conflicts: # actuator/src/main/java/org/tron/core/actuator/VMActuator.java
Mirrors block_fetch_latency for transactions: measures the end-to-end round-trip from sending GET_DATA to receiving the full TXS message, using the timestamp already stored in PeerConnection.advInvRequest at fetch-dispatch time. Records nothing for transactions pushed via gossip (no prior GET_DATA), which is intentional — this metric only captures the active-fetch path. Overhead is ~500 ns per tx (Item allocation + ConcurrentHashMap.remove + currentTimeMillis + histogram observe), negligible even at >1k TPS. Useful for PQ migration baseline / stress-test comparison: shows how much extra time the bigger Falcon-512 payloads add to in-flight transaction propagation, independent of local processing cost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ransaction The original placement in handleTransaction() was dead code: processMessage() drains advInvRequest at L92-95 (before enqueueing tx work onto the smartContract/queue handlers), so by the time the worker thread reaches handleTransaction() the timestamp is already gone and remove(item) always returns null. Move the histogram observe into the same draining loop in processMessage(), using a single currentTimeMillis() reference captured just before the loop. This is both correct (we observe at the only remove() call that ever sees a non-null value) and slightly cheaper (one currentTimeMillis() per TRXS message instead of per tx). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feature: add fastforward hello message latency metric
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
4 participants
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
Adds post-quantum (PQ) signature support to TRON across protocol, crypto, transaction/block validation, TVM precompiles, witness configuration, consensus signing, relay handshakes, governance flags, and tests.
This PR currently supports two PQ schemes:
FN_DSA_512/ Falcon-512ML_DSA_44/ Dilithium-2Each scheme is independently gated by a committee proposal and dispatched through
PQSchemeRegistry.Protocol changes
PQSchemewithUNKNOWN_PQ_SCHEME,FN_DSA_512, andML_DSA_44.PQAuthSig { scheme, public_key, signature }as the common PQ authentication envelope.repeated pq_auth_sigtoTransactionso ECDSA signatures and PQ signatures can co-exist for account permission threshold checks.pq_auth_sigtoBlockHeader; block headers must use either legacywitness_signatureor PQpq_auth_sig.pq_auth_sigtoHelloMessagefor fast-forward / relay authentication by PQ witnesses.Address derivation
0x41 || deriveHash(scheme, public_key)[12..32].PQSchemeRegistry.computeAddress(scheme, publicKey)is the single address derivation entry point.Crypto module
FNDSA512for Falcon-512 signing and verification.MLDSA44for ML-DSA-44 signing and verification.PQSignature,PQSchemeRegistry, andPqKeypairas the shared PQ abstraction layer.PQSchemeRegistry.Governance and activation
ALLOW_FN_DSA_512proposal parameter.ALLOW_ML_DSA_44proposal parameter.VERSION_4_8_2.Witness and consensus support
localwitness_pq.keys.Transaction validation
TransactionCapsulevalidates mixed ECDSA + PQ signatures against the same account permission threshold.Permission.keys[].address.Block validation
TVM precompiles
0x16 verifyFnDsa512: single Falcon-512 verification.0x18 batchValidateFnDsa512: batch Falcon-512 verification with bitmap result.0x12 verifyMlDsa44Eip8051: EIP-8051VERIFY_MLDSAverification using[msg 32B | sig 2420B | expandedPk 20512B].0x19 verifyMlDsa44: existing TRON draft ML-DSA-44 verification using the standard 1312-byte public key.0x1a validateMultiPQSig: unified ECDSA + PQ account-permission threshold verification.0x1b batchValidateMlDsa44: batch ML-DSA-44 verification with bitmap result.Relay / fast-forward support
RelayServicecan sign and verifyHelloMessageusing either legacy signatures orPQAuthSig.Compatibility
UNKNOWN_PQ_SCHEMEis reserved and never treated as a valid signing scheme.Tests
PQSchemeRegistryandPQSignaturetests.