serviceability: add ip_verifier_authority_pk to GlobalState and rotate it via SetAuthority - #4207
serviceability: add ip_verifier_authority_pk to GlobalState and rotate it via SetAuthority#4207elitegreg wants to merge 1 commit into
Conversation
389fe02 to
07b614c
Compare
…e it via SetAuthority The RFC-27 verifier public key is the trust root for IP ownership proof validation. Storing it in onchain global state lets it rotate without a program upgrade. The field is appended to GlobalState, so accounts written before it existed deserialize with Pubkey::default() through the same unwrap_or_default() path feed_authority_pk uses. Pubkey::default() means no verifier is configured, which enforcement must treat as a hard reject rather than as a bypass. SetAuthority gains an optional ip_verifier_authority_pk; BorshDeserializeIncremental keeps transactions built against the old argument layout decoding, with the new field defaulting to None. Authorization is unchanged: GLOBALSTATE_ADMIN or the legacy foundation path. The CLI surfaces the key through global-config authority set/get, and the Rust SDK SetAuthority command builder forwards it. SDK deserializer work for Go, TypeScript, and Python is tracked separately; the regenerated global_state fixture already carries the field for that work to land against.
07b614c to
f2eb557
Compare
juan-malbeclabs
left a comment
There was a problem hiding this comment.
Approving — the change is sound and safe to land.
I read every hunk and traced the surrounding code: the GlobalState layout and its TryFrom, process_set_authority, try_acc_write → resize_account_if_needed, the borsh-incremental derive, the SDK fixture readers, and the remaining GlobalState { .. } literals in the tree.
The thing I specifically went looking for — the account growing 32 bytes on the first GlobalState write after the upgrade — is safe. Every processor that calls try_acc_write(&globalstate, ..) (setauthority, setairdrop, setfeatureflags, the four allowlist add/remove, and the five create_* index bumps) takes system_program as a fixed account, so resize_account_if_needed's invoke(transfer) always has it available. The append-only field, the unwrap_or_default() read path, and the BorshDeserializeIncremental instruction path all behave as advertised, and the legacy-vector test is a good way to pin that down.
No correctness bugs. The inline comments are all non-blocking — the first one is the only one I'd actually push for, and it's a three-line guard. They cluster around one theme: the field documents Pubkey::default() as a meaningful sentinel ("no verifier configured → enforcement must hard-reject"), but nothing in the write path, the read path, or the SDK goldens treats it as one yet. Worth closing before RFC-27 enforcement starts depending on that sentinel.
Ran locally: cargo test -p doublezero-serviceability --lib globalstate (9 passed) and --test setauthority_test (2 passed).
| if let Some(ip_verifier_authority_pk) = value.ip_verifier_authority_pk { | ||
| globalstate.ip_verifier_authority_pk = ip_verifier_authority_pk; | ||
| } |
There was a problem hiding this comment.
Some(Pubkey::default()) is accepted here and written straight through, silently clearing the trust root.
The field's own doc comment defines Pubkey::default() as the "no verifier configured, treat as hard reject" sentinel — so an operator who templates an empty or zero pubkey into --ip-verifier-authority disables every IP-verified user connection with no error and no distinguishable onchain signal. Pubkey::from_str("11111111111111111111111111111111") parses cleanly, so the CLI won't catch it either.
Non-blocking, but the guard is cheap and matches the semantics the field already documents:
if let Some(pk) = value.ip_verifier_authority_pk {
if pk == Pubkey::default() {
return Err(DoubleZeroError::InvalidAccountData.into()); // or the closest existing variant
}
globalstate.ip_verifier_authority_pk = pk;
}I'd only guard the new field — retrofitting the other four is a behavior change that doesn't belong in this PR.
| access_authority: gstate.sentinel_authority_pk, | ||
| feed_authority: gstate.feed_authority_pk, | ||
| health_oracle: gstate.health_oracle_pk, | ||
| ip_verifier_authority: gstate.ip_verifier_authority_pk, |
There was a problem hiding this comment.
An unset verifier renders as the literal 11111111111111111111111111111111, which is indistinguishable from a deliberately configured key.
Concrete case: right after the program upgrade but before the first rotation, an operator runs doublezero global-config authority get to confirm "is the verifier set?", sees a valid-looking base58 pubkey, and concludes yes.
Rendering Pubkey::default() as not set (and null under --json) would match the sentinel the field documents. Only this field needs it — the other four have no documented sentinel.
| "typ": "pubkey" | ||
| }, | ||
| { | ||
| "name": "IpVerifierAuthorityPk", |
There was a problem hiding this comment.
This golden now advertises IpVerifierAuthorityPk, but nothing validates it:
- Go's
GlobalStatestruct (smartcontract/sdk/go/serviceability/state.go:74-88) stops atFeedAuthorityPK, andfixture_test.gohas no GlobalState case at all. - The TS harness skips unknown names (
fixtures.test.ts:if (!(f.name in got)) continue;) and Python does the same (test_fixtures.py:if name not in got: continue).
So these bytes can drift or regress undetected. I know the SDK deserializer work is tracked separately — the point here is just that the golden currently has zero coverage for the field it advertises, which is a slightly worse state than not adding it yet. Adding IpVerifierAuthorityPK to the Go struct plus a GlobalState fixture case would close it.
| use test_helpers::*; | ||
|
|
||
| /// Bring up a program instance with an initialized GlobalState. | ||
| async fn init_globalstate() -> ( |
There was a problem hiding this comment.
Two small things on this file, neither blocking:
-
No test writes
GlobalStatestarting from a legacy-sized account, so the 32-byte grow + rent top-up that mainnet will actually take on the first post-upgrade write is never exercised.setup_program_with_globalconfig_contextexposesset_account, so seeding theLEGACY_GLOBALSTATE_VECTORbytes (already defined instate/globalstate.rs's test module) and then runningSetAuthoritywould cover that path directly. That's the one migration step with real blast radius, and right now only the pure-deserialization half of it is tested. -
init_globalstate()here duplicates the InitGlobalState half oftest_helpers::init_globalstate_and_config— worth extracting a shared helper instead.
| fn gs_with_feed(authority: &Pubkey) -> GlobalState { | ||
| GlobalState { | ||
| feed_authority_pk: *authority, | ||
| ip_verifier_authority_pk: Pubkey::default(), |
There was a problem hiding this comment.
Nit: redundant next to ..GlobalState::default() on the following line, which already produces Pubkey::default() for this field. Same at line 527.
Closes #4196. Part of RFC-27 (rfcs/rfc27-ip-verification.md); tracker #4194.
Summary
GlobalStatecarriesip_verifier_authority_pk, the trust root for RFC-27 IP ownership proof validation, so the verifier key rotates without a program upgrade.SetAuthoritygains an optionalip_verifier_authority_pk; authorization is unchanged (GLOBALSTATE_ADMIN permission or the legacy foundation path).doublezero global-config authority set --ip-verifier-authority <pubkey|me>rotates the key, andauthority getreports it in both the table and JSON views.SetAuthorityCommandforwards the new field.Pubkey::default()means "no verifier configured". Enforcement, which lands with the onchain validation issue, must treat that as a hard reject rather than as "any signature passes".Details
The field is append-only, so accounts written before it existed deserialize with
Pubkey::default()through the sameunwrap_or_default()pathfeed_authority_pkuses. On the instruction side,BorshDeserializeIncrementalkeeps transactions built against the old argument layout decoding, with the new field defaulting toNone.SDK deserializer work for Go, TypeScript, and Python is tracked separately. The
global_statefixture is regenerated here because CI'smake check-fixturesdiffs the goldens against the Rust producer; all three readers ignore trailing bytes and skip meta fields they do not map, so they stay green until that work lands. Running the generator also refreshed itsCargo.lockfrom0.31.0to0.36.0.smartcontract/cli/src/init.rsneeded no change: it sendsInitGlobalStateand carries no authority arguments, and the program's initializer seeds the new field toPubkey::default().Testing Verification
tests/setauthority_test.rs: rotating the verifier key leaves the other authorities, the foundation allowlist, and the feature flags untouched; a second rotation replaces the previous key; and a laterSetAuthoritypassingNonedoes not clear it. It also asserts a freshly initializedGlobalStatereadsPubkey::default(), the state enforcement must reject.Pubkey::default(), and re-serialized with the field set it round-trips with every other field identical. Note that this stored vector is old enough that its bytes end atuser_airdrop_lamports, so appending 32 bytes to it would not have exercised the new field.SetAuthorityArgsunit tests: a pre-field four-option encoding still decodes withip_verifier_authority_pk: None, and the new field round-trips.