fix(config): refactor the config - #40
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 16 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="common/src/main/resources/reference.conf">
<violation number="1" location="common/src/main/resources/reference.conf:789">
P2: Commented PQ key examples contradict the "exactly one of `key`/`seed`" rule by showing both fields with one empty string. If a user uncomments these examples, startup will fail because validation treats any non-null value as present.</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:291">
P2: Hex prefix validation for PQ witness config fields should reject uppercase `0X` and enforce the strict lowercase `0x` prefix policy per project validation rules. The `stripHexPrefix` helper silently normalizes `0X`, allowing inconsistent config formats to pass validation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -764,36 +764,34 @@ localwitness = [ | |||
| # "localwitnesskeystore.json" | |||
There was a problem hiding this comment.
P2: Commented PQ key examples contradict the "exactly one of key/seed" rule by showing both fields with one empty string. If a user uncomments these examples, startup will fail because validation treats any non-null value as present.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/main/resources/reference.conf, line 789:
<comment>Commented PQ key examples contradict the "exactly one of `key`/`seed`" rule by showing both fields with one empty string. If a user uncomments these examples, startup will fail because validation treats any non-null value as present.</comment>
<file context>
@@ -764,36 +764,34 @@ localwitness = [
+ # accountAddress =
+
+ keys = [
+ # { scheme = "FN_DSA_512", seed = "", key = "<4352 hex chars>" },
+ # { scheme = "ML_DSA_44", seed = "", key = "<7744 hex chars>" },
+ # { scheme = "FN_DSA_512", seed = "<96 hex chars>", key = "" }, # drift risk — see above
</file context>
| # "localwitnesskeystore.json" | |
| + # { scheme = "FN_DSA_512", key = "<4352 hex chars>" }, | |
| + # { scheme = "ML_DSA_44", key = "<7744 hex chars>" }, | |
| + # { scheme = "FN_DSA_512", seed = "<96 hex chars>" }, # drift risk — see above | |
| + # { scheme = "ML_DSA_44", seed = "<64 hex chars>" } |
| return new TronError(String.format(format, args), TronError.ErrCode.WITNESS_INIT); | ||
| } | ||
|
|
||
| private static String stripHexPrefix(String hex) { |
There was a problem hiding this comment.
P2: Hex prefix validation for PQ witness config fields should reject uppercase 0X and enforce the strict lowercase 0x prefix policy per project validation rules. The stripHexPrefix helper silently normalizes 0X, allowing inconsistent config formats to pass validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java, line 291:
<comment>Hex prefix validation for PQ witness config fields should reject uppercase `0X` and enforce the strict lowercase `0x` prefix policy per project validation rules. The `stripHexPrefix` helper silently normalizes `0X`, allowing inconsistent config formats to pass validation.</comment>
<file context>
@@ -175,4 +177,121 @@ static byte[] resolveWitnessAddress(
+ return new TronError(String.format(format, args), TronError.ErrCode.WITNESS_INIT);
+ }
+
+ private static String stripHexPrefix(String hex) {
+ if (hex.startsWith("0x") || hex.startsWith("0X")) {
+ return hex.substring(2);
</file context>
| private static String stripHexPrefix(String hex) { | |
| private static String stripHexPrefix(String hex) { | |
| if (hex.startsWith("0x")) { | |
| return hex.substring(2); | |
| } | |
| if (hex.startsWith("0X")) { | |
| throw witnessError("hex prefix must be lowercase '0x', found '0X'"); | |
| } | |
| return hex; | |
| } |
ff6e24c
into
Federico2014:feature/pqc-signature-v4.8.2
2 participants
What does this PR do?
Four related cleanups on the PQC-signature feature line, all in the witness/PQ area:
1. Restructure PQ witness config into a
localwitness_pqsectionConfigBeanFactoryinto a newLocalWitnessPqConfigbean (with a nestedPqEntryConfig), replacing the field-by-field reads inLocalWitnessConfig.fromConfig. The standalonecommon/.../PqEntryConfig.javais removed.localPqWitnessAccountAddresskey is replaced bylocalwitness_pq.accountAddress. Setting bothlocalWitnessAccountAddressandlocalwitness_pq.accountAddressis now rejected at startup (PARAMETER_INIT).LocalWitnessPqConfig.postProcess(): every entry must name aschemeand define exactly one ofkey/seed. Crypto-level validation (scheme support, hex length, public-key recovery) stays inWitnessInitializer.buildPqWitnessesand the keypair-building logic move fromArgsintoWitnessInitializer, decomposed into focused helpers (resolveScheme,keypairFromKey,keypairFromSeed,decodeHex).Argssheds its crypto-module imports.reference.conf/config.confupdated to document the new section.2. Tighten PQ signature error handling
TransactionCapsule.validatePQSignatureGetWeightnow throwsSignatureFormatException(length mismatch) andSignatureException(invalid sig) instead of a genericPermissionException, for more precise diagnostics.approveListis now treated as required (null-guard dropped).TransactionUtil.getTransactionSignWeightrejectspq_auth_sigimmediately when no PQ scheme is activated.3. Simplify
PQSchemeRegistrySchemeInfouses@AllArgsConstructor; the unusedresolve()pass-through and a redundantUNKNOWN_PQ_SCHEMEcheck inrequire()are removed (the existingSCHEMES.get(...) == nullpath already covers it). Doc reflow only otherwise.4. Optimize
RelayServicescheduledHere()->isAnyLocalWitnessActive(),verifyLegacySignature->verifyEcdsaSignature(clearer intent).resolveExpectedSignerAddress()to unify the multi-sign expected-address resolution shared by the ECDSA and PQ verification paths (single source of truth, returnsnull+ logs when the witness account is missing under multi-sign).isFastForwardPeerrewritten as a stream and no longer short-circuits on an emptyfastForwardNodes; clearer "exactly one of signature/pq_auth_sig" diagnostic that includes the observed flags.Why are these changes required?
localPqWitnessAccountAddress+ manually-parsedlocalwitness_pq.keysshape didn't match the*Configbean pattern used elsewhere and scattered validation acrossArgs. Binding the section to a bean centralizes structural validation and removes crypto coupling fromArgs.PermissionExceptionfor malformed/invalid PQ signatures masked the real failure cause; precise exception types improve diagnostics and match the ECDSA path.Behavior changes worth flagging for reviewers
localPqWitnessAccountAddressis gone — move it tolocalwitness_pq.accountAddress. Do not set both ECDSA and PQ account-address overrides.fnDsa512SeedRejectedtest was removed). Falcon keygen is not bit-stable, sokeyis still recommended for production.This PR has been tested by:
LocalWitnessConfigTest(both-address rejection, missing-scheme, missing/both key-seed),ConfigParityGateTest(allowlist forlocalwitness_pq),ArgsPqConfigTest,PQSchemeRegistryTest.Follow up
Extra details