fix *uint256 reuse issue

[qizhou]

This fixes a weird issue that was found in fault proof.

The key problem is that the returned balance is a pointer that can be modified by callers. In most cases, the balance is not reused. However, in some cases that balances is zero, statedb returns a cached zero uint256 (see

op-geth/core/state/statedb.go

Line 330 in e40be92

return common.U2560

). If the cache zero uint256 is modified, geth will result in some unexpected behavior such as account overflow issue.

Test:

Before

1, fault proof will fail after verifying a SGT tx
2, dump the state, and find an account 0xfffffffffffffffffffffffffffffffffffffffe has a negative balance.

After

1, pass fault proof via make verify-devnet
2, 0xfffffffffffffffffffffffffffffffffffffffe has zero balance

[blockchaindevsh]

Looks like no only when zero, even if non-zero , GetBalance will return a reference to the balance field of state object. This is dangerous and makes it possible to change balance skipping the SetBalance. Do you plan to fix the issue in GetBalance and upstream it instead?

[qizhou]

Looks like no only when zero, even if non-zero , GetBalance will return a reference to the balance field of state object. This is dangerous and makes it possible to change balance skipping the SetBalance. Do you plan to fix the issue in GetBalance and upstream it instead?

I feel like this behavior is intentional in geth - I guess that it is good for performance, and it is the caller's responsibility to not reuse the object.

[blockchaindevsh]

Looks like no only when zero, even if non-zero , GetBalance will return a reference to the balance field of state object. This is dangerous and makes it possible to change balance skipping the SetBalance. Do you plan to fix the issue in GetBalance and upstream it instead?

I feel like this behavior is intentional in geth - I guess that it is good for performance, and it is the caller's responsibility to not reuse the object.

Gotcha, maybe we should follow geth's convention and change all places that reuse balance to create a new value instead of clone?

[qizhou]

Looks like no only when zero, even if non-zero , GetBalance will return a reference to the balance field of state object. This is dangerous and makes it possible to change balance skipping the SetBalance. Do you plan to fix the issue in GetBalance and upstream it instead?

I feel like this behavior is intentional in geth - I guess that it is good for performance, and it is the caller's responsibility to not reuse the object.

Gotcha, maybe we should follow geth's convention and change all places that reuse balance to create a new value instead of clone?

The simple fix will always return a new *uint256 in GetGasBalances - because all other methods will rely on this method, we will not have such an issue. The key here is that we don't care the perf at the moment, but we can optimize it later if profiling tells so - in fact, I am not a big fun of early optimization.

[qzhodl]

It’s really hard to find the root cause. I’m curious - what tool are you using to dump and diff the entire state? Is there any documentation or instruction for it?

[qizhou]

It’s really hard to find the root cause. I’m curious - what tool are you using to dump and diff the entire state? Is there any documentation or instruction for it?

geth dump --datadir ./db $bn will work. In addition, in the code, you can call statedb.Dump() to return the state in json.