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

using lzma to compress/decompress state_parts #1771

Closed
wants to merge 1 commit into from
Closed

Conversation

Kouprin
Copy link
Member

@Kouprin Kouprin commented Nov 25, 2019

Fixes #1048.

@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@4774eba). Click here to learn what that means.
The diff coverage is 78.08%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1771   +/-   ##
=========================================
  Coverage          ?   81.25%           
=========================================
  Files             ?      168           
  Lines             ?    36922           
  Branches          ?        0           
=========================================
  Hits              ?    30002           
  Misses            ?     6920           
  Partials          ?        0
Impacted Files Coverage Δ
chain/chain/src/lib.rs 100% <ø> (ø)
chain/chain/src/chain.rs 69.72% <78.08%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4774eba...942002d. Read the comment docs.

_ => {
byzantine_assert!(false);
return Err(ErrorKind::Other(
"set_state_part failed: lzma::decompress failed".into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not show the decompression error here? Might help with debugging

Copy link
Member Author

Choose a reason for hiding this comment

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

It should not fail at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So every byte sequence can be decoded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not failing means that failing caused by clearly malicious data.

@mikhailOK
Copy link
Contributor

is it possible to create a decompression bomb for lzma?

@Kouprin
Copy link
Member Author

Kouprin commented Nov 26, 2019

@Kouprin
Copy link
Member Author

Kouprin commented Nov 26, 2019

https://bomb.codes/bombs
Should we write our own compressor then?

@mikhailOK
Copy link
Contributor

We need a library that allows setting a resource limit for decompression + somehow know an upper bound for the size of one part

@Kouprin
Copy link
Member Author

Kouprin commented Nov 26, 2019

We need a library that allows setting a resource limit for decompression + somehow know an upper bound for the size of one part

Rust-lzma allows to limit memory usage, as usually others do. I'm trying to find a library which allows to set limited output buffer.

We have already limited upper bound of one part with 1 Mb unpacked data.

@mikhailOK
Copy link
Contributor

Can we guarantee 1 MB limit? The logic for splitting is approximate

@Kouprin
Copy link
Member Author

Kouprin commented Nov 26, 2019

Can we guarantee 1 MB limit? The logic for splitting is approximate

Trie relies on memory_usage field while splitting. Are we giving enough room to guarantee that the size of the part is no more than X Mb?

@mikhailOK
Copy link
Contributor

we might be able to guarantee that for some X after the coming storage changes + setting a limit on value sizes

@Kouprin
Copy link
Member Author

Kouprin commented Nov 26, 2019

we might be able to guarantee that for some X after the coming storage changes + setting a limit on value sizes

Yep, it's important. Should we create a task for it?

@mikhailOK
Copy link
Contributor

created #1786 , for now we should set max size to something big - probably not infinite because of bombs, but potentially no smaller than the whole state size

@Kouprin
Copy link
Member Author

Kouprin commented Nov 27, 2019

What number you suggest to set for now?

@mikhailOK
Copy link
Contributor

state_root.memory_usage * 10 + 1_000_000

@Kouprin
Copy link
Member Author

Kouprin commented Nov 28, 2019

state_root.memory_usage * 10 + 1_000_000

I expect it should be a constant.

Anyway, we can wait till #1792 will be accepted.

@mikhailOK
Copy link
Contributor

After #1792 we still need limit on keys and values

@ilblackdragon
Copy link
Member

Is this PR still alive?

@Kouprin
Copy link
Member Author

Kouprin commented Jan 21, 2020

Is this PR still alive?

It's blocked by #1805.

@Kouprin
Copy link
Member Author

Kouprin commented Mar 5, 2020

doesn't seem actual for now

@Kouprin Kouprin closed this Mar 5, 2020
@chefsale chefsale deleted the T1048 branch July 21, 2020 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compress state at StateSync
4 participants