diff --git a/crates/rpc/src/server/api.rs b/crates/rpc/src/server/api.rs index 1308dc7616..9194460ab3 100644 --- a/crates/rpc/src/server/api.rs +++ b/crates/rpc/src/server/api.rs @@ -291,7 +291,8 @@ fn database_error_to_status(err: &DatabaseError) -> Status { DatabaseError::AccountNotFoundInDb(_) | DatabaseError::AccountsNotFoundInDb(_) | DatabaseError::AccountNotPublic(_) => Status::not_found(message), - DatabaseError::TransactionPageExceedsPayloadLimit { .. } => Status::out_of_range(message), + DatabaseError::TransactionPageExceedsPayloadLimit { .. } + | DatabaseError::AccountSyncPageExceedsPayloadLimit { .. } => Status::out_of_range(message), DatabaseError::RangeBeyondTip(_) => Status::invalid_argument(message), _ => Status::internal(message), } diff --git a/crates/store/src/db/mod.rs b/crates/store/src/db/mod.rs index f27ab22292..8dd62d9743 100644 --- a/crates/store/src/db/mod.rs +++ b/crates/store/src/db/mod.rs @@ -685,23 +685,24 @@ impl Db { let mut block_range_start = BlockNumber::GENESIS; let entries_limit = entries_limit.unwrap_or_else(default_storage_map_entries_limit); - let mut page = self + let mut page = match self .select_storage_map_sync_values( account_id, block_num.range_from(block_range_start), Some(entries_limit), ) - .await?; + .await + { + Ok(page) => page, + Err(DatabaseError::AccountSyncPageExceedsPayloadLimit { .. }) => { + return Ok(AccountStorageMapDetails::limit_exceeded(slot_name)); + }, + Err(err) => return Err(err), + }; values.extend(page.values); let mut last_block_included = page.last_block_included; - // If the first page returned no values, the block at block_range_start has more entries - // than the limit allows (e.g. genesis accounts with large storage maps). - if values.is_empty() && last_block_included == block_range_start { - return Ok(AccountStorageMapDetails::limit_exceeded(slot_name)); - } - loop { if page.last_block_included == *block_num || page.last_block_included < block_range_start @@ -710,13 +711,20 @@ impl Db { } block_range_start = page.last_block_included.child(); - page = self + page = match self .select_storage_map_sync_values( account_id, block_num.range_from(block_range_start), Some(entries_limit), ) - .await?; + .await + { + Ok(page) => page, + Err(DatabaseError::AccountSyncPageExceedsPayloadLimit { .. }) => { + return Ok(AccountStorageMapDetails::limit_exceeded(slot_name)); + }, + Err(err) => return Err(err), + }; if page.last_block_included <= last_block_included { return Ok(AccountStorageMapDetails::limit_exceeded(slot_name)); diff --git a/crates/store/src/db/models/queries/accounts.rs b/crates/store/src/db/models/queries/accounts.rs index 996d3d183e..0dd8a79f8e 100644 --- a/crates/store/src/db/models/queries/accounts.rs +++ b/crates/store/src/db/models/queries/accounts.rs @@ -515,12 +515,23 @@ pub(crate) fn select_account_vault_assets( account_id: AccountId, block_range: RangeInclusive, ) -> Result<(BlockNumber, Vec), DatabaseError> { - use schema::account_vault_assets as t; // The protocol does not define these limits. Derive a conservative row limit from the response // payload limit. const ROW_OVERHEAD_BYTES: usize = 2 * size_of::() + size_of::(); // key + asset + block_num const MAX_ROWS: usize = MAX_RESPONSE_PAYLOAD_BYTES / ROW_OVERHEAD_BYTES; + select_account_vault_assets_with_row_limit(conn, account_id, block_range, MAX_ROWS) +} + +/// Like [`select_account_vault_assets`], but with an explicit row cap for pagination tests. +pub(crate) fn select_account_vault_assets_with_row_limit( + conn: &mut SqliteConnection, + account_id: AccountId, + block_range: RangeInclusive, + max_rows: usize, +) -> Result<(BlockNumber, Vec), DatabaseError> { + use schema::account_vault_assets as t; + if !account_id.is_public() { return Err(DatabaseError::AccountNotPublic(account_id)); } @@ -541,13 +552,13 @@ pub(crate) fn select_account_vault_assets( .and(t::block_num.le(block_range.end().to_raw_sql())), ) .order(t::block_num.asc()) - .limit(i64::try_from(MAX_ROWS + 1).expect("should fit within i64")) + .limit(i64::try_from(max_rows + 1).expect("should fit within i64")) .load::<(i64, Vec, Option>)>(conn)?; // If we got more rows than the limit, the last block may be incomplete so we drop it entirely // and derive last_block_included from the remaining rows. let (last_block_included, values) = if let Some(&(last_block_num, ..)) = raw.last() - && raw.len() > MAX_ROWS + && raw.len() > max_rows { let values = raw .into_iter() @@ -555,7 +566,9 @@ pub(crate) fn select_account_vault_assets( .map(AccountVaultValue::from_raw_row) .collect::, DatabaseError>>()?; - let last_block_included = values.last().map_or(*block_range.start(), |v| v.block_num); + ensure_account_sync_page_made_progress(last_block_num, &values)?; + + let last_block_included = values.last().expect("non-empty after progress check").block_num; (last_block_included, values) } else { @@ -568,6 +581,20 @@ pub(crate) fn select_account_vault_assets( Ok((last_block_included, values)) } +/// Returns an error when block-based pagination drops every row in the overflow block. +fn ensure_account_sync_page_made_progress( + truncation_block_num: i64, + values: &[T], +) -> Result<(), DatabaseError> { + if values.is_empty() { + return Err(DatabaseError::AccountSyncPageExceedsPayloadLimit { + block_num: BlockNumber::from_raw_sql(truncation_block_num)?, + }); + } + + Ok(()) +} + /// Query vault assets at a specific block by finding the most recent update for each `vault_key`. /// /// Selects, per vault key, the row whose validity interval covers `block_num`: @@ -777,7 +804,9 @@ pub(crate) fn select_account_storage_map_values_paged( .map(StorageMapValue::from_raw_row) .collect::, DatabaseError>>()?; - let last_block_included = values.last().map_or(*block_range.start(), |v| v.block_num); + ensure_account_sync_page_made_progress(last_block_num, &values)?; + + let last_block_included = values.last().expect("non-empty after progress check").block_num; (last_block_included, values) } else { diff --git a/crates/store/src/db/tests.rs b/crates/store/src/db/tests.rs index 2fecd6ae12..bc20d2fc51 100644 --- a/crates/store/src/db/tests.rs +++ b/crates/store/src/db/tests.rs @@ -1417,7 +1417,7 @@ fn select_storage_map_sync_values_all_entries_in_genesis_block() { .unwrap(); // Insert 3 entries, all in genesis block - for i in 0..3 { + for i in 0..3u32 { queries::insert_account_storage_map_value( &mut conn, account_id, @@ -1430,8 +1430,7 @@ fn select_storage_map_sync_values_all_entries_in_genesis_block() { } // Query with limit=1 so that raw.len() (3) > limit (1), triggering the pagination branch. All - // entries are in block 0, so take_while produces nothing and last_block_num.saturating_sub(1) = - // -1. + // entries are in block 0, so pagination cannot make progress. let result = queries::select_account_storage_map_values_paged( &mut conn, account_id, @@ -1439,19 +1438,14 @@ fn select_storage_map_sync_values_all_entries_in_genesis_block() { 1, ); - // Should not error - should return a valid page (possibly with empty values indicating no - // progress, which the caller interprets as limit_exceeded) - let page = result.expect("should not return an internal error for genesis block entries"); - // The page should indicate no progress was made (stuck at genesis) - assert!( - page.values.is_empty() || page.last_block_included == genesis, - "should indicate pagination did not make progress" + assert_matches!( + result, + Err(crate::errors::DatabaseError::AccountSyncPageExceedsPayloadLimit { block_num }) + if block_num == genesis ); } -/// Tests that single-block overflow works for non-genesis blocks too. All entries are in block 5 -/// and exceed the limit. The function should signal no progress rather than returning incorrect -/// data. +/// Tests that single-block overflow returns an error instead of silently reporting no progress. #[test] fn select_storage_map_sync_values_all_entries_in_single_non_genesis_block() { let mut conn = create_db(); @@ -1469,7 +1463,7 @@ fn select_storage_map_sync_values_all_entries_in_single_non_genesis_block() { ) .unwrap(); - for i in 0..3 { + for i in 0..3u32 { queries::insert_account_storage_map_value( &mut conn, account_id, @@ -1482,12 +1476,123 @@ fn select_storage_map_sync_values_all_entries_in_single_non_genesis_block() { } // limit=1, so 3 rows > 1 triggers pagination. All in block 5. - let page = - queries::select_account_storage_map_values_paged(&mut conn, account_id, block5..=block5, 1) + let result = + queries::select_account_storage_map_values_paged(&mut conn, account_id, block5..=block5, 1); + + assert_matches!( + result, + Err(crate::errors::DatabaseError::AccountSyncPageExceedsPayloadLimit { block_num }) + if block_num == block5 + ); +} + +/// Tests that vault sync pagination rejects single-block overflow the same way as storage maps. +#[test] +fn select_account_vault_sync_values_all_entries_in_single_block() { + use miden_protocol::asset::{Asset, FungibleAsset}; + + let mut conn = create_db(); + let account_id = AccountId::try_from(ACCOUNT_ID_REGULAR_PUBLIC_ACCOUNT_IMMUTABLE_CODE).unwrap(); + // Distinct faucet IDs → distinct vault keys; same faucet would UNIQUE-collide in one block. + let faucet_ids = [ + AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET).unwrap(), + AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET_1).unwrap(), + AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET_2).unwrap(), + ]; + + let block5 = BlockNumber::from(5); + create_block(&mut conn, block5); + + queries::upsert_accounts( + &mut conn, + &[mock_block_account_update(account_id, 0)], + block5, + &queries::PrecomputedPublicAccountStates::new(), + ) + .unwrap(); + + for (i, faucet_id) in faucet_ids.into_iter().enumerate() { + let asset = Asset::Fungible(FungibleAsset::new(faucet_id, (i as u64) + 1).unwrap()); + queries::insert_account_vault_asset(&mut conn, account_id, block5, asset.id(), Some(asset)) .unwrap(); + } + + let result = queries::select_account_vault_assets_with_row_limit( + &mut conn, + account_id, + block5..=block5, + 1, + ); + + assert_matches!( + result, + Err(crate::errors::DatabaseError::AccountSyncPageExceedsPayloadLimit { block_num }) + if block_num == block5 + ); +} + +/// Multi-block vault sync: when the page limit truncates mid-range, keep complete blocks only. +#[test] +fn select_account_vault_sync_values_multi_block_pagination() { + use miden_protocol::asset::{Asset, FungibleAsset}; + + let mut conn = create_db(); + let account_id = AccountId::try_from(ACCOUNT_ID_REGULAR_PUBLIC_ACCOUNT_IMMUTABLE_CODE).unwrap(); + let faucet_1 = AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET).unwrap(); + let faucet_2 = AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET_1).unwrap(); + let faucet_3 = AccountId::try_from(ACCOUNT_ID_PUBLIC_FUNGIBLE_FAUCET_2).unwrap(); + + let block1 = BlockNumber::from(1); + let block2 = BlockNumber::from(2); + let block3 = BlockNumber::from(3); + + create_block(&mut conn, block1); + create_block(&mut conn, block2); + create_block(&mut conn, block3); + + queries::upsert_accounts( + &mut conn, + &[mock_block_account_update(account_id, 0)], + block1, + &queries::PrecomputedPublicAccountStates::new(), + ) + .unwrap(); + queries::upsert_accounts( + &mut conn, + &[mock_block_account_update(account_id, 1)], + block2, + &queries::PrecomputedPublicAccountStates::new(), + ) + .unwrap(); + queries::upsert_accounts( + &mut conn, + &[mock_block_account_update(account_id, 2)], + block3, + &queries::PrecomputedPublicAccountStates::new(), + ) + .unwrap(); + + let asset_1 = Asset::Fungible(FungibleAsset::new(faucet_1, 1).unwrap()); + let asset_2 = Asset::Fungible(FungibleAsset::new(faucet_2, 2).unwrap()); + let asset_3 = Asset::Fungible(FungibleAsset::new(faucet_3, 3).unwrap()); + queries::insert_account_vault_asset(&mut conn, account_id, block1, asset_1.id(), Some(asset_1)) + .unwrap(); + queries::insert_account_vault_asset(&mut conn, account_id, block2, asset_2.id(), Some(asset_2)) + .unwrap(); + queries::insert_account_vault_asset(&mut conn, account_id, block3, asset_3.id(), Some(asset_3)) + .unwrap(); + + // limit=2: fetch 3 rows, drop incomplete block 3, keep blocks 1-2. + let (last_block_included, values) = queries::select_account_vault_assets_with_row_limit( + &mut conn, + account_id, + BlockNumber::GENESIS..=block3, + 2, + ) + .unwrap(); - assert!(page.values.is_empty(), "should have no values when single block exceeds limit"); - assert_eq!(page.last_block_included, block5, "should signal no progress at block 5"); + assert_eq!(values.len(), 2, "should include entries from blocks 1 and 2"); + assert_eq!(last_block_included, block2, "last included block should be 2"); } /// Tests that normal multi-block pagination still works correctly: entries in blocks 1, 2, 3 with diff --git a/crates/store/src/errors.rs b/crates/store/src/errors.rs index ef36cea1a2..359053f45f 100644 --- a/crates/store/src/errors.rs +++ b/crates/store/src/errors.rs @@ -101,6 +101,8 @@ pub enum DatabaseError { use a stricter filter to reduce the number of transactions returned" )] TransactionPageExceedsPayloadLimit { block_num: BlockNumber }, + #[error("account sync updates for block {block_num} would exceed maximum response size")] + AccountSyncPageExceedsPayloadLimit { block_num: BlockNumber }, #[error("data corrupted: {0}")] DataCorrupted(String), #[error(transparent)]