Extract standalone arkd-signer + shared txsigner lib (BREAKING: ARKD_SIGNER_ADDR required) - #1118
Extract standalone arkd-signer + shared txsigner lib (BREAKING: ARKD_SIGNER_ADDR required)#1118Kukks wants to merge 26 commits into
Conversation
…keep arkd dependency-neutral
…GetPubkey deprecated_signers)
…service, and e2e harness rewire
|
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:
WalkthroughChangesThe PR extracts operator signing from Standalone signer implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
The pure arkd-signer holds only the operator key, so it signs the vtxo (tapscript) input but not the connector (wallet-owned key-path) input. The old wallet-as-signer signed both; the split now requires the wallet to co-sign the connector before finalizing the forfeit tx.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🔍 Arkana Code Review — arkd-signer extraction
Clean architectural split. The decomposition is well-motivated and the implementation follows existing patterns. A few issues to address before merge, one of which is protocol-critical.
🚨 P1 — Protocol-Critical: nil-pointer dereference in signer signing loop
pkg/arkd-signer/core/application/signer.go:157
for i, in := range ptx.Inputs {
if bytes.Equal(in.WitnessUtxo.PkScript, txutils.ANCHOR_PKSCRIPT) {If in.WitnessUtxo is nil, this panics. Yes, BuildPrevoutFetcher earlier will error on missing WitnessUtxo, but that's an implicit contract — a refactor could easily break it. A signing service that panics on malformed input is a DoS vector. Add an explicit nil guard:
if in.WitnessUtxo == nil {
continue // or return error
}This same pattern also applies in the wallet's existing SignTransaction (pre-existing issue, not introduced here, but worth noting).
🚨 P1 — Protocol-Critical: fraud.go forfeit signing order needs verification
internal/core/application/fraud.go:167-174
signedForfeitTx, err := s.signer.SignTransactionTapscript(ctx, forfeitTxB64, nil)
// ...
signedForfeitTx, err = s.wallet.SignTransaction(ctx, signedForfeitTx, false)The dual-signing flow (signer signs VTXO tapscript input, wallet signs connector key-path input) is architecturally correct for the split. However:
-
Does
s.wallet.SignTransactionhandle a partially-signed PSBT correctly? The wallet'sSignTransactionin LP mode skips taproot script-path inputs that already have signatures, right? Confirm the wallet won't try to re-sign or reject the already-signed tapscript input. -
Ordering matters: If the wallet's signing depends on seeing the signer's signature in the PSBT (e.g., for sighash computation on the other input), this order is correct. If not, parallel signing might be possible but that's a follow-up. Just confirming the serial order is intentional and correct.
⚠️ P2 — Health handler always returns SERVING
pkg/arkd-signer/interface/grpc/handlers/healthservice.go:14-20
The health check unconditionally returns SERVING. It should delegate to signer.IsReady(ctx) so that orchestrators (Docker healthcheck, k8s liveness probe) can detect a misconfigured signer (e.g., missing secret key that somehow bypassed config validation).
func (h *healthHandler) Check(ctx context.Context, ...) (...) {
if !h.signer.IsReady(ctx) {
return &grpchealth.HealthCheckResponse{Status: grpchealth.HealthCheckResponse_NOT_SERVING}, nil
}
// ...
}⚠️ P2 — No TLS / mTLS support on the signer service
pkg/arkd-signer/interface/grpc/service.go:89
grpc.Creds(insecure.NewCredentials()),The signer holds the operator private key — the most sensitive secret in the system. In production, this service MUST NOT be reachable over cleartext. Even if the plan is "same-host networking only", a single misconfigured firewall rule exposes the signing key. At minimum:
- Add a
ARKD_SIGNER_NO_TLSflag (default false) mirroring arkd's pattern. - Or document explicitly that the signer MUST be deployed behind a sidecar proxy (e.g., Envoy with mTLS).
If this is intentionally deferred, add a TODO and a startup warning log.
⚠️ P2 — CORS wildcard on a signing service
pkg/arkd-signer/interface/grpc/service.go:132-137
w.Header().Set("Access-Control-Allow-Origin", "*")
w.Header().Set("Access-Control-Allow-Headers", "*")Access-Control-Allow-Origin: * on a signing endpoint is concerning. The signer should never be browser-accessible. If the grpc-gateway is only for internal health/status probes, restrict CORS or remove it entirely.
📋 P3 — Cross-repo breakage: 6+ downstream repos need migration
The following repos still reference ARKD_WALLET_SIGNER_KEY in their docker-compose files and will break:
arkade-os/solverarkade-os/introspector-reviewarkade-os/arkd-pentesterarkade-os/emulatorarkade-os/arkade-kotlinarkade-os/bancodarkade-os/layerzero-usdt0-arkade-demo
None of these set ARKD_SIGNER_ADDR, so arkd will fail to start (ARKD_SIGNER_ADDR is required). Consider opening tracking issues or a follow-up PR batch for these repos.
Also, arkade-kotlin still has the LoadSignerKey RPC in its vendored proto — this will need updating.
📋 P3 — Config key length validation missing for current key
pkg/arkd-signer/config/config.go:61-63
buf, err := hex.DecodeString(c.SecretKey)
// ...
prvkey, _ := btcec.PrivKeyFromBytes(buf)The deprecated key parser validates len(buf) != 32 but the current key does not. btcec.PrivKeyFromBytes will silently truncate/pad non-32-byte inputs. Add the same check:
if len(buf) != 32 {
return fmt.Errorf("invalid signer secret key: must be 32 bytes (64 hex chars)")
}✅ What looks good
txsignerlibrary is clean, well-tested, chain-free by design — good reusable primitive.- Deprecated key selection logic faithfully ported from wallet with full parity.
- E2e harness correctly rewired to recreate
arkd-signerinstead ofarkd-walletfor key rotation tests. - Clean removal of
SignModeSignerfrom wallet — no dead code left behind. LoadSignerKeyon wallet returnsUnimplementedrather than silently succeeding — correct migration signal.- Config
String()method properly redacts secrets.
Verdict: Request changes on P1 items. The forfeit signing change is protocol-critical and requires human sign-off even after code fixes.
Clears the new golang.org/x/crypto SSH CVE batch + golang.org/x/net CVE-2026-25680 flagged by Trivy. Mirrors the bump on the #1118 base; self-drops as an empty commit when this branch rebases onto a bumped master.
Clears the new x/crypto SSH CVE batch + x/net CVE-2026-25680 flagged by Trivy. Mirrors the #1118 base bump; self-drops on rebase onto a bumped master.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (3)
pkg/arkd-signer/config/config_test.go (1)
16-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a malformed-length secret key test.
Please add a case where
ARKD_SIGNER_SECRET_KEYis valid hex but not 32 bytes, and assertLoadConfig()fails. That guards the key-length contract directly.🤖 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 `@pkg/arkd-signer/config/config_test.go` around lines 16 - 23, Add a new test function that validates LoadConfig() properly rejects malformed secret keys. Create a test (e.g., TestLoadConfigRejectsInvalidSecretKeyLength) that sets the ARKD_SIGNER_SECRET_KEY environment variable to a valid hexadecimal string that is not 32 bytes in length, calls config.LoadConfig(), and asserts that it returns an error. This ensures the key-length validation contract is properly enforced and guards against accepting keys of incorrect length.pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go (1)
15-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend this test to assert
DeprecatedSignersmapping too.
GetPubkeynow returns both current and deprecated signer metadata. Adding one deprecated-key case here will lock in the handler’s response mapping contract.🤖 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 `@pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go` around lines 15 - 30, The TestSignerHandlerStatusAndPubkey test currently only validates the main public key returned from h.GetPubkey but does not verify the DeprecatedSigners mapping in the response. Extend the test by adding an assertion after the pub.GetPubkey() validation that checks the DeprecatedSigners field in the response object. Create a deprecated key scenario (either by modifying the handler setup or the application initialization) and assert that the DeprecatedSigners map contains the expected key-value mappings, ensuring the handler correctly populates both current and deprecated signer metadata in the GetPubkey response.pkg/arkd-signer/core/application/signer_test.go (1)
93-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd key-selection coverage for CSV-wrapped multisig leaves.
The table at Lines 104-124 only exercises plain multisig leaves. Please add cases for
CSVMultisigClosure/ConditionCSVMultisigClosureso deprecated-key routing regressions are caught.🤖 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 `@pkg/arkd-signer/core/application/signer_test.go` around lines 93 - 124, The test table in TestSignTransactionTapscriptSelectsKeyByLeaf only covers plain multisig leaves with the "current key" and "deprecated key by leaf" cases. Add two additional test cases to the tests slice to cover CSVMultisigClosure and ConditionCSVMultisigClosure leaf types, following the same pattern as the existing cases but using the appropriate closure constructors. This ensures that deprecated-key routing is properly tested for all leaf types, preventing regressions in key selection for CSV-wrapped multisig scenarios.
🤖 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 `@arkdsigner.Dockerfile`:
- Around line 15-25: The runtime container specified by the FROM alpine:3.20
directive is currently running as root by default, which poses a security risk
for the sensitive key material in the arkd-signer binary. Add a USER instruction
to create a non-root user (such as 'appuser' or similar) using a RUN command
with apk add, then add a USER directive before the ENTRYPOINT to switch to this
non-root user so the arkd-signer process runs with reduced privileges instead of
as root.
In `@envs/signer.dev.env`:
- Line 2: The ARKD_SIGNER_SECRET_KEY in the tracked signer.dev.env file contains
real key material, which is a security risk. Replace the actual key value with a
placeholder string (such as a dummy value or empty string) to keep the file safe
for version control. Then configure your local development environment to inject
the actual signer private key from a separate untracked file (such as .env.local
or a secrets management tool) that should be added to .gitignore to prevent
accidental commits of sensitive credentials.
In `@internal/core/application/fraud.go`:
- Around line 173-175: The wallet signing call in the SignTransaction method on
line 175 is passing the entire PSBT, which can cause the wallet to sign all
taproot inputs including the vtxo that was already signed by the operator.
Identify the input index that matches the connectorOutpoint variable and modify
the SignTransaction call to only sign that specific connector input instead of
the entire transaction, or alternatively implement a connector-only signing
method that restricts the operation to just the connector input. Ensure this
scoped signing happens before FinalizeAndExtract is called.
In `@internal/test/e2e/utils_test.go`:
- Around line 594-612: Replace the hardcoded time.Sleep(5 * time.Second) call in
the recreateArkdSigner function with a proper readiness polling mechanism.
Create a new helper function (such as waitForSignerReady) that polls the
arkd-signer health endpoint via HTTP GET request to a health check endpoint,
implementing a retry loop with a configurable deadline (e.g., 30 seconds) and
shorter polling intervals (e.g., 500 milliseconds) instead of a single fixed
delay. This ensures arkd-signer is actually ready before proceeding, reducing
test flakiness on slower runners.
In `@pkg/ark-lib/txsigner/txsigner.go`:
- Around line 101-103: The loop iterating through in.TaprootScriptSpendSig
indexes signatures only by the XOnlyPubKey, which causes signatures from
different leaf hashes to overwrite each other. Modify the loop to filter
signatures based on the active leaf hash before adding them to the args map.
Check if each signature's leaf hash matches the expected active leaf hash before
indexing it. This applies to both the loop at lines 101-103 for
TaprootScriptSpendSig and the similar loop at lines 105-106.
- Around line 43-47: The tapLeaf creation in the RawTxInTapscriptSignature call
currently hardcodes NewBaseTapLeaf which ignores the LeafVersion field from the
PSBT input. Replace the NewBaseTapLeaf call with a function that creates a tap
leaf using both the Script and the LeafVersion from
in.TaprootLeafScript[0].LeafVersion to ensure the correct leaf hash is computed
for non-base leaves. Apply this same fix to the other location mentioned at
lines 52-60.
In `@pkg/arkd-signer/config/config.go`:
- Around line 61-66: The signer secret key parsing (buf decoding before
btcec.PrivKeyFromBytes) lacks 32-byte length validation that exists in the
deprecated key handling code, allowing malformed hex inputs to initialize
unintended keys since PrivKeyFromBytes truncates values exceeding 32 bytes.
After the hex.DecodeString call that populates buf, add a validation check to
ensure len(buf) equals 32 bytes, returning an error if the length does not
match, matching the validation pattern used in the deprecated key handling code
around lines 117-120.
In `@pkg/arkd-signer/core/application/signer.go`:
- Around line 151-160: The switch statement in the signerKeyForLeaf function is
missing cases for CSV-based closure variants that can be returned by
script.DecodeClosure. Currently, when a CSV closure type is encountered, it
falls through to the default case which returns s.key, causing the wrong key to
be used for signing. Add switch cases for the CSV variants of the existing
closure types (look for script.CSVMultisigClosure,
script.CSVCLTVMultisigClosure, script.CSVConditionMultisigClosure or similar
names in the script package) and extract their PubKeys field just like the
existing MultisigClosure, CLTVMultisigClosure, and ConditionMultisigClosure
cases do.
- Around line 109-117: The code currently silently skips invalid inputIndexes
(out-of-range or negative values) during iteration using the slices.Contains
check, which allows the function to return success without actually signing
anything. Add validation logic before the loop that iterates through ptx.Inputs
to check that all values in the inputIndexes parameter are non-negative and
within the valid range of input indices (0 to len(ptx.Inputs)-1). If any index
is invalid, return an error immediately rather than proceeding with the
iteration where invalid indexes will be silently skipped by the slices.Contains
check.
In `@pkg/arkd-signer/go.mod`:
- Line 5: The go.mod file contains conflicting version specifications for the
github.com/btcsuite/btcd/btcec/v2 module where the replace directive pins it to
v2.3.3 while the require directive specifies v2.3.4. Since replace directives
override require directives in Go modules, this creates a misleading requirement
statement. Align the versions by either updating the require directive to match
the replace directive's version (v2.3.3), or remove the replace directive
entirely if version pinning is not necessary. Apply the same fix to the other
go.mod files mentioned (pkg/ark-lib/go.mod and the root go.mod) to maintain
consistency across the module dependencies.
In `@pkg/arkd-signer/interface/grpc/handlers/healthservice.go`:
- Around line 24-29: The Watch method in the healthHandler is returning nil,
which violates the gRPC health checking specification and causes client
performance issues. Replace the nil return statement in the Watch method with
status.Error(codes.Unimplemented, "health watch is not implemented") to properly
signal to clients that the Watch streaming operation is not implemented
according to the gRPC health checking protocol.
In `@pkg/arkd-signer/interface/grpc/service.go`:
- Around line 104-115: The CORS headers in both the isOptionRequest and
isHttpRequest blocks are using wildcard values which are insecure for a signing
service. Replace the wildcard value in Access-Control-Allow-Origin header with a
specific allowlist of trusted origins instead of using asterisk. Consider
implementing a configuration mechanism that validates the request origin against
an allowlist and only sets the Access-Control-Allow-Origin header to the
specific trusted origin making the request, rather than allowing all origins
access to the signer endpoints.
- Around line 92-97: The Shutdown call on s.server is using context.Background()
which can block indefinitely if there are stuck connections. Replace
context.Background() with context.WithTimeout to provide a bounded timeout for
the shutdown operation, ensuring the server terminates within a reasonable time
frame. Consider using a reasonable timeout value (such as a few seconds) to
balance graceful shutdown with guaranteed termination.
- Around line 80-85: The Start method launches the server in a goroutine and
immediately returns nil, so if ListenAndServe fails, the panic occurs
asynchronously outside the method's error handling path. Instead of panicking in
the goroutine, implement a mechanism to detect and return startup failures from
the Start method itself. Consider using a channel or error variable to
communicate from the goroutine whether the server successfully started
listening, then check this result before returning from Start, returning an
error if the listen operation failed.
In `@README.md`:
- Around line 167-170: The README.md documentation shows a concrete private key
value for ARKD_SIGNER_SECRET_KEY which is a security risk and encourages unsafe
copy-paste behavior. Replace the actual key value with a placeholder (like a
series of zeros or a descriptive placeholder) and add a clear command that shows
users how to generate their own random private key instead. This way the
documentation provides guidance on how to create a proper secret without
exposing or promoting the use of a fixed hardcoded value.
---
Nitpick comments:
In `@pkg/arkd-signer/config/config_test.go`:
- Around line 16-23: Add a new test function that validates LoadConfig()
properly rejects malformed secret keys. Create a test (e.g.,
TestLoadConfigRejectsInvalidSecretKeyLength) that sets the
ARKD_SIGNER_SECRET_KEY environment variable to a valid hexadecimal string that
is not 32 bytes in length, calls config.LoadConfig(), and asserts that it
returns an error. This ensures the key-length validation contract is properly
enforced and guards against accepting keys of incorrect length.
In `@pkg/arkd-signer/core/application/signer_test.go`:
- Around line 93-124: The test table in
TestSignTransactionTapscriptSelectsKeyByLeaf only covers plain multisig leaves
with the "current key" and "deprecated key by leaf" cases. Add two additional
test cases to the tests slice to cover CSVMultisigClosure and
ConditionCSVMultisigClosure leaf types, following the same pattern as the
existing cases but using the appropriate closure constructors. This ensures that
deprecated-key routing is properly tested for all leaf types, preventing
regressions in key selection for CSV-wrapped multisig scenarios.
In `@pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go`:
- Around line 15-30: The TestSignerHandlerStatusAndPubkey test currently only
validates the main public key returned from h.GetPubkey but does not verify the
DeprecatedSigners mapping in the response. Extend the test by adding an
assertion after the pub.GetPubkey() validation that checks the DeprecatedSigners
field in the response object. Create a deprecated key scenario (either by
modifying the handler setup or the application initialization) and assert that
the DeprecatedSigners map contains the expected key-value mappings, ensuring the
handler correctly populates both current and deprecated signer metadata in the
GetPubkey response.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f80ad7a-d721-46e5-a8bc-664448ab7021
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumpkg/ark-lib/go.sumis excluded by!**/*.sumpkg/arkd-signer/go.sumis excluded by!**/*.sumpkg/arkd-wallet/go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
MakefileREADME.mdarkdsigner.Dockerfilecmd/arkd-signer/main.gocmd/arkd/commands.gocmd/arkd/flags.godocker-compose.regtest.ymlenvs/signer.dev.envgo.modinternal/config/config.gointernal/core/application/fraud.gointernal/core/ports/wallet.gointernal/infrastructure/tx-builder/covenantless/mocks_test.gointernal/infrastructure/wallet/wallet_client.gointernal/interface/grpc/handlers/signer_manager.gointernal/interface/grpc/service.gointernal/test/e2e/e2e_test.gointernal/test/e2e/utils_test.gopkg/ark-lib/go.modpkg/ark-lib/txsigner/txsigner.gopkg/ark-lib/txsigner/txsigner_test.gopkg/arkd-signer/config/config.gopkg/arkd-signer/config/config_test.gopkg/arkd-signer/core/application/signer.gopkg/arkd-signer/core/application/signer_test.gopkg/arkd-signer/go.modpkg/arkd-signer/interface/grpc/handlers/healthservice.gopkg/arkd-signer/interface/grpc/handlers/signer_handler.gopkg/arkd-signer/interface/grpc/handlers/signer_handler_test.gopkg/arkd-signer/interface/grpc/interceptors/interceptor.gopkg/arkd-signer/interface/grpc/interceptors/logger.gopkg/arkd-signer/interface/grpc/service.gopkg/arkd-wallet/config/config.gopkg/arkd-wallet/core/application/types.gopkg/arkd-wallet/core/application/wallet/service.gopkg/arkd-wallet/go.modpkg/arkd-wallet/interface/grpc/handlers/wallet_handler.gopkg/arkd-wallet/interface/grpc/service.goscripts/build-arkd-signer
💤 Files with no reviewable changes (6)
- internal/infrastructure/tx-builder/covenantless/mocks_test.go
- internal/core/ports/wallet.go
- pkg/arkd-wallet/core/application/types.go
- internal/infrastructure/wallet/wallet_client.go
- cmd/arkd/flags.go
- pkg/arkd-wallet/interface/grpc/service.go
| FROM alpine:3.20 | ||
|
|
||
| RUN apk update && apk upgrade | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| COPY --from=builder /app/bin/arkd-signer /app/ | ||
|
|
||
| ENV PATH="/app:${PATH}" | ||
|
|
||
| ENTRYPOINT [ "arkd-signer" ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Run the runtime container as a non-root user.
The signer holds sensitive key material; running as root increases blast radius if compromised.
Suggested hardening
FROM alpine:3.20
RUN apk update && apk upgrade
WORKDIR /app
-COPY --from=builder /app/bin/arkd-signer /app/
+RUN addgroup -S signer && adduser -S -G signer signer
+COPY --from=builder --chown=signer:signer /app/bin/arkd-signer /app/
ENV PATH="/app:${PATH}"
+USER signer
ENTRYPOINT [ "arkd-signer" ]🤖 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 `@arkdsigner.Dockerfile` around lines 15 - 25, The runtime container specified
by the FROM alpine:3.20 directive is currently running as root by default, which
poses a security risk for the sensitive key material in the arkd-signer binary.
Add a USER instruction to create a non-root user (such as 'appuser' or similar)
using a RUN command with apk add, then add a USER directive before the
ENTRYPOINT to switch to this non-root user so the arkd-signer process runs with
reduced privileges instead of as root.
| ARKD_WALLET_SIGNER_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6 | ||
| ARKD_WALLET_PORT=6161 No newline at end of file | ||
| ARKD_SIGNER_LOG_LEVEL=5 | ||
| ARKD_SIGNER_SECRET_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not commit a real signer private key in a tracked env file.
Line 2 embeds live key material. Replace this with a placeholder and inject the actual key from an untracked local env/secret store.
Suggested fix
-ARKD_SIGNER_SECRET_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6
+ARKD_SIGNER_SECRET_KEY=__SET_IN_LOCAL_UNTRACKED_ENV__📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARKD_SIGNER_SECRET_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6 | |
| ARKD_SIGNER_SECRET_KEY=__SET_IN_LOCAL_UNTRACKED_ENV__ |
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 `@envs/signer.dev.env` at line 2, The ARKD_SIGNER_SECRET_KEY in the tracked
signer.dev.env file contains real key material, which is a security risk.
Replace the actual key value with a placeholder string (such as a dummy value or
empty string) to keep the file safe for version control. Then configure your
local development environment to inject the actual signer private key from a
separate untracked file (such as .env.local or a secrets management tool) that
should be added to .gitignore to prevent accidental commits of sensitive
credentials.
Source: Linters/SAST tools
| // recreate the arkd-signer container with overridden signer keys, then restart | ||
| // arkd so it re-fetches the signer pubkey. The signer is stateless (key from | ||
| // config), so no data volume or wallet unlock is involved. | ||
| func recreateArkdSigner(secretKey, deprecated string) error { | ||
| env := []string{ | ||
| "ARKD_WALLET_SIGNER_KEY=" + signerKey, | ||
| "ARKD_WALLET_DEPRECATED_SIGNER_KEYS=" + deprecated, | ||
| "ARKD_SIGNER_SECRET_KEY=" + secretKey, | ||
| "ARKD_SIGNER_DEPRECATED_KEYS=" + deprecated, | ||
| } | ||
| args := []string{ | ||
| "compose", "-f", "../../../docker-compose.regtest.yml", | ||
| "up", "-d", "--force-recreate", "--no-deps", "arkd-wallet", | ||
| "up", "-d", "--force-recreate", "--no-deps", "arkd-signer", | ||
| } | ||
| if _, err := runCommandWithEnv(env, "docker", args...); err != nil { | ||
| return fmt.Errorf("failed to recreate arkd-wallet: %w", err) | ||
| } | ||
|
|
||
| time.Sleep(8 * time.Second) | ||
|
|
||
| if err := unlockArkdWallet(); err != nil { | ||
| return err | ||
| return fmt.Errorf("failed to recreate arkd-signer: %w", err) | ||
| } | ||
|
|
||
| time.Sleep(5 * time.Second) | ||
|
|
||
| return restartArkd() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace fixed-delay restart with readiness polling for arkd-signer.
Using a hardcoded 5s sleep here can make this test path flaky on slower runners; arkd may restart before signer is ready and fail transiently.
Suggested change
func recreateArkdSigner(secretKey, deprecated string) error {
@@
if _, err := runCommandWithEnv(env, "docker", args...); err != nil {
return fmt.Errorf("failed to recreate arkd-signer: %w", err)
}
- time.Sleep(5 * time.Second)
+ if err := waitForSignerReady(); err != nil {
+ return err
+ }
return restartArkd()
}// Add near other test helpers.
func waitForSignerReady() error {
client := &http.Client{Timeout: 2 * time.Second}
deadline := time.Now().Add(30 * time.Second)
for time.Now().Before(deadline) {
// Use your signer health endpoint exposed by grpc-gateway.
req, _ := http.NewRequest(http.MethodGet, "http://localhost:6061/v1/health", nil)
resp, err := client.Do(req)
if err == nil {
resp.Body.Close()
if resp.StatusCode == http.StatusOK {
return nil
}
}
time.Sleep(500 * time.Millisecond)
}
return fmt.Errorf("arkd-signer did not become ready in time")
}🤖 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 `@internal/test/e2e/utils_test.go` around lines 594 - 612, Replace the
hardcoded time.Sleep(5 * time.Second) call in the recreateArkdSigner function
with a proper readiness polling mechanism. Create a new helper function (such as
waitForSignerReady) that polls the arkd-signer health endpoint via HTTP GET
request to a health check endpoint, implementing a retry loop with a
configurable deadline (e.g., 30 seconds) and shorter polling intervals (e.g.,
500 milliseconds) instead of a single fixed delay. This ensures arkd-signer is
actually ready before proceeding, reducing test flakiness on slower runners.
| tapLeaf := txscript.NewBaseTapLeaf(in.TaprootLeafScript[0].Script) | ||
| signature, err := txscript.RawTxInTapscriptSignature( | ||
| ptx.UnsignedTx, sigHashes, inputIndex, in.WitnessUtxo.Value, | ||
| in.WitnessUtxo.PkScript, tapLeaf, in.SighashType, signingKey, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the PSBT-provided leaf version when computing tapscript signatures.
Line 43 always uses NewBaseTapLeaf, which ignores TaprootLeafScript[0].LeafVersion. For non-base leaves, this computes the wrong leaf hash/signature pair.
Suggested fix
- tapLeaf := txscript.NewBaseTapLeaf(in.TaprootLeafScript[0].Script)
+ tapLeaf := txscript.TapLeaf{
+ LeafVersion: in.TaprootLeafScript[0].LeafVersion,
+ Script: in.TaprootLeafScript[0].Script,
+ }Also applies to: 52-60
🤖 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 `@pkg/ark-lib/txsigner/txsigner.go` around lines 43 - 47, The tapLeaf creation
in the RawTxInTapscriptSignature call currently hardcodes NewBaseTapLeaf which
ignores the LeafVersion field from the PSBT input. Replace the NewBaseTapLeaf
call with a function that creates a tap leaf using both the Script and the
LeafVersion from in.TaprootLeafScript[0].LeafVersion to ensure the correct leaf
hash is computed for non-base leaves. Apply this same fix to the other location
mentioned at lines 52-60.
| if s.server != nil { | ||
| _ = s.server.Shutdown(context.Background()) | ||
| } | ||
| if s.grpcSrv != nil { | ||
| s.grpcSrv.GracefulStop() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a bounded shutdown timeout.
Shutdown(context.Background()) can block indefinitely under stuck connections. Use context.WithTimeout to guarantee termination progress.
Suggested fix
import (
"context"
"errors"
"fmt"
"net/http"
"strings"
+ "time"
@@
if s.server != nil {
- _ = s.server.Shutdown(context.Background())
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ _ = s.server.Shutdown(ctx)
}🤖 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 `@pkg/arkd-signer/interface/grpc/service.go` around lines 92 - 97, The Shutdown
call on s.server is using context.Background() which can block indefinitely if
there are stuck connections. Replace context.Background() with
context.WithTimeout to provide a bounded timeout for the shutdown operation,
ensuring the server terminates within a reasonable time frame. Consider using a
reasonable timeout value (such as a few seconds) to balance graceful shutdown
with guaranteed termination.
| ```sh | ||
| # Make sure to use a random private key, this is just an example. | ||
| export ARKD_WALLET_SIGNER_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6 | ||
| export ARKD_SIGNER_SECRET_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6 | ||
| ``` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid documenting a concrete signer private key value.
Publishing a realistic fixed secret encourages unsafe copy-paste and creates recurring secret-scan noise; prefer a placeholder and a generation command.
Suggested doc tweak
# Make sure to use a random private key, this is just an example.
-export ARKD_SIGNER_SECRET_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6
+export ARKD_SIGNER_SECRET_KEY=<32-byte-hex-private-key>
+# Example generation:
+# export ARKD_SIGNER_SECRET_KEY=$(openssl rand -hex 32)🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 169-169: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 `@README.md` around lines 167 - 170, The README.md documentation shows a
concrete private key value for ARKD_SIGNER_SECRET_KEY which is a security risk
and encourages unsafe copy-paste behavior. Replace the actual key value with a
placeholder (like a series of zeros or a descriptive placeholder) and add a
clear command that shows users how to generate their own random private key
instead. This way the documentation provides guidance on how to create a proper
secret without exposing or promoting the use of a fixed hardcoded value.
Source: Linters/SAST tools
…deprecation Mirrors the #1118 base fix (x/net 0.55 deprecates h2c.NewHandler); self-drops on rebase onto a bumped master.
…deprecation Mirrors the #1118 base fix; arkd-signer service.go suppression hand-applied (this branch modified that file for the emulator registration). Self-drops on rebase onto a bumped master.
…A1019 x/net 0.55 deprecates h2c.NewHandler (still required for unencrypted HTTP/2). Suppress the SA1019 deprecation via a targeted golangci-lint exclusion rather than inline //nolint (which pushed a line past the golines 100-char limit).
…A1019 x/net 0.55 deprecates h2c.NewHandler; suppress SA1019 via golangci-lint exclusion. Mirrors the #1118 base fix; self-drops on rebase onto a bumped master.
…A1019 x/net 0.55 deprecates h2c.NewHandler; suppress SA1019 via golangci-lint exclusion. Mirrors the #1118 base fix; self-drops on rebase onto a bumped master.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
176-179: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReplace the concrete private key with a placeholder.
This issue was raised in a previous review and remains unresolved. Publishing a realistic fixed secret encourages unsafe copy-paste and creates recurring secret-scan noise.
🔒 Suggested doc tweak
# Make sure to use a random private key, this is just an example. -export ARKD_SIGNER_SECRET_KEY=19422b10efd05403820ff6a3365422be2fc5f07f34a6d1603f7298328f0f80f6 +export ARKD_SIGNER_SECRET_KEY=<32-byte-hex-private-key> +# Example generation: +# export ARKD_SIGNER_SECRET_KEY=$(openssl rand -hex 32)🤖 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 `@README.md` around lines 176 - 179, Replace the concrete secret in the README example with a non-sensitive placeholder so it cannot be copy-pasted as a real key or trigger secret-scan noise. Update the example under the ARKD_SIGNER_SECRET_KEY setup to use a clearly dummy value and keep the surrounding guidance in place. Locate and adjust the documentation snippet that shows the environment variable assignment.
🤖 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 `@pkg/arkd-signer/config/config_test.go`:
- Around line 28-38: Make this test hermetic by removing its dependency on
ambient environment state: in config_test.go, the cfg.String redaction
assertions should not assume ARKD_SIGNER_PORT is unset, since
config.LoadConfig() may read a real value from the process environment. Pin
ARKD_SIGNER_PORT in the test setup or change the assertion to verify cfg.Port
directly, while keeping the existing checks around config.LoadConfig(),
cfg.String(), and the secret redaction behavior.
---
Duplicate comments:
In `@README.md`:
- Around line 176-179: Replace the concrete secret in the README example with a
non-sensitive placeholder so it cannot be copy-pasted as a real key or trigger
secret-scan noise. Update the example under the ARKD_SIGNER_SECRET_KEY setup to
use a clearly dummy value and keep the surrounding guidance in place. Locate and
adjust the documentation snippet that shows the environment variable assignment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14b7c352-8f78-4ea7-a6ca-6262b9be3fb2
⛔ Files ignored due to path filters (10)
api-spec/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumpkg/ark-cli/go.sumis excluded by!**/*.sumpkg/ark-lib/go.sumis excluded by!**/*.sumpkg/arkd-signer/go.sumis excluded by!**/*.sumpkg/arkd-wallet/go.sumis excluded by!**/*.sumpkg/client-lib/go.sumis excluded by!**/*.sumpkg/errors/go.sumis excluded by!**/*.sumpkg/kvdb/go.sumis excluded by!**/*.sumpkg/macaroons/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
.golangci.ymlMakefileREADME.mdapi-spec/go.modcmd/arkd-signer/main.gocmd/arkd-wallet/main.gocmd/arkd/main.goenvs/arkd-wallet-nosigner.regtest.envenvs/arkd-wallet.regtest.envgo.modpkg/ark-cli/go.modpkg/ark-lib/go.modpkg/arkd-signer/config/config.gopkg/arkd-signer/config/config_test.gopkg/arkd-signer/go.modpkg/arkd-wallet/go.modpkg/client-lib/go.modpkg/errors/go.modpkg/kvdb/go.modpkg/macaroons/go.mod
💤 Files with no reviewable changes (1)
- envs/arkd-wallet-nosigner.regtest.env
✅ Files skipped from review due to trivial changes (3)
- cmd/arkd-wallet/main.go
- pkg/errors/go.mod
- cmd/arkd/main.go
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/ark-cli/go.mod
- cmd/arkd-signer/main.go
- pkg/kvdb/go.mod
- pkg/arkd-wallet/go.mod
- pkg/macaroons/go.mod
- pkg/client-lib/go.mod
- api-spec/go.mod
- go.mod
- pkg/arkd-signer/config/config.go
- pkg/arkd-signer/go.mod
- pkg/ark-lib/go.mod
- Makefile
| t.Setenv("ARKD_SIGNER_SECRET_KEY", secretKey) | ||
| t.Setenv("ARKD_SIGNER_DEPRECATED_KEYS", deprecatedKey) | ||
|
|
||
| cfg, err := config.LoadConfig() | ||
| require.NoError(t, err) | ||
|
|
||
| out := cfg.String() | ||
| require.NotContains(t, out, secretKey) | ||
| require.NotContains(t, out, deprecatedKey) | ||
| require.Contains(t, out, "***") | ||
| require.Contains(t, out, "6061") // non-sensitive field preserved |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this test hermetic.
config.LoadConfig() reads ARKD_SIGNER_PORT from the real environment, so Line 38 will fail if the test process already has that variable set. Pin the port in this test (or assert against cfg.Port) so the redaction check does not depend on ambient env.
Proposed fix
func TestConfigStringRedactsSecrets(t *testing.T) {
secretKey := "afcd3fa10f82a05fddc9574fdb13b3991b568e89cc39a72ba4401df8abef35f0"
deprecatedKey := "1111111111111111111111111111111111111111111111111111111111111111"
t.Setenv("ARKD_SIGNER_SECRET_KEY", secretKey)
t.Setenv("ARKD_SIGNER_DEPRECATED_KEYS", deprecatedKey)
+ t.Setenv("ARKD_SIGNER_PORT", "7001")
cfg, err := config.LoadConfig()
require.NoError(t, err)
out := cfg.String()
require.NotContains(t, out, secretKey)
require.NotContains(t, out, deprecatedKey)
require.Contains(t, out, "***")
- require.Contains(t, out, "6061") // non-sensitive field preserved
+ require.Contains(t, out, "7001") // non-sensitive field preserved
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Setenv("ARKD_SIGNER_SECRET_KEY", secretKey) | |
| t.Setenv("ARKD_SIGNER_DEPRECATED_KEYS", deprecatedKey) | |
| cfg, err := config.LoadConfig() | |
| require.NoError(t, err) | |
| out := cfg.String() | |
| require.NotContains(t, out, secretKey) | |
| require.NotContains(t, out, deprecatedKey) | |
| require.Contains(t, out, "***") | |
| require.Contains(t, out, "6061") // non-sensitive field preserved | |
| t.Setenv("ARKD_SIGNER_SECRET_KEY", secretKey) | |
| t.Setenv("ARKD_SIGNER_DEPRECATED_KEYS", deprecatedKey) | |
| t.Setenv("ARKD_SIGNER_PORT", "7001") | |
| cfg, err := config.LoadConfig() | |
| require.NoError(t, err) | |
| out := cfg.String() | |
| require.NotContains(t, out, secretKey) | |
| require.NotContains(t, out, deprecatedKey) | |
| require.Contains(t, out, "***") | |
| require.Contains(t, out, "7001") // non-sensitive field preserved |
🤖 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 `@pkg/arkd-signer/config/config_test.go` around lines 28 - 38, Make this test
hermetic by removing its dependency on ambient environment state: in
config_test.go, the cfg.String redaction assertions should not assume
ARKD_SIGNER_PORT is unset, since config.LoadConfig() may read a real value from
the process environment. Pin ARKD_SIGNER_PORT in the test setup or change the
assertion to verify cfg.Port directly, while keeping the existing checks around
config.LoadConfig(), cfg.String(), and the secret redaction behavior.
|
unclear if i need those case checks on |
Bring arkd-signer up to date with master (26 commits behind). Only conflict was a Makefile help-text collision between master's new run-vtxo-chain-simulation target and the branch's run-wallet target; kept both.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker-compose.regtest.yml (2)
73-73: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not ship a known signer private key as the default.
Anyone running this Compose file without overriding
ARKD_SIGNER_SECRET_KEYgets the committed operator key. Because the signer port is published, this can compromise signing if the stack is exposed beyond an isolated regtest environment. Fail closed when the variable is absent, or keep the deterministic fixture key in a dedicated e2e-only override.🤖 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 `@docker-compose.regtest.yml` at line 73, Remove the committed private-key fallback from the ARKD_SIGNER_SECRET_KEY Compose environment entry. Require ARKD_SIGNER_SECRET_KEY to be explicitly provided so startup fails closed when it is absent, or move the deterministic fixture key into a dedicated e2e-only override rather than the default Compose configuration.
62-74: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
arkdwait for signer readiness, not just container startup.Short-form
depends_ononly orders container creation; it does not wait forarkd-signerto accept requests.internal/infrastructure/signer/client.go:25-49immediately callsGetStatus, soarkdcan fail initialization if the signer is still starting. Add a compatible signer healthcheck and usecondition: service_healthy.Also applies to: 92-95
🤖 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 `@docker-compose.regtest.yml` around lines 62 - 74, Update the arkd-signer service definition to add a compatible healthcheck that verifies the signer accepts requests, then change each arkd service dependency on arkd-signer to use condition: service_healthy instead of relying on short-form depends_on. Ensure the healthcheck targets the configured signer port and supports the existing startup flow used by internal/infrastructure/signer/client.go.
🤖 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 `@go.mod`:
- Line 97: Remove the indirect github.com/docker/docker dependency by
eliminating the test-only github.com/golang-migrate/migrate/v4/database/postgres
→ github.com/dhui/dktest dependency path if those tests are no longer needed;
otherwise update that upstream chain to a patched Docker release and regenerate
go.mod/go.sum.
---
Outside diff comments:
In `@docker-compose.regtest.yml`:
- Line 73: Remove the committed private-key fallback from the
ARKD_SIGNER_SECRET_KEY Compose environment entry. Require ARKD_SIGNER_SECRET_KEY
to be explicitly provided so startup fails closed when it is absent, or move the
deterministic fixture key into a dedicated e2e-only override rather than the
default Compose configuration.
- Around line 62-74: Update the arkd-signer service definition to add a
compatible healthcheck that verifies the signer accepts requests, then change
each arkd service dependency on arkd-signer to use condition: service_healthy
instead of relying on short-form depends_on. Ensure the healthcheck targets the
configured signer port and supports the existing startup flow used by
internal/infrastructure/signer/client.go.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35acdf7e-3361-4051-a4a2-c0828ae1243d
📒 Files selected for processing (10)
MakefileREADME.mdcmd/arkd/commands.gocmd/arkd/flags.gocmd/arkd/main.godocker-compose.regtest.ymlgo.modinternal/config/config.gointernal/interface/grpc/service.gointernal/test/e2e/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/arkd/flags.go
- cmd/arkd/main.go
- internal/test/e2e/e2e_test.go
- cmd/arkd/commands.go
- README.md
…ish port The health endpoint returned SERVING unconditionally, so an orchestrator could not tell a ready signer from one that came up without a usable key. It now reflects Signer.IsReady, which /healthz maps to 503, so the new container HEALTHCHECK gates on readiness rather than on the process having started. Start bound the listener inside the serving goroutine, so a port conflict panicked after main had already logged that the signer was listening. Bind first and return the error. The regtest compose published the signer on the host. It holds the operator key and has no auth, and only arkd talks to it over the compose network, so expose it there instead. Nothing referenced 127.0.0.1:6061.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 commit since last pass (644e309 "report real readiness, bind before Start returns, unpublish port"). 3 prior issues fixed, 1 partially fixed; 6 remain open. 1 new gap surfaced by the partial fix. No regressions introduced.
Prior finding closure
✅ Fixed — Health handler returns SERVING unconditionally (pkg/arkd-signer/interface/grpc/handlers/healthservice.go)
644e309 rewires Check and List through h.status(ctx), which delegates to h.signer.IsReady(ctx) with an explicit nil guard on h.signer. TestHealthHandlerReflectsSignerReadiness adds table coverage for all three cases (ready, no-key, nil signer). Fix is correct and well-tested. ✅
✅ Fixed — Start() bind failure panics from goroutine (pkg/arkd-signer/interface/grpc/service.go)
net.Listen("tcp", s.server.Addr) is now called synchronously in Start(), error is returned to the caller (with cancel() to release the gateway connection), and the goroutine uses log.WithError(err).Fatal(...) instead of panic. Port conflicts now surface as a meaningful error from main. ✅
✅ Fixed — Dockerfile has no HEALTHCHECK (arkdsigner.Dockerfile)
HEALTHCHECK --interval=5s --timeout=3s --start-period=5s --retries=5 CMD wget -q --spider "http://127.0.0.1:${ARKD_SIGNER_PORT:-6061}/healthz" || exit 1 is correct. /healthz is registered via gateway.WithHealthzEndpoint, and the health handler now maps NOT_SERVING → HTTP 503, so wget --spider fails on a not-ready signer. ✅
⚠️ Partially fixed — docker-compose publishes signer on host (docker-compose.regtest.yml)
644e309 replaces ports: - "6061:6061" with expose: - "6061" and adds an explanatory comment. The host-side binding is gone; the signer is now reachable only from within the compose network. This is the most important deployment fix in this PR. The underlying structural issues (no gRPC auth, Access-Control-Allow-Origin: * on all HTTP responses) remain — see still-open item below.
🔴 Still open — No auth on signer service
pkg/arkd-signer/interface/grpc/service.go:38 — grpc.Creds(insecure.NewCredentials()) and lines 107–116 (Access-Control-Allow-Origin: * on all HTTP responses). The compose port fix eliminates the immediate regtest exposure, but any host on the compose network (or a production deployment without a firewall) can still request signatures with the operator key. CORS wildcard is particularly wrong on a signing service — it broadens the attack surface to any browser page that can reach the gateway. No change since last pass.
🔴 Still open — fraud.go dual-signing structural risk
internal/core/application/fraud.go:179-194 — The comment added in d56b8fa accurately documents the invariant: the wallet's unscoped signing pass is inert because script.FinalizeVtxoScript keys witness args by xonly pubkey and the wallet forfeit pubkey is absent from forfeit-closure PubKeys. The fix (scoping the wallet call to the connector index via inputIndexes) is deferred because the wallet SignTransaction RPC lacks that parameter. No change. Comment is valuable; structural fix remains open.
🔴 Still open — Nil-pointer implicit ordering in signer.go signing loop
pkg/arkd-signer/core/application/signer.go (signing loop, bytes.Equal(in.WitnessUtxo.PkScript, ...)) — explicit nil guard still absent. The implicit protection from txutils.GetPrevOutputFetcher returning fmt.Errorf("missing witness utxo on input #%d", i) is real and correct. The refactoring hazard remains: any future call site that skips the fetcher step will turn this into a nil dereference panic against a malformed PSBT. A two-line guard at the top of the loop costs nothing and closes the latent risk permanently.
🔴 Still open — --signer-prvkey cold removal
cmd/arkd/flags.go — flag removed with no stub. Operators passing --signer-prvkey receive urfave/cli's generic Incorrect Usage: flag provided but not defined rather than the "key moved to ARKD_SIGNER_SECRET_KEY on arkd-signer" message that already exists in internal/interface/grpc/handlers/signer_manager.go:34-38. No change.
🔴 Still open — Handler-level tests for SignTransaction paths
pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go — Health handler tests added in this commit (good). SignTransaction / SignTransactionTapscript handler paths, the inputIndexes conversion, and the out-of-range guard in signer.sign remain untested at the handler layer. No change.
🔴 Still open — Dual-signing flow has no deterministic test
internal/core/application/fraud.go — broadcastForfeitTx still has no unit or e2e test asserting the final witness is authorized by the operator key. No change.
New finding — compose depends_on doesn't consume the HEALTHCHECK
docker-compose.regtest.yml — arkd depends_on: arkd-signer
Now that arkdsigner.Dockerfile declares a HEALTHCHECK, Docker Compose can gate dependents on readiness rather than container start. But the compose still uses a bare list form:
depends_on:
- arkd-wallet
- arkd-signerwhich only waits for the container to start. Compare pg's entry (same file), which uses condition: service_healthy. Without the condition, arkd races against signer readiness on cold boot even though the Dockerfile HEALTHCHECK now exists and is tracked by Docker. Change to:
depends_on:
arkd-signer:
condition: service_healthyto actually use the health gate you just added.
Minor observation — Watch is a no-op
pkg/arkd-signer/interface/grpc/handlers/healthservice.go:27-30
Watch returns nil without sending a status frame. Any gRPC-native health-watch client sees an immediate stream close. The previous stub (before this PR) sent one SERVING frame; the new one sends nothing. The HTTP path (/healthz → Check) is what the HEALTHCHECK uses, so this doesn't affect the current deployment. Worth fixing if k8s gRPC health probes are ever used.
The Dockerfile HEALTHCHECK added in 644e309 was not consumed by anything: arkd's depends_on used the bare list form, which waits for container start only. Move it to the long form and gate on service_healthy for arkd-signer, pg and redis, all of which have healthchecks. arkd-wallet has none so it stays on service_started. Verified the gate end to end rather than by inspection: the built image carries the healthcheck as CMD-SHELL, and the container goes starting -> healthy in about six seconds, so arkd will not hang waiting on it. Watch returned immediately without sending anything, so a health-watching client saw the stream close having learned nothing. Send the current status, then each change, until the stream context is cancelled.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 commit since last pass (c572c47 "gate arkd on signer health, make Watch stream status"). 2 prior items fixed; 6 remain open. No new issues introduced by the incremental diff.
Prior finding closure
✅ Fixed — compose depends_on doesn't consume the HEALTHCHECK (docker-compose.regtest.yml)
c572c47 converts arkd's depends_on from the bare-list form to the long-map form with condition: service_healthy for arkd-signer, pg, and redis, and condition: service_started for arkd-wallet (which has no healthcheck). The arkdsigner.Dockerfile HEALTHCHECK is now wired end-to-end: the container transitions starting → healthy before arkd starts. This is correct.
✅ Fixed — Watch was a no-op (pkg/arkd-signer/interface/grpc/handlers/healthservice.go:38-68)
c572c47 rewrites Watch to: (1) send the current status synchronously before entering the poll loop, (2) poll every watchPollInterval (1 s), (3) send only on transitions, and (4) return ctx.Err() on cancellation. The implementation is correct for a service without a push-driven readiness signal. fakeWatchServer in signer_handler_test.go correctly tests the first-frame guarantee and the cancellation path with a mutex-guarded slice copy.
🔴 Still open — No auth on signer service
pkg/arkd-signer/interface/grpc/service.go:38 — grpc.Creds(insecure.NewCredentials()) and the HTTP gateway sets Access-Control-Allow-Origin: * on all responses. The compose port unpublish from 644e309 removes the immediate regtest exposure, but any process on the compose network (or any production deployment behind a permissive firewall) can request operator-key signatures on arbitrary PSBTs. No change in this commit.
🔴 Still open — fraud.go dual-signing structural risk
internal/core/application/fraud.go:179-194 — The wallet's unscoped second signing pass over the VTXO tapscript input is documented as inert (correct comment added in d56b8fa), but the structural fix — scoping the wallet call to the connector input index — is still deferred. No change in this commit.
🔴 Still open — Nil-pointer implicit ordering in signing loop
pkg/arkd-signer/core/application/signer.go — bytes.Equal(in.WitnessUtxo.PkScript, ...) still has no explicit nil guard. Protection from txutils.GetPrevOutputFetcher is real but fragile across future refactors. No change in this commit.
🔴 Still open — --signer-prvkey cold removal
cmd/arkd/flags.go — No stub or deprecation shim. Operators passing this flag receive urfave/cli's generic Incorrect Usage: flag provided but not defined rather than the "key moved to ARKD_SIGNER_SECRET_KEY on arkd-signer" message that exists in signer_manager.go:34-38. No change in this commit.
🔴 Still open — Handler-level tests for SignTransaction paths
pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go — SignTransaction / SignTransactionTapscript handler paths, inputIndexes conversion, and the out-of-range guard in signer.sign remain untested at the handler layer. The new Watch test is appreciated; the signing paths are still the gap. No change in this commit.
🔴 Still open — Dual-signing flow has no deterministic test
internal/core/application/fraud.go — broadcastForfeitTx (signer → wallet → extract) still has no unit or e2e test asserting the final witness is authorized by the operator key. TestReactToFraud is noted as flaky in the PR body. No change in this commit.
Incremental diff — no new findings
The three files touched in c572c47 (docker-compose.regtest.yml, healthservice.go, signer_handler_test.go) introduce no new issues. The Watch implementation is correct. The fakeWatchServer test double is well-structured (embedded grpc.ServerStream, mutex-guarded slice copy in sent()). The compose readiness gate is correct and closes a genuine startup race.
The master merge brought x/text v0.39.0 everywhere except pkg/arkd-signer, which this branch introduces, so the master-side bump could not reach it. Same for x/net: v0.55.0 carries the dnsmessage parse panic that v0.56.0 clears.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 5 commits since last pass (master merge c397cff / 4a97334 / 9acc5e7 + merge commit 7e1d60f + dep-align 0513a9a). 0 prior issues fixed; 6 remain open. The incremental diff is entirely a master-merge pull-in plus golang.org/x dependency alignment — none of the six open-issue files were touched.
Prior finding closure
🔴 Still open — No auth on signer service
pkg/arkd-signer/interface/grpc/service.go:38 — grpc.Creds(insecure.NewCredentials()) and Access-Control-Allow-Origin: * on all HTTP responses. No change in this pass. Any process that can reach the signer's port can request operator-key signatures on arbitrary PSBTs. CORS wildcard on a signing service is still the highest-severity deployment concern in this PR.
🔴 Still open — fraud.go dual-signing structural risk
internal/core/application/fraud.go:179-194 — The wallet's unscoped second signing pass over the VTXO tapscript input is documented but structurally unfixed. No change in this pass.
🔴 Still open — Nil-pointer implicit ordering in signing loop
pkg/arkd-signer/core/application/signer.go — bytes.Equal(in.WitnessUtxo.PkScript, ...) still lacks an explicit nil guard. The implicit protection from txutils.GetPrevOutputFetcher is real but fragile across future refactors. No change in this pass.
🔴 Still open — --signer-prvkey cold removal
cmd/arkd/flags.go — No deprecation stub. Operators passing this flag receive urfave/cli's generic Incorrect Usage: flag provided but not defined instead of the migration message that already exists in signer_manager.go:34-38. No change in this pass.
🔴 Still open — Handler-level tests for SignTransaction paths
pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go — SignTransaction / SignTransactionTapscript handler paths, inputIndexes conversion, and the out-of-range guard in signer.sign remain untested at the handler layer. No change in this pass.
🔴 Still open — Dual-signing flow has no deterministic test
internal/core/application/fraud.go — broadcastForfeitTx still has no unit or e2e test asserting the final witness is authorized by the operator key. No change in this pass.
Incremental diff — no new blocking findings
coinselect.go (c397cff) — correct
pkg/arkd-wallet/core/application/wallet/coinselect.go
The branchAndBound DFS, consolidate, and the two selector types (economicalCoinSelector, consolidateFirstCoinSelector) are algorithmically correct. Specific checks:
- Suffix pruning (
suffix[i] = suffix[i+1] + sorted[i].Value(), largest-first sort): correct BnB upper bound. Matches the standard changeless-selection approach used by Bitcoin Core and LDK. bnbMaxTries = 100_000: matches Bitcoin Core's cap; prevents pathological O(2ⁿ) runtime on adversarial UTXO pools.tries--placement (before the two recursive calls, not per-call): counts DFS nodes entered rather than leaves reached. This is a slightly tighter budget than counting leaf evaluations but is conservative in the right direction.append(picked, idx)in the DFS: this reuses the backing array ofpickedwhen cap > len, writingidxto positionlen(picked). The second branch (dfs(idx+1, sum, picked)) then safely overwrites that position via its ownappend. No data corruption occurs because each view of the slice is bounded by its ownlen. However, this is a well-known Go footgun — a future maintainer who adds asort.Sliceor in-place mutation onpickedinside the DFS will introduce a silent bug. Add a brief comment explaining why aliasing is harmless here, or useappend(append([]int(nil), picked...), idx)(defensive copy) to make intent unambiguous.consolidatebreak condition (cs.Num() >= maxInputschecked before push): correctly enforces the cap without over-consuming.service.gowiring (economicalCoinSelectorforselectCoins,consolidateFirstCoinSelector{0}forselectCoinsForWithdraw): the strategies match the use-case intent (fee-optimal for normal sends, UTXO-consolidating for withdrawals).- Test coverage (
coinselect_test.go+ fixture JSON): the fixture-driven table covers both selectors across all code paths (including theexpectErrorcases). Adequate.
GetBlockHeight (4a97334) — correct
pkg/client-lib/explorer/mempool/explorer.go:233-240 and service.go:18-19
json.Unmarshal on the raw /blocks/tip/height response body correctly parses a bare integer JSON value into int64. The -1 sentinel on error is documented in the interface comment. The error message wraps status from e.get() correctly. Tests cover the success, non-200, and malformed-body paths. No issues.
Note: fulmine has its own GetBlockHeight(ctx context.Context) (int64, error) (different signature, different interface) — no cross-repo breakage.
CVE bump (9acc5e7) — correct
golang.org/x/text@v0.39.0 alignment across all modules. No code changes; straightforward security update.
identity.GetXpub stub — acceptable
pkg/client-lib/identity/identity.go:33 — adding GetXpub to the interface with a "not implemented" stub in the single-key implementation is the right pattern here. Only one concrete implementation exists; the test confirms the error contract. No issue.
Summary
This pass introduces no new problems and fixes none of the six outstanding ones. The six findings from prior passes (no signer auth, fraud.go structural risk, nil guard, flag removal without migration shim, missing handler tests, no deterministic dual-sign test) all remain and should be addressed before merge. The master-merge content (coinselect, GetBlockHeight, CVE bump) is clean.
…t witness The gateway answered preflight with Access-Control-Allow-Origin: * on a service that signs with the operator key and has no auth of its own. Gateway JSON posts are not CORS-simple, so that wildcard was the thing letting a page the operator happens to be visiting reach a signer it can route to. The signer's callers are arkd and operator tooling, never a browser, so it now sends no CORS headers and refuses preflight. Handler-level coverage for the signing paths: input indexes are int32 on the wire, so the conversion is exercised for a selected subset, an empty list, and out-of-range values including a negative one. Anchor inputs are asserted to be skipped rather than signed. Pin the dual-signing invariant that broadcastForfeitTx relies on: after the wallet's unscoped second pass adds a signature from its forfeit key to the vtxo input, the witness that finalizes must still be the operator's. Verified the test fails when the wallet forfeit pubkey is placed inside the closure, which is the hazard the comment in fraud.go describes.
|
Addressing the review findings that have been carried across several passes. Three are now fixed in FixedCORS wildcard. Valid, and the sharpest part of the "no auth" finding. The gateway answered preflight with Handler-level tests for the signing paths. Deterministic dual-signing test. Not doing, with reasonsNil guard in the signing loop.
Scoping the wallet's second signing pass to the connector input. Correct in principle, but it needs input indexes on the wallet's
Note on prior passesSeveral of these were decided in earlier rounds but never written down publicly, which is why each pass reports them as untouched. That is on me for not recording them here sooner. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 commit (2d3754da) since last pass — CORS wildcard dropped, handler-level signing tests added, deterministic dual-signing test added. 3 of 6 prior issues fixed; 2 remain open; 2 closed as intentional decisions.
Prior finding closure
✅ Fixed — CORS wildcard on signer service (pkg/arkd-signer/interface/grpc/service.go)
router() no longer sets Access-Control-Allow-Origin: * on any response. OPTIONS requests now return 405 Method Not Allowed with no CORS headers. TestRouterSendsNoCORSHeaders pins all three cases (preflight refused, gateway responses clean, /healthz still reachable). This was the concrete cross-origin attack vector on the signing service — it is gone.
✅ Fixed — Handler-level tests for SignTransaction paths (pkg/arkd-signer/interface/grpc/handlers/signer_handler_test.go)
TestSignerHandlerSignTransactionTapscript covers index selection (single input signed, others untouched), empty-list-signs-all, out-of-range rejection for {2, 99, -1} (the negative-index path via int32 wire type is explicitly tested), anchor skipping, and malformed-PSBT rejection. TestSignerHandlerSignTransaction covers sign-without-extract and the correct failure when extraction is attempted on an incomplete witness. Tests use a real application.New(operator, nil) signer rather than a mock, making these handler+core integration tests. The int32 → int conversion in signer_handler.go:SignTransactionTapscript that was the unguarded path in prior passes is now fully exercised.
✅ Fixed — Dual-signing flow has no deterministic test (internal/core/application/fraud_dualsign_test.go)
TestForfeitWitnessIsAuthorizedByOperatorKey pins both invariants:
- After the wallet's unscoped second pass adds a stray walletForfeit signature to the vtxo input, finalization still produces a witness authorized by the operator key, not the wallet key.
- Without the operator's signature, the wallet's stray signature cannot satisfy the leaf.
witnessHasSigFor reads TaprootScriptSpendSig after FinalizeVtxoScript — this is correct because FinalizeVtxoScript only writes FinalScriptWitness; it does not clear partial-sig fields. One maintenance note: if FinalizeVtxoScript is ever made PSBT BIP-370-conformant (which says finalizers SHOULD clear partial sigs), this helper will fail at require.NotNil(t, sig, "no signature recorded for the requested key") rather than at its intended assertion. A one-line comment on the helper explaining the dependency would save the next reader from a confusing failure.
🔴 Still open — insecure.NewCredentials() transport security (pkg/arkd-signer/interface/grpc/service.go:42)
grpc.Creds(insecure.NewCredentials()) and the gateway-to-gRPC loopback also use plain credentials. Contributor's response correctly notes this is the existing posture from arkd-wallet and that the concrete CORS exploit vector is now closed. The structural concern (any process that can reach the signer's port can request operator-key signatures on arbitrary PSBTs) is real but acknowledged as a separate change. Carry forward.
🔴 Still open — fraud.go wallet's second pass not scoped to the connector input (internal/core/application/fraud.go:179-194)
s.wallet.SignTransaction iterates all tapscript inputs including the vtxo input already signed by the signer. The invariant is documented at the call site and now has a test that catches any regression. The structural fix — passing input indexes to the wallet's SignTransaction RPC — requires a proto change that would expand this already-large PR. Contributor's rationale is sound; defer accepted. Carry forward as a known debt.
Closed — Nil-pointer implicit ordering in signing loop (pkg/arkd-signer/core/application/signer.go)
Closing this finding. The contributor's counter-argument is correct: txutils.GetPrevOutputFetcher iterates every input and returns an error on any nil WitnessUtxo before the signing loop runs; TestSignRejectsMissingWitnessUtxo pins that error path; and continue in the loop body would silently skip an input that currently produces a clear diagnostic, which would be a regression. No guard needed.
Closed — --signer-prvkey removed without migration stub (cmd/arkd/flags.go)
Contributor confirmed intentional: this PR is explicitly breaking, ARKD_SIGNER_ADDR is required, and the migration is documented in the README. Not a correctness issue.
Incremental diff — no new blocking findings
The four files in 2d3754da introduce no new issues. The witnessHasSigFor dependency note above is the only observation worth acting on (minor, non-blocking). The Go 1.26 project uses range-over-int (for i := range n) correctly throughout the new helpers. All new test helpers (multiInputPacket, packetWithAnchor, arkadeLeaf, testOutpoint, forfeitPacket) are structurally sound and create valid PSBTs with proper WitnessUtxo and TaprootLeafScript fields.
witnessHasSigFor looked up the signature bytes in TaprootScriptSpendSig after FinalizeVtxoScript had run. That works today, but a finalizer is allowed to clear partial signatures and BIP-370 says it SHOULD, so the helper would have started failing on its lookup rather than on the assertion it exists to make. Capture both signatures before finalizing instead, which removes the ordering dependency rather than documenting it.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Status: 1 commit (691cb5c8) since last pass — test ordering fix in fraud_dualsign_test.go. 1 prior maintenance observation fixed; 2 remaining open items are both acknowledged deferred debt with documented rationale. No new issues introduced.
Prior finding closure
✅ Fixed — witnessHasSigFor ordering dependency on finalizer not clearing partial sigs (internal/core/application/fraud_dualsign_test.go)
My last pass noted: "if FinalizeVtxoScript is ever made PSBT BIP-370-conformant (which says finalizers SHOULD clear partial sigs), this helper will fail at require.NotNil rather than at its intended assertion."
691cb5c8 eliminates the ordering dependency rather than documenting it. partialSigFor is called before FinalizeVtxoScript runs, capturing both operatorSig and walletSig from TaprootScriptSpendSig. witnessContains then does direct byte-equality against those captured values. The comparison is correct: TaprootScriptSpendSig.Signature holds the raw 64-byte Schnorr signature, and that same 64-byte value is what FinalizeVtxoScript places on the witness stack for a script-path spend. t.Fatalf in partialSigFor is unreachable after the Len assertion but satisfies the compiler cleanly. The fix is better than the comment-based approach I suggested.
🔴 Still open (deferred with rationale) — insecure.NewCredentials() transport security
pkg/arkd-signer/interface/grpc/service.go:42 — plain gRPC credentials on the signer service. Contributor's position (recorded 2d3754da): this matches arkd-wallet's pre-split posture; the CORS vector is gone; real transport auth belongs to both services in a separate change. No change in this commit. Carry forward.
🔴 Still open (deferred with rationale) — fraud.go wallet second pass not scoped to connector input
internal/core/application/fraud.go:179–194 — s.wallet.SignTransaction iterates all tapscript inputs including the vtxo input already signed by the signer. Invariant is documented at the call site, pinned by TestForfeitWitnessIsAuthorizedByOperatorKey, and the structural fix requires a proto change outside this PR's scope. No change in this commit. Carry forward.
Incremental diff — no new findings
691cb5c8 touches only internal/core/application/fraud_dualsign_test.go. The refactoring is mechanically correct: three new helpers (partialSigFor, finalWitness, witnessContains) replace the single monolithic witnessHasSigFor. The test logic is equivalent but no longer relies on the finalizer leaving partial-sig fields populated. No production code changed; no new issues introduced.
|
Changes were requested 2+ days ago. @Kukks need any help addressing the feedback? |
|
Changes were requested 8+ days ago. @Kukks need any help addressing the feedback? |
|
Changes were requested 5 days ago. @Kukks need any help addressing the feedback? |
|
This PR has been open 63+ days with changes requested and no update in 3 weeks. @Kukks need any help with the standalone arkd-signer extraction feedback? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — Standalone arkd-signer (BREAKING)
Verdict: looks ready to merge, with one operator-migration callout. Protocol-critical — requires human sign-off.
This PR extracts the signing key out of arkd-wallet into a dedicated arkd-signer binary. Good architectural move: wallet handles liquidity, signer holds the key. Separation means the signing key can be air-gapped or HSM-backed independently of the wallet.
Breaking changes:
ARKD_SIGNER_ADDRis now required (no longer falls back toARKD_WALLET_ADDR). Deployments that relied on the fallback will fail to start. The README and config comment document this clearly. ✅arkd signer load --signer-prvkeyis removed. Operators who used this to configure an in-wallet signer must switch to runningarkd-signerand pointingARKD_SIGNER_ADDRat it. Migration path is clear in the README. ✅ARKD_WALLET_SIGNER_KEY/ARKD_WALLET_DEPRECATED_SIGNER_KEYSremoved fromarkd-walletenv; replaced byARKD_SIGNER_SECRET_KEY/ARKD_SIGNER_DEPRECATED_KEYSinarkd-signer.
Security observation (class: key exposure surface, not blocking):
The signer's port (6061) is exposed-only in the Docker compose — not ports:-published to the host. Only arkd reaches it over the compose network. This is the right approach: the signer holds the operator key and has no auth. Human reviewers should verify the same is true in any Kubernetes/Nomad manifests used in staging/production. File path: docker-compose.regtest.yml line 66.
Deprecation key format: the ARKD_SIGNER_DEPRECATED_KEYS format (hexkey[:cutoff_unix_seconds]) is well-documented. An unparseable entry, negative value or unknown opcode fails startup — good fail-fast behaviour. ✅
Dockerfile: health check gates on NOT_SERVING → 503 mapping, meaning the container only reports healthy once the key is loaded and usable. ✅
No test coverage regression observed — the existing signing tests should still apply; the new binary's startup is covered by the e2e suite that dials ARKD_SIGNER_ADDR.
Requires human review before merge — touches signing path and introduces a BREAKING deployment change.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — sha 691cb5c
What this does:
Extracts the operator signing key out of arkd-wallet into a new standalone arkd-signer binary. ARKD_SIGNER_ADDR is now required (no longer defaults to the wallet address). The arkd signer load --signer-prvkey flag is removed entirely.
Architecture assessment:
This is the right direction: separating key custody from liquidity management reduces blast radius if either service is compromised. arkd-signer holding only the signing key cannot move on-chain funds on its own.
Specific checks:
-
BREAKING: Any deployment that relied on
ARKD_SIGNER_ADDRdefaulting toARKD_WALLET_ADDRwill fail to start after this change. The README update is clear about this, but the operator runbook / deployment scripts need explicit attention. -
Dockerfile healthcheck: The
/healthzendpoint correctly gates onNOT_SERVING → 503, so the container won't be marked healthy until the key is loaded. Good. -
Key injection: The operator key is now loaded from
ARKD_SIGNER_SECRET_KEYenv var. Ensure that secret is not logged anywhere in the config print at startup (thelog.Infof("arkd-signer config: %s", cfg)line — verifycfg.String()redacts the key). -
Deprecated keys env var: The
ARKD_SIGNER_DEPRECATED_KEYSformat (hex[:unix_cutoff], comma-separated) is documented. An unparseable entry fails startup rather than being silently skipped — good defensive behaviour. -
Version flag: The
Versionvariable injected at build time is passed togrpcservice.NewService. Verify the gRPC reflection/health metadata exposes version info rather than logging it at a sensitive level.
Missing: No migration guide for operators running the combined wallet+signer mode today. Consider adding an upgrade checklist to the PR description or CHANGELOG.
Looks architecturally sound. Requires operator communication before deploy.
|
Changes were requested 66+ days ago on this BREAKING standalone signer extraction. @Kukks need any help addressing the feedback? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — Extract standalone arkd-signer (sha 691cb5c)
Summary
This PR splits the operator signing key out of arkd-wallet into a new arkd-signer binary. ARKD_SIGNER_ADDR becomes required (was optional, previously defaulted to the wallet address).
Architecture
arkd-signerholds the operator key (ARKD_SIGNER_SECRET_KEY) and signs protocol transactionsarkd-walletbecomes a pure liquidity provider (no key)- The
arkd signer load --signer-prvkeyCLI flag is removed — key is set only via env
Things that look good
- Dockerfile healthcheck gates on
NOT_SERVING→ 503, so orchestration waits for key readiness, not just process start ARKD_SIGNER_DEPRECATED_KEYSformat supports optional cutoff dates for key rotation — this is a clean interfacemake run-signerandmake docker-runupdated.golangci.ymlexclusion for theh2c.NewHandlerdeprecation is scoped narrowly and tracked
Security concern to verify before merge
log.Infof("arkd-signer config: %s", cfg) at startup (cmd/arkd-signer/main.go) — please confirm cfg.String() redacts ARKD_SIGNER_SECRET_KEY and ARKD_SIGNER_DEPRECATED_KEYS. If %s calls fmt.Stringer and that implementation prints the raw struct, the operator key would appear in logs. This should be a hard requirement before this lands.
Deployment impact
Every deployment that relied on arkd-wallet as the implicit signer will need to:
- Start a new
arkd-signerprocess - Set
ARKD_SIGNER_ADDRexplicitly - Move the key from
ARKD_WALLET_SIGNER_KEYtoARKD_SIGNER_SECRET_KEY
The migration guide in README.md is clear. Verify ark-infra Terraform/ECS configs are updated before rolling (the ECS staging stack is a good first target).
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #1118 (sha 691cb5c)
Extract standalone arkd-signer + shared txsigner lib
Protocol-critical — BREAKING CHANGE. Human sign-off required. This is a significant operational change.
What changes
- New
arkd-signerbinary (cmd/arkd-signer, pkg/arkd-signer) ARKD_SIGNER_ADDRis now required (no longer defaults to wallet address)--signer-prvkeyCLI flag removed; arkd no longer loads a raw key at runtimerun-wallet-nosignerMakefile target removed- Docker healthcheck on
/healthzblocks until key is usable (good)
Looks correct
- Healthcheck mapping NOT_SERVING → 503 is a clean readiness gate
- Removing the option to pass a private key via CLI flag reduces the attack surface
ARKD_SIGNER_DEPRECATED_KEYSwith optional cutoff timestamps is a well-designed key rotation path
Security concerns to verify (cannot confirm from this diff alone)
- Key logging: does the
config.String()method on the signer config redactARKD_SIGNER_SECRET_KEYandARKD_SIGNER_DEPRECATED_KEYS? The wallet'slog.Infof("arkd-signer config: %s", cfg)line would expose these in logs if not. This is critical. - Deprecated key format parsing: the
<hexkey>[:<cutoff>]format should fail closed on malformed entries (reject the whole list) rather than silently dropping the malformed one. - Does the signer validate that
ARKD_SIGNER_SECRET_KEYis a valid secp256k1 private key on startup, before reporting healthy? A malformed key that only fails at signing time would be a bad failure mode.
Operational note
This is a migration-breaking change for any deployment that relied on the wallet-as-signer default. The upgrade path should be documented — operators need to start arkd-signer before arkd will accept connections.
|
Changes were requested 72+ days ago on the arkd-signer extraction. @Kukks the related PRs (#1121, #1122, #1124) are also 69-72 days old without review. Is this stack actively being iterated, or should it be discussed whether to merge, rebase, or close some of it? A team sync on the signer architecture might unblock this cluster. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review: standalone arkd-signer (sha 691cb5c)
What the PR does
Extracts the operator signing key into a standalone arkd-signer binary. arkd-wallet becomes a pure liquidity provider; signing is no longer optional or collocated with the wallet.
Breaking deployment change
ARKD_SIGNER_ADDR is now required — there is no fallback to ARKD_WALLET_ADDR. Operators who relied on arkd-wallet as their signer will need to:
- Deploy the new
arkd-signerbinary/container. - Set
ARKD_SIGNER_SECRET_KEY(previouslyARKD_WALLET_SIGNER_KEY). - Point
ARKD_SIGNER_ADDRat the new service.
The arkd signer load --signer-prvkey CLI flag is also removed; only --signer-url remains.
Security considerations
Key isolation is a net positive: the signing key now lives in a process with a smaller attack surface than the full wallet service. The healthcheck gates on readiness (/healthz → NOT_SERVING until the key is usable), which prevents a race where arkd connects before the signer is ready.
Key rotation via ARKD_SIGNER_DEPRECATED_KEYS with optional cutoff timestamps is a welcome addition. The format is documented. Reviewers should confirm that the cutoff is enforced server-side (the signer refuses to sign new outputs locked to a deprecated key past its cutoff) and not just informational.
Questions for human reviewers
-
Is there any mTLS or shared-secret authentication between
arkdandarkd-signer? An unauthenticated local gRPC signer is a risk if the host is multi-tenant. -
How does the signer load its key at startup — environment variable only, or also from a file/vault? The README shows env var; a file-path variant would be safer for secrets management in production.
-
The diff shown covers build/Makefile/README changes. The
pkg/arkd-signer/package itself needs review — particularly the key loading, the gRPC service handler, and how deprecated keys are stored in memory (should be zeroed after use). -
Migration path: is there a rollout guide for operators upgrading from a wallet-as-signer setup? A mis-configured
ARKD_SIGNER_ADDRat startup would causearkdto refuse to start, which is correct but needs clear documentation.
|
Changes were requested 74+ days ago. @Kukks need any help addressing the feedback on the standalone arkd-signer extraction? |
|
This PR has been open for 3+ days without a review decision (BREAKING: ARKD_SIGNER_ADDR required). @Kukks is anyone looking at this? |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — sha 691cb5c
This PR extracts the signing function out of arkd-wallet into a standalone arkd-signer service and a shared txsigner library. It is a BREAKING change: ARKD_SIGNER_ADDR is now required (no longer defaults to ARKD_WALLET_ADDR).
What I can verify from this diff:
- Makefile: new build-signer target, run-signer now runs cmd/arkd-signer, run-wallet-nosigner removed.
- README: setup flow is clearly updated. The deprecated-key format (hex:unix-cutoff) is documented.
- golangci.yml: h2c.NewHandler SA1019 exclusion is a reasonable workaround with a tracked follow-up comment.
- The ARKD_SIGNER_ADDR requirement is correctly noted as breaking in the PR title.
What needs human review:
- The arkd-signer binary itself (cmd/arkd-signer) — key storage, memory handling, TLS/auth configuration, and the gRPC surface it exposes. These are security-critical and I haven't seen the full implementation diff.
- The shared txsigner library interface — ensure no ambient signing capabilities leak to the wallet, and that the signer correctly rejects requests it shouldn't honour.
- Key rotation path: the deprecated-key list is parsed from env vars — confirm it's never logged at any level.
From the README and Makefile perspective the split looks well-structured. Flag for human review of the signer binary and auth model before merging.
|
Changes have been requested on this PR for 2+ months. @Kukks is this still being actively worked on, or has it been superseded by another approach? Happy to re-review if there's an updated revision. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — sha 691cb5c
What this does:
Introduces a standalone arkd-signer binary (cmd/arkd-signer). ARKD_SIGNER_ADDR is now a required field with no default. Operators who previously relied on arkd-wallet as signer (the default fallback) must now deploy and configure arkd-signer separately.
Breaking change impact:
- Any deployment that does not set ARKD_SIGNER_ADDR will fail to start (previously it defaulted to ARKD_WALLET_ADDR).
- Makefile run-wallet-nosigner target is removed.
- Documentation is updated accordingly.
Security design:
- ARKD_SIGNER_SECRET_KEY holds the hex-encoded operator signing key.
- ARKD_SIGNER_DEPRECATED_KEYS supports key rotation with optional per-key cutoff timestamps.
- /healthz endpoint gates on readiness (signer must have a usable key) rather than just process liveness — correct.
Questions for human reviewers:
- Is there a migration guide for existing deployments? The BREAKING change should be called out in release notes with a step-by-step.
- What authentication does arkd-signer expose on its gRPC interface? If arkd-signer is network-accessible, does it enforce TLS and/or macaroon auth to prevent an attacker from asking it to sign arbitrary transactions?
- The HEALTHCHECK in arkdsigner.Dockerfile polls localhost:${ARKD_SIGNER_PORT:-6061}/healthz. What if the signer is configured without a key at startup? Does it fail healthz (503) and stay undeployed?
Architecture note: Separating the signer from the wallet is the right security direction — the signing key no longer lives in the same process as the liquidity wallet, so compromise of one does not immediately expose the other.
Ready for human review. Do not merge without a clear migration plan and confirmation that the signer's gRPC interface is properly authenticated.
4 participants
Summary
Splits
arkd-wallet's two responsibilities.arkd-walletbecomes an onchain-wallet-only service; a new standalonearkd-signerservice owns the operator signing key andSignerService. The shared tapscript-signing primitive is factored intopkg/ark-lib/txsigner, reused by the wallet and signer (and adoptable by the emulator — see follow-ups).What's new (additive)
pkg/ark-lib/txsigner— chain-free tapscript PSBT signing primitive (BuildPrevoutFetcher,SignTapscriptInput,ExtractFinalizedTx).pkg/arkd-signer— standalone service (own module,cmd/arkd-signer, Dockerfile, compose service). Pure signer: holds the injected operator key, requires complete PSBTs (no chain access). Full parity with the wallet's old signer, including deprecated signer keys (ARKD_SIGNER_DEPRECATED_KEYS, per-leaf key selection,GetPubkeydeprecated-signers).Clean break
arkd-walletno longer servesSignerService, holds no operator key, and itsLoadSignerKeyRPC is nowUnimplemented.arkdsigner-management: runtime key injection removed; the external-signer URL path is kept;ARKD_SIGNER_ADDRis now required (the silentWALLET_ADDRfallback is gone).arkd signer loadis URL-only (--signer-prvkeyremoved).arkd-signerservice and setARKD_SIGNER_ADDRonarkd(no more fallback).ARKD_WALLET_SIGNER_KEY->ARKD_SIGNER_SECRET_KEY; deprecated keysARKD_WALLET_DEPRECATED_SIGNER_KEYS->ARKD_SIGNER_DEPRECATED_KEYS.arkd signer load --signer-prvkeyis removed; use--signer-url.docker-compose.regtest.yml, the Makefile (build-signer,run-signer),envs/signer.dev.env, and the README are updated accordingly.Test plan
ark-lib/txsigner,arkd-signer(incl. deprecated-key selection +GetDeprecatedPubkeys),arkd-wallet— all green.go vet ./internal/...is clean (incl. test files).arkd-signeris dependency-neutral (no change to arkd'sgo.mod/go.sum).arkdsigner.Dockerfilewas bumped togolang:1.26.5to match the modules' toolchain (it had lagged at1.26.4, breaking the docker build); thesqlite/badgersuite is green.postgres/redisintermittently flakes on timing-heavy flows (TestReactToFraud,TestUnilateralExit), not a deterministic failure. The e2e harness recreatesarkd-signer(not the wallet) for key rotation.Follow-ups (not in this PR)
arkd-walletadoptingtxsignerfor its own LP-mode tapscript branch (removes the last duplicated copy).txsignervia anark-librelease (the "share a library" consolidation).arkd-signer(dropped here to keep the PR dependency-neutral).Summary by CodeRabbit
arkd-signeras a standalone signing service (with health checks, key rotation, and transaction/Taproot script signing).arkd-signeris the signer service:ARKD_SIGNER_ADDRis required, andarkd signer loadaccepts only a signer URL.