Rpc: filters performance improvement #20185

fanatid · 2021-09-24T21:38:02Z

Closes #20177

fanatid · 2021-09-24T22:05:22Z

CI should be fixed after #20182

jstarry · 2021-09-24T22:13:17Z

There's a temp fix on master already that you can rebase off of too

codecov · 2021-09-25T00:27:33Z

Codecov Report

Merging #20185 (4864e33) into master (60fe29b) will decrease coverage by 0.0%.
The diff coverage is 64.6%.

@@            Coverage Diff            @@
##           master   #20185     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         495      495             
  Lines      137701   137831    +130     
=========================================
+ Hits       112832   112914     +82     
- Misses      24869    24917     +48

fanatid · 2021-10-07T05:50:38Z

~~@ChrisBellew~~ (sorry for the false ping!) @CriesofCarrots can I help somehow with review? Should I add tests or split PR for adding new enum variants and deprecation?

CriesofCarrots · 2021-10-07T15:45:57Z

@fanatid , thanks for the ping. Looks like I missed the review notification when I was off the grid in the mountains last weekend. Sorry about that!

CriesofCarrots

This looks great! Just one minor suggestion about a generic DataTooLarge error and nits.

client/src/rpc_filter.rs

cli/src/cluster_query.rs

client/src/rpc_filter.rs

t-nelson

Looking good. Just a few more nits 😉

t-nelson · 2021-10-13T03:56:38Z

client/src/rpc_filter.rs

-                            bs58::decode(&bytes)
+                        use MemcmpEncodedBytes::*;
+                        match &compare.bytes {
+                            Binary(bytes) if bytes.len() > 128 => {


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

client/src/rpc_filter.rs

t-nelson · 2021-10-13T04:00:36Z

client/src/rpc_filter.rs

+                            }
+                            Base58(bytes) if bytes.len() > 175 => Err(RpcFilterError::DataTooLarge),
+                            Base64(bytes) if bytes.len() > 172 => Err(RpcFilterError::DataTooLarge),
+                            Bytes(bytes) if bytes.len() > 128 => Err(RpcFilterError::DataTooLarge),


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

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.

t-nelson · 2021-10-13T04:02:54Z

client/src/rpc_filter.rs

+                                .map_err(RpcFilterError::DecodeError),
+                            Base58(bytes) => {
+                                let bytes = bs58::decode(&bytes).into_vec()?;
+                                if bytes.len() > 128 {


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

client/src/rpc_filter.rs

t-nelson

thanks for addressing! lgtm now once CI's happy*

r+

* looks like a rebase on master will get you past the audit failure

CriesofCarrots · 2021-10-13T20:52:27Z

client/src/rpc_filter.rs

+                        match &compare.bytes {
+                            Binary(bytes) if bytes.len() > MAX_DATA_BASE58_SIZE => {
+                                Err(RpcFilterError::Base58DataTooLarge)
+                            }
+                            Base58(bytes) if bytes.len() > MAX_DATA_BASE58_SIZE => {
+                                Err(RpcFilterError::DataTooLarge)
+                            }
+                            Base64(bytes) if bytes.len() > MAX_DATA_BASE64_SIZE => {
+                                Err(RpcFilterError::DataTooLarge)
+                            }
+                            Bytes(bytes) if bytes.len() > MAX_DATA_SIZE => {
+                                Err(RpcFilterError::DataTooLarge)
+                            }
+                            _ => Ok(()),
+                        }?;
+                        match &compare.bytes {


I still find it more confusing than helpful to have two match statements, esp since the four cases have to handled separately.
All the other changes look great!

With added changes I find this bit more confusing too 😞 but once MemcmpEncodedBytes::Binary would be removed statements can be merged and simplified a lot!

Pull request has been modified.

CriesofCarrots · 2021-10-14T05:31:06Z

The CI failure is legit now; looks like there's a test that needs a fixup

fanatid · 2021-10-14T17:21:23Z

Updated. CI is green now 🎉

CriesofCarrots · 2021-10-14T18:43:19Z

Thanks for all the work here!

* Add Base58,Base64,Bytes to MemcmpEncodedBytes * Rpc: decode memcmp before filtering accounts * Add deprecated attribute * Add Memcmp::bytes * Fix clippy for deprecated * Another clippy fix * merge RpcFilterError::DataTooLarge * add deprecation for Base58DataTooLarge * change filter data size limit * strict data size len for base58 * add magic numbers * fix tests (cherry picked from commit e9a427b)

* Add Base58,Base64,Bytes to MemcmpEncodedBytes * Rpc: decode memcmp before filtering accounts * Add deprecated attribute * Add Memcmp::bytes * Fix clippy for deprecated * Another clippy fix * merge RpcFilterError::DataTooLarge * add deprecation for Base58DataTooLarge * change filter data size limit * strict data size len for base58 * add magic numbers * fix tests (cherry picked from commit e9a427b) Co-authored-by: Kirill Fomichev <[email protected]>

* Add Base58,Base64,Bytes to MemcmpEncodedBytes * Rpc: decode memcmp before filtering accounts * Add deprecated attribute * Add Memcmp::bytes * Fix clippy for deprecated * Another clippy fix * merge RpcFilterError::DataTooLarge * add deprecation for Base58DataTooLarge * change filter data size limit * strict data size len for base58 * add magic numbers * fix tests

* Add Base58,Base64,Bytes to MemcmpEncodedBytes * Rpc: decode memcmp before filtering accounts * Add deprecated attribute * Add Memcmp::bytes * Fix clippy for deprecated * Another clippy fix * merge RpcFilterError::DataTooLarge * add deprecation for Base58DataTooLarge * change filter data size limit * strict data size len for base58 * add magic numbers * fix tests (cherry picked from commit e9a427b)

* Add Base58,Base64,Bytes to MemcmpEncodedBytes * Rpc: decode memcmp before filtering accounts * Add deprecated attribute * Add Memcmp::bytes * Fix clippy for deprecated * Another clippy fix * merge RpcFilterError::DataTooLarge * add deprecation for Base58DataTooLarge * change filter data size limit * strict data size len for base58 * add magic numbers * fix tests (cherry picked from commit e9a427b) Co-authored-by: Kirill Fomichev <[email protected]>

* Add Base58,Base64,Bytes to MemcmpEncodedBytes * Rpc: decode memcmp before filtering accounts * Add deprecated attribute * Add Memcmp::bytes * Fix clippy for deprecated * Another clippy fix * merge RpcFilterError::DataTooLarge * add deprecation for Base58DataTooLarge * change filter data size limit * strict data size len for base58 * add magic numbers * fix tests

This reverts commit 0497972.

vercel bot temporarily deployed to Preview September 24, 2021 21:38 Inactive

mergify bot added the community Community contribution label Sep 24, 2021

mergify bot requested a review from a team September 24, 2021 21:38

fanatid added 2 commits September 25, 2021 01:14

Add Base58,Base64,Bytes to MemcmpEncodedBytes

e2a62d7

Rpc: decode memcmp before filtering accounts

5a36ad9

fanatid force-pushed the rpc-filter-memcmp branch from 456ba73 to 5a36ad9 Compare September 24, 2021 22:14

vercel bot temporarily deployed to Preview September 24, 2021 22:14 Inactive

fanatid added 2 commits September 25, 2021 09:05

Add deprecated attribute

a38a140

Add Memcmp::bytes

5db8808

vercel bot temporarily deployed to Preview September 25, 2021 06:07 Inactive

Fix clippy for deprecated

750634e

vercel bot temporarily deployed to Preview September 25, 2021 06:13 Inactive

Another clippy fix

73ce0db

vercel bot temporarily deployed to Preview September 25, 2021 06:16 Inactive

mvines requested a review from CriesofCarrots September 26, 2021 16:15

CriesofCarrots reviewed Oct 7, 2021

View reviewed changes

mergify bot requested a review from a team October 7, 2021 16:27

fanatid added 2 commits October 7, 2021 22:20

Merge remote-tracking branch 'upstream/master' into rpc-filter-memcmp

e573789

merge RpcFilterError::DataTooLarge

02c478f

vercel bot temporarily deployed to Preview October 7, 2021 19:58 Inactive

CriesofCarrots reviewed Oct 7, 2021

View reviewed changes

client/src/rpc_filter.rs Outdated Show resolved Hide resolved

mergify bot requested a review from a team October 7, 2021 20:06

add deprecation for Base58DataTooLarge

0c3e724

vercel bot temporarily deployed to Preview October 7, 2021 20:17 Inactive

t-nelson reviewed Oct 7, 2021

View reviewed changes

client/src/rpc_filter.rs Outdated Show resolved Hide resolved

strict data size len for base58

5c7969a

vercel bot temporarily deployed to Preview October 8, 2021 05:33 Inactive

t-nelson reviewed Oct 13, 2021

View reviewed changes

add magic numbers

cc0881d

t-nelson previously approved these changes Oct 13, 2021

View reviewed changes

KeKo6988 approved these changes Oct 13, 2021

View reviewed changes

CriesofCarrots reviewed Oct 13, 2021

View reviewed changes

mergify bot requested a review from a team October 13, 2021 20:53

Merge remote-tracking branch 'upstream/master' into rpc-filter-memcmp

6010a29

fix tests

4864e33

CriesofCarrots added the v1.8 label Oct 14, 2021

CriesofCarrots merged commit e9a427b into solana-labs:master Oct 14, 2021

mergify bot mentioned this pull request Oct 14, 2021

Rpc: filters performance improvement (backport #20185) #20703

Merged

fanatid deleted the rpc-filter-memcmp branch October 14, 2021 18:50

CriesofCarrots added the v1.7 label Oct 21, 2021

mergify bot mentioned this pull request Oct 21, 2021

Rpc: filters performance improvement (backport #20185) #20862

Merged

CriesofCarrots mentioned this pull request Oct 26, 2021

Restore getProgramAccounts spl-token secondary-index functionality #20993

Merged

frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Revert "Rpc: filters performance improvement (solana-labs#20185)"

40a1d4b

This reverts commit 0497972.

fanatid mentioned this pull request Dec 8, 2022

rpc: fix possible panics in optimize_filters #29146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rpc: filters performance improvement #20185

Rpc: filters performance improvement #20185

fanatid commented Sep 24, 2021

fanatid commented Sep 24, 2021

jstarry commented Sep 24, 2021

codecov bot commented Sep 25, 2021 •

edited

Loading

fanatid commented Oct 7, 2021 •

edited

Loading

CriesofCarrots commented Oct 7, 2021

CriesofCarrots left a comment

t-nelson left a comment

t-nelson Oct 13, 2021

t-nelson Oct 13, 2021

fanatid Oct 13, 2021

t-nelson Oct 13, 2021

t-nelson left a comment

CriesofCarrots Oct 13, 2021

fanatid Oct 14, 2021

CriesofCarrots commented Oct 14, 2021

fanatid commented Oct 14, 2021

CriesofCarrots commented Oct 14, 2021

Rpc: filters performance improvement #20185

Rpc: filters performance improvement #20185

Conversation

fanatid commented Sep 24, 2021

fanatid commented Sep 24, 2021

jstarry commented Sep 24, 2021

codecov bot commented Sep 25, 2021 • edited Loading

Codecov Report

fanatid commented Oct 7, 2021 • edited Loading

CriesofCarrots commented Oct 7, 2021

CriesofCarrots left a comment

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

t-nelson Oct 13, 2021

Choose a reason for hiding this comment

t-nelson Oct 13, 2021

Choose a reason for hiding this comment

fanatid Oct 13, 2021

Choose a reason for hiding this comment

t-nelson Oct 13, 2021

Choose a reason for hiding this comment

t-nelson left a comment

Choose a reason for hiding this comment

CriesofCarrots Oct 13, 2021

Choose a reason for hiding this comment

fanatid Oct 14, 2021

Choose a reason for hiding this comment

CriesofCarrots commented Oct 14, 2021

fanatid commented Oct 14, 2021

CriesofCarrots commented Oct 14, 2021

codecov bot commented Sep 25, 2021 •

edited

Loading

fanatid commented Oct 7, 2021 •

edited

Loading