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

Add the block size and withdrawal fields to eth_getBlockByHash #1244

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

Conversation

akorchyn
Copy link
Contributor

@akorchyn akorchyn commented Apr 8, 2024

What was wrong?

Resolves: #960

How was it fixed?

I have added length implementations for a couple of structs.
I added the withdrawals and size fields to eth_getBlockHash

I noticed that the implementation of transaction size wasn't evaluated correctly. As per default implementation, the length is calculated from encode().len().
Is it possible that transaction encoding is implemented incorrectly?

To-Do

@akorchyn akorchyn changed the title feat: populated size and withdrawals for eth_getBlockByHash Add the block size and withdrawal fields to eth_getBlockByHash Apr 8, 2024
@akorchyn
Copy link
Contributor Author

akorchyn commented Apr 8, 2024

I don't really understand these failing tests. Could somebody point out the direction of the problem?
FAIL [ 0.549s] portal-bridge types::era1::tests::test_era1::case_1_era1
FAIL [ 3.570s] portal-bridge types::era1::tests::test_era1::case_2_era1
FAIL [ 3.560s] portal-bridge types::era1::tests::test_era1::case_3_era1

@@ -397,7 +418,7 @@ impl BlobTransaction {
+ self.gas_limit.length()
+ self.to.length()
+ self.value.length()
+ self.data.len()
+ self.data.length()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ self.data.length()
+ self.data.len()

change this back to len() and see if it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually doesn't change anything from the perspective of encoding length. I have changed it just to keep it the same for all fields_len methods :)

Copy link
Contributor Author

@akorchyn akorchyn Apr 8, 2024

Choose a reason for hiding this comment

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

But I tried, and the test still failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought there might be an issue with transaction encoding, but it seems fine. I have added couple of tests if you don't mind

Copy link
Member

Choose a reason for hiding this comment

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

Nothing wrong with adding tests

I would dump one that failed with one that didn't fail and output the values as hex. This isn't really my priority to look into what is causing this problem at the moment, but impl From<Withdrawal> for reth_rpc_types::Withdrawal { if you remove this and nothing else does it work again?

Copy link
Member

Choose a reason for hiding this comment

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

My idea is maybe this is causing a .into() elsewhere in the code to pull this instead for some unknown reason, but that is just my best guess not really looking into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your help anyway. I ll look it in coming days

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.

Add block size field to eth_getBlockByHash
2 participants