fix(go): migrate to solana-go v2 and harden verification#214
Conversation
Greptile SummaryThis PR migrates the Go SDK from the vendored
Confidence Score: 5/5Safe to merge. The security-critical paths (managed-signer drain guard, cosign slot restriction, atomic challenge assembly) are all backed by targeted regression tests, and the solana-go v2 migration is a clean import path swap with no behavioral changes to public APIs. The three security-hardening additions are each independently defended: rejectManagedSourceOwner fails closed on every infrastructure error, the structural verifier changes are tested against multisig and ATA-alias shapes, and the atomic challenge helpers mirror the existing appendChallenge pattern exactly. No partial-success paths were found that could emit an unpayable 402 or skip operator signing. No files require special attention. The most security-sensitive paths — exact.go, verify.go, and usage.go — all have direct test coverage for the new code paths added by this PR. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Middleware
participant ExactAdapter
participant StructuralVerifier
participant OnChainRPC
Client->>Middleware: HTTP request with X-Payment header
Middleware->>ExactAdapter: VerifyAndSettle(req)
ExactAdapter->>StructuralVerifier: VerifyExactTransaction(tx, reqs)
StructuralVerifier-->>ExactAdapter: ok / VerifyError
ExactAdapter->>OnChainRPC: rejectManagedSourceOwner(tx, managedSigners)
OnChainRPC-->>ExactAdapter: GetAccountInfoWithOpts(source)
ExactAdapter-->>ExactAdapter: check owner bytes[32:64] vs managed signers
alt owner is managed signer
ExactAdapter-->>Middleware: PaymentError code invalid_exact_svm_payload
else RPC/inspection failure
ExactAdapter-->>Middleware: PaymentError code source_owner_check_failed
else ok
ExactAdapter->>ExactAdapter: cosign slot 0 only
ExactAdapter->>OnChainRPC: SendEncodedTransaction + awaitConfirmation
ExactAdapter-->>Middleware: Payment
Middleware-->>Client: 200 + X-Payment-Response
end
note over Middleware,ExactAdapter: 402 challenge path
Middleware->>ExactAdapter: appendChallenge(gate)
ExactAdapter->>ExactAdapter: AcceptsEntry(gate) entry or err
ExactAdapter->>ExactAdapter: ChallengeHeaders(gate) headers or err
alt both succeed
ExactAdapter-->>Middleware: entry + headers appended atomically
Middleware-->>Client: 402 + payment-required header
else either fails
ExactAdapter-->>Middleware: error protocol dropped from offer
Middleware-->>Client: 500 challenge_generation_failed if all adapters fail
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant Middleware
participant ExactAdapter
participant StructuralVerifier
participant OnChainRPC
Client->>Middleware: HTTP request with X-Payment header
Middleware->>ExactAdapter: VerifyAndSettle(req)
ExactAdapter->>StructuralVerifier: VerifyExactTransaction(tx, reqs)
StructuralVerifier-->>ExactAdapter: ok / VerifyError
ExactAdapter->>OnChainRPC: rejectManagedSourceOwner(tx, managedSigners)
OnChainRPC-->>ExactAdapter: GetAccountInfoWithOpts(source)
ExactAdapter-->>ExactAdapter: check owner bytes[32:64] vs managed signers
alt owner is managed signer
ExactAdapter-->>Middleware: PaymentError code invalid_exact_svm_payload
else RPC/inspection failure
ExactAdapter-->>Middleware: PaymentError code source_owner_check_failed
else ok
ExactAdapter->>ExactAdapter: cosign slot 0 only
ExactAdapter->>OnChainRPC: SendEncodedTransaction + awaitConfirmation
ExactAdapter-->>Middleware: Payment
Middleware-->>Client: 200 + X-Payment-Response
end
note over Middleware,ExactAdapter: 402 challenge path
Middleware->>ExactAdapter: appendChallenge(gate)
ExactAdapter->>ExactAdapter: AcceptsEntry(gate) entry or err
ExactAdapter->>ExactAdapter: ChallengeHeaders(gate) headers or err
alt both succeed
ExactAdapter-->>Middleware: entry + headers appended atomically
Middleware-->>Client: 402 + payment-required header
else either fails
ExactAdapter-->>Middleware: error protocol dropped from offer
Middleware-->>Client: 500 challenge_generation_failed if all adapters fail
end
Reviews (24): Last reviewed commit: "fix(go): distinguish source owner check ..." | Re-trigger Greptile |
MemoryChannelStore.DeleteChannel deleted from s.data but never s.locks, so the per-channel mutex map grew without bound for the process lifetime; delete from both under the same mu. BuildCredential silently skipped the compute-unit price/limit instructions when their builders errored (if ix, err := ...; err == nil), so a charge could be signed without a compute-unit limit. Propagate the error like the surrounding code does.
…ckages strconv.FormatUint/ParseUint over fmt.Sprintf/Sscanf for integer conversion, drop single-case switch and redundant []byte and import aliases, and MixedCaps/initialism fixes. No behavior change.
Collapse NewChallengeWithSecret/NewChallengeWithSecretFull into one constructor whose optional expires/digest/description/opaque fields are set via named ChallengeOption values, so the three same-typed optional strings can no longer be transposed silently at a call site. Also fix the wire package doc comment (was 'core') and rename the cap parameter that shadowed the builtin.
PaymentIdentifierInfo.Id becomes ID; the JSON tag stays 'id' so the wire shape is unchanged.
Drop the test-only SetRPCForTests method from the public X402Upto and inject the RPC client through a new UptoConfig.RPCClient field instead. Extract the adapter's paykit.Config to UptoConfig mapping into buildUptoConfig so the adapter test can inject a deterministic client without a public test seam.
Adapter.AcceptsEntry/ChallengeHeaders and UsageAdapter now return (value, error) so a base58 or serialization failure surfaces to the caller instead of being swallowed. The 402 challenge writer logs and drops the offending protocol rather than emitting a half-built header, and signerBridge decodes the operator pubkey once at construction where the error can propagate. Move ContextWith*ForTests helpers out of the production surface into export_test.go.
The interface refactor added error-return branches (adapter accepts/ challenge builders, RPC config, binding checks) that dropped statement coverage below the 91% CI gate. Add in-package tests for the exact and mpp adapters (Protocol, ChallengeHeaders, VerifyAcceptedBinding, normalizeNetwork, missing-secret and bad-recipient constructors) and the 402 challenge-writer error-drop paths so each new branch is exercised.
The upto tests set engine.rpc directly (an unexported field) after construction, which bypassed the public RPCClient config field this PR introduced to replace SetRPCForTests. Pass RPCClient in the UptoConfig literal so the tests exercise the same injection path real callers use.
Ran go fix across the Go SDK and harness modules: interface{} -> any, the
slices/maps stdlib helpers, strings.SplitSeq iteration, min/max builtins, and
range-over-int/loopvar rewrites. Mechanical, no behavior change; the generated
codama client under protocols/programs/paymentchannels is left untouched.
build, vet, gofmt, and the full go test suite pass.
Add scripts/check-go-fix.sh and run it in the Go lint job so CI fails when the Go 1.26 language/stdlib modernizers (go fix -inline=false) would still change any non-generated source, keeping the SDK on newer idioms (any, min/max, new(expr), range-over-int, slices/maps, SplitSeq, wg.Go) without manual review comments. The inline analyzer is excluded because it applies opt-in //go:fix inline directives and is cache sensitive; generated codama clients are excluded by their AUTOGENERATED header. On failure the gate prints the suggested diff so the fix is copy-pasteable.
94ee9db to
6ff0a6c
Compare
|
@greptile-apps review |
|
@greptile-apps please run a fresh full review of the latest commit. |
|
Merge conflicts |
…eanup # Conflicts: # go/paykit/adapters/x402/upto.go # go/protocols/x402/upto.go # go/protocols/x402/upto_test.go
|
@greptile-apps please review |
…pdates DeleteChannel deleted s.locks[channelID] alongside s.data[channelID]. A concurrent UpdateChannel that already obtained the channel's mutex from channelLock would then serialize against a different mutex minted by the next channelLock call for the same id (the old one having been deleted), so two updaters could interleave their read-modify-write and lose an update. Keep the per-channel lock for the process lifetime instead; the lock map is bounded by the number of distinct channel ids ever seen. Adds a regression test asserting channelLock returns a stable mutex across DeleteChannel (fails if the lock-delete is reintroduced).
check-go-fix.sh captured 'go fix -diff' stdout with 2>/dev/null and treated an empty diff as passing. Because go fix -diff exits non-zero whenever it finds diffs (like gofmt -l), the exit code cannot distinguish that from a real error, and a build/parse failure printed only to the discarded stderr, yielding an empty diff and a green gate -- a fail-open on the harness/go-client's only CI signal. Capture stderr instead and fail the gate closed when it is non-empty.
|
Folded two fixes on top of the cleanup (kept the standalone attempt out — the solana-go ->
|
|
@greptile-apps please review |
|
@greptile-apps please re-review. The prior 4/5's only finding (P2: |
…o fix/go-idiomatic-cleanup
…ix/go-security-hardening
|
@greptile-apps please review the current head. |
|
@greptile-apps please re-review the final combined Go head. Adversarial review added off-localnet open transaction binding, confirmed/finalized gates, durable top-up replay consumption, delegated managed-source ownership checks, safe transaction re-marshalling, fail-closed challenge generation, and clone-isolation tests; full/race/lint/consumer guards are green. |
|
@greptile-apps please re-review exact head |
|
@greptile-apps please review |
|
@greptile-apps please review |
|
@greptile-apps please review |
e68ca71
into
solana-foundation:split/pr216-ci-harness-mega
Rebasing this leaf onto the harness base tip pulls in PR #214 (solana-go v2), which rewrites the four go dependency manifests (harness/go-{client,server}/go.{mod,sum}). Their delivery-head blob no longer matches the 216 source, so the ledger reconciles them from integrated to open_pr owned by #214 and repoints the surviving integrated evidence at the current delivery head. Produced by scripts/reconcile-pr216-open-deliveries.mjs; validate-pr216-ledger passes.
Reconcile #239's on-chain Channel-account state binding onto the base's solana-go v2 migration and hardened open/top-up verification (#214). Adds the authoritative state-binding layer that no sibling or the base delivers: fetchAndBindChannelAccount, NewOpenStateTxVerifier, bindConfirmedOpenChannel, validateBoundOpenChannel, NewTopUpStateTxVerifier and confirmedTransactionSlot. Session open/top-up state is bound to the on-chain Channel account read at the confirmed slot; an unconfirmed or economically mismatched channel is rejected (fail closed). Base store-safety (env-gated) is preserved; the config-field guard reconciles to honor the same PAY_KIT_ALLOW_INMEMORY_REPLAY_STORE opt-in. Ports #239's v1 solana-go usage to v2 and keeps base's structural verifiers + concurrency/replay hardening; drops #239's duplicate top-up-fence and legacy verifier redefinitions superseded by the base.
Give the shared test operator a real signing key (sessionTestOperatorKey backs sessionTestRecipient) and co-sign the rentPayer slot so signature-only open fixtures reproduce a fully signed confirmed transaction. Base #214 verifies open signatures cryptographically, so the prior dummy-signature fixtures no longer pass. Also restores base's env-gated nil-store default (a merge artifact had re-added an unconditional off-localnet rejection that ignored the opt-in).
Base #214 hardened open verification (real wire-bound signatures, no placeholder acceptance) and the session store policy became env-gated. Update the stale Go test fixtures/assertions to the reconciled behavior: - flip TestNewOpenTxVerifierKeepsStructuralPlaceholderAcceptance to TestNewOpenTxVerifierRejectsPlaceholderSignature: the seam now rejects the pending placeholder ("must be a real transaction signature") - assert the real confirmed signature and derived PDA in the signature-only open test rather than the dummy reference the caller supplied - thread the derived channel PDA into the top-up test and reflect the post-top-up deposit in the authoritative on-chain account - match the reconciled off-localnet store-policy message, and enforce the unmarked-store rejection at the runtime safety gate (where it now lives) rather than at construction - rename the idle-close no-signer test and assert the no-op: closeOnIdle bails before touching the channel when no merchant signer is configured Test-only; no production source changed.
…216 equivalence ledger Those five redelivery PRs landed on main rather than the #219 line, so their open_pr followUps are corrected from 'Land PR #X' to note the merge and the revalidation that follows when #219 rebases onto main. Validator + self-test still pass (113 commits, 237 paths accounted for).
Go SDK idiomatic cleanup,
solana-gov2 migration, and the Go security-hardening delta, delivered as one per-language PR onto #219.What this PR contains
github.com/solana-foundation/solana-go/v2across the SDK and Go harness modules;gagliardetto/solana-gois not reintroduced.Stack equivalence
#224 is now subsumed: its final head and this PR's current head are the same commit,
1e8d8d2166657be224d341e1d2de1503f4077e02. This preserves the separate review history while presenting maintainers with the requested single green Go delivery into #219.Diff note
The cross-language
uptofiles visible against #219 come from already-landed #220 because #219 trailsmainby that commit; they are not authored by this Go PR.Verification
go test ./...gofmt -lcleanThe routed full-language harness failures are integration-base gaps that clear as their corresponding language PRs land into #219; the Go-owned checks are required to remain green.