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.

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 uses base58 for large binary leading to slow response #10140

Closed
ryoqun opened this issue May 20, 2020 · 8 comments · Fixed by #11474
Closed

RPC uses base58 for large binary leading to slow response #10140

ryoqun opened this issue May 20, 2020 · 8 comments · Fixed by #11474
Assignees
Labels
security Pull requests that address a security vulnerability
Milestone

Comments

@ryoqun
Copy link
Member

ryoqun commented May 20, 2020

Problem

RPC responses encodes account.data with base58:

data: bs58::encode(account.data.clone()).into_string(),

base58 isn't suited for large arbitrary-sized binary blobs. it's only for hash: #9490 (comment)

And it's very easy to attack dos. Also, viewing program account data (which tends to be large by definition) takes long time to retrieve via RPC.

$ time curl -s -X POST -H 'Content-Type: application/json' -d '{"id":1,"jsonrpc":"2.0","method":"getAccountInfo","params":["SysvarS1otHistory11111111111111111111111111",{"commitment":"max"}]}' http://tds.solana.com -o - | wc -c
179217

real    0m24.143s
user    0m0.005s
sys     0m0.007s

$ time curl -s -X POST -H 'Content-Type: application/json' -d '{"id":1,"jsonrpc":"2.0","method":"getAccountInfo","params":["SysvarRecentB1ockHashes11111111111111111111",{"commitment":"max"}]}' http://tds.solana.com -o - | wc -c
8389

real    0m0.377s
user    0m0.005s
sys     0m0.005s

$ time curl -s -X POST -H 'Content-Type: application/json' -d '{"id":1,"jsonrpc":"2.0","method":"getAccountInfo","params":["SysvarC1ock11111111111111111111111111111111",{"commitment":"max"}]}' http://tds.solana.com -o - | wc -c
239

real    0m0.304s
user    0m0.002s
sys     0m0.010s

As predicted theoretically, this is quadratic time increase to the size of account.data:

$ pry
[9] pry(main)> 179217.0 / 8389
=> 21.363332935987604
[11] pry(main)> (377 - 304) * 21.363332935987604 ** 2
=> 33316.615571771246    # =~ 24.143s

Proposed Solution

Use base64? But compat... ;)

FYI: @CriesofCarrots

Discovered via #9490.

@ryoqun ryoqun added the security Pull requests that address a security vulnerability label May 20, 2020
@mvines
Copy link
Member

mvines commented May 20, 2020

This would likely reduce the load on our RPC servers significantly.

@mvines mvines added this to the v1.3.0 milestone May 20, 2020
@mvines mvines modified the milestones: v1.3.0, v1.4.0, v1.2.18 Aug 5, 2020
@garious
Copy link
Contributor

garious commented Aug 6, 2020

If a new API endpoint because we want a new encoding, why base64 and not unicode via String::from_utf8?

@garious
Copy link
Contributor

garious commented Aug 6, 2020

For this endpoint, how about we return an error for large accounts, and perhaps that error includes a hash of the account data?

@mvines
Copy link
Member

mvines commented Aug 6, 2020

I was thinking of allowing the caller to supply their preferred encoding as an option to some of the existing RPC methods

@garious
Copy link
Contributor

garious commented Aug 6, 2020

Umm, so caller has the option to DOS?

@mvines
Copy link
Member

mvines commented Aug 6, 2020

Imposing a max response length for base58 at the same time would solve that and allow for a smooth migration. Large base58 responses already take forever or time out already. Run solana account TokenSVp5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o, ~10 seconds to respond in base58

@garious
Copy link
Contributor

garious commented Aug 6, 2020

Got it, yeah, perfect

@mvines
Copy link
Member

mvines commented Aug 8, 2020

Priority here should be on adding an optional user-provided encoding to getAccountInfo to improve response time. Streamlining the other RPC methods that return base58 can be secondary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants