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

bound VariadicValue multiple variant #1488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

librelois
Copy link
Contributor

VariadicValue::Multiple variant should be bounded to prevent heavy computation on cartesian product

@librelois librelois requested a review from sorpaas as a code owner August 9, 2024 14:02
@boundless-forest
Copy link
Collaborator

boundless-forest commented Aug 21, 2024

Does that have a significant performance issue, or could you please provide a detailed example of the case?

I find that this code snippet was copied from the previous parity Ethereum, and I wonder why it did not have this restriction before.

@librelois
Copy link
Contributor Author

@boundless-forest this change comes from an internal review of possible flaws in the code, and we're trying to bound all unbounded inputs.
The question is, what's a reasonable limit?

I wonder why it did not have this restriction before.

parity ethereum hasn't been used in production for years, it's not shocking that this client had several flaws that were less annoying at a time when ethereum was much less popular and therefore less hacked. So I wouldn't rely on it.

@boundless-forest
Copy link
Collaborator

The question is, what's a reasonable limit?

Yes. This limit will affect two areas: filter addresses and topics. For topics, a value over 4 is sufficient. However, for addresses, there is no explicitly defined number in the Ethereum protocol.

Have you considered the default JSON-RPC payload size restriction? It might address your concern.

@@ -30,6 +30,8 @@ use serde_json::{from_value, Value};

use crate::types::{BlockNumberOrHash, Log};

const VARIADIC_MULTIPLE_MAX_SIZE: usize = 16;
Copy link
Contributor

Choose a reason for hiding this comment

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

16 will be too low for FilterAddress as mentioned by @boundless-forest

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.

3 participants