core/state: read from snap error should continue to read from db

[Halimao]

// If the snapshot is unavailable or reading from it fails, load from the database.
if s.db.snap == nil || err != nil {

If I understood the comment correctly, we should cover the original err with _, content, _, err = rlp.Split(enc).

If not, I think it would be fine to return common.Hash{} when there is an error with rlp.Split(enc).

_, content, _, err := rlp.Split(enc)
if err != nil {
    s.db.setError(err)
        return common.Hash{}
}

[fjl]

I don't see how this improves anything. If we get something in content we may as well return it.

[Halimao]

@fjl Hi sir, what I mean is:

If we don't cover the err enc, err = s.db.snap.Storage(s.addrHash, crypto.Keccak256Hash(key.Bytes())), we won't into the if s.db.snap == nil || err != nil {} block.

As s.db.snap != nil and the err of s.db.snap.Storage is nil too.

[Halimao]

This PR is to ensure us can go into the if s.db.snap == nil || err != nil {} block when If the snapshot is unavailable or reading from it fails, load from the database

[fjl]

Ahh, I see, it's because err is shadowed. I still think it makes no difference in the end. The idea with the condition below is just to ensure that, while the snapshot is being built or if it not in use, we will use another method to retrieve the item. If the snapshot is there, but contains an invalid item, we don't want to skip over that and hit the DB.

[Halimao]

Ahh, I see, it's because err is shadowed. I still think it makes no difference in the end. The idea with the condition below is just to ensure that, while the snapshot is being built or if it not in use, we will use another method to retrieve the item. If the snapshot is there, but contains an invalid item, we don't want to skip over that and hit the DB.

@fjl Then I think we should return it directly by

_, content, _, err := rlp.Split(enc)
if err != nil {
    s.db.setError(err)
        return common.Hash{}
}

Or it would update s.originStorage[key] value by

s.originStorage[key] = value

[Halimao]

Ahh, I see, it's because err is shadowed. I still think it makes no difference in the end. The idea with the condition below is just to ensure that, while the snapshot is being built or if it not in use, we will use another method to retrieve the item. If the snapshot is there, but contains an invalid item, we don't want to skip over that and hit the DB.

@fjl Then I think we should return it directly by

_, content, _, err := rlp.Split(enc)
if err != nil {
  s.db.setError(err)
        return common.Hash{}
}

Or it would update s.originStorage[key] value by

s.originStorage[key] = value

@fjl Hi sir, shall we return directly here?