Compress the PoV block before sending it over the network #2288

bkchr · 2021-01-18T22:34:51Z

This pr changes the way we send PoV blocks over the network. We now
compress the PoV block before it is send over the network. This should
reduce the size significant for PoVs which contain the runtime WASM for
example.

This pr changes the way we send PoV blocks over the network. We now compress the PoV block before it is send over the network. This should reduce the size significant for PoVs which contain the runtime WASM for example.

node/network/protocol/src/lib.rs

ordian · 2021-01-19T10:02:01Z

I wonder whether we should evaluate other compression algorithms like zstd and lz4 before commiting a network protocol change.

bkchr · 2021-01-19T13:19:21Z

I wonder whether we should evaluate other compression algorithms like zstd and lz4 before commiting a network protocol change.

Would you like to do this evaluation?

pepyakin · 2021-01-19T13:22:10Z

node/network/protocol/src/lib.rs

+
+	impl CompressedPoV {
+		/// Create from the given [`PoV`].
+		pub fn from_pov(pov: &PoV) -> Result<Self, CompressedPoVError> {


Nit (i.e. feel free to ignore): I can't help but suggest to name those: compress and uncompress

burdges · 2021-01-19T15:08:49Z

If I understand, there are actually several different compression algorithms compatible with roughly the same gzip-like decompression algorithm, although flate2 does not present this picture, so maybe not exactly true. If that's true, then parachains who wish to spend more time compressing could do so by modifying their own code.

ordian · 2021-01-19T21:21:10Z

Would you like to do this evaluation?

Sure, I can do it tomorrow. Please send me an example of a typical large PoV. Do we scale encode it before compressing?

bkchr · 2021-01-19T21:40:56Z

Would you like to do this evaluation?

Sure, I can do it tomorrow. Please send me an example of a typical large PoV. Do we scale encode it before compressing?

I don't have one. Yes they are SCALE encoded.

I compared the compression between gzip and lz4 of a runtime wasm. Gzip clearly wins here.

ordian · 2021-01-20T12:46:41Z

https://github.com/ordian/bench-compression-algorithms

zstd seems better than gzip for our use-case.

pepyakin · 2021-01-20T13:18:58Z

To have a full picture I think these benches should also show decoding compression/performance.

ordian · 2021-01-20T13:24:05Z

To have a full picture I think these benches should also show decoding compression/performance.

They do, I just omitted them in readme, because decoding is much faster than encoding for all of them.

However, now that you brought it up, I'd like to raise a concern about zip bombs [1], a carefully crafted encoded PoV can be used for a DDoS attack. I don't know if any of these are resistant to them.

coriolinus · 2021-01-20T14:04:15Z

Does anyone know of a compression format designed to be resistant to zip bombs? I'm not read-up on the subject, but if there's a mitigation as simple as "prohibit DEFLATE", then that might be the way to go.

pepyakin · 2021-01-20T14:06:49Z

However, now that you brought it up, I'd like to raise a concern about zip bombs [1], a carefully crafted encoded PoV can be used for a DDoS attack. I don't know if any of these are resistant to them.

That's a good point.

Since we are using a streaming decoder we can set up a shut-off condition that stops whenever the decoded data exceeds the raw PoV limit (4 MiB as of now?).

As long as the internal implementation is not susceptible for excessive allocation we should be fine.

ordian · 2021-01-20T14:14:27Z

Since we are using a streaming decoder we can set up a shut-off condition that stops whenever the decoded data exceeds the raw PoV limit (4 MiB as of now?).

Agree, this might be the simplest way to mitigate the issue, but I don't know if 4 MiB is big enough.

burdges · 2021-01-20T14:25:48Z

Just an aside: zstd has a cool feature of trained dictionaries. It's supposedly only for small files, but people discuss it for log files. We could not permit untrusted dictionaries, but conceivably someone might one day build a canonical WASM dictionary, which we just fix until WASM changes radically. This is definitely future work and not relevant right now.

node/network/protocol/src/lib.rs

bkchr · 2021-01-21T14:49:15Z

I created an issue to switch to the actual maximum povblock size: #2298

bkchr · 2021-01-21T14:50:20Z

I also made the decompression/compression fail for browser nodes. I did not find any suitable zstd implementation at the moment that compiles for WASM. As all this stuff is currently anyway going to be replaced, I think this is the best solution for now.

roadmap/implementers-guide/src/types/network.md

ordian · 2021-01-21T14:50:14Z

node/network/protocol/src/lib.rs

+			struct InputDecoder<'a, T: std::io::BufRead>(&'a mut zstd::Decoder<T>, usize);
+			impl<'a, T: std::io::BufRead> parity_scale_codec::Input for InputDecoder<'a, T> {
+				fn read(&mut self, into: &mut [u8]) -> Result<(), parity_scale_codec::Error> {
+					if self.1.saturating_add(into.len()) > MAX_POV_BLOCK_SIZE {


where do we modify self.1?
can we have a test for this please?

We modify self.1 directly here? We call saturating_add in this line.

saturating_add takes self by value, not &mut self
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=da4bed5cde4b3b145bc8754b705aeb20

I'm an idiot 🤦

ordian · 2021-01-21T14:57:33Z

I also made the decompression/compression fail for browser nodes. I did not find any suitable zstd implementation at the moment that compiles for WASM. As all this stuff is currently anyway going to be replaced, I think this is the best solution for now.

Should we ditch browser feature and web-wasm check ? AFAIK light client will use https://github.com/paritytech/smoldot? cc @tomaka

It's strange that zstd doesn't compile to WASM though, found relevant issue: gyscos/zstd-rs#48.
EDIT: https://github.com/gyscos/zstd-rs/blob/7f19ce66b6dd542d2c7ede9571f2648ba65ba6f0/Cargo.toml#L38

node/network/protocol/Cargo.toml

Co-authored-by: Andronik Ordian <[email protected]>

tomaka · 2021-01-21T15:56:50Z

Should we ditch browser feature and web-wasm check ? AFAIK light client will use https://github.com/paritytech/smoldot?

If we can keep the Wasm check alive without too much efforts, then we should keep it alive.
However since parachains are way higher priority, I don't think we should block this PR on the Wasm check.

bkchr · 2021-01-21T15:58:40Z

Should we ditch browser feature and web-wasm check ? AFAIK light client will use https://github.com/paritytech/smoldot?

If we can keep the Wasm check alive without too much efforts, then we should keep it alive.
However since parachains are way higher priority, I don't think we should block this PR on the Wasm check.

Wasm check is happy. It would just fail to decompress/compress a PoVBlock on a browser node. IMHO a browser node should not do this anyway.

When the day comes that we want this, we can clearly get it work. However, I already spent to much time on this.

pepyakin

Apart from the pending comments, looks good.

…adot into bkchr-compress-pov-block

rphmeier · 2021-01-21T17:54:12Z

bot merge

ghost · 2021-01-21T17:54:15Z

Waiting for commit status.

cheme · 2021-03-31T13:30:53Z

node/network/protocol/src/lib.rs

+		/// Compress the given [`PoV`] and returns a [`CompressedPoV`].
+		#[cfg(not(target_os = "unknown"))]
+		pub fn compress(pov: &PoV) -> Result<Self, CompressedPoVError> {
+			zstd::encode_all(pov.encode().as_slice(), 3).map_err(|_| CompressedPoVError::Compress).map(Self)


@bkchr , I was wondering if you did test with something like (curious if it gives similar ratio):

let mut dest = Vec::new(); pov.encode_to(Encoder::new(&mut dest, 3)

Edit: I did test it on a different context, it looks identical with encoder (checked against zstd using cmd not actually encode_all).

Compress the PoV block before sending it over the network

175ceca

This pr changes the way we send PoV blocks over the network. We now compress the PoV block before it is send over the network. This should reduce the size significant for PoVs which contain the runtime WASM for example.

bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 18, 2021

bkchr mentioned this pull request Jan 18, 2021

GZip encode CandidateCommitments #2289

Closed

pepyakin reviewed Jan 18, 2021

View reviewed changes

node/network/protocol/src/lib.rs Outdated Show resolved Hide resolved

bkchr added 2 commits January 19, 2021 13:23

Preallocate 1KB

247ae6b

Try something..

f3eb3b5

pepyakin reviewed Jan 19, 2021

View reviewed changes

rphmeier reviewed Jan 20, 2021

View reviewed changes

node/network/protocol/src/lib.rs Outdated Show resolved Hide resolved

bkchr added 3 commits January 21, 2021 15:16

Switch to zstd and some renamings

3c5dd79

Make compression/decompression fail in browsers

579ed3d

Use some sane maximum value

9bbd97e

bkchr mentioned this pull request Jan 21, 2021

Use correct PoVBlock maximum size on decompres #2298

Open

ordian reviewed Jan 21, 2021

View reviewed changes

node/network/protocol/Cargo.toml Show resolved Hide resolved

bkchr and others added 2 commits January 21, 2021 16:08

Update roadmap/implementers-guide/src/types/network.md

9d35a32

Co-authored-by: Andronik Ordian <[email protected]>

Fix and add test

3f9a2fc

pepyakin approved these changes Jan 21, 2021

View reviewed changes

rphmeier and others added 2 commits January 21, 2021 12:19

add

4547aef

Merge branch 'bkchr-compress-pov-block' of github.com:paritytech/polk…

ddfcc48

…adot into bkchr-compress-pov-block

ghost merged commit 11797c7 into master Jan 21, 2021

ghost deleted the bkchr-compress-pov-block branch January 21, 2021 18:04

ordian mentioned this pull request Mar 14, 2021

Request based collation fetching #2594

Closed

cheme reviewed Mar 31, 2021

View reviewed changes

bkchr mentioned this pull request Sep 26, 2022

Member Request polkadot-fellows/seeding#3

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compress the PoV block before sending it over the network #2288

Compress the PoV block before sending it over the network #2288

bkchr commented Jan 18, 2021

ordian commented Jan 19, 2021

bkchr commented Jan 19, 2021

pepyakin Jan 19, 2021

burdges commented Jan 19, 2021

ordian commented Jan 19, 2021

bkchr commented Jan 19, 2021

ordian commented Jan 20, 2021

pepyakin commented Jan 20, 2021

ordian commented Jan 20, 2021

coriolinus commented Jan 20, 2021

pepyakin commented Jan 20, 2021

ordian commented Jan 20, 2021

burdges commented Jan 20, 2021

bkchr commented Jan 21, 2021

bkchr commented Jan 21, 2021

ordian Jan 21, 2021

bkchr Jan 21, 2021

ordian Jan 21, 2021

bkchr Jan 21, 2021

ordian commented Jan 21, 2021 •

edited

Loading

tomaka commented Jan 21, 2021

bkchr commented Jan 21, 2021

pepyakin left a comment

rphmeier commented Jan 21, 2021

ghost commented Jan 21, 2021

cheme Mar 31, 2021 •

edited

Loading

Compress the PoV block before sending it over the network #2288

Compress the PoV block before sending it over the network #2288

Conversation

bkchr commented Jan 18, 2021

ordian commented Jan 19, 2021

bkchr commented Jan 19, 2021

pepyakin Jan 19, 2021

Choose a reason for hiding this comment

burdges commented Jan 19, 2021

ordian commented Jan 19, 2021

bkchr commented Jan 19, 2021

ordian commented Jan 20, 2021

pepyakin commented Jan 20, 2021

ordian commented Jan 20, 2021

coriolinus commented Jan 20, 2021

pepyakin commented Jan 20, 2021

ordian commented Jan 20, 2021

burdges commented Jan 20, 2021

bkchr commented Jan 21, 2021

bkchr commented Jan 21, 2021

ordian Jan 21, 2021

Choose a reason for hiding this comment

bkchr Jan 21, 2021

Choose a reason for hiding this comment

ordian Jan 21, 2021

Choose a reason for hiding this comment

bkchr Jan 21, 2021

Choose a reason for hiding this comment

ordian commented Jan 21, 2021 • edited Loading

tomaka commented Jan 21, 2021

bkchr commented Jan 21, 2021

pepyakin left a comment

Choose a reason for hiding this comment

rphmeier commented Jan 21, 2021

ghost commented Jan 21, 2021

cheme Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

ordian commented Jan 21, 2021 •

edited

Loading

cheme Mar 31, 2021 •

edited

Loading