feat(crypto): add Falcon-512 post-quantum signature support#26
feat(crypto): add Falcon-512 post-quantum signature support#26Federico2014 wants to merge 9 commits into
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.
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.
| * part of {@code raw_data}.</li> | ||
| * </ol> | ||
| */ | ||
| static long validatePQSignature(Transaction transaction, Permission permission, |
There was a problem hiding this comment.
还有 BlockCapsule里面也有几乎一样的 validatePQSignature， 要把他们合并成一个通用的函数
| if (this.transaction.getRawData().getContractCount() <= 0) { | ||
| throw new ValidateSignatureException("miss contract"); | ||
| } | ||
| if (legacyCount + pqCount > dynamicPropertiesStore.getTotalSignNum()) { |
There was a problem hiding this comment.
when !dynamicPropertiesStore.allowFnDsa512()
pqCount should reset = 0
| int legacyCount = this.transaction.getSignatureCount(); | ||
| int pqCount = this.transaction.getPqAuthSigCount(); | ||
|
|
||
| if (pqCount > 0 && !dynamicPropertiesStore.isAnyPqSchemeAllowed()) { |
There was a problem hiding this comment.
这个 dynamicPropertiesStore.isAnyPqSchemeAllowed() flag 挺好，那就统一用这个来表明是否允许pq算法吧，因为有的地方用dynamicPropertiesStore.allowFnDsa512() ，就容易混乱，预编译合约那里也都改成dynamicPropertiesStore.isAnyPqSchemeAllowed()这个吧
| } else { //transfer from shielded address | ||
| if (this.transaction.getSignatureCount() > 0) { | ||
| if (this.transaction.getSignatureCount() > 0 | ||
| || this.transaction.getPqAuthSigCount() > 0) { |
There was a problem hiding this comment.
if (this.transaction.getSignatureCount() > 0
|| (this.transaction.getPqAuthSigCount() > 0 && dynamicPropertiesStore.isAnyPqSchemeAllowed()) )
| long maxCreateAccountTxSize = dynamicPropertiesStore.getMaxCreateAccountTxSize(); | ||
| int signatureCount = trx.getInstance().getSignatureCount(); | ||
| long sigOverhead = signatureCount * PER_SIGN_LENGTH; | ||
| if (trx.getInstance().getPqAuthSigCount() > 0) { |
There was a problem hiding this comment.
if (dynamicPropertiesStore.isAnyPqSchemeAllowed())
|
|
||
| /** Returns true iff at least one post-quantum signature scheme is currently activated. */ | ||
| public boolean isAnyPqSchemeAllowed() { | ||
| return allowFnDsa512(); |
There was a problem hiding this comment.
这里建议结合 PQSchemeRegistry.SCHEMES, 遍历里面所有的schemes 检查下面 isPqSchemeAllowed 这样PQSchemeRegistry里面任何添加新的schemes 这里都会自动逻辑同步。不然，这里就绑定死了FnDsa512，如何有任何PQ算法增删，这里都要记得去改，不然就造成bug了。
| return false; | ||
| } | ||
| switch (scheme) { | ||
| case UNKNOWN_PQ_SCHEME: // proto3 default → Falcon-512 (see PQSchemeRegistry#resolve) |
There was a problem hiding this comment.
[SHOULD] Drop the UNKNOWN_PQ_SCHEME fall-through to FN_DSA_512
This switch maps UNKNOWN_PQ_SCHEME (proto3 default-zero) onto allowFnDsa512(). Combined with PQSchemeRegistry.resolve(UNKNOWN_PQ_SCHEME) -> FN_DSA_512, this permanently freezes wire-format scheme=0 to mean Falcon-512. Once mainnet activates, a future ML-DSA addition cannot distinguish 'client did not set scheme' from 'client meant FN_DSA_512' — only a hard fork can recover.
Suggestion: Return false for UNKNOWN_PQ_SCHEME here and have PQSchemeRegistry.resolve reject scheme=0 too. All producers (WitnessInitializer / PbftMessage / TransactionCapsule / BlockCapsule) already set scheme explicitly, so migration cost is near zero.
|
|
||
| @Override | ||
| public long getEnergyForData(byte[] data) { | ||
| return 4000; |
There was a problem hiding this comment.
[SHOULD] Reconcile on-chain energy pricing with the PR description
The PR description quotes "15000 energy" for the PQ precompiles, but the code constants are:
- 0x16 VerifyFnDsa512 base:
return 4000(this line) - 0x17 ValidateMultiFnDsa512:
PQ_ENERGY_PER_SIGN = 2000/ECDSA_ENERGY_PER_SIGN = 1500(line ~2531) - 0x18 BatchValidateFnDsa512:
ENERGY_PER_SIGN = 2000(line ~2683)
Energy is a consensus parameter — integrators will price contracts based on whichever number they read. The mismatch will mislead.
Suggestion: Pick the canonical number once (factor in the offline economic model assessment) and align both directions — either update PR description / release notes to 4000/2000/2000, or update the constants to whatever the economic model concluded.
| // permission missing, etc.). Surface a clear cause; DposTask's | ||
| // Throwable handler will log and the witness will miss this slot, | ||
| // but the producer thread keeps running. | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
[SHOULD] Harden toggle-back liveness with three-layer protection
This throw surfaces the symptom (PQ-only miner cannot sign because scheme is no longer usable) but leaves the underlying liveness gap open: an ALLOW_FN_DSA_512 proposal can flip 1 → 0 while PQ-only SRs remain in the active set, causing those slots to be missed every round until rotation. Mirror risk exists for a future 'PQ-only' restriction enabling without all SRs holding PQ keys.
Suggested three-layer mitigation (complementary, not exclusive):
-
Governance layer — before mainnet activation, require all active SRs to hold both ECDSA and PQ signing capability. Zero-code; communicated via release notes / SR onboarding.
-
Protocol hard gate — in
ProposalApproveActuator(orManager.maintenancewhen applying the new value), scan next maintenance period's active SR list. If toggle-back (1 → 0) leaves any PQ-only SR without an ECDSA key, reject the proposal. Mirror logic for a future PQ-only restriction. -
Runtime fallback — when a single-signature-scheme proposal takes effect, evict from the active SR set any SR that does not satisfy the new requirement (so they don't get scheduled as producers).
L2 + L3 together turn this from a fragile operational convention into a protocol-level liveness invariant. L1 can run as a separate operational track.
| byte[] pk = new byte[PUBLIC_KEY_LENGTH]; | ||
| System.arraycopy(privateKey, PRIVATE_KEY_LENGTH, pk, 0, PUBLIC_KEY_LENGTH); | ||
| return pk; | ||
| } |
There was a problem hiding this comment.
if (privateKey == null) {
throw new IllegalArgumentException("privateKey must not be null");
}
| System.arraycopy(privateKey, f.length + g.length, bigF, 0, bigF.length); | ||
| FalconPrivateKeyParameters sk = new FalconPrivateKeyParameters(PARAMS, f, g, bigF, new byte[0]); | ||
| FalconSigner signer = new FalconSigner(); | ||
| signer.init(true, new ParametersWithRandom(sk, new SecureRandom())); |
There was a problem hiding this comment.
每次签名 new 一个 SecureRandom。在 Linux 上首次构造会走 /dev/random（或NativePRNG），可能有几 ms 的播种开销。标准做法：
private static final SecureRandom RNG = new SecureRandom();
...
signer.init(true, new ParametersWithRandom(sk, RNG));
| boolean hasKeystore = !lwConfig.getKeystores().isEmpty(); | ||
| boolean hasPqKeys = config.hasPath(ConfigKey.LOCAL_WITNESS_PQ_KEYS) | ||
| && !config.getStringList(ConfigKey.LOCAL_WITNESS_PQ_KEYS).isEmpty(); | ||
| if (hasPqKeys && (hasCliPriv || hasCfgPriv || hasKeystore)) { |
There was a problem hiding this comment.
pqkeys跟 CfgPriv 不能同时有吗，就config.conf文件里面不能填localwitness 和 localwitness_pq 两个吗？如果一拖多个SR, 有的SR用ECDSA有的用PQ
| int privHexLen = PQSchemeRegistry.getPrivateKeyLength(scheme) * 2; | ||
| int extHexLen = privHexLen + PQSchemeRegistry.getPublicKeyLength(scheme) * 2; | ||
| List<String> pqPrivateKeys = new ArrayList<>(pqEntries.size()); | ||
| List<String> pqPublicKeys = new ArrayList<>(pqEntries.size()); |
There was a problem hiding this comment.
这里我建议添加一个：
public class PqKeypair {
String privateKey;
String publicKey;
}
使用：
List<PqKeypair> pqKeys = new ArrayList<>();
pqKeys.add(new PqKeypair(priv, pub));
String priv = pqKeys.get(0).getPrivateKey(); // ← 一眼看懂
String pub = pqKeys.get(0).getPublicKey();
因为pqPrivateKeys和pqPublicKeys其实是一一对应的紧密关系，俩array分散存放，后续使用的函数还得检查一下
| return !getInstance().getBlockHeader().getWitnessSignature().isEmpty(); | ||
| BlockHeader header = getInstance().getBlockHeader(); | ||
| return !header.getWitnessSignature().isEmpty() | ||
| || !header.getPqAuthSig().getSignature().isEmpty(); |
There was a problem hiding this comment.
[SHOULD] hasWitnessSignature() is weak on the PQ branch
return !header.getWitnessSignature().isEmpty()
|| !header.getPqAuthSig().getSignature().isEmpty();The PQ side only checks whether the signature bytes are non-empty. It does not verify that pq_auth_sig.scheme is set (proto3 default-zero means UNKNOWN_PQ_SCHEME), nor that the signature length matches the scheme. A malformed or partially-populated PQAuthSig (e.g. signature bytes from an aborted producer flow, scheme left at 0) passes this gate and later fails deeper in the verify path where the failure mode is less obvious.
Suggestion: tighten the PQ check to also require pq_auth_sig.scheme != UNKNOWN_PQ_SCHEME (or use PQSchemeRegistry.resolve(scheme).isPresent()), and consider asserting signature.size() == PQSchemeRegistry.getSignatureLength(scheme) for fail-fast. Mirrors the mutex invariant documented on the proto: {witness_signature, pq_auth_sig} is exactly one — empty bytes shouldn't be the only signal.
| "PQ private keys must be set for PQ-only witness nodes", | ||
| TronError.ErrCode.WITNESS_INIT); | ||
| } | ||
| if (pqPrivateKeys.size() != pqPublicKeys.size()) { |
| System.arraycopy(privateKey, f.length + g.length, bigF, 0, bigF.length); | ||
| FalconPrivateKeyParameters sk = new FalconPrivateKeyParameters(PARAMS, f, g, bigF, new byte[0]); | ||
| FalconSigner signer = new FalconSigner(); | ||
| signer.init(true, new ParametersWithRandom(sk, new SecureRandom())); |
There was a problem hiding this comment.
[NIT] Document randomized signing here
new SecureRandom() per sign() invocation is correct — BC's Falcon implementation requires fresh randomness on every signature. But it's worth a one-line javadoc to set integrator expectations: keygen is deterministic from the 48-byte seed, signing is NOT — same (key, message) yields different signature bytes each call.
Why this matters: any downstream code that caches or dedups by sha(signature_bytes) will silently fail. The protocol-level dedup correctly keys on the derived address (PrecompiledContracts.java PQ branch at line ~2636), but integrators reading FNDSA512 in isolation may not infer that constraint.
Suggestion: add a one-liner above sign(): // Signing is randomized; same (key, hash) yields different sig bytes per call. Only keygen is deterministic from the seed.
| } | ||
| }); | ||
|
|
||
| private LoadingCache<String, List<PQAuthSig>> pqSignCache = CacheBuilder.newBuilder() |
There was a problem hiding this comment.
[SHOULD] Declare explicit GC / memory bound for pqSignCache
pqSignCache mirrors the existing agreeCommitMsgCache policy, but each entry now holds a ~1.6KB Falcon signature instead of a ~65B ECDSA sig — roughly 25× per-entry footprint. Under the same TTL/capacity policy, peak memory grows accordingly. There is no visible cap or eviction comment that flags this to operators.
Suggestion: (a) add a class-level javadoc with worst-case bytes (e.g. agreeNodeCount * SIGNATURE_LENGTH * window-slots ≈ N MB); (b) consider a configurable size cap or a separate, tighter eviction policy for the PQ cache so PQ-heavy deployments can tune memory without affecting ECDSA flow.
| param.setAgreeNodeCount(parameter.getAgreeNodeCount()); | ||
| List<Miner> miners = new ArrayList<>(); | ||
| List<String> privateKeys = Args.getLocalWitnesses().getPrivateKeys(); | ||
| List<String> pqPrivateKeys = Args.getLocalWitnesses().getPqPrivateKeys(); |
There was a problem hiding this comment.
Please use List<PqKeypair> pqKeys = Args.getLocalWitnesses().getPqKeypairs(); and remove the below unnecessary pqPublicKeys.size() != pqPrivateKeys.size()
| Miner miner = param.new Miner(privateKey, ByteString.copyFrom(privateKeyAddress), | ||
| ByteString.copyFrom(witnessAddress)); | ||
| miners.add(miner); | ||
| } else if (pqPrivateKeys.size() > 1) { |
There was a problem hiding this comment.
add DynamicPropertiesStore.isAnyPqSchemeAllowed() here and above in line 53?
| scheme, Hex.toHexString(pqAddress), miners.size()); | ||
| } | ||
| } else if (pqPrivateKeys.size() == 1) { | ||
| miners.add(buildPQOnlyMinerFromKeypair(param, pqPrivateKeys.get(0), pqPublicKeys.get(0))); |
There was a problem hiding this comment.
这个buildPQOnlyMinerFromKeypair 一定要单独写吗？跟上面当 pqPrivateKeys.size() > 1 时逻辑大量相同？
|
|
||
| private final ValidateMultiFnDsa512 contract = new ValidateMultiFnDsa512(); | ||
|
|
||
| @Before |
There was a problem hiding this comment.
[NIT] Pair this @before with @after to reset VMConfig.allowFnDsa512
@Before flips VMConfig.initAllowFnDsa512(true) but no @After resets it. Subsequent tests in the same JVM that read VMConfig.allowFnDsa512() observe a stale true, which hides gate-disabled regressions.
FnDsaPrecompileTest correctly pairs Before/After — same pattern should apply here.
Suggestion:
@After
public void tearDown() {
VMConfig.initAllowFnDsa512(false);
}Or capture the prior value in @Before and restore it for full isolation.
| import org.tron.protos.Protocol.PQAuthSig; | ||
| import org.tron.protos.Protocol.PQScheme; | ||
|
|
||
| public class PbftPQMessageTest { |
There was a problem hiding this comment.
[NIT] Document this test's isolation scope vs BlockCapsule.validateSignature
This test uses new BlockCapsule(Block.getDefaultInstance()) and never wires ChainBaseManager, so it deliberately sidesteps the global state required by BlockCapsule.validateSignature (which reads DynamicPropertiesStore). That's fine for testing PbftMessage.analyzeSignature in isolation — but a future reader copying this pattern to test block-level validation will hit NullPointerException and waste time tracing it.
Suggestion: add a class-level javadoc making the scope explicit, e.g.:
/**
* Tests PbftMessage.analyzeSignature directly without spinning up ChainBaseManager.
* Block-level validation (which depends on DynamicPropertiesStore) is covered
* separately in BlockCapsulePQTest.
*/
| } | ||
| byte[] witnessAddress = miner.getWitnessAddress().toByteArray(); | ||
| AccountCapsule accountCapsule = chainBaseManager.getAccountStore().get(witnessAddress); | ||
| if (accountCapsule == null || !accountCapsule.getInstance().hasWitnessPermission()) { |
There was a problem hiding this comment.
这个如果是SR自己产块没有把witness权限给别人，默认情况下 hasWitnessPermission是会返回空的，是正常的，不该返回null.
| } | ||
| } | ||
|
|
||
| private PQScheme resolveWitnessScheme(Miner miner) { |
There was a problem hiding this comment.
这个 resolveWitnessScheme 干了挺多事的
- 检查是否允许pq签名
- 检查miner是否有签名权限。
- 然后返回签名scheme
就是这个函数名称看起来只是获取下scheme呢？不要隐含这么多逻辑，要么就在调用方显式的调各种逻辑。
| } | ||
|
|
||
| private PQScheme resolveWitnessScheme(Miner miner) { | ||
| if (!chainBaseManager.getDynamicPropertiesStore().isAnyPqSchemeAllowed()) { |
There was a problem hiding this comment.
需要把 chainBaseManager.getDynamicPropertiesStore().isAnyPqSchemeAllowed() 放到 line1765也就是调用方那里，不要深入函数内部再检查这个了。
| PQScheme scheme = 1; | ||
| bytes public_key = 2; | ||
| bytes signature = 3; | ||
| } |
There was a problem hiding this comment.
这里也需要PB签名
和对应输入解析
message TXInput {
message raw {
bytes txID = 1;
int64 vout = 2;
bytes pubKey = 3;
}
raw raw_data = 1;
bytes signature = 4;
}
| * off-line and ship both halves to the node. | ||
| */ | ||
| @Getter | ||
| private List<String> pqPrivateKeys = Lists.newArrayList(); |
There was a problem hiding this comment.
From here use PqKeypair
public class PqKeypair {
String privateKey;
String publicKey;
}
| boolean hasCliPriv = StringUtils.isNotBlank(cmd.privateKey); | ||
| boolean hasCfgPriv = !lwConfig.getPrivateKeys().isEmpty(); | ||
| boolean hasKeystore = !lwConfig.getKeystores().isEmpty(); | ||
| boolean hasPqKeys = config.hasPath(ConfigKey.LOCAL_WITNESS_PQ_KEYS) |
| @@ -0,0 +1,13 @@ | |||
| package org.tron.core.config.args; | |||
|
|
|||
| public final class ConfigKey { | |||
3 participants
Summary
Adds post-quantum (PQ) signature support to TRON with FN-DSA / Falcon-512 (FIPS-206 draft) as the launch scheme. The change spans the protocol, crypto, chainbase, actuator, framework, and TVM layers, gated behind a single proposal
ALLOW_FN_DSA_512.Protocol changes (
Tron.proto)PQScheme—UNKNOWN_PQ_SCHEME = 0is reserved for forward compatibility;FN_DSA_512 = 1is the launch scheme so on-chain entries pay zero bytes for the default tag.PQAuthSig { PQScheme scheme; bytes public_key; bytes signature; }carries the scheme tag, raw public key (896 B for Falcon-512), and signature (≤ 752 B for Falcon-512).BlockHeader.raw_datagainspq_auth_sig. A block carries exactly one of{witness_signature, pq_auth_sig}— they are mutually exclusive at the header level (no dual-signing).Transactiongainsrepeated PQAuthSig pq_auth_sig. ECDSAsignatureentries andpq_auth_sigentries may co-exist on multi-sig transactions; each co-signer is matched against the permission by its derived address (no positionalkey_idalignment) and contributes weight independently to the permission's threshold.Address derivation
0x41 ‖ Keccak-256(public_key)[12..32]— the same scheme used for ECDSA accounts. The public key acts as the in-band fingerprint, so accounts do not need a separate stored PQ key blob.PQSchemeRegistry.computeAddress(scheme, publicKey).Crypto module
FNDSAwraps BouncyCastle 1.79's Falcon-512 with deterministic key derivation from a 48-byte seed.SEED_LENGTH = 48,PRIVATE_KEY_LENGTH = 1280(f‖g‖F),PUBLIC_KEY_LENGTH = 896,SIGNATURE_LENGTH ≤ 752,PRIVATE_KEY_WITH_PUBLIC_KEY_LENGTH = 2176(f‖g‖F‖h).FNDSA(byte[] seed)— strict 48-byte seed.FNDSA(byte[] priv, byte[] pub)— strict 1280/896 split.FNDSA.fromPrivateKeyWithPublicKey(byte[])— static factory for the 2176-byte extended form. (A secondFNDSA(byte[])overload would clash with the seed constructor, hence the factory method.)derivePublicKey(byte[] extendedPriv)extractshfrom the extended encoding — needed because BouncyCastle has no public API to recomputeh = g · f⁻¹ mod qfrom the bare 1280-byte private key (BC issue Feature/event uint parse tronprotocol/java-tron#2297).PQSchemeRegistryis the single dispatch point forsign / verify / computeAddress / key lengthqueries byPQScheme.Witness configuration
localwitness_pq_scheme = "FN_DSA_512"selects the active scheme.localwitness_pq_keysis now a flat list of hex strings, each the 4352-char extended formf‖g‖F‖h. The redundant{priv, pub}dual-field shape was removed — the public key is derivable from the extended encoding viaderivePublicKey, so storing both fields was pure duplication.Argsparses entries withgetStringList(...), validates the hex length against the active scheme, and slices into the existing(priv, pub)lists used byLocalWitnesses/ConsensusService.TVM precompiles
0x16 verifyFnDsa512(msg, publicKey, signature)— single-signature verification, energy 15 000.0x17 ValidateMultiSignPQ— mixed ECDSA + PQ multi-sign verification against an account permission. Per-signature energy: 1 500 (ECDSA), 15 000 (PQ).0x18 BatchValidateSignPQ(msg, sigs[], pubKeys[])— bitmap-style batch verifier returning a packed result word. Same per-signature pricing as0x17.Account-level wiring
Account.Permissionaccepts entries whoseaddresswas derived from a PQ public key;ValidateMultiSignPQmatches signatures against permission keys by address.AccountCreateContractwhose owner address comes from a PQ permission, signed viaPQAuthSig.Test coverage
0x16/0x17/0x18covering happy path, malformed input, mixed ECDSA+PQ permissions, and bitmap encoding.PQWitnessNode+PQClient): boots an in-process node with a Falcon-512 witness keypair, broadcasts a Falcon-512 TRX transfer (~1670 B vs. 175 B ECDSA), produces a PQ-signed block.Proposal gating
All PQ behaviour is gated on
ALLOW_FN_DSA_512. Pre-activation, every PQ codepath either rejects or is unreachable; post-activation the behaviour above becomes available.Compatibility
pq_auth_sigfields are zero-encoded when absent.Updates after rebase to develop
Refinements layered on top of the original PR #23 squash:
ALLOW_FN_DSA_512activation flow — proposal validation is now fork-gated onVERSION_4_8_2and accepts both0and1(toggleable post-activation), matchingALLOW_HARDEN_EXCHANGE_CALCULATION. ID renumbered to99(was100in the original PR) —97/98were taken on develop byALLOW_HARDEN_RESOURCE_CALCULATION/ALLOW_HARDEN_EXCHANGE_CALCULATION, so99is the next free slot.localwitness.account_addressis now optional. When omitted, the witness address is derived from the first PQ public key viaPQSchemeRegistry.computeAddress(scheme, publicKey). PQ-only setup is consolidated insideWitnessInitializer.initFromPQOnly(scheme, priv, pub, accountAddress)instead of being scattered acrossArgs.java.Summary by CodeRabbit
New Features
Chores
Tests