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

Passing GOSSIP_MAX_SIZE to snappy uncompressor #8899

Merged

Conversation

lucassaldanha
Copy link
Member

@lucassaldanha lucassaldanha commented Dec 9, 2024

PR Description

In this PR we are passing the network config GOSSIP_MAX_SIZE to SnappyBlockCompressor#uncompress to check the size of an uncompressed message (as per spec). This way we can check the size of the uncompressed message without actually decompressing it.

TODO

  • Decide what value we want to use on MAX_COMPRESSED_GOSSIP_SIZE (LibP2PParamsFactory)
  • GossipHandler has a check for the compressed message size. Decide if we want to keep it.
  • Add README

Fixed Issue(s)

related to #8895

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

looks good...

@rolfyone rolfyone self-requested a review December 9, 2024 20:21
@lucassaldanha lucassaldanha force-pushed the gossip-check-uncompressed-size branch from 0ae1ed9 to 373b638 Compare December 9, 2024 23:58
@lucassaldanha lucassaldanha marked this pull request as ready for review December 10, 2024 00:15
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM

@tbenr tbenr enabled auto-merge (squash) December 10, 2024 14:33
@tbenr tbenr disabled auto-merge December 10, 2024 14:36
@lucassaldanha lucassaldanha merged commit dbed1e9 into Consensys:master Dec 10, 2024
17 checks passed
@lucassaldanha lucassaldanha deleted the gossip-check-uncompressed-size branch December 10, 2024 20:21
.isInstanceOf(DecodingException.class);
}

@Test
void uncompress_uncompressedLengthLongerThanMaxLength() {
void uncompress_uncompressedLengthLongerThanSszLenghtBounds() {
Copy link
Contributor

Choose a reason for hiding this comment

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

- uncompress_uncompressedLengthLongerThanSszLenghtBounds()
+ uncompress_uncompressedLengthLongerThanSszLenghthBounds()

}

@Test
void uncompress_uncompressedLengthEqualThanMaxBytesLength() throws DecodingException {
Copy link
Contributor

Choose a reason for hiding this comment

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

- uncompress_uncompressedLengthEqualThanMaxBytesLength()
+ uncompress_uncompressedLengthEqualToMaxBytesLength()

Comment on lines +77 to +79
final Bytes original = Bytes.fromHexString("0x010203040506");
final long smallMaxBytesLength = 3;
assertThat(smallMaxBytesLength).isLessThan(original.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

To be a little more precise, I would probably do something like:

assertThat(original.size()).isNotEqualTo(0);
final long smallMaxBytesLength = original.size() - 1;

@@ -24,16 +24,24 @@
*/
public class SnappyBlockCompressor {

public Bytes uncompress(final Bytes compressedData, final SszLengthBounds lengthBounds)
public Bytes uncompress(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add a check to compress to ensure that the uncompressed size is <= maxBytesLength.

Likewise, clients MUST NOT emit or propagate messages larger than this limit.

https://github.com/ethereum/consensus-specs/blob/3d180786af8f661a9dc53d1aadf0f89972a56024/specs/phase0/p2p-interface.md?plain=1#L276

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.

4 participants