Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Return error for excluded secondary-index keys #17193

Merged
merged 4 commits into from
May 13, 2021
+85 −36
Merged

Return error for excluded secondary-index keys #17193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1757522
Add runtime helpers to check secondary indexes for key
May 12, 2021
a991910
Add custom rpc error
May 12, 2021
a9b23ce
Check secondary-index key inclusion in rpc
May 12, 2021
32118af
Clone complete AccountSecondaryIndexes into rpc to avoid bank query
May 13, 2021
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
15 changes: 15 additions & 0 deletions client/src/rpc_custom_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub const JSON_RPC_SERVER_ERROR_TRANSACTION_PRECOMPILE_VERIFICATION_FAILURE: i64
pub const JSON_RPC_SERVER_ERROR_SLOT_SKIPPED: i64 = -32007;
pub const JSON_RPC_SERVER_ERROR_NO_SNAPSHOT: i64 = -32008;
pub const JSON_RPC_SERVER_ERROR_LONG_TERM_STORAGE_SLOT_SKIPPED: i64 = -32009;
pub const JSON_RPC_SERVER_ERROR_KEY_EXCLUDED_FROM_SECONDARY_INDEX: i64 = -32010;

pub enum RpcCustomError {
BlockCleanedUp {
Expand All @@ -40,6 +41,9 @@ pub enum RpcCustomError {
LongTermStorageSlotSkipped {
slot: Slot,
},
KeyExcludedFromSecondaryIndex {
index_key: String,
},
}

#[derive(Debug, Serialize, Deserialize)]
Expand Down Expand Up @@ -117,6 +121,17 @@ impl From<RpcCustomError> for Error {
message: format!("Slot {} was skipped, or missing in long-term storage", slot),
data: None,
},
RpcCustomError::KeyExcludedFromSecondaryIndex { index_key } => Self {
code: ErrorCode::ServerError(
JSON_RPC_SERVER_ERROR_KEY_EXCLUDED_FROM_SECONDARY_INDEX,
),
message: format!(
"{} excluded from account secondary indexes; \
this RPC method unavailable for key",
index_key
),
data: None,
},
}
}
}
96 changes: 61 additions & 35 deletions core/src/rpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use solana_metrics::inc_new_counter_info;
use solana_perf::packet::PACKET_DATA_SIZE;
use solana_runtime::{
accounts::AccountAddressFilter,
accounts_index::{AccountIndex, IndexKey},
accounts_index::{AccountIndex, AccountSecondaryIndexes, IndexKey},
bank::Bank,
bank_forks::{BankForks, SnapshotConfig},
commitment::{BlockCommitmentArray, BlockCommitmentCache, CommitmentSlots},
Expand Down Expand Up @@ -125,7 +125,7 @@ pub struct JsonRpcConfig {
pub enable_bigtable_ledger_storage: bool,
pub enable_bigtable_ledger_upload: bool,
pub max_multiple_accounts: Option<usize>,
pub account_indexes: HashSet<AccountIndex>,
pub account_indexes: AccountSecondaryIndexes,
pub rpc_threads: usize,
pub rpc_bigtable_timeout: Option<Duration>,
pub minimal_api: bool,
Expand Down Expand Up @@ -360,11 +360,11 @@ impl JsonRpcRequestProcessor {
check_slice_and_encoding(&encoding, data_slice_config.is_some())?;
let keyed_accounts = {
if let Some(owner) = get_spl_token_owner_filter(program_id, &filters) {
self.get_filtered_spl_token_accounts_by_owner(&bank, &owner, filters)
self.get_filtered_spl_token_accounts_by_owner(&bank, &owner, filters)?
} else if let Some(mint) = get_spl_token_mint_filter(program_id, &filters) {
self.get_filtered_spl_token_accounts_by_mint(&bank, &mint, filters)
self.get_filtered_spl_token_accounts_by_mint(&bank, &mint, filters)?
} else {
self.get_filtered_program_accounts(&bank, program_id, filters)
self.get_filtered_program_accounts(&bank, program_id, filters)?
}
};
let result =
Expand Down Expand Up @@ -1559,7 +1559,7 @@ impl JsonRpcRequestProcessor {
));
}
let mut token_balances: Vec<RpcTokenAccountBalance> = self
.get_filtered_spl_token_accounts_by_mint(&bank, &mint, vec![])
.get_filtered_spl_token_accounts_by_mint(&bank, &mint, vec![])?
.into_iter()
.map(|(address, account)| {
let amount = TokenAccount::unpack(&account.data())
Expand Down Expand Up @@ -1607,7 +1607,8 @@ impl JsonRpcRequestProcessor {
}));
}

let keyed_accounts = self.get_filtered_spl_token_accounts_by_owner(&bank, owner, filters);
let keyed_accounts =
self.get_filtered_spl_token_accounts_by_owner(&bank, owner, filters)?;
let accounts = if encoding == UiAccountEncoding::JsonParsed {
get_parsed_token_accounts(bank.clone(), keyed_accounts.into_iter()).collect()
} else {
Expand Down Expand Up @@ -1659,13 +1660,13 @@ impl JsonRpcRequestProcessor {
];
// Optional filter on Mint address, uses mint account index for scan
let keyed_accounts = if let Some(mint) = mint {
self.get_filtered_spl_token_accounts_by_mint(&bank, &mint, filters)
self.get_filtered_spl_token_accounts_by_mint(&bank, &mint, filters)?
} else {
// Filter on Token Account state
filters.push(RpcFilterType::DataSize(
TokenAccount::get_packed_len() as u64
));
self.get_filtered_program_accounts(&bank, &token_program_id, filters)
self.get_filtered_program_accounts(&bank, &token_program_id, filters)?
};
let accounts = if encoding == UiAccountEncoding::JsonParsed {
get_parsed_token_accounts(bank.clone(), keyed_accounts.into_iter()).collect()
Expand Down Expand Up @@ -1693,7 +1694,7 @@ impl JsonRpcRequestProcessor {
bank: &Arc<Bank>,
program_id: &Pubkey,
filters: Vec<RpcFilterType>,
) -> Vec<(Pubkey, AccountSharedData)> {
) -> Result<Vec<(Pubkey, AccountSharedData)>> {
let filter_closure = |account: &AccountSharedData| {
filters.iter().all(|filter_type| match filter_type {
RpcFilterType::DataSize(size) => account.data().len() as u64 == *size,
Expand All @@ -1705,16 +1706,24 @@ impl JsonRpcRequestProcessor {
.account_indexes
.contains(&AccountIndex::ProgramId)
{
bank.get_filtered_indexed_accounts(&IndexKey::ProgramId(*program_id), |account| {
// The program-id account index checks for Account owner on inclusion. However, due
// to the current AccountsDb implementation, an account may remain in storage as a
// zero-lamport AccountSharedData::Default() after being wiped and reinitialized in later
// updates. We include the redundant filters here to avoid returning these
// accounts.
account.owner() == program_id && filter_closure(account)
})
if !self.config.account_indexes.include_key(program_id) {
return Err(RpcCustomError::KeyExcludedFromSecondaryIndex {
index_key: program_id.to_string(),
}
.into());
}
Ok(
bank.get_filtered_indexed_accounts(&IndexKey::ProgramId(*program_id), |account| {
// The program-id account index checks for Account owner on inclusion. However, due
// to the current AccountsDb implementation, an account may remain in storage as a
// zero-lamport AccountSharedData::Default() after being wiped and reinitialized in later
// updates. We include the redundant filters here to avoid returning these
// accounts.
account.owner() == program_id && filter_closure(account)
}),
)
} else {
bank.get_filtered_program_accounts(program_id, filter_closure)
Ok(bank.get_filtered_program_accounts(program_id, filter_closure))
}
}

Expand All @@ -1724,7 +1733,7 @@ impl JsonRpcRequestProcessor {
bank: &Arc<Bank>,
owner_key: &Pubkey,
mut filters: Vec<RpcFilterType>,
) -> Vec<(Pubkey, AccountSharedData)> {
) -> Result<Vec<(Pubkey, AccountSharedData)>> {
// The by-owner accounts index checks for Token Account state and Owner address on
// inclusion. However, due to the current AccountsDb implementation, an account may remain
// in storage as a zero-lamport AccountSharedData::Default() after being wiped and reinitialized in
Expand All @@ -1746,13 +1755,22 @@ impl JsonRpcRequestProcessor {
.account_indexes
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the way you did this is ok. However, the .account_indexes is specified in the config to rpc also from validator. I specifically had to chop out the include/exclude info. So, you could just keep a copy of the include/exclude info in rpc directly. There are 2 sources of truth then, but we already do that with account_indexes.

Copy link
Contributor

Choose a reason for hiding this comment

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

here is where I exclude the include/exclude info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I failed to track where the verbose/concise account_indexes were split. Yeah, let's do that; saves any runtime helpers needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeffwashington done here: 32118af
Can you take a look?

I considered changing the type to Arc<AccountSecondaryIndexes> to reduce to only one source of truth, but the level of churn was very high. Let me know if you prefer that, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect.
I prefer not an arc myself. The secondary config is created once at startup and never changed and looked up on each modification to the accounts index. 2 copies is ok with me.

.contains(&AccountIndex::SplTokenOwner)
{
bank.get_filtered_indexed_accounts(&IndexKey::SplTokenOwner(*owner_key), |account| {
account.owner() == &spl_token_id_v2_0()
&& filters.iter().all(|filter_type| match filter_type {
RpcFilterType::DataSize(size) => account.data().len() as u64 == *size,
RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()),
})
})
if !self.config.account_indexes.include_key(owner_key) {
return Err(RpcCustomError::KeyExcludedFromSecondaryIndex {
index_key: owner_key.to_string(),
}
.into());
}
Ok(bank.get_filtered_indexed_accounts(
&IndexKey::SplTokenOwner(*owner_key),
|account| {
account.owner() == &spl_token_id_v2_0()
&& filters.iter().all(|filter_type| match filter_type {
RpcFilterType::DataSize(size) => account.data().len() as u64 == *size,
RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()),
})
},
))
} else {
self.get_filtered_program_accounts(bank, &spl_token_id_v2_0(), filters)
}
Expand All @@ -1764,7 +1782,7 @@ impl JsonRpcRequestProcessor {
bank: &Arc<Bank>,
mint_key: &Pubkey,
mut filters: Vec<RpcFilterType>,
) -> Vec<(Pubkey, AccountSharedData)> {
) -> Result<Vec<(Pubkey, AccountSharedData)>> {
// The by-mint accounts index checks for Token Account state and Mint address on inclusion.
// However, due to the current AccountsDb implementation, an account may remain in storage
// as be zero-lamport AccountSharedData::Default() after being wiped and reinitialized in later
Expand All @@ -1785,13 +1803,21 @@ impl JsonRpcRequestProcessor {
.account_indexes
.contains(&AccountIndex::SplTokenMint)
{
bank.get_filtered_indexed_accounts(&IndexKey::SplTokenMint(*mint_key), |account| {
account.owner() == &spl_token_id_v2_0()
&& filters.iter().all(|filter_type| match filter_type {
RpcFilterType::DataSize(size) => account.data().len() as u64 == *size,
RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()),
})
})
if !self.config.account_indexes.include_key(mint_key) {
return Err(RpcCustomError::KeyExcludedFromSecondaryIndex {
index_key: mint_key.to_string(),
}
.into());
}
Ok(
bank.get_filtered_indexed_accounts(&IndexKey::SplTokenMint(*mint_key), |account| {
account.owner() == &spl_token_id_v2_0()
&& filters.iter().all(|filter_type| match filter_type {
RpcFilterType::DataSize(size) => account.data().len() as u64 == *size,
RpcFilterType::Memcmp(compare) => compare.bytes_match(&account.data()),
})
}),
)
} else {
self.get_filtered_program_accounts(bank, &spl_token_id_v2_0(), filters)
}
Expand Down
4 changes: 4 additions & 0 deletions runtime/src/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,10 @@ impl Accounts {
.0
}

pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool {
self.accounts_db.account_indexes.include_key(key)
}

pub fn load_all(&self, ancestors: &Ancestors) -> Vec<(Pubkey, AccountSharedData, Slot)> {
self.accounts_db.scan_accounts(
ancestors,
Expand Down
4 changes: 4 additions & 0 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4246,6 +4246,10 @@ impl Bank {
.load_by_index_key_with_filter(&self.ancestors, index_key, filter)
}

pub fn account_indexes_include_key(&self, key: &Pubkey) -> bool {
self.rc.accounts.account_indexes_include_key(key)
}

pub fn get_all_accounts_with_modified_slots(&self) -> Vec<(Pubkey, AccountSharedData, Slot)> {
self.rc.accounts.load_all(&self.ancestors)
}
Expand Down
2 changes: 1 addition & 1 deletion validator/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2116,7 +2116,7 @@ pub fn main() {
rpc_bigtable_timeout: value_t!(matches, "rpc_bigtable_timeout", u64)
.ok()
.map(Duration::from_secs),
account_indexes: account_indexes.indexes.clone(),
account_indexes: account_indexes.clone(),
},
rpc_addrs: value_t!(matches, "rpc_port", u16).ok().map(|rpc_port| {
(
Expand Down