serviceability: tell an IP conflict apart from an already-subscribed user - #4159
serviceability: tell an IP conflict apart from an already-subscribed user#4159Jared-dz wants to merge 2 commits into
Conversation
…ibed user CreateUser and CreateSubscribeUser both answered AccountAlreadyInitialized for two refusals that share neither cause nor remedy: - a user already exists at the requested client IP with a DIFFERENT device, owner, type or tenant. The User PDA is derived from (client_ip, user_type) with no device dimension, so this is what two devices claiming one IP looks like, and the caller has to pick another IP. - CreateSubscribeUser matched an existing user EXACTLY. The subscription is already in place, so there is nothing for the caller to change. It is an error rather than a no-op only because falling through would tick a second feed seat and push a duplicate feed_pks entry that delete would double-release. One code for both meant a client could not tell which it had hit, so it could not tell the user which. They are now UserExistsWithDifferentAttributes (Custom(105)) and SubscribeUserAlreadyExists (Custom(106)), appended to the end of DoubleZeroError so no existing code shifts. The IP-uniqueness invariant is now written where it lives — the get_user_pda seed derivation — since it is a property of the address, not a check somewhere, and adding a device dimension there would silently drop it. Adds the missing test for the duplicate-subscribe branch, which had no coverage at all. Its retry flips `publisher` deliberately: an identical instruction compiles to identical transaction bytes against the same blockhash, so the runtime dedupes it by signature and returns the first transaction's success without reaching the program. create_user_core does not compare that flag, so the request still matches exactly and still lands in the duplicate branch.
|
Tracked in malbeclabs/infra#2198, part of the upstream-gap set in malbeclabs/infra#2200 (board 59). |
The assertions inlined 105 and 106, so renumbering the enum would leave them passing against codes the program no longer returns. A named constant would only move the problem: the wire code comes from the match in `impl From<DoubleZeroError> for ProgramError`, so anything written by hand can drift from it. `custom_code` in test_helpers derives the code through that conversion. Casting the variant would be wrong here — `DoubleZeroError` has no `#[repr(u32)]` and declaration order does not match the mapping: `InvalidExchangePubkey` is the third variant declared but maps to `Custom(3)`, while `InvalidLocationPubkey`, declared fifth, maps to `Custom(2)`. `as u32` would silently produce a wrong code, so the doc comment records that. All five sites are match arms, and a pattern cannot hold a computed value, so the code is bound and compared in a guard — the shape `assert_custom_error` in accesspass_dzf_locked.rs already uses. The tests passing is what proves the derivation: a wrong mapping fails the guard and falls to the panic arm. Also cites the PR number in the CHANGELOG entry alongside the infra ticket, as the neighbouring entries do.
|
Pushed The assertions inlined 105 and 106. The testing rules ask for the code to be derived from the error enum so renumbering cannot silently rot a test. A named
All five sites are match arms and a pattern cannot hold a computed value, so the code is bound and compared in a guard — the shape The CHANGELOG cited only the infra ticket. Now Still 50 test binaries green; clippy clean on |
nikw9944
left a comment
nikw9944
left a comment
There was a problem hiding this comment.
One thing to change before merge: SubscribeUserAlreadyExists asserts a subscription state the program never checks, and the changelog tells clients to treat it as a no-op — which will make a real subscribe get silently dropped. Two small follow-ons: a sentinel comment that still describes an unreachable path, and the Go SDK error-name table that the PR description says doesn't exist.
- An SDK error-name table does exist, contrary to the PR description:
programErrorsinsmartcontract/sdk/go/serviceability/errors.go:103maps codes to names for the operator-facing log lines inexecutor.go:118. It stops at 90, so 91–104 already print as "unknown error code N" and 105/106 would join them. Adding the two entries (ideally backfilling 91–104) carries this PR's diagnosability goal through to Go callers.
| "A user already exists at this client IP with a different device, owner, type or tenant" | ||
| )] | ||
| UserExistsWithDifferentAttributes, // variant 105 | ||
| #[error("This user is already subscribed to the requested multicast group")] |
There was a problem hiding this comment.
The error tells the caller they are already subscribed to the group they asked for, but the code never checks group membership — it fires whenever any matching user account exists, so a user subscribed to nothing, or to a different group, is told they are already subscribed. create_user_core takes no mgroup account; its Ok(None) (create_core.rs:240) compares only owner, device_pk, user_type and tenant_pk, and create_subscribe.rs:147 turns that into this error. Behavior is unchanged, but CHANGELOG.md:10 tells clients it means "there is nothing to do", so one that follows it silently drops a real subscribe.
#[error("A user already exists at this client IP; change group roles with UpdateMulticastGroupRoles or SubscribeFeed")]
UserAlreadyExists, // variant 106Worth a test hitting CreateSubscribeUser with a different group than the existing user holds.
| // the atomic create+allocate+activate path. Without those accounts the user is created | ||
| // Pending and never reaches Activated, so the next poll cycle would re-attempt creation | ||
| // and trip AccountAlreadyInitialized. | ||
| // and trip SubscribeUserAlreadyExists. |
There was a problem hiding this comment.
The comment describes a fallback that cannot happen: a zero dz_prefix_count does not create a Pending user, it makes the whole instruction fail. The program rejects dz_prefix_count == 0 up front at create_subscribe.rs:71, and the count comes from device.dz_prefixes.len() (dz_ledger_reader.rs:313), so a device with no prefixes hits that hard failure rather than a duplicate on the next cycle.
2 participants
CreateUserandCreateSubscribeUserboth answeredAccountAlreadyInitializedfor two refusals that share neither cause nor remedy, so a client could not tell which it had hit — and therefore could not tell the user which.(client_ip, user_type)with no device dimension, so this is what two devices claiming one IP looks like — the second derives the first one's accountUserExistsWithDifferentAttributes—Custom(105)CreateSubscribeUsermatched an existing user exactlySubscribeUserAlreadyExists—Custom(106)The second is an error rather than an idempotent no-op (which is what
CreateUserdoes) only because falling through would tick a second feed seat and push a duplicatefeed_pksentry that delete would then double-release.Both variants are appended to the end of
DoubleZeroError, so no existing code shifts.Not a seed change
The obvious-looking fix — adding a device dimension to the User PDA — is not viable in one PR: RFC-10's compatibility window forbids writing a new address space in the release that adds it, it would orphan every live
User, and it would require regenerating the Go/Python/TS fixtures. It would also remove the IP-uniqueness guarantee rather than fix anything, by giving each device its own account. So this PR makes the existing behavior diagnosable and writes the invariant down atget_user_pda, where it actually lives — it is a property of the seed derivation, not a check somewhere that could be moved.Tests
different_device/different_tenant/different_owner) now pinCustom(105).feed_subscription_test.rs's duplicate-subscribe test pinsCustom(106).test_create_subscribe_user_duplicate_is_refused_distinctly— that branch had no coverage at all. It also asserts the subscription is untouched afterwards: one seat, not two.That new test's retry flips
publisheron purpose, and the comment says why: an identical instruction compiles to identical transaction bytes against the same blockhash, so the runtime dedupes by signature and hands back the first transaction's success without ever reaching the program — the test passed while proving nothing until this was fixed.create_user_corecompares owner, device, user type and tenant but not the publisher/subscriber flags, so the request still matches exactly and still lands in the duplicate branch.Verification
cargo test -p doublezero-serviceability— 50 test binaries, all green (301 unit + the integration suites; these run natively viaprocessor!, no SBF build needed).cargo clippy --all-targetsclean ondoublezero-serviceabilityanddoublezero-sentinel;cargo fmtapplied.No SDK error tables mirror these codes, so nothing was regenerated. One stale comment in
crates/sentinel/src/dz_ledger_writer.rsnamed the old error for this exact path and is corrected.### BreakingCHANGELOG entry included, since the wire code changes for both paths.Coordination: #4120 also touches
create_subscribe.rs. This only changes theelsebranch's error value, so it should be a small hand-merge either way — happy to rebase after it lands.