Refactor benchmark command to use engine abstraction and add Metal engine support with validation - #22
Conversation
…gine support with validation
…in validation and comparison benchmarking
…or" and add Metal engine support with GPU batch size configuration
…ocumentation and CLI help text
…ith proper cleanup of sensitive data
…etrics table including pattern info, difficulty breakdown, and success rates
…n parsing and engine resolution
…alysis and TUI support
…keystore fixes CPU (12.8x faster, ~3.36M addr/s): - Replace big.Int ScalarBaseMult with decred secp256k1 plus Jacobian point-addition chaining over a per-batch seed, batched inversion, one-shot Keccak, byte-level nibble matching, and a reused hasher - Move chain initialization out of the worker loop and double-buffer refills to hide batch costs Metal (27x faster, ~243k addr/s with validation): - Kernel now reports match indices so CPU validation only re-checks matches (validation cost drops from 24.8s to ~2ms per 1M keys) - Add fixed-base 4-bit comb table with precomputed Jacobian points and pooled Metal buffers to avoid per-batch allocation - Rewrite field arithmetic as 4x64 limbs to dodge an MSL compiler stack-corruption bug with large 32-bit limb arrays; fix the wrap correction in fe_add (2^32 term was added to the wrong limb), a lost carry in fe_mul's t[k+2] += c1, the p-2 exponent constant, and the fe_inv loop skipping bit 0 (returned +/- z^-1) - Verified against geth: comb table, kernel pubkeys, addresses, and full chain intermediates Default path: auto resolves to CPU until Metal is competitive. Keystore V3: - Use geth naming convention UTC--<timestamp>--<address>.json and locate files by directory scan - Derive the real Ethereum address in EncryptPrivateKey (was a zero placeholder) and enforce IV/ciphertext/MAC/salt lengths in Validate()
- use go env for install paths, platform detection, and version metadata - include Metal-aware darwin/arm64 builds and install/uninstall targets - ignore generated release binaries with a wildcard pattern
Upgrade vulnerable crypto and transitive modules to patched releases. This clears all 15 Dependabot ranges and removes the reachable x/text issue found by govulncheck.
Auditoria manual do código Go, do empacotamento, dos workflows de CI e do histórico Git, com as cinco categorias solicitadas mapeadas para o equivalente estrutural de uma CLI local (sem banco, sem auth, sem frontend). 13 achados verificados (4 altas, 3 médias, 5 baixas, 1 informativa) e 10 pontos fortes. Destaques: - F1 (alta): a engine CPU entrega chaves privadas Ethereum encadeadas (k0, k0+1, ... k0+4095), então carteiras da mesma execução não são independentes. Confirmado empiricamente contra a API real. - F2/F3 (altas): senha do keystore V3 e chave Solana gravadas em texto claro ao lado dos artefatos cifrados. - F4 (alta): backup de Bitcoin salva mnemônica que não deriva a chave gerada. - F5/F8/F10: controles declarados na CLI que nunca chegam à camada que executa. - F9/F12: injeção de script no workflow de release, action não pinada e dependabot.yml inerte. Entrega o relatório em PDF e Markdown, mais o gerador (venv isolado com reportlab+matplotlib) para regerar ambos a partir de docs/security-audit/achados.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VS3G7uWtzTjo1Vx3iDqiK5
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
I've identified 3 critical defects that must be fixed before merging:
Critical Issues
- internal/engine/metal_darwin_arm64.go - Resource leak: Metal command buffer lacks deferred cleanup, causing GPU memory accumulation
- internal/worker/pool.go - Race condition: Unsynchronized worker.status access creates data race potential
- internal/cli/benchmark.go - Division by zero risk: Missing validation for iterations parameter can crash the application
All issues have been commented with specific code suggestions that can be committed directly.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| @@ -154,6 +154,26 @@ func (p *Pool) GetStatsCollector() *StatsCollector { | |||
| return p.statsCollector | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
🛑 Race Condition: Protect worker.status access with the mutex. The status field is accessed without holding the lock, creating a data race that can cause incorrect status reads or memory corruption in concurrent scenarios.
| worker.mu.Lock() | |
| defer worker.mu.Unlock() | |
| if worker.status != StatusIdle { |
| metalValidation, err = resolveMetalValidationConfig(cmd, selection.Resolved, metalValidation) | ||
| if err != nil { | ||
| return benchmarkOptions{}, err | ||
| } |
There was a problem hiding this comment.
🛑 Crash Risk: Add validation to prevent division by zero. If iterations is zero, this will cause a panic that crashes the application.
| } | |
| if iterations == 0 { | |
| return fmt.Errorf("iterations must be greater than zero") | |
| } | |
| avgTime := totalTime / time.Duration(iterations) |
| "constant uchar KECCAKF_ROTC[25] = {\n" | ||
| " 0, 1, 62, 28, 27,\n" | ||
| " 36, 44, 6, 55, 20,\n" | ||
| " 3, 10, 43, 25, 39,\n" |
There was a problem hiding this comment.
🛑 Resource Leak: Add deferred cleanup for the Metal command buffer to prevent resource leaks when errors occur during encoding or commit operations. Without proper cleanup, GPU resources will accumulate and eventually cause memory pressure or crashes.
| " 3, 10, 43, 25, 39,\n" | |
| cmdBuffer := e.commandQueue.MakeCommandBuffer() | |
| defer cmdBuffer.Release() |
PR Summary by QodoAdd engine abstraction, validated Metal support, and faster key generation
AI Description
Diagram
High-Level Assessment
Files changed (46)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if workerID < len(p.chains) && p.chains[workerID] != nil { | ||
| return p.chains[workerID], nil |
There was a problem hiding this comment.
🔴 Repeated generation corrupts shared key chains
For multiple wallets, unfinished workers reuse each PrivateKeyChain without synchronization. Concurrent NextKey calls can pair an address with the wrong private key.
Prompt for agents
Fix shared PrivateKeyChain concurrency in internal/worker/pool.go. GenerateWalletWithContext returns after one worker sends a result but does not cancel or join the other workers. The next call for multi-wallet generation retrieves the same chain by worker ID, so old and new workers call NextKey concurrently. Ensure each generation call stops and joins all workers before returning, and ensure a chain cannot be consumed concurrently. Preserve chain reuse only after the previous consumer has fully stopped.
Was this helpful? React with 👍 or 👎 to provide feedback.
| chain := &PrivateKeyChain{ | ||
| batches: make(chan *chainBatch, 1), | ||
| current: first, | ||
| } | ||
| go chain.filler() | ||
| return chain, nil |
There was a problem hiding this comment.
🔴 CPU benchmarks leak background workers
Each RunBenchmark worker leaves its key chain open after completion. Repeated benchmarks retain goroutines and large precomputed batches until process exit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| stageStart = time.Now() | ||
| if !addressMatches(address[:], prefix, suffix) { | ||
| return stages, fmt.Errorf("metal match validation failed: gpu index %d is not a match", index) | ||
| } | ||
| stages.Match += time.Since(stageStart) |
There was a problem hiding this comment.
🟡 Checksum benchmarks count invalid matches
With case-sensitive checksum criteria, validateMetalBatch checks only raw hexadecimal digits. Metal counts wrong-case addresses, making its totals incomparable with CPU results.
Was this helpful? React with 👍 or 👎 to provide feedback.
Code Review by Qodo
1. Metal batch size unbounded
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # Add labels | ||
| LABEL org.opencontainers.image.title="bloco-vgen" \ | ||
| org.opencontainers.image.description="An Ethereum like Wallet Generator" \ | ||
| org.opencontainers.image.description="An Ethereum like Vanity Generator" \ |
There was a problem hiding this comment.
1. Protected dockerfile was edited 📘 Rule violation § Compliance
The PR changes the pre-existing Dockerfile, although repository policy protects every pre-existing legacy-project file from modification and the PR description documents no approved exception. Implement the metadata change without editing the protected file, or document the required exception process.
Agent Prompt
## Issue description
The PR edits the pre-existing `Dockerfile`, which is protected by the repository's non-negotiable legacy-file policy.
## Issue Context
`AGENTS.md` states that pre-existing legacy-project files must never be deleted, modified, or overwritten. The PR description contains no documented approved exception.
## Fix Focus Areas
- Dockerfile[94-94]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Base de dados da auditoria de segurança do bloco-vanity-generator. | ||
|
|
||
| Este módulo concentra TODO o conteúdo do relatório (achados, pontos fortes, | ||
| recomendações e issues do GitHub). Os geradores (`gerar_relatorio.py`) apenas |
There was a problem hiding this comment.
2. Security audit exceeds scope 📘 Rule violation ⚙ Maintainability
The PR adds a 1,197-line security-audit data module plus a generator and generated reports, which is unrelated to the stated benchmark-engine refactor and Metal support. This sweeping documentation artifact tree has no rationale in the PR description and should be moved to a separately scoped change.
Agent Prompt
## Issue description
The large `docs/security-audit` artifact tree is outside this PR's benchmark and Metal-engine scope.
## Issue Context
The PR description provides no rationale for adding the audit database, report generator, generated Markdown/PDF reports, or graphs. Move these artifacts to a separately scoped PR.
## Fix Focus Areas
- docs/security-audit/achados.py[1-1197]
- docs/security-audit/gerar_relatorio.py[1-1229]
- docs/security-audit/README.md[1-58]
- docs/security-audit/relatorio-auditoria-seguranca.md[1-1239]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| batchSize := options.BatchSize | ||
| if batchSize <= 0 { | ||
| batchSize = DefaultMetalBatchSize | ||
| } |
There was a problem hiding this comment.
4. Metal batch size unbounded 🐞 Bug ☼ Reliability
The new Metal paths accept any positive CLI batch size and multiply it for Go and Metal allocations without an upper-bound or overflow check. A very large --gpu-batch-size or benchmark --batch-size can therefore panic on integer overflow/allocation or terminate the process through memory exhaustion instead of returning validation error.
Agent Prompt
## Issue description
User-controlled Metal batch sizes are only checked for positivity before being used in multiplied allocations.
## Issue Context
Define a safe maximum, reject values above it in both generation and benchmark option parsing, and use checked multiplication before all byte/buffer size calculations.
## Fix Focus Areas
- internal/engine/metal_darwin_arm64.go[849-872]
- internal/engine/metal_darwin_arm64.go[1088-1095]
- internal/cli/benchmark.go[161-168]
- internal/cli/commands.go[1599-1605]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if CGO_ENABLED=1 GOOS=darwin GOARCH=arm64 go build \ | ||
| -ldflags="-w -s -X main.version=${{ needs.create-release.outputs.version }} -X main.commit=${{ github.sha }} -X main.date=$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | ||
| -o dist/bloco-vgen \ |
There was a problem hiding this comment.
5. Metal release metadata unset 🐞 Bug ≡ Correctness
The new Metal release build sets lowercase linker symbols main.version, main.commit, and main.date, while the executable reads main.Version, main.GitCommit, and main.BuildTime. The published Metal artifact therefore retains dev/unknown metadata instead of its release version, commit, and build time.
Agent Prompt
## Issue description
The Metal release job injects linker values into symbol names that do not match the variables used by the executable.
## Issue Context
Go linker symbol names are case-sensitive. Align this job with the uppercase variables and use `BuildTime` rather than `date`.
## Fix Focus Areas
- .github/workflows/release.yaml[183-186]
- cmd/bloco-vgen/main.go[17-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| cpuResult, err := app.runBenchmarkEngine(ctx, cpuOptions, sampleInterval, nil) | ||
| if err != nil { | ||
| resultCase.Error = err.Error() | ||
| return resultCase |
There was a problem hiding this comment.
6. Comparison failures exit successfully 🐞 Bug ≡ Correctness
runBenchmarkComparisonCase converts invalid criteria and CPU benchmark failures—including context cancellation—into resultCase.Error, and the outer comparison always returns a nil error. As a result, an interrupted or failed comparison can emit an error-bearing report while the CLI exits with status 0, preventing automation from detecting that no valid comparison completed.
Agent Prompt
## Issue description
Fatal comparison failures are stored as report text and never affect the command error or exit status.
## Issue Context
Keep expected Metal-unavailable results reportable, but propagate context cancellation, invalid criteria, and CPU/baseline failures from the comparison loop.
## Fix Focus Areas
- internal/cli/benchmark.go[289-310]
- internal/cli/benchmark.go[313-347]
- internal/cli/benchmark.go[818-822]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| name := entry.Name() | ||
| if strings.HasPrefix(name, "UTC--") && strings.HasSuffix(name, suffix) { | ||
| return filepath.Join(ks.config.OutputDirectory, name), nil |
There was a problem hiding this comment.
7. Keystore cleanup leaves duplicates 🐞 Bug ⛨ Security
Timestamped naming now permits multiple keystore files for the same address, but lookup returns only the first matching entry and RemoveKeystoreFiles deletes only that one. Re-saving an address and then removing it can therefore leave other encrypted private-key files behind after the shared password file is deleted.
Agent Prompt
## Issue description
Address lookup selects one timestamped keystore although multiple files for the address can exist, making cleanup incomplete.
## Issue Context
For removal, enumerate and safely delete every regular file matching the exact geth address suffix (plus the legacy file). Keep single-file lookup deterministic if it remains needed for other callers.
## Fix Focus Areas
- internal/crypto/keystore.go[1492-1495]
- internal/crypto/keystore.go[1934-1956]
- internal/crypto/keystore.go[1987-2014]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if block := m.renderEngineInfoBlock(pad); block != "" { | ||
| content.WriteString(block) | ||
| content.WriteString("\n") | ||
| } |
There was a problem hiding this comment.
8. Stats overview no longer renders 🐞 Bug ≡ Correctness
StatsModel.View replaces the pattern-overview rendering call with the new optional engine block and never invokes renderPatternOverview elsewhere. Interactive stats output consequently loses its pattern, difficulty, probability, and expected-attempt overview even when no engine information is provided.
Agent Prompt
## Issue description
Adding engine diagnostics accidentally removed the existing pattern overview from the stats view.
## Issue Context
Render the engine block in addition to, not instead of, `renderPatternOverview`, preserving appropriate spacing in both zero and nonzero engine-info cases.
## Fix Focus Areas
- internal/tui/stats.go[171-183]
- internal/tui/stats.go[226-255]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| for cat, qtd in por_cat.items(): | ||
| a("| %s | %d | %s |" % (cat, qtd, A.ROTULO_SEVERIDADE[pior_severidade(cat)].title())) | ||
| a("") | ||
| a("Gráficos: `gráficos/severidade-rosca.png` e `gráficos/categoria-barras.png`.") |
There was a problem hiding this comment.
9. Generated chart links are broken 🐞 Bug ≡ Correctness
The report generator writes chart references under gráficos/ with an accented character, while it creates both images under graficos/. Every regenerated Markdown report therefore points to paths that do not exist in the repository.
Agent Prompt
## Issue description
Generated Markdown references a differently spelled directory from the one receiving generated chart files.
## Issue Context
Use the unaccented `graficos/` path consistently in generated report text.
## Fix Focus Areas
- docs/security-audit/gerar_relatorio.py[1037-1037]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
🟡 Changes recommended
There are build/release and runtime recovery issues (incorrect ldflags injection, GOPATH-derived install path edge case, and non-recovering chain failure handling) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an engine abstraction for generation/benchmarking, adds Metal engine plumbing and diagnostics, and refactors hot-path vanity matching/crypto to support higher-throughput execution and richer benchmark reporting.
Changes:
- Added
internal/engineabstraction (CPU + Metal availability scaffolding) and expanded result types with engine/device/batch diagnostics. - Refactored worker generation to use a chained private-key generator and centralized vanity matching helpers.
- Enhanced TUI to display engine diagnostics and added benchmark comparison UI + expanded tests; updated keystore naming/validation for geth compatibility.
File summaries
| File | Description |
|---|---|
| pkg/wallet/types.go | Extends generation/benchmark result structs with engine + validation diagnostics. |
| Makefile | Reworked build/install targets with ldflags metadata and CGO policy for Metal. |
| internal/worker/pool.go | Uses chained key generation and vanity matcher helpers in CPU worker pool. |
| internal/worker/metal_pool.go | Adds a WorkerPool implementation backed by the engine abstraction for Metal generation. |
| internal/vanity/matching.go | Centralizes address matching + checksum helpers for reuse. |
| internal/tui/stats.go | Adds engine info rendering and adjusts table update flow. |
| internal/tui/progress.go | Introduces EngineInfo and renders engine diagnostics in the progress TUI. |
| internal/tui/progress_test.go | Tests engine info rows and rendering in TUI models. |
| internal/tui/manager.go | Adds constructors that preload engine diagnostics into TUI models. |
| internal/tui/logo.go | Removes unused old logo comments. |
| internal/tui/benchmark.go | Adds engine diagnostics rendering; changes benchmark state/view behavior. |
| internal/tui/benchmark_compare.go | New TUI for CPU vs Auto vs Metal benchmark comparison. |
| internal/engine/types.go | Defines engine names/options, selection/validation logic, and factories. |
| internal/engine/synthetic.go | Adds synthetic address generation + hex pattern nibble utilities. |
| internal/engine/metal_unavailable.go | Non-darwin/arm64/cgo stub for Metal availability + constructor error. |
| internal/engine/metal_darwin_arm64_test.go | Metal kernel and generation validation tests gated by build tags. |
| internal/engine/engine_test.go | Tests engine resolution/validation and CPU/Metal benchmark behavior. |
| internal/engine/cpu.go | Implements CPU benchmark engine using chained key generation + stage timings. |
| internal/engine/comb_table_test.go | Validates Metal comb table correctness against reference implementations. |
| internal/engine/comb_kernel_test.go | Validates Metal comb kernel-derived pubkeys against CPU references. |
| internal/crypto/keystore.go | Strengthens keystore validation and switches to geth-compatible UTC filename format. |
| internal/crypto/keystore_test.go | Updates tests for new keystore validation + filename lookup behavior. |
| internal/crypto/keystore_geth_compat_test.go | Adds interop tests proving geth can decrypt generated keystores. |
| internal/crypto/chain.go | Adds chained private-key generator, fast Keccak(64) impl, and nibble matcher utilities. |
| internal/crypto/chain_test.go | Tests chained key derivation correctness and matcher consistency. |
| internal/crypto/chain_bench_test.go | Benchmarks keccak and chain refill/NextKey performance. |
| internal/cli/commands_test.go | Expands CLI tests for benchmark engine options, validation, and TUI engine info. |
| go.sum | Updates dependency checksums for new/updated modules. |
| go.mod | Adds secp256k1 dependency and bumps several golang.org/x/* / indirect deps. |
| docs/security-audit/README.md | Documents how to regenerate the security audit report artifacts. |
| docs/security-audit/.gitignore | Ignores venv and pycache for the audit tooling. |
| docs/CODE_ANALYSIS.md | Updates product naming in analysis doc. |
| Dockerfile | Updates OCI description string to “Vanity Generator”. |
| .gitignore | Ignores built binaries (bloco-vgen, bloco-vgen-*). |
| .github/workflows/release.yaml | Adds Metal-enabled macOS arm64 release job and notes in release text. |
| _reversa_sdd/internal/cli/questions.md | Updates Q-CLI-011 content (product naming). |
| _reversa_sdd/internal/cli/contracts.md | Updates version command contract output naming. |
Review details
- Files reviewed: 41/46 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if CGO_ENABLED=1 GOOS=darwin GOARCH=arm64 go build \ | ||
| -ldflags="-w -s -X main.version=${{ needs.create-release.outputs.version }} -X main.commit=${{ github.sha }} -X main.date=$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ | ||
| -o dist/bloco-vgen \ | ||
| ./cmd/bloco-vgen; then |
| # Binary install directory. `go env GOBIN` is empty unless explicitly set, so | ||
| # fall back to the computed GOPATH/bin. This matches `go install` semantics. | ||
| GOBIN := $(shell $(GOCMD) env GOBIN) | ||
| GOPATH := $(shell $(GOCMD) env GOPATH) | ||
| ifeq ($(strip $(GOBIN)),) | ||
| GOBIN := $(GOPATH)/bin | ||
| endif |
| var keyBytes [32]byte | ||
| var pub [64]byte | ||
| keyBytes, pub, err = chain.NextKey() | ||
| if err != nil { | ||
| p.poolManager.GetCryptoPool().PutPrivateKeyBuffer(privateKeyBytes) | ||
| if p.logger != nil { | ||
| context := map[string]interface{}{ | ||
| "worker_id": workerID, | ||
| "attempts": attempts, | ||
| } | ||
| if logErr := p.logger.LogError("address_generation", err, context); logErr != nil { | ||
| if logErr := p.logger.LogError("crypto_key_generation", err, context); logErr != nil { | ||
| _ = logErr | ||
| } | ||
| } | ||
| continue | ||
| } |
| switch m.state { | ||
| case BenchmarkStateProgress: | ||
| return m.renderProgressView() | ||
| case BenchmarkStateTransitioning: | ||
| return m.renderTransitionView() | ||
| return m.renderProgressView() | ||
| case BenchmarkStateResults: | ||
| return m.renderResultsView() | ||
| return m.renderProgressView() | ||
| default: |
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
LlamaPReview — Blocking issues found
Default CPU generation emits sequential, correlated private keys within each 4096-key chain
Exact-head CI remains unresolved (4 failed); no CI-dependent merge-safety claim is made.
Owner action: Draw a fresh random seed for every emitted wallet or restore independent crypto/rand key generation on the CPU path; remove cross-call chain reuse in Pool.getChain. Add a test asserting two wallets from count=2 are not k, k+1. Reconcile the audit finding the cited evidence with the implementation.
4 further items in details.
Risk path
Tracks the flow from CLI invocation to escaped wallet keys, highlighting where the CPU chain correlation defect and the unverified Metal validation step sit.
sequenceDiagram
participant CLI as bloco-vgen generate
participant ENG as engine dispatch
participant CPU as CPU pool
participant MET as Metal pool
participant OUT as returned wallet
CLI->>ENG: getGenerationEngineOptions (auto/cpu/metal)
ENG->>CPU: ResolveGeneration auto → CPU (default)
ENG->>MET: ResolveGeneration metal (only if forced/available)
CPU->>CPU: Pool.getChain → seed k0
note over CPU: PR change — chain reused per worker across calls<br/>keys k0, k0+1, ..., k0+4095
critical CPU unsafe path
CPU->>OUT: wallet with k0+i key (correlated)
end
MET->>MET: GPU runMetalMatch + findValidatedMetalGenerationCandidate
note over MET: PR change — candidate validation bodies unseen
critical Metal uncertainty
MET->>OUT: wallet if candidate validated (unverified)
end
Review details and evidence
| Priority | File | Finding | Evidence |
|---|---|---|---|
| P1 | internal/worker/pool.go |
Default CPU generation emits sequential, correlated private keys within each 4096-key chain | confirmed |
| P1 | internal/engine/cpu.go |
Lint fails with 13 unused/staticcheck issues, all in PR-created or modified files | confirmed |
Finding details
P1 · Lint fails with 13 unused/staticcheck issues, all in PR-created or modified files
internal/engine/cpu.go
The exact-head golangci-lint run exits 1 with 13 issues: unused transitionTime, renderTransitionView, renderResultsView (internal/tui/benchmark.go); unused StatsModel.renderPatternOverview (internal/tui/stats.go); unused resolveCommandEngine (internal/cli/benchmark.go); unused runEthereumAddressAttempt, runEthereumPublicKeyAttempt, generateEthereumPrivateKeyAttempt, throughputForDuration (internal/engine/cpu.go); staticcheck QF1008 in engine_test.go and commands_test.go; staticcheck S1000 (select with single channel case) in internal/crypto/chain.go. All flagged files are new or modified in this PR, so the lint regression blocks merge under the repository's lint gate.
Verification boundary: confirmed; scope: bounded reviewed context.
Material unknowns
- The bodies of findValidatedMetalGenerationCandidate, NormalizeMetalValidationMode, and ValidateMetalGenerationCriteria could not be retrieved; whether Metal candidates are actually CPU-verified and key-range-checked before being returned is unestablished. If these helpers do not perform full CPU re-verification, the PR's 'Metal with validation' claim is not met; this decides whether the Metal objective closure is satisfied.
- Check: Retrieve and read the three helper definitions in the exact-head source and confirm they validate candidates on CPU before returning.
- The bodies of generateSingleWallet and generateMultipleWallets were not retrieved, so it is unknown whether persistence/keystore behavior beyond the chain swap changed on the CPU path. If these helpers changed keystore naming or persistence logic, there could be additional regressions not covered by the CPU key-source finding.
- Check: Inspect generateSingleWallet/generateMultipleWallets in the exact-head source to confirm only the key-source change was made.
- Callers of GetKeystoreFilePath were not observable; the new logic scans for UTC--...--.json and falls back to legacy only if not found, whereas the old behavior returned the legacy path unconditionally. If callers load keystores from directories containing only legacy files, the import/export/load flows may regress.
- Check: Verify all callers of GetKeystoreFilePath handle legacy-only directories correctly and add a regression test if needed.
LlamaPReview checks
- Reviewed changed regions in
internal/worker/pool.go. - Reviewed changed regions in
internal/crypto/chain.go. - Reviewed changed regions in
internal/crypto/chain_test.go. - Reviewed changed regions in
docs/security-audit/relatorio-auditoria-seguranca.md. - Reviewed changed regions in
internal/engine/cpu.go. - Reviewed changed regions in
internal/cli/benchmark.go.
LlamaPReview is an open-source pull request reviewer. See how a review is built, from signed webhook to publication.
| // getChain returns the chained key generator for a worker, creating it on | ||
| // first use. Chains are reused across GenerateWalletWithContext calls so | ||
| // batch generation does not rebuild the (expensive) point chain per wallet. | ||
| func (p *Pool) getChain(workerID int) (*crypto.PrivateKeyChain, error) { |
There was a problem hiding this comment.
P1 | Confidence: High
Pool.getChain persists one PrivateKeyChain per worker and reuses it across GenerateWalletWithContext calls. The chain emits k0, k0+1,..., k0+4095 and is refilled with a new seed after 4096 keys. Multiple wallets from one invocation (or repeated calls on the same pool) therefore have private keys separated by exactly 1, not independent randomness. Recovering any two keys from the same chain reveals the seed and all other keys in that chain. The PR's own audit report rates this as high severity. chain_test.go verifies the sequential invariant (expected key = k0 + i) and cross-checks derivation against go-ethereum, but no test asserts non-correlation across outputs.
Evidence: changed region in internal/worker/pool.go; changed region in internal/crypto/chain.go; changed region in internal/crypto/chain_test.go; changed region in docs/security-audit/relatorio-auditoria-seguranca.md.
Fecha #15, #16, #17, #18, #19, #20 e #21. #15 Chaves privadas Ethereum entregues não eram independentes A engine CPU varre candidatos em lotes encadeados (k0, k0+1, ...) e entregava essas chaves como carteiras, então duas carteiras da mesma execução ficavam a poucas unidades de distância. A cadeia agora é aposentada assim que entrega uma chave (Pool.retireChain), de modo que cada chave entregue vem de um seed próprio. PrivateKeyChain.Close encerra o filler, evitando vazar goroutine e memória do lote a cada descarte. #16 Segredos em texto claro ao lado dos artefatos cifrados O arquivo <endereço>.pwd e o <endereço>.key do Solana passam a ser opt-in (--write-password-file / --write-plaintext-key). A senha é exibida uma vez ao fim da execução, ou vai para o arquivo de --output. Ao corrigir isso apareceu um defeito maior que o relatado: a geração de keystore para Solana falhava por completo (ToECDSA sobre chave Ed25519 de 64 bytes), então nada era gravado e a chave só existia no stdout. O endereço agora é derivado apenas para secp256k1, a validação do ciphertext aceita 32 ou 64 bytes, e o .json do Solana guarda um KeyStore V3 realmente cifrado — em vez do placeholder cujo campo "note" afirmava falsamente que a chave estava cifrada. #17 Backup de Bitcoin não restaurava a carteira A mnemônica era sorteada sem relação com a chave e era o único artefato salvo. O caminho padrão agora persiste um KeyStore V3 cifrado (alternativa prevista na issue) e não anexa mais mnemônica falsa. --with-mnemonic passa a funcionar para Bitcoin derivando a chave de verdade em m/44'/0'/0'/0/0, espelhando o que o Ethereum já fazia. #18 Controles de KDF e de permissão nunca aplicados Primeiro o piso de segurança do scrypt (n >= 16384 e 128*n*r >= 16 MiB, espelhando o preset "low" como o piso do PBKDF2 já fazia), depois a ligação dos parâmetros: EncryptPrivateKeyWithKDF passa a usar config.KDFParams, com cópia do mapa para o salt não ser reaproveitado entre keystores. O piso é reaplicado antes de cifrar. KeyStore.FileMode agora afeta as escritas e a validação recusa qualquer bit fora de owner-rw. #19 Permissões frouxas Logs passam de 0644 para 0600 (na criação e na rotação, com chmod explícito para arquivos pré-existentes) e o diretório de keystores de 0755 para 0700. CheckDirectoryPermissions passa a inspecionar o modo: recusa diretório world-writable e alerta sobre bits de grupo/outros. #20 Cadeia de CI github.event.inputs.tag sai do corpo do run: e passa por env, com validação por regex de semver; as demais interpolações do release seguem o mesmo padrão. trivy-action fixada em @0.28.0. dependabot.yml preenchido com gomod e github-actions. #21 Flags inertes e --network sem validação --output e --format implementados (text/json/csv), gravando com 0600 e sem imprimir a chave privada no terminal. Network validado por lista fechada em GenerationCriteria.Validate, falhando antes de gerar qualquer chave. Testes: 30 novos casos de regressão, incluindo um que reproduz o achado #15 (sem a correção, acusa carteiras a distância 1, 2, 3...). go test ./... e go vet limpos; suíte completa também sob -race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VS3G7uWtzTjo1Vx3iDqiK5
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| resultCase.MetalSpeedupVsAuto = metalResult.AverageSpeed / resultCase.Auto.AverageSpeed | ||
| } | ||
| resultCase.MetalCoefficientOfVariation = benchmarkCoefficientOfVariation(metalResult.SpeedSamples) | ||
| resultCase.MetalStable = len(metalResult.SpeedSamples) > 1 && resultCase.MetalCoefficientOfVariation <= phase6StabilityCVThreshold |
There was a problem hiding this comment.
🟡 Metal comparisons always report instability
Comparison runs omit the callback that collects SpeedSamples, then synthesize one sample. The two-sample stability gate can never accept Metal.
Prompt for agents
Decouple Metal speed-sample collection from the presence of an external callback. internal/engine/metal_darwin_arm64.go should collect interval samples whenever sampleInterval is positive, while invoking onSample only when non-nil. Then the comparison stability gate in internal/cli/benchmark.go can evaluate multiple real samples.
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.