diff --git a/CHANGELOG.md b/CHANGELOG.md index 87af8fb96b..1abd42ffd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ All notable changes to this project will be documented in this file. ### Breaking +- Serviceability + - User creation no longer answers two unrelated refusals with the same error. `CreateUser` and `CreateSubscribeUser` both returned `AccountAlreadyInitialized` for conditions that share neither cause nor remedy: a user already existing at the requested client IP with a **different** device, owner, type or tenant, versus a `CreateSubscribeUser` that matched an existing user **exactly**. The first is what two devices claiming one IP looks like — the User PDA is derived from `(client_ip, user_type)` with no device dimension, so the second device derives the first one's account — and the caller has to pick another IP. The second means the subscription is already in place and there is nothing to do. A client seeing one code could not tell which it had, so it could not say which. They are now `UserExistsWithDifferentAttributes` (`Custom(105)`) and `SubscribeUserAlreadyExists` (`Custom(106)`); both variants are appended to the end of `DoubleZeroError`, so no existing code shifts. Anything matching `AccountAlreadyInitialized` on either path needs updating. (malbeclabs/infra#2171, #4159) + ### Changes - CLI diff --git a/crates/sentinel/src/dz_ledger_writer.rs b/crates/sentinel/src/dz_ledger_writer.rs index f64aa7a858..1c2a2b5feb 100644 --- a/crates/sentinel/src/dz_ledger_writer.rs +++ b/crates/sentinel/src/dz_ledger_writer.rs @@ -91,7 +91,7 @@ pub fn build_create_multicast_publisher_instructions( // When dz_prefix_count > 0 we append the ResourceExtension accounts so the contract takes // 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. let (user_pda, _) = get_user_pda(program_id, &user.client_ip, SvcUserType::Multicast); let mut create_user_accounts = vec![ AccountMeta::new(user_pda, false), diff --git a/smartcontract/programs/doublezero-serviceability/src/error.rs b/smartcontract/programs/doublezero-serviceability/src/error.rs index 74d9564ba0..ca894b9b92 100644 --- a/smartcontract/programs/doublezero-serviceability/src/error.rs +++ b/smartcontract/programs/doublezero-serviceability/src/error.rs @@ -216,6 +216,12 @@ pub enum DoubleZeroError { UserFeedLimitExceeded, // variant 103 #[error("An EdgeSeat feed seat is only held by a Multicast user")] EdgeSeatIsMulticastOnly, // variant 104 + #[error( + "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")] + SubscribeUserAlreadyExists, // variant 106 } impl From for ProgramError { @@ -326,6 +332,8 @@ impl From for ProgramError { DoubleZeroError::UserDeviceMismatch => ProgramError::Custom(102), DoubleZeroError::UserFeedLimitExceeded => ProgramError::Custom(103), DoubleZeroError::EdgeSeatIsMulticastOnly => ProgramError::Custom(104), + DoubleZeroError::UserExistsWithDifferentAttributes => ProgramError::Custom(105), + DoubleZeroError::SubscribeUserAlreadyExists => ProgramError::Custom(106), } } } @@ -437,6 +445,8 @@ impl From for DoubleZeroError { 102 => DoubleZeroError::UserDeviceMismatch, 103 => DoubleZeroError::UserFeedLimitExceeded, 104 => DoubleZeroError::EdgeSeatIsMulticastOnly, + 105 => DoubleZeroError::UserExistsWithDifferentAttributes, + 106 => DoubleZeroError::SubscribeUserAlreadyExists, _ => DoubleZeroError::Custom(e), } } diff --git a/smartcontract/programs/doublezero-serviceability/src/pda.rs b/smartcontract/programs/doublezero-serviceability/src/pda.rs index 2009e6ad06..e01323056e 100644 --- a/smartcontract/programs/doublezero-serviceability/src/pda.rs +++ b/smartcontract/programs/doublezero-serviceability/src/pda.rs @@ -55,6 +55,14 @@ pub fn get_user_old_pda(program_id: &Pubkey, index: u128) -> (Pubkey, u8) { Pubkey::find_program_address(&[SEED_PREFIX, SEED_USER, &index.to_le_bytes()], program_id) } +/// The seeds carry the client IP and user type but no device, so a `(ip, user_type)` pair +/// addresses exactly one User account network-wide. Two devices asking for the same client IP +/// therefore derive the same address, and the second one fails in `create_user_core` with +/// `UserExistsWithDifferentAttributes` rather than getting its own account. +/// +/// That IP uniqueness is a property of this derivation, not a check somewhere: adding a device +/// dimension here would give each device its own account and silently drop the guarantee, and it +/// would orphan every live User, so it is not a change to make casually. pub fn get_user_pda(program_id: &Pubkey, ip: &Ipv4Addr, user_type: UserType) -> (Pubkey, u8) { Pubkey::find_program_address( &[SEED_PREFIX, SEED_USER, &ip.octets(), &[user_type as u8]], diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs b/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs index b4a974419a..128ae5fc55 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/user/create_core.rs @@ -58,8 +58,8 @@ pub struct CreateUserCoreResult { /// device validation, max users checks, epoch check) and sets up the initial User struct. /// /// Returns `Ok(None)` when the user already exists and matches the requested owner, device, -/// user type, and tenant; a mismatch errors with `AccountAlreadyInitialized`, and a banned user -/// with `InvalidStatus`. +/// user type, and tenant; a mismatch errors with `UserExistsWithDifferentAttributes`, and a +/// banned user with `InvalidStatus`. /// /// Callers are responsible for: /// - Parsing the required resource extension accounts @@ -224,7 +224,12 @@ pub fn create_user_core( user_type, requested_tenant ); - return Err(ProgramError::AccountAlreadyInitialized); + // The User PDA is derived from the client IP, so the common way to reach this is two + // devices claiming one IP: the second request derives the first device's account and + // mismatches on device_pk. Distinct from the exact-duplicate case in + // create_subscribe, which used to share this error code and left a client unable to + // tell "that IP belongs to another device" from "you are already subscribed". + return Err(DoubleZeroError::UserExistsWithDifferentAttributes.into()); } // A ban is terminal; fail fast instead of leaving the caller polling a user that will // never activate. diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/user/create_subscribe.rs b/smartcontract/programs/doublezero-serviceability/src/processors/user/create_subscribe.rs index 9842aa066c..1f06de2cd4 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/user/create_subscribe.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/user/create_subscribe.rs @@ -12,7 +12,6 @@ use solana_program::{ account_info::{next_account_info, AccountInfo}, entrypoint::ProgramResult, msg, - program_error::ProgramError, pubkey::Pubkey, }; use std::net::Ipv4Addr; @@ -141,8 +140,11 @@ pub fn process_create_subscribe_user( )? else { // A duplicate is an error here, not a no-op: falling through would tick a second feed - // seat and push a duplicate feed_pks entry that delete would double-release. - return Err(ProgramError::AccountAlreadyInitialized); + // seat and push a duplicate feed_pks entry that delete would double-release. Named + // distinctly from the mismatch case in create_user_core, which shares neither cause nor + // remedy: this request matched an existing user exactly, so the subscription is already + // in place and the caller has nothing to change. + return Err(DoubleZeroError::SubscribeUserAlreadyExists.into()); }; // EdgeSeat multicast metro gate: the group must be joinable via a feed on the pass serving the diff --git a/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs b/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs index 71acd66f78..dfa1be6dbb 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/create_subscribe_user_test.rs @@ -48,7 +48,12 @@ use doublezero_serviceability::{ }, }; use solana_program_test::*; -use solana_sdk::{instruction::AccountMeta, pubkey::Pubkey, signature::Signer}; +use solana_sdk::{ + instruction::{AccountMeta, InstructionError}, + pubkey::Pubkey, + signature::Signer, + transaction::TransactionError, +}; use std::net::Ipv4Addr; mod test_helpers; @@ -2543,3 +2548,114 @@ async fn test_publisher_disconnect_delete_decrements_publishers_count() { "subscribers_count must NOT change — user was created as publisher" ); } + +// ============================================================================ +// Duplicate subscribe +// ============================================================================ + +/// Re-sending an identical CreateSubscribeUser is refused, and with its own error code. +/// +/// For CreateUser an exact match is an idempotent no-op, but here falling through would tick a +/// second feed seat and push a duplicate feed_pks entry that delete would double-release. Both +/// this and the attribute-mismatch case in create_user_core used to return +/// AccountAlreadyInitialized, which left a caller unable to tell "you are already subscribed" +/// (nothing to do) from "that client IP belongs to another device" (pick another IP) — different +/// causes with different remedies. This pins the codes apart. +#[tokio::test] +async fn test_create_subscribe_user_duplicate_is_refused_distinctly() { + let client_ip = [100, 0, 0, 9]; + let f = setup_create_subscribe_fixture(client_ip).await; + let CreateSubscribeFixture { + mut banks_client, + payer, + program_id, + globalstate_pubkey, + device_pubkey, + accesspass_pubkey, + mgroup_pubkey, + user_ip, + user_tunnel_block, + multicast_publisher_block, + tunnel_ids, + dz_prefix_block, + .. + } = f; + + let (user_pubkey, _) = get_user_pda(&program_id, &user_ip, UserType::Multicast); + let args = UserCreateSubscribeArgs { + user_type: UserType::Multicast, + cyoa_type: UserCYOA::GREOverDIA, + client_ip: user_ip, + publisher: false, + subscriber: true, + tunnel_endpoint: Ipv4Addr::UNSPECIFIED, + dz_prefix_count: 1, + owner: Pubkey::default(), + }; + let accounts = vec![ + AccountMeta::new(user_pubkey, false), + AccountMeta::new(device_pubkey, false), + AccountMeta::new(mgroup_pubkey, false), + AccountMeta::new(accesspass_pubkey, false), + AccountMeta::new(globalstate_pubkey, false), + AccountMeta::new(user_tunnel_block, false), + AccountMeta::new(multicast_publisher_block, false), + AccountMeta::new(tunnel_ids, false), + AccountMeta::new(dz_prefix_block, false), + ]; + + let recent_blockhash = banks_client.get_latest_blockhash().await.unwrap(); + execute_transaction( + &mut banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::CreateSubscribeUser(args.clone()), + accounts.clone(), + &payer, + ) + .await; + + // The retry flips `publisher`, for a reason worth stating: an instruction identical to the + // first would compile to identical transaction bytes against the same blockhash, so the + // runtime would dedupe it by signature and hand back the first one's success without ever + // reaching the program. `create_user_core` compares owner, device, user type and tenant — + // not the publisher/subscriber flags — so this still matches an existing user exactly and + // still lands in the duplicate branch, while being a distinct transaction. + let recent_blockhash = banks_client.get_latest_blockhash().await.unwrap(); + let err = execute_transaction_expect_failure( + &mut banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::CreateSubscribeUser(UserCreateSubscribeArgs { + publisher: true, + ..args + }), + accounts, + &payer, + ) + .await + .expect_err("a duplicate subscribe must be refused"); + + match err { + BanksClientError::TransactionError(TransactionError::InstructionError( + 0, + // SubscribeUserAlreadyExists, not the mismatch code. + InstructionError::Custom(code), + )) if code == custom_code(DoubleZeroError::SubscribeUserAlreadyExists) => {} + other => panic!("expected SubscribeUserAlreadyExists, got {other:?}"), + } + + // The subscription is untouched: exactly one seat, not two. + let mgroup = get_account_data(&mut banks_client, mgroup_pubkey) + .await + .expect("MulticastGroup should exist") + .get_multicastgroup() + .unwrap(); + assert_eq!(mgroup.subscriber_count, 1); + let user = get_account_data(&mut banks_client, user_pubkey) + .await + .expect("User should exist") + .get_user() + .unwrap(); + assert_eq!(user.subscribers, vec![mgroup_pubkey]); +} diff --git a/smartcontract/programs/doublezero-serviceability/tests/feed_subscription_test.rs b/smartcontract/programs/doublezero-serviceability/tests/feed_subscription_test.rs index f75dd82303..a71f500941 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/feed_subscription_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/feed_subscription_test.rs @@ -1125,9 +1125,11 @@ async fn test_duplicate_create_subscribe_user_rejected() { match err { BanksClientError::TransactionError(TransactionError::InstructionError( 0, - InstructionError::AccountAlreadyInitialized, - )) => {} - other => panic!("expected AccountAlreadyInitialized, got {other:?}"), + // SubscribeUserAlreadyExists: this request matched an existing user exactly, unlike + // UserExistsWithDifferentAttributes, which means the IP belongs to another device. + InstructionError::Custom(code), + )) if code == custom_code(DoubleZeroError::SubscribeUserAlreadyExists) => {} + other => panic!("expected SubscribeUserAlreadyExists, got {other:?}"), } assert_eq!(seat_users(&read_pass(&mut f).await, &feed), 1); diff --git a/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs b/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs index 0a427bbba8..c7a0108544 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/test_helpers.rs @@ -1,4 +1,6 @@ use borsh::to_vec; +// Re-exported so the glob that pulls in `custom_code` also pulls in its argument type. +pub use doublezero_serviceability::error::DoubleZeroError; use doublezero_serviceability::{ entrypoint::process_instruction, instructions::*, @@ -21,6 +23,7 @@ use doublezero_serviceability::{ use solana_program_test::*; use solana_sdk::{ instruction::{AccountMeta, Instruction}, + program_error::ProgramError, pubkey::Pubkey, signature::{Keypair, Signer}, transaction::Transaction, @@ -62,6 +65,23 @@ pub fn test_payer() -> Keypair { Keypair::try_from(&TEST_PAYER_BYTES[..]).unwrap() } +/// The `Custom` code a `DoubleZeroError` reaches a client as. +/// +/// Derived through the conversion rather than written down, so a test cannot +/// drift from `impl From for ProgramError`. Casting the variant +/// would be wrong here: `DoubleZeroError` has no `#[repr(u32)]`, and its codes +/// come from that impl's match rather than from declaration order. The two +/// disagree — `InvalidExchangePubkey` is the third variant declared but maps to +/// `Custom(3)` while `InvalidLocationPubkey`, declared fifth, maps to +/// `Custom(2)` — so `as u32` would silently produce the wrong code. +#[allow(dead_code)] +pub fn custom_code(error: DoubleZeroError) -> u32 { + match ProgramError::from(error.clone()) { + ProgramError::Custom(code) => code, + other => panic!("{error:?} does not map to a custom error code: {other:?}"), + } +} + #[allow(dead_code)] pub async fn init_test() -> (BanksClient, Pubkey, Keypair, solana_program::hash::Hash) { let program_id = Pubkey::new_unique(); diff --git a/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs b/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs index b689380191..abfc8ca0a8 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/user_tests.rs @@ -1672,9 +1672,11 @@ async fn test_user_create_existing_different_device_rejected() { match err { BanksClientError::TransactionError(TransactionError::InstructionError( 0, - InstructionError::AccountAlreadyInitialized, - )) => {} - other => panic!("expected AccountAlreadyInitialized, got {other:?}"), + // Distinct from the exact-duplicate subscribe case. A pattern cannot + // hold a computed value, so the code is bound and compared. + InstructionError::Custom(code), + )) if code == custom_code(DoubleZeroError::UserExistsWithDifferentAttributes) => {} + other => panic!("expected UserExistsWithDifferentAttributes, got {other:?}"), } // The user still points at the original device. @@ -1771,9 +1773,11 @@ async fn test_user_create_existing_different_tenant_rejected() { match err { BanksClientError::TransactionError(TransactionError::InstructionError( 0, - InstructionError::AccountAlreadyInitialized, - )) => {} - other => panic!("expected AccountAlreadyInitialized, got {other:?}"), + // Distinct from the exact-duplicate subscribe case. A pattern cannot + // hold a computed value, so the code is bound and compared. + InstructionError::Custom(code), + )) if code == custom_code(DoubleZeroError::UserExistsWithDifferentAttributes) => {} + other => panic!("expected UserExistsWithDifferentAttributes, got {other:?}"), } // The user still has no tenant. @@ -1873,9 +1877,11 @@ async fn test_user_create_existing_different_owner_rejected() { match err { BanksClientError::TransactionError(TransactionError::InstructionError( 0, - InstructionError::AccountAlreadyInitialized, - )) => {} - other => panic!("expected AccountAlreadyInitialized, got {other:?}"), + // Distinct from the exact-duplicate subscribe case. A pattern cannot + // hold a computed value, so the code is bound and compared. + InstructionError::Custom(code), + )) if code == custom_code(DoubleZeroError::UserExistsWithDifferentAttributes) => {} + other => panic!("expected UserExistsWithDifferentAttributes, got {other:?}"), } // The user still belongs to the original owner.