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

Allow safe/finalized for eth_getLogs #9686

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

adytzu2007
Copy link
Contributor

No description provided.

@adytzu2007
Copy link
Contributor Author

This fixes issue #8454

Copy link
Collaborator

@AskAlexSharov AskAlexSharov left a comment

Choose a reason for hiding this comment

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

i think better change from -2 to -4 (or -5) , instead of removing validation at all

@adytzu2007
Copy link
Contributor Author

Hey @AskAlexSharov, I checked and the unmarshal code doesn't allow explicit negative numbers so the only way to get a negative number in this code would be due to a symbolic name such as "safe".
Also, if GetBlockNumber gets a negative number smaller than -5 it seems that it would treat it as an uint64 so it would become a very big number and fail when trying to read the hash for it. Should I add a check here that number.Int64() is positive instead?

@AskAlexSharov
Copy link
Collaborator

@adytzu2007 what geth does in this case? we may do something compatible if it's good

@adytzu2007
Copy link
Contributor Author

AFAICT, geth doesn't run any other validation on the block number in GetLogs once it unmarshals it from JSON. It also doesn't support latestExecuted and requires a hex number instead of also accepting an unsigned number.

Copy link
Collaborator

@yperbasis yperbasis left a comment

Choose a reason for hiding this comment

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

Please remove now unused "math/big" from the import

@yperbasis yperbasis enabled auto-merge (squash) March 13, 2024 09:21
@yperbasis yperbasis merged commit 90bf55d into ledgerwatch:devel Mar 13, 2024
7 checks passed
@adytzu2007 adytzu2007 deleted the safe_finalized_eth_getLogs branch March 13, 2024 09:43
mriccobene pushed a commit to mriccobene/erigon that referenced this pull request Mar 13, 2024
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.

None yet

3 participants