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

Fix truncated hex in anvil dump_state #8216

Merged
merged 9 commits into from
Sep 13, 2024

Conversation

AxelAramburu
Copy link
Contributor

@AxelAramburu AxelAramburu commented Jun 20, 2024

Motivation

Fixes #8179

An issue reported is leading zeros in hex can be truncated and leading to deserialize failure with anvil_dumpState

Solution

Like @mattsse recommend in the issue post, I change the type of storage from U256 to B256.

pub storage: BTreeMap<U256, U256>,

The modifications don't break the actual implemented tests:

Before

...

"0x068e44eb31e111028c41598e4535be7468674d0a": { // line 32, a contract address

...

"0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc": "0x68e44eb31e111028c41598e4535be7468674d0a", // line 172, a reference to that contract
"0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103": "0x9e2803ad9ecc0a848c8a045329524d295f6ee44f",
"0xd30e835d3f35624761057ff5b27d558f97bd5be034621e62240e5c0b784abe68": "0x9965507d1a55bcc2695c58ba16fb37d819b0a4dc"

After

...

"0x068e44eb31e111028c41598e4535be7468674d0a": { // line 32, a contract address

...

"0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc": "0x000000000000000000000000068e44eb31e111028c41598e4535be7468674d0a", // <--- truncated zero is preserved
"0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103": "0x0000000000000000000000009e2803ad9ecc0a848c8a045329524d295f6ee44f",
"0xd30e835d3f35624761057ff5b27d558f97bd5be034621e62240e5c0b784abe68": "0x0000000000000000000000009965507d1a55bcc2695c58ba16fb37d819b0a4dc"

Let me know if I can improve my contribution

@@ -348,7 +348,7 @@ pub struct SerializableAccountRecord {
pub nonce: u64,
pub balance: U256,
pub code: Bytes,
pub storage: BTreeMap<U256, U256>,
pub storage: BTreeMap<U256, B256>,
Copy link
Member

Choose a reason for hiding this comment

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

this should be flipped, the key should always be b256

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 change also the key type

Copy link
Member

@DaniPopes DaniPopes Jun 20, 2024

Choose a reason for hiding this comment

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

That's good, but the value should stay U256, BTreeMap<B256, U256>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I the value is U256 the bug is not corrected:
#8179

{
"0x0000000000000000000000000000000000000000000000000000000000000000":"0x1",
"0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc":"0x68e44eb31e111028c41598e4535be7468674d0a", // <--- The truncated zero
"0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103":"0x9e2803ad9ecc0a848c8a045329524d295f6ee44f",
"0xd30e835d3f35624761057ff5b27d558f97bd5be034621e62240e5c0b784abe68":"0x9965507d1a55bcc2695c58ba16fb37d819b0a4dc"}},

@@ -348,7 +348,7 @@ pub struct SerializableAccountRecord {
pub nonce: u64,
pub balance: U256,
pub code: Bytes,
pub storage: BTreeMap<U256, U256>,
pub storage: BTreeMap<B256, B256>,
Copy link
Member

Choose a reason for hiding this comment

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

this is also how the regular genesis account is formatted with hash -> hash

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 see an example of genesis.json storage field:

      "storage": {
        "0x0000000000000000000000000000000000000000000000000000000000000001": "0x02",
        "0x0000000000000000000000000000000000000000000000000000000000000002": "0x04",
        "0x29ecdbdf95c7f6ceec92d6150c697aa14abeb0f8595dd58d808842ea237d8494": "0x01",
        "0x6aa118c6537572d8b515a9f9154be55a3377a8de7991cd23bf6e5ceb368688e3": "0x01",
        ...
      }

The type is pub storage: BTreeMap<B256, U256>, but it doesnt resolve the issue:
#8179

Copy link
Member

Choose a reason for hiding this comment

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

please cargo +nightly fmt

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's good

@@ -564,7 +564,7 @@ impl Backend {
slot: U256,
val: B256,
) -> DatabaseResult<()> {
self.db.write().await.set_storage_at(address, slot, U256::from_be_bytes(val.0))
self.db.write().await.set_storage_at(address, B256::from(slot), B256::from(val.0))
Copy link
Member

Choose a reason for hiding this comment

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

B256::from(val.0) is not needed here?

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 fixed it thanks

Copy link
Member

Choose a reason for hiding this comment

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

something went wrong here, why are all lines changed?
line endings?

@zerosnacks
Copy link
Member

Hi @AxelAramburu why did you close your PR? Do you need any help / pointers? Would be great to get this fix in

@KholdStare
Copy link
Contributor

@AxelAramburu Running into this issue. Can you re-open?

@zerosnacks zerosnacks reopened this Jul 25, 2024
@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 31, 2024
@zerosnacks zerosnacks added T-bug Type: bug C-anvil Command: anvil labels Jul 31, 2024
@0xalpharush
Copy link
Contributor

What remains to be done here? I am also having issues decoding the state dump due to odd-length fields which are expected to be hex encoded

@mattsse mattsse requested a review from klkvr as a code owner September 12, 2024 06:41
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

actual changes look good, can we undo the changes in backend/mem/mod.rs @zerosnacks then this should be good

@yash-atreya yash-atreya enabled auto-merge (squash) September 12, 2024 08:35
@yash-atreya yash-atreya requested a review from mattsse September 12, 2024 08:35
@KholdStare
Copy link
Contributor

KholdStare commented Sep 12, 2024

Maybe this is wrong, but I think there should be a "round-trip" test in general for the dump state functionality. Given a state, does dump_state -> load_state -> dump_state produce the same result as the first dump_state? It should catch any random regressions that may happen in the future.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

good point, we should be able to easily add one

@yash-atreya could you please add such a test? but we can also do this as a followup

@yash-atreya yash-atreya merged commit 84e63fe into foundry-rs:master Sep 13, 2024
20 checks passed
rplusq pushed a commit to rplusq/foundry that referenced this pull request Sep 25, 2024
* Fix truncated hex in anvil dump_state

* Change type of key

* fixs

* rustfmt

* fix

---------

Co-authored-by: unknown <[email protected]>
Co-authored-by: Matthias Seitz <[email protected]>
Co-authored-by: Yash Atreya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-anvil Command: anvil T-bug Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hex data being truncated in anvil_dumpState
7 participants