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

Don't format if 'withdrawals' coming back from get_block is null #2941

Merged
merged 2 commits into from
May 1, 2023

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented May 1, 2023

What was wrong?

Before 1.11.1, Geth was sending back the withdrawal field as nil (See: ethereum/go-ethereum#26707)

Closes #2932

How was it fixed?

Added a is_not_null check to the withdrawals field on the block formatter results.

Todo:

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@kclowes kclowes requested review from fselmo and pacrob May 1, 2023 18:22
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

I see... so this should no longer be an issue past geth v1.11.1? I'm good with adding this in. No need to update any tests then either since we test for the empty list return value already.

@@ -297,7 +297,9 @@ def apply_list_to_array_formatter(formatter: Any) -> Callable[..., Any]:
)
),
"transactionsRoot": apply_formatter_if(is_not_null, to_hexbytes(32)),
"withdrawals": apply_formatter_to_array(withdrawal_result_formatter),
"withdrawals": apply_formatter_if(
is_not_null, apply_formatter_to_array(withdrawal_result_formatter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually noticed this will return an empty tuple if it is empty... I think we need a bugfix to make this formatter use apply_list_to_array_formatter():

    "withdrawals": apply_formatter_if(
         is_not_null, apply_list_to_array_formatter(withdrawal_result_formatter),
     ),

This is what the transaction and uncle formatters use. This might be a relevant PR to sneak this into.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually noticed this will return an empty tuple if it is empty

Ah, interesting. I was seeing an empty list. Out of curiousity, what provider were you using?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I do see that with geth, hmm. I was messing around with a rough draft of what eth-tester would look like plugged into the latest py-evm. I think I'd still rather have the list-like formatter for consistency (which I see was already added) but I think it's good to know it likely won't affect the user base too much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, gotcha. Yeah, I think it makes sense to have it in there in case it comes back as a tuple.

@kclowes kclowes marked this pull request as ready for review May 1, 2023 19:45
@kclowes kclowes merged commit a87816c into ethereum:main May 1, 2023
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.

TypeError: Could not format invalid type of None for field 'withdrawals' on 6.2.0
2 participants