cerc-io/ipld-eth-server
18
0
Fork 1
Code Issues 10 Pull Requests 1 Actions Packages Projects Releases 37 Wiki Activity

The storage isn't cleared after the destruction of the contract #95

New Issue
Closed
opened 2021-09-06 14:51:41 +00:00 by n0cte · 9 comments
n0cte commented 2021-09-06 14:51:41 +00:00 (Migrated from github.com)
Copy Link
  • Branch: empty_data_encoder
  • Test: "get storage after selfdestruct" ​
- Branch: empty_data_encoder - Test: "get storage after selfdestruct" ​
levlite commented 2021-09-06 15:19:19 +00:00 (Migrated from github.com)
Copy Link

Hi Ilnure we need more details around this bug, and also what are you working on that you ran into this? Please provide more info, in Russian if need be I can have it translated, thank you

Hi Ilnure we need more details around this bug, and also what are you working on that you ran into this? Please provide more info, in Russian if need be I can have it translated, thank you
n0cte commented 2021-09-06 15:43:19 +00:00 (Migrated from github.com)
Copy Link

I needed to add a check for function RetrieveStorageAtByAddressAndStorageSlotAndBlockHash in the case when storage is empty. In the integration tests for the GLDToken contract, I added a destroy function for the contract. After that i started comparing storage data before destoying and after.

Geth:
before: [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 54 53 201 173 197 222 160 0 0]
after: [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]

ipld-eth-server:
before: [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 54 53 201 173 197 222 160 0 0]
after: [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 54 53 201 173 197 222 160 0 0]

i think, ipld-eth-server should return zero values after contract destoying

I needed to add a check for function `RetrieveStorageAtByAddressAndStorageSlotAndBlockHash` in the case when storage is empty. In the integration tests for the `GLDToken` contract, I added a destroy function for the contract. After that i started comparing storage data before destoying and after. Geth: before: `[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 54 53 201 173 197 222 160 0 0]` after: `[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]` ipld-eth-server: before: `[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 54 53 201 173 197 222 160 0 0]` after: `[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 54 53 201 173 197 222 160 0 0]` i think, ipld-eth-server should return zero values after contract destoying
AFDudley commented 2021-09-06 16:22:38 +00:00
Owner
Copy Link

We are a little in the weeds for me, but isn't there a flag we're using someplace else to address this? Or put another way, I suspect that the current release of the stack marks that data correctly it's just the mark isn't making it all the way to whatever you're accessing.

We are a little in the weeds for me, but isn't there a flag we're using someplace else to address this? Or put another way, I suspect that the current release of the stack marks that data correctly it's just the mark isn't making it all the way to whatever you're accessing.
i-norden commented 2021-09-06 18:35:33 +00:00
Member
Copy Link

When a state or storage node is destroyed post EIP-158 an entry at that path/leaf_key is made in the database that is flagged with the "Removed" node type to signify as such.

This issue I think makes an assumption that this is not occurring, but we do not know that the issue is at this level yet and in fact the unit test coverage of statediffing geth suggests this is not where the issue lies.

What is most likely occurring is that "Removed" node entries are in the database but are not being properly considered when querying the database.

Please see:
https://github.com/vulcanize/ipld-eth-server/pull/94#discussion_r699574336
and
https://github.com/vulcanize/ipld-eth-server/pull/94#discussion_r703038391

When a state or storage node is destroyed post EIP-158 an entry at that path/leaf_key is made in the database that is flagged with the "Removed" node type to signify as such. This issue I think makes an assumption that this is not occurring, but we do not know that the issue is at this level yet and in fact the unit test coverage of statediffing geth suggests this is not where the issue lies. What is most likely occurring is that "Removed" node entries are in the database but are not being properly considered when querying the database. Please see: https://github.com/vulcanize/ipld-eth-server/pull/94#discussion_r699574336 and https://github.com/vulcanize/ipld-eth-server/pull/94#discussion_r703038391
👍 1
levlite commented 2021-09-07 16:19:01 +00:00 (Migrated from github.com)
Copy Link

@ramilexe can you please translate Ian's message above and make sure it is conveyed to Ilnur?

@ramilexe can you please translate Ian's message above and make sure it is conveyed to Ilnur?
i-norden commented 2021-09-07 16:23:34 +00:00
Member
Copy Link

Copying over from slack:

So the issue is probably with these stored functions https://github.com/vulcanize/ipld-eth-server/blob/master/db/migrations/00014_create_stored_functions.sql#L4 and https://github.com/vulcanize/ipld-eth-server/blob/master/db/migrations/00014_create_stored_functions.sql#L22

Or in how they are being used here and the other related pg queries below it https://github.com/vulcanize/ipld-eth-server/blob/master/pkg/eth/ipld_retriever.go#L118

If its not at the ipld-eth-server level it is at the indexer level, but not the builder level. There are unit tests for these edge cases for the statediff builder but I cant recall if there are for the indexer (although it isn't really an edge case at that level, since it's the same db model being indexed in the same capacity/fashion).

Copying over from slack: So the issue is probably with these stored functions https://github.com/vulcanize/ipld-eth-server/blob/master/db/migrations/00014_create_stored_functions.sql#L4 and https://github.com/vulcanize/ipld-eth-server/blob/master/db/migrations/00014_create_stored_functions.sql#L22 Or in how they are being used here and the other related pg queries below it https://github.com/vulcanize/ipld-eth-server/blob/master/pkg/eth/ipld_retriever.go#L118 If its not at the ipld-eth-server level it is at the indexer level, but not the builder level. There are [unit tests](https://github.com/vulcanize/go-ethereum/blob/statediff/statediff/builder_test.go#L1318) for these edge cases for the statediff builder but I cant recall if there are for the indexer (although it isn't really an edge case at that level, since it's the same db model being indexed in the same capacity/fashion).
n0cte commented 2021-09-10 06:56:01 +00:00 (Migrated from github.com)
Copy Link

I tried running sql scripts (for block 54 in my environment):

Query:

SELECT storage_cids.cid, data, block_number, was_storage_removed(storage_path, block_number, '0x64e75b19d9ed7b06213b57d69d1ca03e5164071c33016620c8f9e9f9861cce62') AS removed
FROM eth.storage_cids
  INNER JOIN eth.state_cids ON (storage_cids.state_id = state_cids.id)
  INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id)
  INNER JOIN public.blocks ON (storage_cids.mh_key = blocks.key)
WHERE state_leaf_key = '0x3336da5dab0fa31eb4f9e1408dcacdb7bc0fc5da9837d1629be1010049e56dd8'
  AND storage_leaf_key = '0x405787fa12a823e0f2b7631cc41b3ba8828b3321ca811111fa75cd3aa3bb5ace'
  AND block_number <= (SELECT block_number FROM eth.header_cids WHERE block_hash = '0x64e75b19d9ed7b06213b57d69d1ca03e5164071c33016620c8f9e9f9861cce62')
  AND header_cids.id = (SELECT canonical_header_id(block_number))
  ORDER BY block_number DESC

Result:

cid | data | block_number | removed
bagmacgzasfla7wzuessejihdtrxqd5lqxc57egukbbricizz2ssrltex4uvq | 7KAwV4f6Eqgj4PK3YxzEGzuogoszIcqBERH6dc06o7tazoqJNjXJrcXeoAAA | 53 | false

Query:

SELECT state_cids.cid, data, block_number, was_state_removed(state_path, block_number, '0x64e75b19d9ed7b06213b57d69d1ca03e5164071c33016620c8f9e9f9861cce62') AS removed
FROM eth.state_cids
  INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id)
  INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key)
WHERE state_leaf_key = '0x3336da5dab0fa31eb4f9e1408dcacdb7bc0fc5da9837d1629be1010049e56dd8'
  AND block_number <= 54
ORDER BY block_number DESC

Result:

cid | data | block_number | removed
baglacgzayxjemamg64rtzet6pwznzrydydsqbnstzkbcoo337lmaixmfurya | (EMPTY) | 54 | false
baglacgzatlspm5do4dernn2fwdmb42d2u67snrvhsaviarxbkeenxikfeyjq | -GmgIDbaXasPox60-eFAjcrNt7wPxdqYN9Fim-EBAEnlbdi4RvhEAYCgWTyxXtWK9XGnhbGQ8s-BRv7G6p_T2uIOEc3otVN9b8ugSFMHT4v_2dHaXBDzcd0PHJYgo92KC0xVQyynWsH9-1k | 53 | true

I think both scripts are working not correctly. Function was_storage_removed return not correct result. Second sql with LIMIT 1 also return bad result in function was_state_removed

I used two versions of docker:

vulcanize/dapptools:v0.29.0-v1.10.8-statediff-0.0.26
vulcanize/statediff-migrations:v0.6.0

vulcanize/dapptools:v0.29.0-v1.10.2-statediff-0.0.19
vulcanize/statediff-migrations:v0.4.0
I tried running sql scripts (for block 54 in my environment): Query: ```sql SELECT storage_cids.cid, data, block_number, was_storage_removed(storage_path, block_number, '0x64e75b19d9ed7b06213b57d69d1ca03e5164071c33016620c8f9e9f9861cce62') AS removed FROM eth.storage_cids INNER JOIN eth.state_cids ON (storage_cids.state_id = state_cids.id) INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id) INNER JOIN public.blocks ON (storage_cids.mh_key = blocks.key) WHERE state_leaf_key = '0x3336da5dab0fa31eb4f9e1408dcacdb7bc0fc5da9837d1629be1010049e56dd8' AND storage_leaf_key = '0x405787fa12a823e0f2b7631cc41b3ba8828b3321ca811111fa75cd3aa3bb5ace' AND block_number <= (SELECT block_number FROM eth.header_cids WHERE block_hash = '0x64e75b19d9ed7b06213b57d69d1ca03e5164071c33016620c8f9e9f9861cce62') AND header_cids.id = (SELECT canonical_header_id(block_number)) ORDER BY block_number DESC ``` Result: ``` cid | data | block_number | removed bagmacgzasfla7wzuessejihdtrxqd5lqxc57egukbbricizz2ssrltex4uvq | 7KAwV4f6Eqgj4PK3YxzEGzuogoszIcqBERH6dc06o7tazoqJNjXJrcXeoAAA | 53 | false ``` Query: ```sql SELECT state_cids.cid, data, block_number, was_state_removed(state_path, block_number, '0x64e75b19d9ed7b06213b57d69d1ca03e5164071c33016620c8f9e9f9861cce62') AS removed FROM eth.state_cids INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id) INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key) WHERE state_leaf_key = '0x3336da5dab0fa31eb4f9e1408dcacdb7bc0fc5da9837d1629be1010049e56dd8' AND block_number <= 54 ORDER BY block_number DESC ``` Result: ``` cid | data | block_number | removed baglacgzayxjemamg64rtzet6pwznzrydydsqbnstzkbcoo337lmaixmfurya | (EMPTY) | 54 | false baglacgzatlspm5do4dernn2fwdmb42d2u67snrvhsaviarxbkeenxikfeyjq | -GmgIDbaXasPox60-eFAjcrNt7wPxdqYN9Fim-EBAEnlbdi4RvhEAYCgWTyxXtWK9XGnhbGQ8s-BRv7G6p_T2uIOEc3otVN9b8ugSFMHT4v_2dHaXBDzcd0PHJYgo92KC0xVQyynWsH9-1k | 53 | true ``` I think both scripts are working not correctly. Function `was_storage_removed` return not correct result. Second sql with `LIMIT 1` also return bad result in function `was_state_removed` I used two versions of docker: ``` vulcanize/dapptools:v0.29.0-v1.10.8-statediff-0.0.26 vulcanize/statediff-migrations:v0.6.0 ``` ``` vulcanize/dapptools:v0.29.0-v1.10.2-statediff-0.0.19 vulcanize/statediff-migrations:v0.4.0 ```
i-norden commented 2021-09-13 13:40:26 +00:00
Member
Copy Link

Thanks @n0cte, I think @arijitAD has fixed the state query issue in #99 but it's not clear what the issue is for storage.

I should have noticed this sooner, but the reason we needed to use these was_x_removed functions was because, prior to https://github.com/vulcanize/go-ethereum/pull/58, we did not have the leaf key present for "Removed" nodes and so we would not find them using the basic query against leaf_key. Now that we have leaf_key present, these functions are unnecessary (and indeed broken in this new context).

E.g. instead of

SELECT state_cids.cid, data, was_state_removed(state_path, block_number, $2) AS removed
FROM eth.state_cids
    INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id)
    INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key)
WHERE state_leaf_key = $1
AND block_number <= (SELECT block_number
    FROM eth.header_cids
    WHERE block_hash = $2)
AND header_cids.id = (SELECT canonical_header_id(block_number))
ORDER BY block_number DESC
LIMIT 1

We can just do

SELECT state_cids.cid, data, node_type
FROM eth.state_cids
    INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id)
    INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key)
WHERE state_leaf_key = $1
AND block_number <= (SELECT block_number
    FROM eth.header_cids
    WHERE block_hash = $2)
AND header_cids.id = (SELECT canonical_header_id(block_number))
ORDER BY block_number DESC
LIMIT 1

This will return the latest node entry for a specific leaf key, including "Removed" types. We can check if the node_type returned is "Removed" and if it is handle the zero value appropriately. Alternatively, if we want to get the latest state of a node at a leaf_key (last value before it was "Removed"), we just need to add a condition to the query to exclude "Removed" node_types.

I.e.

SELECT state_cids.cid, data
FROM eth.state_cids
    INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id)
    INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key)
WHERE state_leaf_key = $1
AND block_number <= (SELECT block_number
    FROM eth.header_cids
    WHERE block_hash = $2)
AND header_cids.id = (SELECT canonical_header_id(block_number))
AND node_type IS NOT 3
ORDER BY block_number DESC
LIMIT 1
Thanks @n0cte, I think @arijitAD has fixed the state query issue in #99 but it's not clear what the issue is for storage. I should have noticed this sooner, but the reason we needed to use these `was_x_removed` functions was because, prior to https://github.com/vulcanize/go-ethereum/pull/58, we did not have the leaf key present for "Removed" nodes and so we would not find them using the basic query against leaf_key. Now that we have leaf_key present, these functions are unnecessary (and indeed broken in this new context). E.g. instead of ```sql SELECT state_cids.cid, data, was_state_removed(state_path, block_number, $2) AS removed FROM eth.state_cids INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id) INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key) WHERE state_leaf_key = $1 AND block_number <= (SELECT block_number FROM eth.header_cids WHERE block_hash = $2) AND header_cids.id = (SELECT canonical_header_id(block_number)) ORDER BY block_number DESC LIMIT 1 ``` We can just do ```sql SELECT state_cids.cid, data, node_type FROM eth.state_cids INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id) INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key) WHERE state_leaf_key = $1 AND block_number <= (SELECT block_number FROM eth.header_cids WHERE block_hash = $2) AND header_cids.id = (SELECT canonical_header_id(block_number)) ORDER BY block_number DESC LIMIT 1 ``` This will return the latest node entry for a specific leaf key, including "Removed" types. We can check if the `node_type` returned is "Removed" and if it is handle the zero value appropriately. Alternatively, if we want to get the latest state of a node at a leaf_key (last value before it was "Removed"), we just need to add a condition to the query to exclude "Removed" node_types. I.e. ```sql SELECT state_cids.cid, data FROM eth.state_cids INNER JOIN eth.header_cids ON (state_cids.header_id = header_cids.id) INNER JOIN public.blocks ON (state_cids.mh_key = blocks.key) WHERE state_leaf_key = $1 AND block_number <= (SELECT block_number FROM eth.header_cids WHERE block_hash = $2) AND header_cids.id = (SELECT canonical_header_id(block_number)) AND node_type IS NOT 3 ORDER BY block_number DESC LIMIT 1 ```
arijitAD commented 2021-09-17 12:04:02 +00:00 (Migrated from github.com)
Copy Link

Fixed this in https://github.com/vulcanize/ipld-eth-server/pull/98

Fixed this in https://github.com/vulcanize/ipld-eth-server/pull/98
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels   
bug

Something isn't working

  
critical

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
Epic

   
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
Integration tests

   
invalid

This doesn't seem right

  
question

Further information is requested

  
v5

   
wontfix

This will not be worked on

  
Copied from Github

An issue or PR manually copied from GitHub.

  
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
bug
critical
documentation
duplicate
enhancement
Epic
good first issue
help wanted
Integration tests
invalid
question
v5
wontfix
Copied from Github
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/ipld-eth-server#95
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?