feat(plugins): migrate keystore CLI from FullNode to Toolkit#12
feat(plugins): migrate keystore CLI from FullNode to Toolkit#12barbatos2011 wants to merge 6 commits intodevelopfrom
Conversation
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
2 times, most recently
from
c46573a to
730aa0a
Compare
plugins/build.gradle
Outdated
| implementation fileTree(dir: 'libs', include: '*.jar') | ||
| testImplementation project(":framework") | ||
| testImplementation project(":framework").sourceSets.test.output | ||
| implementation project(":crypto") |
There was a problem hiding this comment.
Recommended change:
implementation(project(":crypto")) {
exclude(group: 'io.github.tronprotocol', module: 'libp2p')
exclude(group: 'io.prometheus')
exclude(group: 'org.aspectj')
exclude(group: 'org.apache.httpcomponents')
}
Without exclusions, this pulls in all transitive dependencies from the crypto module, including:
- libp2p (P2P networking)
- prometheus (metrics)
- aspectj (AOP)
- httpcomponents (HTTP client)
None of these are used by the keystore commands (new, import, list, update), which only need:
- Bouncy Castle crypto (SCrypt, AES)
- Signature utilities (SignUtils, SignInterface)
- Basic utilities (ByteArray, Utils)
Impact: This should reduce Toolkit.jar from 86 MB to 56 MB.
| return 1; | ||
| } | ||
|
|
||
| boolean ecKey = true; |
There was a problem hiding this comment.
ecKey should be a configurable parameter, defaulting to true, and it can be set to false to support the SM2 algorithm.
There was a problem hiding this comment.
yes，The “migration” is not behaviorally equivalent for non-EC deployments: SM2 users will generate/import keystores with the wrong engine semantics.
| return 1; | ||
| } | ||
|
|
||
| boolean ecKey = true; |
There was a problem hiding this comment.
ecKey should be a configurable parameter, defaulting to true, and it can be set to false to support the SM2 algorithm.
| return 1; | ||
| } | ||
|
|
||
| boolean ecKey = true; |
| } | ||
|
|
||
| // Decrypt with old password | ||
| boolean ecKey = true; |
| char[] pwd1 = console.readPassword("Enter password: "); | ||
| char[] pwd2 = console.readPassword("Confirm password: "); | ||
| String password1 = new String(pwd1); | ||
| String password2 = new String(pwd2); |
There was a problem hiding this comment.
Sensitive Data in Memory. Console.readPassword() correctly returns a char[] to prevent memory leaks, but it is immediately converted into an immutable String. In Java, a String cannot be manually wiped and remains in memory until garbage collection. Recommendation: Handle passwords directly as char[] or byte[] where possible, and immediately wipe the array using Arrays.fill(pwd1, '\0') after use.
|
The |
|
|
||
| boolean ecKey = true; | ||
| SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), ecKey); | ||
| String fileName = WalletUtils.generateWalletFile(password, keyPair, keystoreDir, true); |
There was a problem hiding this comment.
Loose File Permissions. The new Keystore file is created without explicitly setting restrictive file permissions. It will be created with the system's default umask (often 644, world-readable). Since this file contains an encrypted private key, Recommendation: Explicitly set the file permissions to 600 (owner read/write only) immediately after creation using Files.setPosixFilePermissions.
| } | ||
| } | ||
|
|
||
| private String readPassword() throws IOException { |
There was a problem hiding this comment.
Logic Duplication. The readPassword() method, including the logic for handling the --password-file argument and console input, is copy-pasted across multiple classes. Recommendation: Extract this interactive input and file reading logic into a common utility class (e.g., KeystoreCliUtils) within the plugins module to reduce duplication and improve maintainability.
| private static final String FilePath = "Wallet"; | ||
|
|
||
| public static void start() { | ||
| System.err.println("WARNING: --keystore-factory is deprecated and will be removed " |
There was a problem hiding this comment.
Since the --keystore-factory command is officially being deprecated and now prints a warning to the console, please also add the @Deprecated annotation to the class declaration KeystoreFactory.
| System.err.println(" keystore import - Import a private key"); | ||
| System.err.println(" keystore list - List keystores"); | ||
| System.err.println(" keystore update - Change password"); | ||
| System.err.println(); |
There was a problem hiding this comment.
The deprecation warning (lines 23-30) points users to the new Toolkit.jar commands:
Please use: java -jar Toolkit.jar keystore
keystore new - Generate a new keystore
keystore import - Import a private key
keystore list - List keystores
keystore update - Change password
However, the help() method inside the old REPL (lines 109-115) only shows the legacy commands without mentioning the new Toolkit.jar alternative:
private void help() {
System.out.println("You can enter the following command: ");
System.out.println("GenKeystore");
System.out.println("ImportPrivateKey");
System.out.println("Exit or Quit");
System.out.println("Input any one of them, you will get more tips.");
}
Impact: Users who continue using the old --keystore-factory REPL (during the transition period) won't see the deprecation notice when they type help, missing the opportunity to learn about the new commands.
Suggested fix: Update the help() method to include the deprecation notice:
private void help() {
System.out.println("NOTE: --keystore-factory is deprecated. Use Toolkit.jar instead:");
System.out.println(" java -jar Toolkit.jar keystore new|import|list|update");
System.out.println();
System.out.println("Legacy commands (will be removed):");
System.out.println("GenKeystore");
System.out.println("ImportPrivateKey");
System.out.println("Exit or Quit");
}
This ensures consistent messaging throughout the user experience.
|
|
||
| // Decrypt with old password | ||
| boolean ecKey = true; | ||
| WalletFile walletFile = MAPPER.readValue(keystoreFile, WalletFile.class); |
There was a problem hiding this comment.
keystore update rewrites the only keystore file in place via MAPPER.writeValue(keystoreFile, newWalletFile) after decrypting it. If serialization fails or the process dies mid-write, the user can lose the only copy of the keystore. This should write to a temp file and atomically rename on
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
3 times, most recently
from
41ceeb3 to
fd9e353
Compare
| private static final String FilePath = "Wallet"; | ||
|
|
||
| public static void start() { | ||
| System.err.println("WARNING: --keystore-factory is deprecated and will be removed " |
There was a problem hiding this comment.
What is the intended timeline for removing KeystoreFactory from FullNode entirely?
Options:
-
Keep indefinitely - FullNode users who have scripts/automation depending on
--keystore-factorycan continue using it without changes. The deprecation is just guidance. -
Remove in next major release - Add a timeline (e.g., "will be removed in v4.0") to give users time to migrate.
-
Remove soon (e.g., next release) - Since Toolkit.jar provides identical functionality, there's no technical reason to maintain the code in FullNode.
Suggested clarification:
If the intent is option 1 or 2, consider updating the warning message to include a timeline or clarify that the feature will continue to work during a transition period:
WARNING: --keystore-factory is deprecated and will be removed in a future release.
Please use: java -jar Toolkit.jar keystore
(This feature will remain functional until version X.X)
This helps operators plan their migration accordingly.
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
from
fd9e353 to
6643897
Compare
| @@ -0,0 +1,19 @@ | |||
| package org.tron.plugins; | |||
There was a problem hiding this comment.
It should be better to place the keystore-related classes in a separate package.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 30 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="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java:32">
P2: `readSinglePassword` validates password strength when reading from file but skips validation when reading from console. For a method intended to accept an existing password (no confirmation prompt), the strength check on the file path would incorrectly reject valid short passwords. Remove the validation to match the console path.</violation>
</file>
<file name="framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java">
<violation number="1" location="framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java:15">
P2: Add a timeout to prevent this test from hanging CI if the REPL loop doesn't terminate. Since `KeystoreFactory.start()` enters a `Scanner`-based `while(in.hasNextLine())` loop, any unexpected input-stream behavior will block indefinitely.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreList.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreList.java:75">
P2: JSON serialization errors return exit code 0 (success). Since this tool is explicitly designed for CI/CD scripting, returning success when `--json` output could not be written will silently break downstream consumers. Return a non-zero exit code on write failure.</violation>
</file>
<file name="plugins/src/test/java/org/tron/plugins/KeystoreListTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreListTest.java:132">
P2: This assertion is a false-positive risk: `"".split("\\n")` returns `[""]` (length 1), so the test passes even when the command outputs nothing. Add a non-empty check on the output before asserting line count.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreNew.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreNew.java:50">
P2: Unnecessary expensive scrypt decrypt. The `keyPair` is already in memory — use `Credentials.create(keyPair).getAddress()` to derive the address directly instead of reading and decrypting the file just written.</violation>
</file>
<file name="crypto/src/test/java/org/tron/keystore/CredentialsTest.java">
<violation number="1" location="crypto/src/test/java/org/tron/keystore/CredentialsTest.java:33">
P2: Missing `Assert.fail()` after the statement expected to throw. If `Credentials.create(...)` stops throwing, this test silently passes, hiding the regression. Add a `fail()` call, or use `@Test(expected = IllegalArgumentException.class)` instead.</violation>
</file>
<file name="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java:53">
P2: `testNewKeystoreJsonOutput` doesn't verify JSON output. The `StringWriter out` is allocated but never asserted against, and the command writes JSON through `System.out` rather than picocli's writer. This test only validates that `--json` doesn't crash, not that the output is correct.
Consider having `KeystoreNew` use the picocli `spec.commandLine().getOut()` writer for output so it can be captured in tests, or redirect `System.out` in the test to verify the JSON content.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java:105">
P2: `console.readPassword()` returns `null` on EOF (e.g., Ctrl+D). This causes an NPE on `new String(key)`, and then `Arrays.fill(key, '\0')` in the `finally` block throws a second NPE that suppresses the first, making it hard to diagnose.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if (passwordFile != null) { | ||
| byte[] bytes = Files.readAllBytes(passwordFile.toPath()); | ||
| String password = new String(bytes, StandardCharsets.UTF_8).trim(); | ||
| if (!WalletUtils.passwordValid(password)) { |
There was a problem hiding this comment.
P2: readSinglePassword validates password strength when reading from file but skips validation when reading from console. For a method intended to accept an existing password (no confirmation prompt), the strength check on the file path would incorrectly reject valid short passwords. Remove the validation to match the console path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java, line 32:
<comment>`readSinglePassword` validates password strength when reading from file but skips validation when reading from console. For a method intended to accept an existing password (no confirmation prompt), the strength check on the file path would incorrectly reject valid short passwords. Remove the validation to match the console path.</comment>
<file context>
@@ -0,0 +1,129 @@
+ if (passwordFile != null) {
+ byte[] bytes = Files.readAllBytes(passwordFile.toPath());
+ String password = new String(bytes, StandardCharsets.UTF_8).trim();
+ if (!WalletUtils.passwordValid(password)) {
+ System.err.println("Invalid password: must be at least 6 characters.");
+ return null;
</file context>
framework/src/test/java/org/tron/program/KeystoreFactoryDeprecationTest.java
Outdated
Show resolved
Hide resolved
| String output = baos.toString(StandardCharsets.UTF_8.name()); | ||
| String[] lines = output.trim().split("\\n"); | ||
| // Should list only the valid keystore, not the readme.json or notes.txt | ||
| assertEquals("Should list only 1 valid keystore", 1, lines.length); |
There was a problem hiding this comment.
P2: This assertion is a false-positive risk: "".split("\\n") returns [""] (length 1), so the test passes even when the command outputs nothing. Add a non-empty check on the output before asserting line count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/test/java/org/tron/plugins/KeystoreListTest.java, line 132:
<comment>This assertion is a false-positive risk: `"".split("\\n")` returns `[""]` (length 1), so the test passes even when the command outputs nothing. Add a non-empty check on the output before asserting line count.</comment>
<file context>
@@ -0,0 +1,137 @@
+ String output = baos.toString(StandardCharsets.UTF_8.name());
+ String[] lines = output.trim().split("\\n");
+ // Should list only the valid keystore, not the readme.json or notes.txt
+ assertEquals("Should list only 1 valid keystore", 1, lines.length);
+ } finally {
+ System.setOut(originalOut);
</file context>
cd5acbf to
21ce211
Compare
|
@0xbigapple I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 28 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="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java:59">
P2: The password comparison via `String` undermines the `char[]` zeroing. `password2` is created as an immutable `String` solely for comparison, so its contents persist in memory despite zeroing `pwd2`. Compare the char arrays directly with `Arrays.equals(pwd1, pwd2)` instead.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java:115">
P2: `isValidPrivateKey` rejects keys with a `0x` prefix, but `ByteArray.fromHexString` (used to parse the key on the next line) strips that prefix. Users providing the common `0x`-prefixed format will get an unhelpful error. Either strip the prefix before validation or accept it in the regex.</violation>
</file>
<file name="plugins/src/test/java/org/tron/plugins/KeystoreImportTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreImportTest.java:166">
P2: This test only checks exit code but doesn't verify the imported key roundtrips correctly. Since the test's purpose is to validate whitespace trimming on the key file, it should also verify the decrypted private key matches the original — otherwise a subtle trimming bug could store the wrong key undetected. Compare with `testImportWithKeyFileAndPasswordFile` which does full roundtrip verification.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| } | ||
| try { | ||
| String password1 = new String(pwd1); | ||
| String password2 = new String(pwd2); |
There was a problem hiding this comment.
P2: The password comparison via String undermines the char[] zeroing. password2 is created as an immutable String solely for comparison, so its contents persist in memory despite zeroing pwd2. Compare the char arrays directly with Arrays.equals(pwd1, pwd2) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java, line 59:
<comment>The password comparison via `String` undermines the `char[]` zeroing. `password2` is created as an immutable `String` solely for comparison, so its contents persist in memory despite zeroing `pwd2`. Compare the char arrays directly with `Arrays.equals(pwd1, pwd2)` instead.</comment>
<file context>
@@ -0,0 +1,116 @@
+ }
+ try {
+ String password1 = new String(pwd1);
+ String password2 = new String(pwd2);
+
+ if (!password1.equals(password2)) {
</file context>
6364cbc to
f655164
Compare
|
@barbatos2011 I have started the AI code review. It will take a few minutes to complete. |
Code ReviewOverall design is solid — module split is correct, security patterns are thorough, backward compatibility is maintained. A few items worth discussing: 1.
|
There was a problem hiding this comment.
4 issues found across 28 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="plugins/src/main/java/common/org/tron/plugins/KeystoreList.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreList.java:86">
P2: `printEmptyJson()` hides JSON serialization failures, causing `keystore list --json` to return exit code 0 even when output generation fails in empty/missing-directory cases.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java:116">
P2: Private key handling converts secrets to immutable String values, which cannot be wiped from memory after use.</violation>
</file>
<file name="crypto/src/test/java/org/tron/keystore/CredentialsTest.java">
<violation number="1" location="crypto/src/test/java/org/tron/keystore/CredentialsTest.java:47">
P3: Remove the assertion that unequal credentials must have different hash codes; hash collisions are allowed and this can cause flaky tests.</violation>
</file>
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java:33">
P1: Using `.trim()` on the password removes valid leading and trailing spaces, breaking backward compatibility and parity with interactive input for users whose passwords contain spaces. Use regex to strip only trailing line terminators instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| if (passwordFile != null) { | ||
| byte[] bytes = Files.readAllBytes(passwordFile.toPath()); | ||
| try { | ||
| String password = new String(bytes, StandardCharsets.UTF_8).trim(); |
There was a problem hiding this comment.
P1: Using .trim() on the password removes valid leading and trailing spaces, breaking backward compatibility and parity with interactive input for users whose passwords contain spaces. Use regex to strip only trailing line terminators instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java, line 33:
<comment>Using `.trim()` on the password removes valid leading and trailing spaces, breaking backward compatibility and parity with interactive input for users whose passwords contain spaces. Use regex to strip only trailing line terminators instead.</comment>
<file context>
@@ -0,0 +1,117 @@
+ if (passwordFile != null) {
+ byte[] bytes = Files.readAllBytes(passwordFile.toPath());
+ try {
+ String password = new String(bytes, StandardCharsets.UTF_8).trim();
+ if (!WalletUtils.passwordValid(password)) {
+ System.err.println("Invalid password: must be at least 6 characters.");
</file context>
| return 0; | ||
| } | ||
|
|
||
| private void printEmptyJson() { |
There was a problem hiding this comment.
P2: printEmptyJson() hides JSON serialization failures, causing keystore list --json to return exit code 0 even when output generation fails in empty/missing-directory cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreList.java, line 86:
<comment>`printEmptyJson()` hides JSON serialization failures, causing `keystore list --json` to return exit code 0 even when output generation fails in empty/missing-directory cases.</comment>
<file context>
@@ -0,0 +1,95 @@
+ return 0;
+ }
+
+ private void printEmptyJson() {
+ try {
+ Map<String, Object> result = new LinkedHashMap<>();
</file context>
5c17803 to
21f53fd
Compare
|
@barbatos2011 I have started the AI code review. It will take a few minutes to complete. |
Move keystore package (Wallet, WalletUtils, Credentials, WalletFile)
from framework to crypto module to enable reuse by Toolkit.jar without
pulling in the entire framework dependency.
- Replace Args.getInstance().isECKeyCryptoEngine() calls with injected
boolean ecKey parameter in Wallet.decrypt(), WalletUtils.loadCredentials(),
generateNewWalletFile(), generateFullNewWalletFile(), generateLightNewWalletFile()
- Update callers (KeystoreFactory, WitnessInitializer) to pass ecKey
- Add implementation project(":crypto") to plugins build
- Merge two CredentialsTest files (fix keystroe typo dir) into crypto module
- Move WalletFileTest to crypto module
- CipherException already in common module, no move needed
Add picocli subcommands for keystore management in Toolkit.jar: - `keystore new`: generate new keypair and encrypt to keystore file - `keystore import`: import existing private key into keystore file Both commands support: - --password-file for non-interactive password input - --keystore-dir for custom output directory - --json for structured output - Console.readPassword() for no-echo interactive input - Clear error when no TTY available (directs to --password-file) Import reads private key from --key-file or interactive prompt, never from command-line arguments (security: avoids ps/history exposure). Also adds roundtrip property tests (100 random encrypt/decrypt cycles) and cross-implementation compatibility tests to crypto module. Note: jqwik was planned for property testing but replaced with plain JUnit loops due to Gradle dependency verification overhead.
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
3 times, most recently
from
f30d460 to
cc02b8c
Compare
…store-factory Add remaining keystore subcommands to Toolkit.jar: - `keystore list`: display all keystore files and addresses in a directory - `keystore update <address>`: re-encrypt a keystore with a new password Both support --keystore-dir, --json, and --password-file options. Add deprecation warning to --keystore-factory in FullNode.jar, directing users to the new Toolkit.jar keystore commands. The old REPL continues to function normally during the transition period.
barbatos2011
force-pushed
the
001-keystore-toolkit-migration
branch
from
cc02b8c to
1492a49
Compare
- KeystoreUpdate: findKeystoreByAddress now detects multiple keystores with the same address and returns an error with file list, instead of silently picking one nondeterministically. - KeystoreImport: scan directory before writing and print WARNING if a keystore for the same address already exists. Import still proceeds (legitimate use case) but user is made aware. - KeystoreUpdate: strip UTF-8 BOM from password file before parsing. - KeystoreUpdate: add comment clarifying old password skips validation. - KeystoreList: add version != 3 check to filter non-keystore JSON.
There was a problem hiding this comment.
1 issue found across 2 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="plugins/src/main/java/common/org/tron/plugins/KeystoreUpdate.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreUpdate.java:206">
P2: When duplicates are detected, `findKeystoreByAddress` returns `null`, and the caller blindly prints `"No keystore found for address: …"`. The user sees both the duplicate warning and a contradictory "not found" message. Either return a sentinel/throw so the caller can distinguish the two cases, or move the duplicate error handling to the caller.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| System.err.println("Please remove duplicates and retry."); | ||
| return null; | ||
| } | ||
| return matches.isEmpty() ? null : matches.get(0); |
There was a problem hiding this comment.
P2: When duplicates are detected, findKeystoreByAddress returns null, and the caller blindly prints "No keystore found for address: …". The user sees both the duplicate warning and a contradictory "not found" message. Either return a sentinel/throw so the caller can distinguish the two cases, or move the duplicate error handling to the caller.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreUpdate.java, line 206:
<comment>When duplicates are detected, `findKeystoreByAddress` returns `null`, and the caller blindly prints `"No keystore found for address: …"`. The user sees both the duplicate warning and a contradictory "not found" message. Either return a sentinel/throw so the caller can distinguish the two cases, or move the duplicate error handling to the caller.</comment>
<file context>
@@ -183,16 +183,26 @@ private File findKeystoreByAddress(String targetAddress) {
+ System.err.println("Please remove duplicates and retry.");
+ return null;
+ }
+ return matches.isEmpty() ? null : matches.get(0);
}
}
</file context>
…rity tips - Reject duplicate-address import by default, add --force flag to override - Add friendly error messages when --password-file or --key-file not found - Print security tips after keystore new/import (aligned with geth output) - Add file existence checks in KeystoreCliUtils and KeystoreUpdate
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="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java:190">
P2: Hardcoded `/tmp/nonexistent-pw.txt` is fragile and platform-dependent. If this file exists on the build host the test fails; on Windows `/tmp` doesn't exist. Derive the path from `tempFolder` to guarantee it doesn't exist, consistent with every other test in this class.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| CommandLine cmd = new CommandLine(new Toolkit()); | ||
| int exitCode = cmd.execute("keystore", "new", | ||
| "--keystore-dir", dir.getAbsolutePath(), | ||
| "--password-file", "/tmp/nonexistent-pw.txt"); |
There was a problem hiding this comment.
P2: Hardcoded /tmp/nonexistent-pw.txt is fragile and platform-dependent. If this file exists on the build host the test fails; on Windows /tmp doesn't exist. Derive the path from tempFolder to guarantee it doesn't exist, consistent with every other test in this class.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java, line 190:
<comment>Hardcoded `/tmp/nonexistent-pw.txt` is fragile and platform-dependent. If this file exists on the build host the test fails; on Windows `/tmp` doesn't exist. Derive the path from `tempFolder` to guarantee it doesn't exist, consistent with every other test in this class.</comment>
<file context>
@@ -180,6 +180,18 @@ public void testNewKeystoreSpecialCharPassword() throws Exception {
+ CommandLine cmd = new CommandLine(new Toolkit());
+ int exitCode = cmd.execute("keystore", "new",
+ "--keystore-dir", dir.getAbsolutePath(),
+ "--password-file", "/tmp/nonexistent-pw.txt");
+
+ assertEquals("Should fail when password file not found", 1, exitCode);
</file context>
| "--password-file", "/tmp/nonexistent-pw.txt"); | |
| "--password-file", new File(tempFolder.getRoot(), "nonexistent-pw.txt").getAbsolutePath()); |
4 participants
What
Migrate the
--keystore-factoryinteractive REPL from FullNode.jar to Toolkit.jar as picocli subcommands, extract the keystore core library fromframeworktocryptomodule, and add new capabilities (list, update, SM2 support, JSON output, password-file support).Why
The current
--keystore-factoryhas several limitations:--password-fileor structured outputScanner-based password input echoes to terminal; no--key-fileoption forces private keys to be typed interactively or passed as command-line arguments (visible inps, shell history)Wallet.java,WalletUtils.java) lives inframeworkand callsArgs.getInstance(), preventing reuse byplugins(Toolkit.jar) without pulling in the entire framework dependency chainChanges
Commit 1:
refactor(crypto): extract keystore library from framework moduleExtract the keystore package from
frameworktocryptomodule to break the framework dependency:Wallet.javacrypto/, removedArgsimport, addedboolean ecKeyparameter todecrypt(), changed MAC comparison to constant-timeMessageDigest.isEqual()WalletUtils.javacrypto/, removedArgsimport, addedboolean ecKeyparameter togenerateNewWalletFile(),generateFullNewWalletFile(),generateLightNewWalletFile(),loadCredentials()Credentials.java,WalletFile.javacrypto/unchangedWitnessInitializer.javaArgs.getInstance().isECKeyCryptoEngine()toloadCredentials()KeystoreFactory.javaCommonParameter.getInstance().isECKeyCryptoEngine()to keystore callsplugins/build.gradleimplementation project(":crypto")with exclusions (libp2p, prometheus, aspectj, httpcomponents)CredentialsTest.javakeystroetypo directory) intocryptomoduleWalletFileTest.javacryptomoduleCommit 2:
feat(plugins): add keystore new and import commands to ToolkitAdd picocli subcommands for the two core keystore operations:
Security design:
Console.readPassword()(no echo), with null check for EOF (Ctrl+D)char[]compared directly viaArrays.equals()(avoids creating unnecessary String), zeroed infinallyblocksbyte[]zeroed after reading viaArrays.fill(bytes, (byte) 0)--key-fileor interactive prompt, never from command-line arguments0x-prefixed keys (common Ethereum format)Files.setPosixFilePermissionsObjectMapper.writeValueAsString(), not string concatenation--sm2flag for SM2 algorithm supportShared utilities:
KeystoreCliUtils—readPassword(),ensureDirectory()(usesFiles.createDirectories),setOwnerOnly(),printJson()ensureDirectory()usesFiles.createDirectories()to avoid TOCTOU race conditionsToolkit.javaupdated to registerKeystore.classin subcommandsTest infrastructure:
Commit 3:
feat(plugins): add keystore list and update commands, deprecate --keystore-factorylist: scans directory for keystore JSON files, displays address + filename, skips non-keystore files. JSON serialization failure returns exit code 1.update: decrypts with old password, re-encrypts with new password. Atomic file write (temp file +Files.move(ATOMIC_MOVE)) prevents corruption on interrupt. Wrong old password fails without modifying the file. Supports Windows line endings in password file.@Deprecatedannotation onKeystoreFactoryclass + deprecation warning instart()+ migration notice inhelp()methodWitnessInitializerKeystoreTestverifies new tool's keystore can be loaded byWitnessInitializer.initFromKeystore()Scope
localwitnesskeystoreconfig —WitnessInitializercontinues to load keystore files at node startup--keystore-factoryflag remains functional with a deprecation warningTest
WitnessInitializerTestandSupplementTestupdated for new signatures.WitnessInitializerTest,ArgsTest,SupplementTestall passcheckstyleMain+checkstyleTest)Cross-implementation compatibility
Migration from --keystore-factory
--keystore-factoryToolkit.jar keystoreGenKeystorekeystore newImportPrivateKeykeystore importkeystore listkeystore update--password-file,--key-file--json--sm2--keystore-dirConsole.readPassword)The old
--keystore-factoryremains functional with a deprecation warning during the transition period.Usage
Commands
newimportlistupdateCommon Options
--keystore-dir <path>./Wallet)--json--password-file <path>--sm2--key-file <path>importonly)Examples