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.

Sign up for GitHub

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

Jump to bottom

Fix key remapping for eth-tester client #1630

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Fix key remapping for eth-tester client #1630

merged 2 commits into from
Aug 3, 2022

Conversation

pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Apr 24, 2020 •
edited
Loading

What was wrong?

  • The key for the receipts root is wrong when using the EthereumTesterProvider
  • The key for the bloom filter was also wrong
  • The standard formatter for logsBloom wasn't setup to correctly handle the integer format used by eth-tester.

How was it fixed?

  • Fixed the typo on the key remappings for logsBloom and receiptRoot
  • Adjusted formatter for logsBloom to correctly handle integer values that aren't the full bit-length

Cute Animal Picture

cats-dogs-and-human-furniture

@pipermerriam pipermerriam requested a review from wolovim April 24, 2020 19:40
@pipermerriam pipermerriam force-pushed the piper/fix-eth-tester-receiptsRoot-key-mapping branch from eed8f29 to 70f4400 Compare April 24, 2020 20:11
@pipermerriam
Copy link
Member Author

I'll deal with the failures here and ping you when this is actually ready for review @marcgarreau

@wolovim
Copy link
Member

wolovim commented Apr 24, 2020

Beside the point, but I'm interested in more context around receiptsRoot if you have it, e.g., are you using it for anything in particular? With the exception of this value, the getBlock payload is identical between web3py and web3js.

@pipermerriam
Copy link
Member Author

I'm not actually using it. I just ran into these small key/naming discrepancies while working on an app that consumes all the block data and loads it into a relational database. I didn't go as far as actually using the receiptsRoot to verify the receipts but if you're question is related to how the value is constructed I can point you at the right place in the py-evm code.

pipermerriam
pipermerriam commented May 14, 2020
View reviewed changes
docs/web3.eth.rst
@@ -348,7 +348,7 @@ The following methods are available on the ``web3.eth`` namespace.
'nonce': '0x3b05c6d5524209f1',
'number': 2000000,
'parentHash': '0x57ebf07eb9ed1137d41447020a25e51d30a0c272b5896571499c82c33ecb7288',
'receiptRoot': '0x84aea4a7aad5c5899bd5cfc7f309cc379009d30179316a2a7baa4a2ea4a438ac',
'receiptsRoot': '0x84aea4a7aad5c5899bd5cfc7f309cc379009d30179316a2a7baa4a2ea4a438ac',
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is technically a breaking change, but it is also a bugfix... I'm inclined to just highlight this well in the release notes. Alternative would be to duplicate this value into both versions of the key (plural and singular) and then remove the singular one in v6...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd probably lean towards keeping both, but I don't have a strong preference either way. Maybe we could note that in the newsfragment?

@kclowes
Copy link
Collaborator

kclowes commented Oct 15, 2020

It looks like this is ready to be merged after a newsfragment gets added. Want me to pick that up @pipermerriam?

fselmo
fselmo approved these changes Aug 2, 2022
View reviewed changes
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.

This seems like a low-hanging fruit to scoop into v6. I can rebase, add a newsfragment, and merge it if this looks good to you @kclowes 👀.

@kclowes
Copy link
Collaborator

kclowes commented Aug 3, 2022

Yep, sounds good to me! Thanks @fselmo!

@fselmo fselmo force-pushed the piper/fix-eth-tester-receiptsRoot-key-mapping branch 2 times, most recently from eb1a5cf to b160124 Compare August 3, 2022 17:51
@pipermerriam @fselmo
Fix key remapping for eth-tester client
9ac13f9 
- Cleanup formatting errors
- Update typing
- Add test for incorrectly mapped key
fselmo added a commit to pipermerriam/web3.py that referenced this pull request Aug 3, 2022
@fselmo
Add newsfragment for eth-tester key remappings ethereum#1630
33ed07f
@fselmo fselmo force-pushed the piper/fix-eth-tester-receiptsRoot-key-mapping branch from b160124 to 33ed07f Compare August 3, 2022 18:03
@fselmo fselmo force-pushed the piper/fix-eth-tester-receiptsRoot-key-mapping branch from 33ed07f to 3e9e4bc Compare August 3, 2022 20:11
kclowes
kclowes approved these changes Aug 3, 2022
View reviewed changes
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@fselmo fselmo merged commit a497a75 into ethereum:master Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolovim wolovim wolovim left review comments

@kclowes kclowes kclowes approved these changes

@fselmo fselmo fselmo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pipermerriam @wolovim @kclowes @fselmo