sdk-go/stress: refuse CreateUser under RFC-27 enforcement and name the proof errors - #4252
Draft
elitegreg wants to merge 2 commits into
Draft
sdk-go/stress: refuse CreateUser under RFC-27 enforcement and name the proof errors#4252elitegreg wants to merge 2 commits into
elitegreg wants to merge 2 commits into
Conversation
require-ip-ownership-proof was never set in any e2e test, so the enforcement dimension of RFC-27 had no coverage at all and SetIPOwnershipProofFeatureFlag had no callers. Two tests with the flag set. The working path still reaches BGP. A client with no verifier to reach is refused by the program with IpOwnershipProofRequired, which is one of only two proof errors reachable end to end: the SDK pre-flights version, payer, client_ip, user_type and the signature before building a transaction, so those rejections never leave the client, and the epoch window needs a ledger epoch a devnet never advances past 0. The wildcard access pass is the case RFC-27 exists for and had no e2e coverage of any kind: all 72 access-pass call sites in e2e name a --client-ip. A pass at the 0.0.0.0 PDA authorizes its payer for any routable address, which is the shape the shred-oracle issues, so the proof is the only thing binding client_ip. Covered both ways: with a proof the user binds the observed address, without one the create is rejected. The sentinel exemption is covered too, because enforcement must not break the oracle path. The manager is the sentinel authority in a local devnet, so a manager-side user create still succeeds while a client-paid one does not. An address mismatch is asserted client-side, where the guard actually lives: connect binds its proof request to the address it provisions and refuses a proof for any other. ClientSpec.DaemonClientIP sets the daemon to an address the container does not own, which is what makes the two disagree. Everything that can share a devnet does, because a devnet is the expensive part of an e2e test and an extra client is one small container. The first four outcomes are subtests over one devnet with three clients; only the mismatch case needs a second, since DaemonClientIP is fixed when the container starts.
…e proof errors Executor.CreateUser hand-packs a 12-byte payload with no ip_proof discriminant, no Instructions sysvar account and no Ed25519 instruction. It works today only because UserCreateArgs is BorshDeserializeIncremental and a None proof is accepted while require-ip-ownership-proof is clear. The moment the flag is set for an environment every creation this path submits fails with IpOwnershipProofRequired (105); the payer is not the sentinel authority, so the exemption does not apply. Teaching the builder to carry a proof would not help. The verifier signs only the address it observes a request originate from, and the one production consumer is the device-stress orchestrator, whose ClientIPBase + idx addresses are synthetic. Binding an address the caller does not control is the exact thing RFC-27 exists to prevent, so the tool is gated instead: CreateUser reads the feature flag once, caches it, and refuses with a message naming the flag and what to do about it, rather than spending a transaction per user to collect the same rejection. A failure to read global state warns and proceeds -- the program is the authority on enforcement, and an unreadable account is not grounds for refusing to submit anything. The ip_proof discriminant is now emitted explicitly, so the Go payload equals the Rust fixture byte for byte and the test drops its "minus the discriminant" exception. Relying on incremental defaulting left the assertion unable to tell an intentionally absent proof from a truncated payload. The custom-error map stopped at 90, so every RFC-27 rejection rendered as "unknown error code 105". Codes 91-118 gained names, and ClassifyProgramError annotates a transaction error with a named ProgramError so callers can match it with errors.Is while the original RPC error stays reachable. Uint128.Lo64 isolates a pre-existing wart: ByteReader.ReadU128 fills .High from the first eight encoded bytes, which Borsh little-endian makes the low half, so the fields are named the wrong way round. Fixtures and existing tests bake in that convention, so the accessor documents it in one place rather than renaming fields here. Also corrects CLAUDE.md, which still described the Go SDK as read-only.
elitegreg
force-pushed
the
gm/e2e-ip-proof-enforcement
branch
from
September 1, 2026 19:58
632189a to
abc59f7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4225. Part of RFC-27 (
rfcs/rfc27-ip-verification.md, tracker #4194).Decision: option B, gate the tool
The issue offered two paths and recommended B. Two independent findings confirm it:
crates/doublezero-ip-verifier/src/server.rs,resolve_client_ip).Executor.CreateUser's one production consumer is the device-stress orchestrator, whose addresses areClientIPBase + idx— synthetic, and not addresses it originates traffic from. Binding an address the caller does not control is the exact thing RFC-27 exists to prevent, so porting the proof path would mean inventing a bypass for it.genericInstruction.skipPermissionInjectexists because the processor detects the optional tenant account byaccounts.len(). Appending an Instructions sysvar account — which a proof requires — shifts that count and mis-classifies accounts.Summary of Changes
CreateUserrefuses whenrequire-ip-ownership-proofis set, with a message naming the flag and what to do instead, rather than spending a transaction per user to collectIpOwnershipProofRequired(105). The flag is read fromGlobalStateonce and cached behind async.Once, like the existing Permission PDA lookup. A failure to read global state warns and proceeds — the program is the authority on enforcement, and an unreadable account is not grounds for refusing to submit anything.ip_proofdiscriminant is emitted explicitly (12 → 13 bytes), so the Go payload equals the Rustuser_create_argsfixture byte for byte.TestBuildCreateUserInstructiondrops its "minus the discriminant" exception. Relying onBorshDeserializeIncrementalto default the field left the assertion unable to tell an intentionally absent proof from a truncated payload.unknown error code 105. Codes 91–118 gained names, andClassifyProgramErrorannotates a transaction error with a comparableProgramErrorso callers can match it witherrors.Is;Unwrap() []errorkeeps the originaljsonrpc.RPCErrorreachable.CLAUDE.mdno longer describes the Go SDK as read-only.A pre-existing bug found on the way, deliberately not fixed here
ByteReader.ReadU128assignsUint128.Highfrom the first eight encoded bytes, but Borsh writes a u128 little-endian — those are the low half. Confirmed against the fixture:global_state.jsonhasAccountIndex: 42andglobal_state.binencodes it as2a 00 00 …in the first eight bytes, which land in.High. The two fields are named the wrong way round.No production Go reads
.High/.Low— only the deserializer writes them — butclient_test.gobakes in the swapped convention (Uint128{High: 12, Low: 0}), so correcting it touches those tests and belongs in its own change. This PR isolates the wart behind a documentedUint128.Lo64()accessor and does not rename anything. Worth its own issue.Testing Verification
TestBuildCreateUserInstructionnow asserts the full Rust fixture with no trailing-byte exception.TestCreateUserRefusesWhenProofRequired: with the flag set, nothing is submitted (sentTransactionsempty), the derived user PDA is still returned so a caller can correlate the refusal, and a second attempt re-uses the cached flags rather than re-reading global state.TestCreateUserProceedsWhenProofNotRequired: with the flag clear, the create is submitted — so the guard cannot pass by refusing everything.TestGlobalStateIsFeatureEnabledround-trips through the real deserializer rather than constructing aUint128by hand, and checks that bit 1 is not mistaken for bit 2.TestClassifyProgramErrorMapsIPOwnershipProofRequiredasserts the named match, the absence of a match against a sibling class, and that the underlyingjsonrpc.RPCErrorsurvives.go build ./...and the fullsmartcontract/sdk/go/...plustools/stress/...suites pass;golangci-lintreports 0 issues on the changed package.