Integrate zstd compression into chain exchange #842

masih · 2025-01-22T14:49:02Z

The GPBFT message exchange over pubsub already uses zstd compression on top of CBOR encoded messages. The work here integrates the same style of compression for chain exchange messages, with additional unification of the encoding mechanism across the two.

The work refactors the root level encoding implementation into a generic encoder decoder that both chain exchange and gpbft used. Tests and benchmarks are updated to reflect this.

The benchmarking of partial gmessage encoding is also adjusted to fix a few redundant statements and bugs in testing.

Fixes #819 #843

codecov · 2025-01-22T15:03:11Z

Codecov Report

Attention: Patch coverage is 61.40351% with 22 lines in your changes missing coverage. Please review.

Project coverage is 67.32%. Comparing base (ded3d04) to head (13487e2).

Files with missing lines	Patch %	Lines
internal/encoding/encoding.go	61.90%	11 Missing and 5 partials ⚠️
chainexchange/pubsub.go	44.44%	2 Missing and 3 partials ⚠️
host.go	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
- Coverage   67.38%   67.32%   -0.07%     
==========================================
  Files          83       83              
  Lines        9017     9024       +7     
==========================================
- Hits         6076     6075       -1     
- Misses       2408     2412       +4     
- Partials      533      537       +4

Files with missing lines	Coverage Δ
host.go	`66.10% <83.33%> (+1.51%)`	⬆️
chainexchange/pubsub.go	`70.04% <44.44%> (+1.43%)`	⬆️
internal/encoding/encoding.go	`61.90% <61.90%> (ø)`

... and 6 files with indirect coverage changes

Stebalien · 2025-01-22T18:12:59Z

Can we add a compression bomb test? I think we're fine, but it would be nice to have a test.

The fact that we're stream decoding into the CBOR decoder coupled with the fact that the CBOR decoder has a bunch of limits should protect us against memory issues.
The fact that our CBOR decoder won't read through unlimited input (e.g., doesn't have logic that skips unknown fields, etc.) means we can't get "stuck" when decoding. But it would be nice if we could wrap the decompressed reader in a limited reader before passing it off to the CBOR decoder, just in case. That way we can guarantee that we'll process at most N bytes of CBOR before giving up.

Kubuxu · 2025-01-22T18:26:03Z

chainexchange/pubsub.go

 }

 func NewPubSubChainExchange(o ...Option) (*PubSubChainExchange, error) {
 	opts, err := newOptions(o...)
 	if err != nil {
 		return nil, err
 	}
+	zstd, err := encoding.NewZSTD[*Message]()


Are we going with ZSTD by default?

For chain exchange yes. For GPBFT it's configurable via manifest.

Happy to make it configurable for chain exchange too if you think it's worth doing.

masih · 2025-01-22T19:15:14Z

Can we add a compression bomb test?

Sure. Captured #843

masih · 2025-01-23T13:49:57Z

@Stebalien the message encoding decoding off Gossipsub does not function in streaming manner. Instead, both the publisher and subscriber hand over []byte slices to/fro the pubsub subsystem to do its thing.

The default maximum message size in pubsub is set to 1 MiB, which as far as I can tell has not been overridden in Lotus. This limit is large enough for the purposes of F3. So, what I have done in 05bc578 is to explicitly set the maximum decoded value in zstd to 1 MiB. For sanity, I have also restricted encoder to refuse to encode values that would hit that limit.

Does this cover your concern re expansion attack vector in the context of zstd compression?

Stebalien · 2025-01-23T16:34:34Z

That's good but isn't the zstd coder streaming? Looking at the docs in:

https://pkg.go.dev/github.com/klauspost/compress/zstd#WithDecoderMaxMemory

That restricts the max memory held at any point in time while streaming, not the max that can be read from the stream in total (unless I'm misreading it).

masih · 2025-01-23T16:39:41Z

That's good but isn't the zstd coder streaming?

It has two modes: streaming where one needs to allocate a decoder per io.reader, or nonstreaming where one declares a reader with nil io.reader and uses DecodeAll. The latter is what the implementation in this repo does. The rationale for this choice was reduced GC.

(unless I'm misreading it).

The documentation reads: "WithDecoderMaxMemory allows to set a maximum decoded size for in-memory non-streaming operations or maximum window size for streaming operations. This can be used to control memory usage of potentially hostile content. ".

Right?

Stebalien · 2025-01-23T16:52:26Z

That's good but isn't the zstd coder streaming?

It has two modes: streaming where one needs to allocate a decoder per io.reader, or nonstreaming where one declares a reader with nil io.reader and uses DecodeAll. The latter is what the implementation in this repo does. The rationale for this choice was reduced GC.

(unless I'm misreading it).

The documentation reads: "WithDecoderMaxMemory allows to set a maximum decoded size for in-memory non-streaming operations or maximum window size for streaming operations. This can be used to control memory usage of potentially hostile content. ".

Right?

Oh, I see. I assumed we used streaming decoding. Then yeah, it should work (and is actually required).

The GPBFT message exchange over pubsub already uses zstd compression on top of CBOR encoded messages. The work here integrates the same style of compression for chain exchange messages, with additional unification of the encoding mechanism across the two. The work refactors the root level encoding implementation into a generic encoder decoder that both chain exchange and gpbft used. Tests and benchmarks are updated to reflect this. The benchmarking of partial gmessage encoding is also adjusted to fix a few redundant statements and bugs in testing. Fixes #819

The default message size limit in GossipSub is 1 MiB, which is unchanged in Lotus. This means when decompressing values, we can never have a valid compressed message that expands to larger than 1 MiB. Set this limit explicitly in the zstd decoder.

masih · 2025-01-24T17:20:28Z

@Kubuxu @Stebalien Can I get a review on this PR please?

Stebalien

LGTM!

Stebalien · 2025-01-24T18:29:11Z

internal/encoding/encoding.go

+}
+
+func (c *ZSTD[T]) Decode(v []byte, t T) error {
+	cborEncoded, err := c.decompressor.DecodeAll(v, make([]byte, 0, len(v)))


Future change: we should use a buffer pool for these short-lived buffers (https://pkg.go.dev/sync#Pool). If we do that, we can also allocate these buffers with 1MiB capacities and use WithDecodeAllCapLimit to avoid any allocations.

masih self-assigned this Jan 22, 2025

masih requested a review from Kubuxu January 22, 2025 14:50

Kubuxu reviewed Jan 22, 2025

View reviewed changes

masih mentioned this pull request Jan 22, 2025

Add compression bomb test #843

Open

BigLep requested a review from Kubuxu January 23, 2025 02:11

masih requested a review from Stebalien January 23, 2025 13:50

masih linked an issue Jan 24, 2025 that may be closed by this pull request

Add compression bomb test #843

Open

masih added 2 commits January 24, 2025 17:16

masih force-pushed the masih/chainex-zstd-compression branch from 05bc578 to 13487e2 Compare January 24, 2025 17:19

Stebalien approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate zstd compression into chain exchange #842

Integrate zstd compression into chain exchange #842

masih commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

Stebalien commented Jan 22, 2025

Kubuxu Jan 22, 2025

masih Jan 22, 2025

masih commented Jan 22, 2025

masih commented Jan 23, 2025 •

edited

Loading

Stebalien commented Jan 23, 2025

masih commented Jan 23, 2025 •

edited

Loading

Stebalien commented Jan 23, 2025

masih commented Jan 24, 2025

Stebalien left a comment

Stebalien Jan 24, 2025

Integrate zstd compression into chain exchange #842

Are you sure you want to change the base?

Integrate zstd compression into chain exchange #842

Conversation

masih commented Jan 22, 2025 • edited Loading

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

Stebalien commented Jan 22, 2025

Kubuxu Jan 22, 2025

Choose a reason for hiding this comment

masih Jan 22, 2025

Choose a reason for hiding this comment

masih commented Jan 22, 2025

masih commented Jan 23, 2025 • edited Loading

Stebalien commented Jan 23, 2025

masih commented Jan 23, 2025 • edited Loading

Stebalien commented Jan 23, 2025

masih commented Jan 24, 2025

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Jan 24, 2025

Choose a reason for hiding this comment

masih commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

masih commented Jan 23, 2025 •

edited

Loading

masih commented Jan 23, 2025 •

edited

Loading