Require Intent for GetVtxoChain/GetVirtualTxs#904
Require Intent for GetVtxoChain/GetVirtualTxs#904bitcoin-coder-bob wants to merge 18 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds configurable indexer transaction exposure, intent-based PSBT proof validation, short-lived auth-token generation/validation, a SignMessage gRPC/schema and wallet signing API, nullable DB timestamp handling, tests/fixtures for auth/signing, and Go toolchain/CI/Dockerfile bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IndexerHandler
participant IndexerService
participant Validator as Intent/PSBT Validator
participant Signer
participant Database
Client->>IndexerHandler: GetVtxoChain(vtxoKey, intent)
IndexerHandler->>IndexerService: GetVtxoChain(vtxoKey, intent)
IndexerService->>IndexerService: Check txExposure
alt Withheld/Private
IndexerService->>Validator: ValidateIntentWithProof(intent)
Validator->>Database: Fetch vtxo data
Database-->>Validator: vtxo
Validator-->>IndexerService: validation result
alt Valid
IndexerService->>Signer: createAuthToken(outpoint)
Signer-->>IndexerService: authToken
IndexerService->>Database: Fetch vtxo chain
Database-->>IndexerService: chain
IndexerService-->>IndexerHandler: VtxoChainResp(chain, authToken)
else Invalid
IndexerService-->>IndexerHandler: error
end
else Public
IndexerService->>Database: Fetch vtxo chain
Database-->>IndexerService: chain
IndexerService-->>IndexerHandler: VtxoChainResp(chain)
end
IndexerHandler-->>Client: GetVtxoChainResponse
sequenceDiagram
participant Client
participant VirtualTxsHandler
participant IndexerService
participant TokenValidator as Auth Token Validator
participant Database
Client->>VirtualTxsHandler: GetVirtualTxs(authToken, txids)
VirtualTxsHandler->>IndexerService: GetVirtualTxs(authToken, txids)
IndexerService->>IndexerService: Check txExposure
alt Withheld
alt authToken present
IndexerService->>TokenValidator: validateAuthToken(authToken)
TokenValidator-->>IndexerService: valid
IndexerService->>Database: Fetch virtual txs (with signatures)
Database-->>IndexerService: txs
else no authToken
IndexerService->>Database: Fetch virtual txs
Database-->>IndexerService: txs (strip arkd sigs)
end
else Private
IndexerService->>TokenValidator: validateAuthToken(authToken)
TokenValidator-->>IndexerService: valid/invalid
alt valid
IndexerService->>Database: Fetch virtual txs (with signatures)
Database-->>IndexerService: txs
else invalid
IndexerService-->>VirtualTxsHandler: error (auth required)
end
else Public
IndexerService->>Database: Fetch virtual txs
Database-->>IndexerService: txs
end
IndexerService-->>VirtualTxsHandler: VirtualTxsResp
VirtualTxsHandler-->>Client: GetVirtualTxsResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@api-spec/protobuf/ark/v1/indexer.proto`:
- Around line 204-208: The GET request payload includes the Intent message
(fields intent.proof and intent.message) which can be large and exceed
URL/query-length limits; change the API so REST clients send Intent in the HTTP
body instead of query params — either (a) remove Intent from GetVtxoChainRequest
and create a separate POST RPC (e.g., GetVtxoChainWithIntent) that accepts
IndexerOutpoint + IndexerPageRequest + Intent in the request body, or (b) keep
the same RPC but update the google.api.http annotation to use POST and map the
Intent to the HTTP body (or a new wrapper message) so proof/message are sent in
the request body rather than as query parameters; apply this change around the
GetVtxoChainRequest and Intent message references.
In `@api-spec/protobuf/signer/v1/service.proto`:
- Around line 31-36: The SignMessage RPC binding currently exposes an
unauthenticated HTTP endpoint; either remove the HTTP gateway binding for the
SignMessage RPC (remove the option (meshapi.gateway.http) block on rpc
SignMessage(SignMessageRequest) returns (SignMessageResponse)) or add
authentication middleware so the endpoint requires macaroons like the main arkd
service; if adding auth, ensure the SignerService/SignMessage handler validates
macaroons on incoming requests (same macaroon check used by arkd) or restrict
the HTTP gateway to listen only on localhost so the endpoint is not reachable on
all interfaces.
In `@internal/core/application/indexer.go`:
- Around line 424-433: In the TxExposureWithheld branch, ValidateIntentWithProof
errors are being swallowed; change the logic in the case for TxExposureWithheld
so that after calling i.ValidateIntentWithProof(ctx, vtxoKey, intentForProof)
you distinguish validation failures from infrastructure/internal errors and only
suppress errors that indicate an invalid proof, while returning unexpected
errors to the caller; use the error type/value returned by
ValidateIntentWithProof (or wrap/inspect it) to detect "invalid proof" vs
DB/internal faults and only proceed to call i.createAuthToken when validation
succeeded, otherwise return the unexpected error instead of dropping it.
- Around line 497-509: The fallback incorrectly treats vtxoKey.Txid as raw tx
hex causing tx.Deserialize to always fail; replace the stubbed assignment with a
real fetch from the wallet (use s.wallet.GetTransaction(ctx,
vtxoOutpoint.Txid.String() or correct id) to obtain txhex and check its error),
then pass the returned raw hex into tx.Deserialize; ensure you handle and
propagate errors from s.wallet.GetTransaction (return an
errors.INVALID_PSBT_INPUT with InputMetadata on failure) and only store the
deserialized wire.MsgTx into boardingTxs[vtxoOutpoint.Txid] when deserialization
succeeds.
- Around line 483-484: The loop over outpoints can index past proof.Inputs
(psbtInput := proof.Inputs[idx+1]); add a bounds check before using idx+1 to
avoid a panic: inside the for loop that iterates outpoints, verify that idx+1 <
len(proof.Inputs) (or len(proof.Inputs) > idx+1) and handle the mismatch (return
an error, log and continue, or skip) instead of indexing blindly; update the
code around the outpoints loop and psbtInput usage to enforce this guard and
surface a clear error message if sizes are inconsistent.
- Around line 582-594: The TODO must be resolved: if the proof PSBT requires a
server counter-signature, call SignTransactionTapscript(ctx,
intentForProof.Proof) and use its returned signed PSBT when calling
intent.Verify (i.e., replace intentForProof.Proof with the signed result), and
propagate any error into the same INVALID_INTENT_PROOF path; additionally remove
or redact logging of the full intentForProof.Proof and intentForProof.Message
(do not log raw PSBT or full message), instead log only non-sensitive metadata
(e.g., proof ID or truncated hash) or omit these fields in the Tracef call to
avoid exposing input/outpoint/script details.
- Around line 414-448: The code in GetVtxoChain currently hardcodes
someExposureFromConfig := "public", bypassing auth branches; replace that
hardcoded value with the instance config i.txExposure (e.g., use exposure :=
i.txExposure), validate exposure != "" and then switch on TxExposure(exposure)
so the Withheld/Private branches (which call i.ValidateIntentWithProof and
i.createAuthToken) can execute as intended; ensure error messages remain the
same and no other logic changes in the switch.
- Around line 78-99: The code currently serializes the signer pubkey with
schnorr.SerializePubKey producing a 32-byte x-only key (in NewIndexerService)
which breaks ECDSA verification in validateAuthToken (which assumes full parity
via schnorr.ParsePubKey) and prevents matching PSBT PartialSig.PubKey (33-byte
compressed) in GetVirtualTxs; change NewIndexerService to store the full 33-byte
compressed pubkey (serialize the returned pubkey as the compressed form instead
of x-only) into indexerService.signerPubkey, update validateAuthToken to
parse/verify using btcec.ParsePubKey (or the equivalent compressed-key parser)
instead of schnorr.ParsePubKey, and ensure GetVirtualTxs compares
PartialSig.PubKey against the 33-byte compressed signerPubkey (so bytes.Equal
can match) rather than a 32-byte x-only key.
- Around line 753-786: validateAuthToken currently tries to parse a DER ECDSA
signature and double-hashes the msg, which is incompatible with
createAuthToken/SignMessage (which produce 64-byte schnorr signatures over the
raw message). Update validateAuthToken to parse the signature as a schnorr
signature (use the schnorr parse/verify API) instead of ecdsa.ParseDERSignature,
and verify against the raw msg (do not call chainhash.DoubleHashB on msg);
continue to parse the signer pubkey with schnorr.ParsePubKey(i.signerPubkey) and
call the schnorr verify method to return the boolean result.
In `@pkg/arkd-wallet/core/application/wallet/service.go`:
- Around line 731-739: SignMessage currently produces an ECDSA DER signature
(uses ecdsa.Sign) while the verifier in indexer.go expects Schnorr signatures
from a Schnorr-serialized public key (signerPubkey), so auth tokens will always
fail verification; fix by making signing and verification use the same scheme —
either change SignMessage to call schnorr.Sign(...) and return the Schnorr
signature format to match the existing verifier, or (alternatively) update
indexer.go to parse/derive the ECDSA public key and verify with
ecdsa.ParseDERSignature()/sig.Verify using that ECDSA pubkey; locate SignMessage
in service.go and the verification code in indexer.go (where signerPubkey is
parsed) and make both sides use the same signature algorithm (prefer using
schnorr.Sign if you want minimal changes to indexer.go).
🧹 Nitpick comments (4)
api-spec/openapi/swagger/ark/v1/indexer.openapi.json (1)
489-495: Verify:authTokenquery parameter lacks description.Other query parameters in this spec (e.g.,
spendableOnly,pendingOnly) include adescriptionfield. The newauthTokenparameter and theintent.proof/intent.messageparameters (Lines 578–591) omit descriptions. Adding brief descriptions would improve the developer experience for REST API consumers, especially given the new exposure modes (public/withheld/private) that affect how these fields are used.README.md (1)
111-111: Description could be more precise.The current description "Require intent for getting vtxo chain" undersells the feature. It also governs
GetVirtualTxsauth-token behavior and PSBT signature stripping inwithheldmode. A slightly more descriptive phrasing like "Controls indexer transaction data exposure level" would better convey the scope to operators.Minor nit — this won't block anything.
internal/config/config.go (2)
630-637:IndexerService()creates a new instance on every call — consider caching.Unlike
AppService()which caches inc.svc, this method creates a newIndexerService(including asigner.GetPubkeygRPC call) on every invocation. Currently it's only called once fromnewServer, but to stay consistent with other accessors and prevent accidental regressions:♻️ Proposed caching pattern
Add an
indexerSvcfield to theConfigstruct and cache the result:+ indexerSvc application.IndexerServiceThen update the accessor:
func (c *Config) IndexerService() (application.IndexerService, error) { + if c.indexerSvc != nil { + return c.indexerSvc, nil + } if c.signer == nil { if err := c.signerService(); err != nil { return nil, err } } - return application.NewIndexerService(c.repo, c.signer, c.IndexerTxExposure) + svc, err := application.NewIndexerService(c.repo, c.signer, c.IndexerTxExposure) + if err != nil { + return nil, err + } + c.indexerSvc = svc + return c.indexerSvc, nil }
567-571: Consider using theTxExposureconstants instead of hardcoded strings.The application layer defines
TxExposurePublic,TxExposureWithheld, andTxExposurePrivateconstants for theTxExposuretype. Using those here would keep validation in sync with the domain layer and prevent drift.♻️ Suggested change
- if c.IndexerTxExposure != "public" && c.IndexerTxExposure != "withheld" && - c.IndexerTxExposure != "private" { - return fmt.Errorf("indexer tx exposure must be either 'public', 'withheld', or 'private'") - } + if c.IndexerTxExposure != string(application.TxExposurePublic) && + c.IndexerTxExposure != string(application.TxExposureWithheld) && + c.IndexerTxExposure != string(application.TxExposurePrivate) { + return fmt.Errorf("indexer tx exposure must be either 'public', 'withheld', or 'private'") + }
| message GetVtxoChainRequest { | ||
| IndexerOutpoint outpoint = 1; | ||
| IndexerPageRequest page = 2; | ||
| Intent intent = 3; | ||
| } |
There was a problem hiding this comment.
Intent in a GET request may hit URL length limits for REST/HTTP clients.
The Intent message (with proof and message fields) is added to GetVtxoChainRequest, which maps to a GET endpoint. The proof data (PSBT signature) could be large, and when serialized as query parameters (intent.proof, intent.message), it may exceed URL length limits in browsers or proxies (commonly ~2KB–8KB). gRPC clients are unaffected.
Consider whether a POST mapping (or an alternative endpoint) is needed for REST clients submitting large proofs.
🤖 Prompt for AI Agents
In `@api-spec/protobuf/ark/v1/indexer.proto` around lines 204 - 208, The GET
request payload includes the Intent message (fields intent.proof and
intent.message) which can be large and exceed URL/query-length limits; change
the API so REST clients send Intent in the HTTP body instead of query params —
either (a) remove Intent from GetVtxoChainRequest and create a separate POST RPC
(e.g., GetVtxoChainWithIntent) that accepts IndexerOutpoint + IndexerPageRequest
+ Intent in the request body, or (b) keep the same RPC but update the
google.api.http annotation to use POST and map the Intent to the HTTP body (or a
new wrapper message) so proof/message are sent in the request body rather than
as query parameters; apply this change around the GetVtxoChainRequest and Intent
message references.
There was a problem hiding this comment.
i dont think the intents are that big?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Intent type definition
rg -n "type Intent struct" --type=go -A 5
# Find where Intent.Proof is populated or used
rg -n "Intent\{" --type=go -C 3
# Look for PSBT serialization or encoding in the context of Intent
rg -n "Proof.*base64|base64.*Proof" --type=go -C 2
# Find the types.proto file that defines Intent message
fd -e proto types.proto -x cat {}
Length of output: 16159
@bitcoin-coder-bob You're probably right for typical use cases!
Looking at the code, the Intent.Proof field contains a base64-encoded PSBT. For a transaction with 1-2 inputs, that would be around 400-1,100 bytes encoded, which is well within URL length limits.
My concern was for edge cases where someone creates an intent with many inputs (5-10+ UTXOs being spent). A PSBT with that many inputs could reach 2-3KB+ in base64, and when combined with the other query parameters (outpoint.txid, outpoint.vout, page params), it might approach the ~2KB limit enforced by some older proxies or restrictive API gateways.
Question: In practice, is there a constraint on how many inputs a typical intent would have? If intents typically stay at 1-3 inputs, then this isn't a real concern and I was being overly cautious.
internal/core/application/indexer.go
Outdated
| case TxExposureWithheld: | ||
| // validate the intent proof/message to allow access to the full chain | ||
| err = i.ValidateIntentWithProof(ctx, vtxoKey, intentForProof) | ||
| if err == nil { | ||
| authToken, err = i.createAuthToken(ctx, vtxoKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| // if intent failed validation, we just dont supply an auth token |
There was a problem hiding this comment.
Withheld mode silently swallows all ValidateIntentWithProof errors, including infrastructure failures.
If ValidateIntentWithProof fails due to a DB error or internal issue (not just invalid proof), the error is silently discarded and the caller simply gets no auth token. Consider distinguishing between "invalid proof" (expected, no auth token) and unexpected infrastructure errors (should be surfaced).
🤖 Prompt for AI Agents
In `@internal/core/application/indexer.go` around lines 424 - 433, In the
TxExposureWithheld branch, ValidateIntentWithProof errors are being swallowed;
change the logic in the case for TxExposureWithheld so that after calling
i.ValidateIntentWithProof(ctx, vtxoKey, intentForProof) you distinguish
validation failures from infrastructure/internal errors and only suppress errors
that indicate an invalid proof, while returning unexpected errors to the caller;
use the error type/value returned by ValidateIntentWithProof (or wrap/inspect
it) to detect "invalid proof" vs DB/internal faults and only proceed to call
i.createAuthToken when validation succeeded, otherwise return the unexpected
error instead of dropping it.
There was a problem hiding this comment.
we could require they always pass validation, not just use it as a gate to determine if they get an auth token
There was a problem hiding this comment.
@bitcoin-coder-bob, that's a good point! Making validation mandatory in withheld mode would address the concern cleanly—infrastructure errors would bubble up naturally instead of being silently dropped, and you'd avoid the ambiguity of "partial access without proof."
If you require valid intent in withheld mode (just like private mode), the key difference would be:
- Withheld: Valid intent required → always get auth token → GetVirtualTxs strips arkd signatures if auth token is somehow invalid/missing later
- Private: Valid intent required → always get auth token → GetVirtualTxs returns error if auth token is invalid/missing
This would change the semantics of withheld from "optional proof for better access" to "proof always required, but downstream behavior differs." Is that the direction you want to take? It would simplify the logic and make error handling more explicit.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/config/config.go`:
- Around line 573-575: The long fmt.Errorf call in the check for
supportedIndexerTxExposures.supports(c.IndexerTxExposure) exceeds golines; fix
by wrapping the format arguments across lines so the string literal is shorter —
e.g. keep the format string in-line and move supportedIndexerTxExposures to the
next line (or split the message into two concatenated strings) so the call to
fmt.Errorf("indexer txn exposure type not supported, please select one of: %s",
supportedIndexerTxExposures) becomes formatted across lines and satisfies
golines while leaving the logic in supportedIndexerTxExposures.supports and
c.IndexerTxExposure unchanged.
In `@internal/core/application/indexer_test.go`:
- Around line 109-112: Tests construct indexerService with a 32-byte x-only key
via schnorr.SerializePubKey but production uses 33-byte compressed keys
(pubkey.SerializeCompressed()) and validateAuthToken will parse compressed keys
with btcec.ParsePubKey; update each test construction of indexerService (where
signerPubkey is set) to use mockSigner.publicKey.SerializeCompressed() (or the
equivalent compressed byte slice) instead of schnorr.SerializePubKey so the test
bytes match NewIndexerService/validateAuthToken behavior.
- Around line 70-76: Reformat the long stub signature for SignTransaction to
satisfy golines by breaking the parameter list across lines: locate the method
func (m *mockSignerService) SignTransaction and split its parameters (ctx
context.Context, partialTx string, extractRawTx bool) onto separate lines (or
otherwise wrap) while keeping the receiver, parameter names/types and the
(string, error) return intact; run gofmt/golines after editing to confirm CI
passes.
In `@internal/core/application/indexer.go`:
- Around line 636-646: The loop currently removes arkd signatures from
ptx.Inputs[j].PartialSigs, but Taproot virtual transactions store signatures in
TaprootScriptSpendSig; update the stripping logic in the same block to iterate
ptx.Inputs[j].TaprootScriptSpendSig (instead of PartialSigs), filter out entries
whose PubKey equals i.signerPubkey, and assign the filtered slice back to
ptx.Inputs[j].TaprootScriptSpendSig so arkd's taproot signatures are actually
removed in withheld-mode.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/core/application/indexer.go`:
- Around line 648-652: The code currently re-serializes the PSBT with
ptx.Serialize(&b) and assigns b.String() to virtualTxs[idx], which produces raw
binary but callers (NewFromRawBytes(..., true) and others) expect base64; update
the assignment to base64-encode the serialized bytes instead of storing raw
bytes so virtualTxs remains base64-encoded (use the bytes from the
strings.Builder or ptx.Serialize output and encode with
base64.StdEncoding.EncodeToString before assigning to virtualTxs[idx]).
- Around line 489-490: The current check conflates DB errors with "not found" by
using `if err != nil || len(vtxosResult) == 0` after calling
`i.repoManager.Vtxos().GetVtxos(ctx, []domain.Outpoint{vtxoOutpoint})`; change
this to first handle and return (or propagate/log) when `err != nil` so
infrastructure/database failures surface, and only when `err == nil` check
`len(vtxosResult) == 0` to decide to fall back to the on-chain wallet path;
update the surrounding logic that uses `vtxosResult` accordingly (references:
`GetVtxos`, `vtxosResult`, `err`, `vtxoOutpoint`).
🧹 Nitpick comments (2)
internal/config/config.go (1)
637-644: Missing nil-guard forc.wallet— onlyc.signeris defensively initialized.
IndexerService()checks and lazily initializesc.signerbut notc.wallet. If this method is ever called beforeValidate()finishes (or in a test/path that skips wallet init),c.walletwill benil, causing a nil-pointer dereference insideNewIndexerServiceor later inValidateIntentWithProofwheni.wallet.GetTransactionis called.Suggested fix
func (c *Config) IndexerService() (application.IndexerService, error) { + if c.wallet == nil { + if err := c.walletService(); err != nil { + return nil, err + } + } if c.signer == nil { if err := c.signerService(); err != nil { return nil, err } } return application.NewIndexerService(c.repo, c.signer, c.wallet, c.IndexerTxExposure) }internal/core/application/indexer.go (1)
759-792: Auth tokens have no expiry and no scope binding to the requested txids.
validateAuthTokenonly checks the signature — it does not verify that the outpoint embedded in the token relates to the txids being requested inGetVirtualTxs, nor does it enforce a time-to-live. A single valid token grants indefinite access to any virtual txs.If this is acceptable for the current threat model (the token just attests the caller passed through
GetVtxoChain), consider adding a comment documenting that decision. Otherwise, adding a timestamp to the signed payload and verifying token age would limit the blast radius of a leaked token.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/core/application/indexer.go`:
- Around line 515-525: Add a nil-check for psbtInput.WitnessUtxo immediately
after retrieving psbtInput to avoid panics when a proof PSBT omits WitnessUtxo;
if nil, return errors.INVALID_PSBT_INPUT (similar style to existing errors) with
a descriptive message like "missing WitnessUtxo for input" and attach the same
metadata (errors.InputMetadata{Txid: proofTxid, InputIndex: idx + 1}); this
prevents dereferences of psbtInput.WitnessUtxo when comparing PkScript
(prevout.PkScript vs psbtInput.WitnessUtxo.PkScript) and when comparing Value.
🧹 Nitpick comments (2)
internal/core/application/indexer.go (2)
601-610: Auth validation runs after DB fetch and pagination — wasted work when token is rejected.
GetTxsWithTxidsandpaginateexecute before the exposure switch. ForPrivatemode, invalid tokens cause an immediate rejection, making the preceding DB query unnecessary. Moving the auth check before the fetch would avoid the wasted I/O on unauthorized requests.
85-102:txExposureis not validated at construction time — invalid values surface only at request time.
NewIndexerServiceaccepts any string fortxExposure. An invalid value (e.g., a typo in config) won't be caught until a request hits thedefaultbranch inGetVtxoChainorGetVirtualTxs. Validating at startup would fail fast and prevent a misconfigured server from silently serving errors.Proposed fix
func NewIndexerService( repoManager ports.RepoManager, signer ports.SignerService, wallet ports.WalletService, txExposure string, ) (IndexerService, error) { + switch TxExposure(txExposure) { + case TxExposurePublic, TxExposureWithheld, TxExposurePrivate: + default: + return nil, fmt.Errorf("invalid tx exposure value: %q", txExposure) + } + pubkey, err := signer.GetPubkey(context.Background())
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 (1)
internal/core/application/indexer.go (1)
612-696:⚠️ Potential issue | 🔴 Critical
TxExposurePubliccase inGetVirtualTxsreturns empty results — transactions are never fetched.The
Publicbranch (line 625–626) falls through without callingGetTxsWithTxidsor populatingtxs/virtualTxs. The variables are initialized as empty slices (lines 617–618), soGetVirtualTxsalways returns an empty response when exposure is"public".Both
Withheld(line 636) andPrivate(line 682) correctly callGetTxsWithTxidsand paginate. ThePubliccase is missing this.Proposed fix
switch TxExposure(i.txExposure) { case TxExposurePublic: // no need to validate auth + txs, err = i.repoManager.Rounds().GetTxsWithTxids(ctx, txids) + if err != nil { + return nil, err + } + virtualTxs, reps = paginate(txs, page, maxPageSizeVirtualTxs) case TxExposureWithheld:
🤖 Fix all issues with AI agents
In `@internal/core/application/indexer.go`:
- Around line 423-441: When TxExposure is TxExposurePrivate, don't silently
swallow all errors from ValidateIntentWithProof; distinguish between a
validation-failure (invalid intent/proof) and an infrastructure error
(DB/signer). Change the block around ValidateIntentWithProof in the indexer to:
call ValidateIntentWithProof(vtxoKey, intentForProof); if err == nil proceed to
createAuthToken; if err indicates a validation rejection (use the specific
sentinel/error type returned by ValidateIntentWithProof, e.g. ErrInvalidIntent
or a ValidationError) then continue without an auth token as before; otherwise
(for non-validation errors) return that error immediately. Update
ValidateIntentWithProof to export or wrap a sentinel/typed error if needed so
the caller can use errors.Is / type assertion to detect validation vs
infrastructure errors; keep references to TxExposurePrivate,
ValidateIntentWithProof, createAuthToken and GetVirtualTxs in your changes.
🧹 Nitpick comments (1)
internal/config/config.go (1)
637-649:c.repois not nil-checked before being passed toNewIndexerService.
IndexerService()lazily ensureswalletandsignerare initialized but passesc.repowithout checking. If called beforeValidate()(which initializesc.repoviac.repoManager()), this will passnilto the constructor. Other factory methods likeAppService()similarly assume prior initialization, so this may be by convention — but unlike wallet/signer, there's no guard here.Suggested guard
func (c *Config) IndexerService() (application.IndexerService, error) { + if c.repo == nil { + if err := c.repoManager(); err != nil { + return nil, err + } + } if c.wallet == nil {
1 participant
Issue: #843
The general flow is that a caller calls
GetVtxoChainwhich yields[]txidswhich they then pass a param to a call toGetVirtualTxs. The idea of this PR is to create an auth token insideGetVtxoChainif the user can prove ownership of the intent they passed in, then to use this auth token when callingGetVirtualTxs. The spirit here is that: "A public virtual mempool means griefing attacks by third party is possible. Imagine you received a vtxo and now someone can just start unilaterally exiting your vtxo and you end up back onchain. Really annoying and can cause a number of problems with smart contracts."This auth token is a message signed by the server's pubkey along with an outpoint, resulting in stateless auth system.
We need go-sdk changes:
A new intent param must be passed to
GetVtxoChainand the response will now include a new auth token (can be empty string as well). Depending on the new config for arkd for theINDEXER_TX_EXPOSUREfield (with the 3 options beingpublic,private, orwithheld) will determine the creation and enforcement of this auth token.We will also want tests against
GetVtxoChain, the newValidateIntentWithProofandGetVirtualTxsWhen
public: no auth token created, empty auth token in response ofGetVtxoChainand an empty auth token to be passed toGetVirtualTxs.When
private: the caller must passValidateIntentWithProofto get an auth token, and the auth token is required to callGetVirtualTxsWhen
withheld: the caller ofGetVtxoChainhas the option to pass OR failValidateIntentWithProofwhich will determine if they do or do not get an auth token. If they get a token they can use it when callingGetVrtualTxsand the behavior will function the same way as ifpublicwere set. However if they pass an invalid or empty auth token, we must remove from each PSBT the signature of arkd so the user cannot construct the full broadcastable txn.The newly added
ValidateIntentWithProofis largely the same flow as found here:arkd/internal/core/application/service.go
Lines 2048 to 2200 in 368bb9f
The
indexerServicenow that it needs to be able to sign a message for the auth token, and needs access to the server's pubkey (to locate inside the psbt for removal), we have given theindexerServiceaccess toports.SignerServiceandports.WalletService(needed for the SignMessage call).Requires changes in the go-sdk for
GetVtxoChain(which now requires an Intent be passed) and toGetVirtualTxs(which now requires an AuthToken to be passed). Thus the e2e tests without this sdk change will only work when the exposure type is set topublic.Note that theres a couple small changes in the sqlite. I re-ran
sqlc generateand it yielded couple changes to theAfterfield.Summary by CodeRabbit
New Features
Documentation
Tests