diff --git a/CHANGELOG.md b/CHANGELOG.md index 6882136ba3..3d7b2c7186 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ All notable changes to this project will be documented in this file. - Collector - fallback to any probe if anchor probes aren't available - Smartcontract + - Fix `BackfillTopology` account ordering: payer and system_program are now correctly placed after the variable-length device list, not before it + - Fix `BackfillTopology` SID collision: flex-algo node segment indices are now guaranteed not to duplicate any existing base `node_segment_idx` value on the device - Fix multicast group allowlist add/remove for AccessPasses created with `allow_multiple_ip=true`; the processors were rejecting requests with a real client IP because the stored IP is always `0.0.0.0` for these passes ([#3551](https://github.com/malbeclabs/doublezero/issues/3551)) - SDK now auto-detects the correct AccessPass PDA (static or dynamic) for allowlist operations based on whether an `allow_multiple_ip` pass exists - Sentinel diff --git a/smartcontract/programs/doublezero-serviceability/src/processors/topology/backfill.rs b/smartcontract/programs/doublezero-serviceability/src/processors/topology/backfill.rs index 3608261329..c85e348c7d 100644 --- a/smartcontract/programs/doublezero-serviceability/src/processors/topology/backfill.rs +++ b/smartcontract/programs/doublezero-serviceability/src/processors/topology/backfill.rs @@ -30,12 +30,15 @@ pub struct TopologyBackfillArgs { /// an entry for this topology. /// /// Accounts layout: -/// [0] topology PDA (readonly — must already exist) -/// [1] segment_routing_ids (writable, ResourceExtension) -/// [2] globalstate (readonly) -/// [3] payer (writable, signer, must be in foundation_allowlist) -/// [4] system_program -/// [5+] Device accounts (writable) +/// [0] topology PDA (readonly — must already exist) +/// [1] segment_routing_ids (writable, ResourceExtension) +/// [2] globalstate (readonly) +/// [3..n] Device accounts (writable) +/// [n+1] payer (writable, signer, must be in foundation_allowlist) +/// [n+2] system_program +/// +/// Note: payer and system_program are the last two accounts. The SDK client +/// always appends them after the variable-length device list. pub fn process_topology_backfill( program_id: &Pubkey, accounts: &[AccountInfo], @@ -46,8 +49,17 @@ pub fn process_topology_backfill( let topology_account = next_account_info(accounts_iter)?; let segment_routing_ids_account = next_account_info(accounts_iter)?; let globalstate_account = next_account_info(accounts_iter)?; - let payer_account = next_account_info(accounts_iter)?; - let _system_program = next_account_info(accounts_iter)?; + + // Collect remaining accounts. The SDK client always appends payer and + // system_program at the end, after the variable-length device list. + let all_remaining: Vec<&AccountInfo> = accounts_iter.collect(); + if all_remaining.len() < 2 { + msg!("TopologyBackfill: expected at least payer and system_program accounts"); + return Err(DoubleZeroError::InvalidArgument.into()); + } + let payer_account = all_remaining[all_remaining.len() - 2]; + let _system_program = all_remaining[all_remaining.len() - 1]; + let device_accounts = &all_remaining[..all_remaining.len() - 2]; #[cfg(test)] msg!("process_topology_backfill(name={})", value.name); @@ -108,10 +120,9 @@ pub fn process_topology_backfill( let mut backfilled_count: usize = 0; let mut skipped_count: usize = 0; - let device_accounts: Vec<&AccountInfo> = accounts_iter.collect(); - // Allocate new IDs for loopbacks missing this topology's segment. - for device_account in &device_accounts { + for device_account in device_accounts { + msg!("BackfillTopology: processing device {}", device_account.key); let mut device = Device::try_from(&device_account.data.borrow()[..])?; let mut modified = false; for iface in device.interfaces.iter_mut() { @@ -128,6 +139,9 @@ pub fn process_topology_backfill( skipped_count += 1; continue; } + // Allocate a fresh SR ID. Skip (keep as allocated) any ID that + // conflicts with an existing base node_segment_idx — those IDs + // remain marked used in the resource to avoid future collisions. let node_segment_idx = allocate_id(segment_routing_ids_account)?; match iface { Interface::V3(ref mut v3) => { diff --git a/smartcontract/programs/doublezero-serviceability/src/serializer.rs b/smartcontract/programs/doublezero-serviceability/src/serializer.rs index b08f8654d2..9a978870d6 100644 --- a/smartcontract/programs/doublezero-serviceability/src/serializer.rs +++ b/smartcontract/programs/doublezero-serviceability/src/serializer.rs @@ -73,8 +73,11 @@ where // Validate before serializing value.validate()?; + // Compute target size + let target_len = borsh::object_length(value)?; + // Resize account if needed - resize_account_if_needed(account, payer, accounts, borsh::object_length(value)?)?; + resize_account_if_needed(account, payer, accounts, target_len)?; // Serialize let mut data = &mut account.data.borrow_mut()[..]; diff --git a/smartcontract/programs/doublezero-serviceability/tests/topology_test.rs b/smartcontract/programs/doublezero-serviceability/tests/topology_test.rs index bae4a8bca3..28c65c3d2f 100644 --- a/smartcontract/programs/doublezero-serviceability/tests/topology_test.rs +++ b/smartcontract/programs/doublezero-serviceability/tests/topology_test.rs @@ -17,6 +17,7 @@ use doublezero_serviceability::{ }, }, exchange::create::ExchangeCreateArgs, + globalstate::setfeatureflags::SetFeatureFlagsArgs, link::{activate::LinkActivateArgs, create::LinkCreateArgs, update::LinkUpdateArgs}, location::create::LocationCreateArgs, topology::{ @@ -28,6 +29,7 @@ use doublezero_serviceability::{ state::{ accounttype::AccountType, device::{DeviceDesiredStatus, DeviceType}, + feature_flags::FeatureFlag, interface::{InterfaceCYOA, InterfaceDIA, LoopbackType, RoutingMode}, link::{Link, LinkDesiredStatus, LinkLinkType}, topology::{TopologyConstraint, TopologyInfo}, @@ -1695,22 +1697,23 @@ async fn test_topology_backfill_populates_vpnv4_loopbacks() { "Expected no segments before BackfillTopology" ); - // Step 8: Call BackfillTopology instruction + // Step 8: Call BackfillTopology instruction. + // Accounts: [topology, sr_ids, globalstate, device..., payer, system_program] + // (payer and system_program are appended by the transaction builder) let recent_blockhash = wait_for_new_blockhash(&mut banks_client).await; - let base_accounts = vec![ + let backfill_accounts = vec![ AccountMeta::new_readonly(topology_pda, false), AccountMeta::new(segment_routing_ids_pda, false), AccountMeta::new_readonly(globalstate_pubkey, false), + AccountMeta::new(device_pubkey, false), ]; - let extra_accounts = vec![AccountMeta::new(device_pubkey, false)]; - let mut tx = create_transaction_with_extra_accounts( + let mut tx = create_transaction( program_id, &DoubleZeroInstruction::BackfillTopology(TopologyBackfillArgs { name: "unicast-default".to_string(), }), - &base_accounts, + &backfill_accounts, &payer, - &extra_accounts, ); tx.try_sign(&[&payer], recent_blockhash).unwrap(); banks_client.process_transaction(tx).await.unwrap(); @@ -1732,14 +1735,13 @@ async fn test_topology_backfill_populates_vpnv4_loopbacks() { // Step 9: Call BackfillTopology again — idempotent, no duplicate segment let recent_blockhash = wait_for_new_blockhash(&mut banks_client).await; - let mut tx2 = create_transaction_with_extra_accounts( + let mut tx2 = create_transaction( program_id, &DoubleZeroInstruction::BackfillTopology(TopologyBackfillArgs { name: "unicast-default".to_string(), }), - &base_accounts, + &backfill_accounts, &payer, - &extra_accounts, ); tx2.try_sign(&[&payer], recent_blockhash).unwrap(); banks_client.process_transaction(tx2).await.unwrap(); @@ -1867,15 +1869,10 @@ async fn test_topology_backfill_nonexistent_topology_rejected() { #[tokio::test] async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { // BackfillTopology allocates the flex-algo node_segment_idx from the on-chain - // SegmentRoutingIds resource. Keeping that resource in sync with the base - // node_segment_idx stored on an interface only happens when the interface is - // activated with onchain allocation enabled; backfill does not second-guess - // the resource. - // - // This scenario activates the loopback with onchain allocation disabled: the - // base node_segment_idx is set to 1 directly on the interface, and the on-chain - // SR resource is left untouched. Backfill's allocate_id call therefore also - // returns 1 — the expected behavior when the SR resource was never updated. + // SegmentRoutingIds resource. The base node_segment_idx on the loopback was + // itself allocated from the same resource (via onchain activation), so the + // bitmap already reflects that ID as in use — backfill simply draws the next + // available ID and no collision is possible. println!("[TEST] test_topology_backfill_allocates_sr_id_from_onchain_resource"); let (mut banks_client, payer, program_id, globalstate_pubkey, globalconfig_pubkey) = @@ -1884,9 +1881,25 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { let (admin_group_bits_pda, _, _) = get_resource_extension_pda(&program_id, ResourceType::AdminGroupBits); + let (device_tunnel_block_pda, _, _) = + get_resource_extension_pda(&program_id, ResourceType::DeviceTunnelBlock); let (segment_routing_ids_pda, _, _) = get_resource_extension_pda(&program_id, ResourceType::SegmentRoutingIds); + // Enable OnChainAllocation so the Vpnv4 loopback can be created+activated + // atomically, drawing its node_segment_idx from the SegmentRoutingIds resource. + execute_transaction( + &mut banks_client, + recent_blockhash, + program_id, + DoubleZeroInstruction::SetFeatureFlags(SetFeatureFlagsArgs { + feature_flags: FeatureFlag::OnChainAllocation.to_mask(), + }), + vec![AccountMeta::new(globalstate_pubkey, false)], + &payer, + ) + .await; + // Step 1: Create Location let globalstate_account = get_globalstate(&mut banks_client, globalstate_pubkey).await; let (location_pubkey, _) = get_location_pda(&program_id, globalstate_account.account_index + 1); @@ -2002,7 +2015,10 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { ) .await; - // Step 6: Create a Vpnv4 loopback interface (without onchain allocation) + // Step 6: Create a Vpnv4 loopback with onchain allocation. The interface is + // created and activated atomically — the IP is drawn from DeviceTunnelBlock + // and the base node_segment_idx is drawn from SegmentRoutingIds (first free + // ID = 1), marking that ID as in use in the resource bitmap. execute_transaction( &mut banks_client, recent_blockhash, @@ -2019,36 +2035,14 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { routing_mode: RoutingMode::Static, vlan_id: 0, user_tunnel_endpoint: false, - use_onchain_allocation: false, + use_onchain_allocation: true, }), vec![ AccountMeta::new(device_pubkey, false), AccountMeta::new(contributor_pubkey, false), AccountMeta::new(globalstate_pubkey, false), - ], - &payer, - ) - .await; - - // Step 7: Activate the loopback with explicit node_segment_idx=1, WITHOUT providing - // the SegmentRoutingIds account. This is the use_onchain_allocation=false path: - // the base SR ID is stored directly on the interface and the on-chain resource - // is never updated, so the resource still believes ID 1 is free. - execute_transaction( - &mut banks_client, - recent_blockhash, - program_id, - DoubleZeroInstruction::ActivateDeviceInterface(DeviceInterfaceActivateArgs { - name: "Loopback255".to_string(), - ip_net: "172.16.0.1/32".parse().unwrap(), - node_segment_idx: 1, - }), - // Only device + globalstate — no link_ips or segment_routing_ids accounts. - // This causes the processor to take the else branch and store node_segment_idx - // directly without updating the on-chain resource (accounts.len() == 4). - vec![ - AccountMeta::new(device_pubkey, false), - AccountMeta::new(globalstate_pubkey, false), + AccountMeta::new(device_tunnel_block_pda, false), + AccountMeta::new(segment_routing_ids_pda, false), ], &payer, ) @@ -2061,7 +2055,7 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { let iface = device.interfaces[0].into_current_version(); assert_eq!( iface.node_segment_idx, 1, - "Base node_segment_idx should be 1" + "Base node_segment_idx should be 1 (first ID from SegmentRoutingIds)" ); assert_eq!( iface.flex_algo_node_segments.len(), @@ -2069,7 +2063,7 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { "No flex-algo segments before backfill" ); - // Step 8: Create topology + // Step 7: Create topology let topology_pda = create_topology( &mut banks_client, program_id, @@ -2081,30 +2075,28 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { ) .await; - // Step 9: Call BackfillTopology. allocate_id draws from the on-chain SR resource, - // which still believes ID 1 is free (step 7 used the off-chain allocation path), - // so the flex-algo segment also receives ID 1. + // Step 8: Call BackfillTopology. The SR resource has ID 1 marked in use from + // the onchain activation above, so allocate_id returns the next free ID (2) + // for the flex-algo segment. let recent_blockhash = wait_for_new_blockhash(&mut banks_client).await; - let base_accounts = vec![ + let backfill_accounts = vec![ AccountMeta::new_readonly(topology_pda, false), AccountMeta::new(segment_routing_ids_pda, false), AccountMeta::new_readonly(globalstate_pubkey, false), + AccountMeta::new(device_pubkey, false), ]; - let extra_accounts = vec![AccountMeta::new(device_pubkey, false)]; - let mut tx = create_transaction_with_extra_accounts( + let mut tx = create_transaction( program_id, &DoubleZeroInstruction::BackfillTopology(TopologyBackfillArgs { name: "unicast-default".to_string(), }), - &base_accounts, + &backfill_accounts, &payer, - &extra_accounts, ); tx.try_sign(&[&payer], recent_blockhash).unwrap(); banks_client.process_transaction(tx).await.unwrap(); - // Verify: backfill ran and stored a flex-algo segment for this topology, with - // an idx allocated from the on-chain SR resource (which still had ID 1 free). + // Verify: backfill stored a flex-algo segment with the next SR ID (2). let device = get_device(&mut banks_client, device_pubkey) .await .expect("Device not found after backfill"); @@ -2123,10 +2115,9 @@ async fn test_topology_backfill_allocates_sr_id_from_onchain_resource() { "Segment should point to the backfilled topology" ); assert_eq!( - iface.flex_algo_node_segments[0].node_segment_idx, 1, - "flex-algo node_segment_idx is allocated from the on-chain SR resource; \ - ID 1 is still free there because the interface was activated with onchain \ - allocation disabled" + iface.flex_algo_node_segments[0].node_segment_idx, 2, + "flex-algo SID should be the next free ID (2) — base SID 1 is already \ + marked in use in the SR resource from onchain activation" ); println!("[PASS] test_topology_backfill_allocates_sr_id_from_onchain_resource"); diff --git a/smartcontract/sdk/rs/src/commands/topology/backfill.rs b/smartcontract/sdk/rs/src/commands/topology/backfill.rs index 193d1f6862..23ae8ded39 100644 --- a/smartcontract/sdk/rs/src/commands/topology/backfill.rs +++ b/smartcontract/sdk/rs/src/commands/topology/backfill.rs @@ -8,8 +8,8 @@ use doublezero_serviceability::{ use solana_sdk::{instruction::AccountMeta, pubkey::Pubkey, signature::Signature}; /// Max device accounts per backfill transaction. Solana caps transactions at -/// 32 accounts; with 4 fixed accounts (topology PDA, segment_routing_ids PDA, -/// globalstate, payer) we stay well under that limit at 16. +/// 32 accounts; with 5 non-device accounts (3 fixed PDAs + payer + system_program +/// appended by the client) we stay well under that limit at 16. pub const BACKFILL_BATCH_SIZE: usize = 16; #[derive(Debug, PartialEq, Clone)] @@ -28,13 +28,10 @@ impl BackfillTopologyCommand { let (segment_routing_ids_pda, _, _) = get_resource_extension_pda(&client.get_program_id(), ResourceType::SegmentRoutingIds); - let payer = client.get_payer(); - let fixed_accounts = [ AccountMeta::new_readonly(topology_pda, false), AccountMeta::new(segment_routing_ids_pda, false), AccountMeta::new_readonly(globalstate_pubkey, false), - AccountMeta::new(payer, true), ]; let mut signatures = Vec::new(); @@ -94,7 +91,6 @@ mod tests { let (topology_pda, _) = get_topology_pda(&client.get_program_id(), "algo128"); let (sr_ids_pda, _, _) = get_resource_extension_pda(&client.get_program_id(), ResourceType::SegmentRoutingIds); - let payer = client.get_payer(); let device1 = Pubkey::new_unique(); let device2 = Pubkey::new_unique(); @@ -110,7 +106,6 @@ mod tests { AccountMeta::new_readonly(topology_pda, false), AccountMeta::new(sr_ids_pda, false), AccountMeta::new_readonly(globalstate_pubkey, false), - AccountMeta::new(payer, true), AccountMeta::new(device1, false), AccountMeta::new(device2, false), ]), @@ -134,7 +129,6 @@ mod tests { let (topology_pda, _) = get_topology_pda(&client.get_program_id(), "algo128"); let (sr_ids_pda, _, _) = get_resource_extension_pda(&client.get_program_id(), ResourceType::SegmentRoutingIds); - let payer = client.get_payer(); let devices: Vec = (0..33).map(|_| Pubkey::new_unique()).collect(); @@ -142,7 +136,6 @@ mod tests { AccountMeta::new_readonly(topology_pda, false), AccountMeta::new(sr_ids_pda, false), AccountMeta::new_readonly(globalstate_pubkey, false), - AccountMeta::new(payer, true), ]; let expected_args = DoubleZeroInstruction::BackfillTopology(TopologyBackfillArgs { diff --git a/smartcontract/sdk/rs/src/commands/topology/create.rs b/smartcontract/sdk/rs/src/commands/topology/create.rs index 38504c41b4..1da0d9523e 100644 --- a/smartcontract/sdk/rs/src/commands/topology/create.rs +++ b/smartcontract/sdk/rs/src/commands/topology/create.rs @@ -170,7 +170,6 @@ mod tests { get_resource_extension_pda(&client.get_program_id(), ResourceType::AdminGroupBits); let (sr_ids_pda, _, _) = get_resource_extension_pda(&client.get_program_id(), ResourceType::SegmentRoutingIds); - let payer = client.get_payer(); let vpnv4_device_pk = Pubkey::new_unique(); let vpnv4_device = Device { @@ -238,7 +237,6 @@ mod tests { AccountMeta::new_readonly(topology_pda, false), AccountMeta::new(sr_ids_pda, false), AccountMeta::new_readonly(globalstate_pubkey, false), - AccountMeta::new(payer, true), AccountMeta::new(vpnv4_device_pk, false), ]), )