Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions accounts-db/src/accounts_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@ impl AccountsFile {
Ok((Self::AppendVec(av), num_accounts))
}

/// Creates a new AccountsFile for the underlying storage at `path`
///
/// This version of `new()` may only be called when reconstructing storages as part of startup.
/// It trusts the snapshot's value for `current_len`, and relies on later index generation or
/// accounts verification to ensure it is valid.
pub fn new_for_startup(
path: impl Into<PathBuf>,
current_len: usize,
storage_access: StorageAccess,
) -> Result<Self> {
let av = AppendVec::new_for_startup(path, current_len, storage_access)?;
Ok(Self::AppendVec(av))
}

/// true if this storage can possibly be appended to (independent of capacity check)
//
// NOTE: Only used by ancient append vecs "append" method, which is test-only now.
Expand Down
32 changes: 32 additions & 0 deletions accounts-db/src/append_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ pub enum AppendVecError {

#[error("offset ({0}) is larger than file size ({1})")]
OffsetOutOfBounds(usize, usize),

#[error("file size ({1}) and current length ({0}) do not match for '{0}'")]
SizeMismatch(PathBuf, usize, u64),
}

/// A slice whose contents are known to be valid.
Expand Down Expand Up @@ -443,6 +446,35 @@ impl AppendVec {
Ok((new, num_accounts))
}

/// Creates a new AppendVec for the underlying storage at `path`
///
/// This version of `new()` may only be called when reconstructing storages as part of startup.
/// It trusts the snapshot's value for `current_len`, and relies on later index generation or
/// accounts verification to ensure it is valid.
pub fn new_for_startup(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for new_from_file compared with new_from_startup?

I understand the difference between the two, but when will we want to actually use new_from_file instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, new_from_file() is now only called in tests, benches, and store-tool.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason it isn't marked DCOU?

Copy link
Copy Markdown
Author

@brooksprumo brooksprumo Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably can be. I wouldn't want to do that in this PR though.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It seems relevant to this PR: You are replacing the function with a new implementation and deprecating the old one for non-test usage.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not deprecating the old one though. It may currently be called only by dcou-stuff, but should we remove/deprecate/make-dcou it? I'm not sure; it seems like a normal/useful constructor, esp. since it is safe and can be called anywhere. I'd personally rather leave it for now. Maybe we revisit that? But that's why I don't want it within this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(drive by from bp pr) +1 deprecation or dcou

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #6894 for DCOU.

path: impl Into<PathBuf>,
current_len: usize,
storage_access: StorageAccess,
) -> Result<Self> {
let new = Self::new_from_file_unchecked(path, current_len, storage_access)?;

// The current_len is allowed to be either exactly the same as file_size, or
// u64-aligned-equivalent to file_size. This is because `flush` and `shink` compute the
// required file size with *aligned* stored size per account, including the very last
// account. So the file size may have padding to the next u64-alignment.
// For our usage, this is still safe as these padding bytes could never be used for an
// account. This renders the `get_` and `scan_` functions safe.
if (current_len as u64 == new.file_size)
|| (u64_align!(current_len) as u64 == new.file_size)
{
Ok(new)
} else {
Err(AccountsFileError::AppendVecError(
AppendVecError::SizeMismatch(new.path.clone(), current_len, new.file_size),
))
}
}

/// Creates an appendvec from file without performing sanitize checks or counting the number of accounts
#[cfg_attr(not(unix), allow(unused_variables))]
pub fn new_from_file_unchecked(
Expand Down
4 changes: 2 additions & 2 deletions runtime/src/serde_snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -916,8 +916,8 @@ pub(crate) fn reconstruct_single_storage(
append_vec_id: AccountsFileId,
storage_access: StorageAccess,
) -> Result<Arc<AccountStorageEntry>, SnapshotError> {
let (accounts_file, _num_accounts) =
AccountsFile::new_from_file(append_vec_path, current_len, storage_access)?;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so new_from_file is used for only tests now?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCOU-only, yeah: #6552 (comment)

let accounts_file =
AccountsFile::new_for_startup(append_vec_path, current_len, storage_access)?;
Ok(Arc::new(AccountStorageEntry::new_existing(
*slot,
append_vec_id,
Expand Down
Loading