Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rpc: filters performance improvement #20185

Merged
merged 14 commits into from
Oct 14, 2021
Merged

Rpc: filters performance improvement #20185

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
e2a62d7
Add Base58,Base64,Bytes to MemcmpEncodedBytes
fanatid Sep 24, 2021
5a36ad9
Rpc: decode memcmp before filtering accounts
fanatid Sep 24, 2021
a38a140
Add deprecated attribute
fanatid Sep 25, 2021
5db8808
Add Memcmp::bytes
fanatid Sep 25, 2021
750634e
Fix clippy for deprecated
fanatid Sep 25, 2021
73ce0db
Another clippy fix
fanatid Sep 25, 2021
e573789
Merge remote-tracking branch 'upstream/master' into rpc-filter-memcmp
fanatid Oct 7, 2021
02c478f
merge RpcFilterError::DataTooLarge
fanatid Oct 7, 2021
0c3e724
add deprecation for Base58DataTooLarge
fanatid Oct 7, 2021
ab99129
change filter data size limit
fanatid Oct 8, 2021
5c7969a
strict data size len for base58
fanatid Oct 8, 2021
cc0881d
add magic numbers
fanatid Oct 13, 2021
6010a29
Merge remote-tracking branch 'upstream/master' into rpc-filter-memcmp
fanatid Oct 14, 2021
4864e33
fix tests
fanatid Oct 14, 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
4 changes: 2 additions & 2 deletions cli/src/cluster_query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1720,15 +1720,15 @@ pub fn process_show_stakes(
// Filter by `StakeState::Stake(_, _)`
rpc_filter::RpcFilterType::Memcmp(rpc_filter::Memcmp {
offset: 0,
bytes: rpc_filter::MemcmpEncodedBytes::Binary(
bytes: rpc_filter::MemcmpEncodedBytes::Base58(
CriesofCarrots marked this conversation as resolved.
Show resolved Hide resolved
bs58::encode([2, 0, 0, 0]).into_string(),
),
encoding: Some(rpc_filter::MemcmpEncoding::Binary),
}),
// Filter by `Delegation::voter_pubkey`, which begins at byte offset 124
rpc_filter::RpcFilterType::Memcmp(rpc_filter::Memcmp {
offset: 124,
bytes: rpc_filter::MemcmpEncodedBytes::Binary(
bytes: rpc_filter::MemcmpEncodedBytes::Base58(
vote_account_pubkeys[0].to_string(),
),
encoding: Some(rpc_filter::MemcmpEncoding::Binary),
Expand Down
14 changes: 7 additions & 7 deletions cli/src/program.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,18 +1148,18 @@ fn get_buffers(
) -> Result<CliUpgradeableBuffers, Box<dyn std::error::Error>> {
let mut filters = vec![RpcFilterType::Memcmp(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![1, 0, 0, 0]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![1, 0, 0, 0]).into_string()),
encoding: None,
})];
if let Some(authority_pubkey) = authority_pubkey {
filters.push(RpcFilterType::Memcmp(Memcmp {
offset: ACCOUNT_TYPE_SIZE,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![1]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![1]).into_string()),
encoding: None,
}));
filters.push(RpcFilterType::Memcmp(Memcmp {
offset: ACCOUNT_TYPE_SIZE + OPTION_SIZE,
bytes: MemcmpEncodedBytes::Binary(
bytes: MemcmpEncodedBytes::Base58(
bs58::encode(authority_pubkey.as_ref()).into_string(),
),
encoding: None,
Expand Down Expand Up @@ -1201,18 +1201,18 @@ fn get_programs(
) -> Result<CliUpgradeablePrograms, Box<dyn std::error::Error>> {
let mut filters = vec![RpcFilterType::Memcmp(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![3, 0, 0, 0]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![3, 0, 0, 0]).into_string()),
encoding: None,
})];
if let Some(authority_pubkey) = authority_pubkey {
filters.push(RpcFilterType::Memcmp(Memcmp {
offset: ACCOUNT_TYPE_SIZE + SLOT_SIZE,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![1]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![1]).into_string()),
encoding: None,
}));
filters.push(RpcFilterType::Memcmp(Memcmp {
offset: ACCOUNT_TYPE_SIZE + SLOT_SIZE + OPTION_SIZE,
bytes: MemcmpEncodedBytes::Binary(
bytes: MemcmpEncodedBytes::Base58(
bs58::encode(authority_pubkey.as_ref()).into_string(),
),
encoding: None,
Expand All @@ -1236,7 +1236,7 @@ fn get_programs(
bytes.extend_from_slice(programdata_address.as_ref());
let filters = vec![RpcFilterType::Memcmp(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(bytes).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(bytes).into_string()),
encoding: None,
})];

Expand Down
95 changes: 69 additions & 26 deletions client/src/rpc_filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use thiserror::Error;
#![allow(deprecated)]
use {std::borrow::Cow, thiserror::Error};

#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
Expand All @@ -15,15 +16,31 @@ impl RpcFilterType {
let encoding = compare.encoding.as_ref().unwrap_or(&MemcmpEncoding::Binary);
match encoding {
MemcmpEncoding::Binary => {
let MemcmpEncodedBytes::Binary(bytes) = &compare.bytes;

if bytes.len() > 128 {
Err(RpcFilterError::Base58DataTooLarge)
} else {
bs58::decode(&bytes)
use MemcmpEncodedBytes::*;
match &compare.bytes {
Binary(bytes) if bytes.len() > 128 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just bump this to the base58 max. No harm in relaxing the limit

Err(RpcFilterError::Base58DataTooLarge)
}
Base58(bytes) if bytes.len() > 175 => Err(RpcFilterError::DataTooLarge),
Base64(bytes) if bytes.len() > 172 => Err(RpcFilterError::DataTooLarge),
fanatid marked this conversation as resolved.
Show resolved Hide resolved
Bytes(bytes) if bytes.len() > 128 => Err(RpcFilterError::DataTooLarge),
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to test this since there's no expensive decoding to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why we should not test this value? Test for this value is not only about expensive decoding but also about a performant filter. Or I'm wrong?
Other comments are fixed.

_ => Ok(()),
}?;
match &compare.bytes {
Binary(bytes) => bs58::decode(&bytes)
.into_vec()
.map(|_| ())
.map_err(|e| e.into())
.map_err(RpcFilterError::DecodeError),
Base58(bytes) => {
let bytes = bs58::decode(&bytes).into_vec()?;
if bytes.len() > 128 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is unnecessary now. Fine to keep it, but let's name the magic number here too

Err(RpcFilterError::DataTooLarge)
} else {
Ok(())
}
}
Base64(bytes) => base64::decode(&bytes).map(|_| ()).map_err(Into::into),
fanatid marked this conversation as resolved.
Show resolved Hide resolved
Bytes(_) => Ok(()),
}
}
}
Expand All @@ -34,10 +51,24 @@ impl RpcFilterType {

#[derive(Error, PartialEq, Debug)]
pub enum RpcFilterError {
#[error("bs58 decode error")]
DecodeError(#[from] bs58::decode::Error),
#[error("encoded binary data should be less than 129 bytes")]
DataTooLarge,
#[deprecated(
since = "1.9.0",
note = "Error for MemcmpEncodedBytes::Binary which is deprecated"
)]
#[error("encoded binary (base 58) data should be less than 129 bytes")]
Base58DataTooLarge,
#[deprecated(
since = "1.9.0",
note = "Error for MemcmpEncodedBytes::Binary which is deprecated"
)]
#[error("bs58 decode error")]
DecodeError(bs58::decode::Error),
#[error("base58 decode error")]
Base58DecodeError(#[from] bs58::decode::Error),
#[error("base64 decode error")]
Base64DecodeError(#[from] base64::DecodeError),
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
Expand All @@ -49,7 +80,14 @@ pub enum MemcmpEncoding {
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[serde(rename_all = "camelCase", untagged)]
pub enum MemcmpEncodedBytes {
#[deprecated(
since = "1.9.0",
note = "Please use MemcmpEncodedBytes::Base58 instead"
)]
Binary(String),
Base58(String),
Base64(String),
Bytes(Vec<u8>),
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
Expand All @@ -63,14 +101,18 @@ pub struct Memcmp {
}

impl Memcmp {
pub fn bytes_match(&self, data: &[u8]) -> bool {
pub fn bytes(&self) -> Option<Cow<Vec<u8>>> {
use MemcmpEncodedBytes::*;
match &self.bytes {
MemcmpEncodedBytes::Binary(bytes) => {
let bytes = bs58::decode(bytes).into_vec();
if bytes.is_err() {
return false;
}
let bytes = bytes.unwrap();
Binary(bytes) | Base58(bytes) => bs58::decode(bytes).into_vec().ok().map(Cow::Owned),
Base64(bytes) => base64::decode(bytes).ok().map(Cow::Owned),
Bytes(bytes) => Some(Cow::Borrowed(bytes)),
}
}

pub fn bytes_match(&self, data: &[u8]) -> bool {
match self.bytes() {
Some(bytes) => {
if self.offset > data.len() {
return false;
}
Expand All @@ -79,6 +121,7 @@ impl Memcmp {
}
data[self.offset..self.offset + bytes.len()] == bytes[..]
}
None => false,
}
}
}
Expand All @@ -94,55 +137,55 @@ mod tests {
// Exact match of data succeeds
assert!(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![1, 2, 3, 4, 5]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![1, 2, 3, 4, 5]).into_string()),
encoding: None,
}
.bytes_match(&data));

// Partial match of data succeeds
assert!(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![1, 2]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![1, 2]).into_string()),
encoding: None,
}
.bytes_match(&data));

// Offset partial match of data succeeds
assert!(Memcmp {
offset: 2,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![3, 4]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![3, 4]).into_string()),
encoding: None,
}
.bytes_match(&data));

// Incorrect partial match of data fails
assert!(!Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![2]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![2]).into_string()),
encoding: None,
}
.bytes_match(&data));

// Bytes overrun data fails
assert!(!Memcmp {
offset: 2,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![3, 4, 5, 6]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![3, 4, 5, 6]).into_string()),
encoding: None,
}
.bytes_match(&data));

// Offset outside data fails
assert!(!Memcmp {
offset: 6,
bytes: MemcmpEncodedBytes::Binary(bs58::encode(vec![5]).into_string()),
bytes: MemcmpEncodedBytes::Base58(bs58::encode(vec![5]).into_string()),
encoding: None,
}
.bytes_match(&data));

// Invalid base-58 fails
assert!(!Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary("III".to_string()),
bytes: MemcmpEncodedBytes::Base58("III".to_string()),
encoding: None,
}
.bytes_match(&data));
Expand All @@ -157,7 +200,7 @@ mod tests {
assert_eq!(
RpcFilterType::Memcmp(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(base58_bytes.to_string()),
bytes: MemcmpEncodedBytes::Base58(base58_bytes.to_string()),
encoding: None,
})
.verify(),
Expand All @@ -172,7 +215,7 @@ mod tests {
assert_eq!(
RpcFilterType::Memcmp(Memcmp {
offset: 0,
bytes: MemcmpEncodedBytes::Binary(base58_bytes.to_string()),
bytes: MemcmpEncodedBytes::Base58(base58_bytes.to_string()),
encoding: None,
})
.verify(),
Expand Down
Loading