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

all: use uint256 in state #28598

Merged
merged 2 commits into from
Jan 23, 2024
Merged

all: use uint256 in state #28598

merged 2 commits into from
Jan 23, 2024

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 24, 2023

This change makes use of uin256 to represent balance in state. It touches primarily upon statedb, stateobject and state processing, trying to avoid changes in transaction pools, core types, rpc and tracers.

Addresses #28578

Some notable differences:

  • A big.Int spits out the decimal form, in String() and MarshalJson etc, whereas uint256.Int spits out hex-form. This caused a few failures in the tracer-engine, since the bignum js did not expect to parse hex. It also causes a test-failure in TestIterativeDump due to output format change. I'm not sure if we should fix these, or if we should instead switch uint256.Int over to default to decimal output over hex, to maintain compatibility with big.Int. (See conversion: behavioural changes in String, MarshalText and MarshalJSON holiman/uint256#144)

For reviewers

  • All production-code changes are in the first commit.
  • The remaining commits are only changes to _test files (or utilities for use in testing, e.g. /tests/)

There are some subtleties to be aware of. Consider the following example:

func VerifyThingy(value *big.Int) (uint64, error){
    value.Add(value, value) // <-- Can overflow with uint256
    if !value.IsUint64(){ // <-- Overflow makes it so that it now fits into 64 bits, and no error is emitted 
        return errors.New("Oh no goes above 64 bits")
    }
    return value.Uint64(), nil
}    

The code above is perfectly fine with big.Int, but as soon as we switch to uint256.Int, the code is buggy. The reason for this is that Add, Mul and Sub in big.Int are lossless, whereas they are lossy in uint256.Int. Therefore, we need to ensure that in all places where it matter, the second return-value from these operations, overflow, is checked.

And to be extra clear: the github diffview may hide the place where this type of error sits, since the diff is far away from where the bug is.

@@ -27,4 +31,6 @@ var (
Big32 = big.NewInt(32)
Big256 = big.NewInt(256)
Big257 = big.NewInt(257)

U2560 = uint256.NewInt(0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
U2560 = uint256.NewInt(0)
U256_0 = uint256.NewInt(0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did that originally. But my editor went ahead and changed it. I think there might be something special about numerics and underscores.

@holiman holiman force-pushed the u256_state branch 2 times, most recently from e373279 to 47c0888 Compare November 28, 2023 09:18
@holiman
Copy link
Contributor Author

holiman commented Nov 30, 2023

This is going to bitrot. Please review. I'd say that the most important parts to check are :

  • state_transition.go
  • instructions.go
  • evm.go
  • transaction pool validations
  • Checking if there are any generated files that I have missed to regenerate

@@ -260,7 +265,8 @@ func (st *StateTransition) buyGas() error {
st.gasRemaining += st.msg.GasLimit

st.initialGas = st.msg.GasLimit
st.state.SubBalance(st.msg.From, mgval)
mgvalU256, _ := uint256.FromBig(mgval)
Copy link
Member

Choose a reason for hiding this comment

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

This can not overflow because mgval is guaranteed to be <= balanceCheck and we check that balance check does not overflow

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM, I had some questions/clarifications but I think the PR is good to go

@holiman
Copy link
Contributor Author

holiman commented Nov 30, 2023

I think the PR is good to go

Thanks! I am pushing a change re the stack pushes though

@itsdevbear
Copy link
Contributor

this PR brings me so much joy inside

expected := new(big.Int).Add(
new(big.Int).SetUint64(block.GasUsed()*block.Transactions()[0].GasTipCap().Uint64()),
ethash.ConstantinopleBlockReward,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is kind of lazy here. Should we just compute the expected value as uint256 as well and then compare?

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 we do it in big.Int, then the test can simply do the calculations, and not worry about overflows or negatives. You might call it lazy, but on the other hand, it means that the test becomes less encumbered with overflow spot-checks.

All in all, I think it's better the way it is (I started changing it, and didn't find the changed code to be an improvement)

@fjl
Copy link
Contributor

fjl commented Dec 8, 2023

I wonder if we should use non-pointer uint256.Int to represent the balance. That way, we could save a lot of new() calls and rely on implicit copy. It might even reduce allocations during state transition.

@holiman
Copy link
Contributor Author

holiman commented Dec 8, 2023

I wonder if we should use non-pointer uint256.Int to represent the balance.

Yeah, that might be a good idea. Maybe I'll do a follow-up commit, and if it becomes too large, I'll put it into a separate PR

@rjl493456442
Copy link
Member

Deployed in on bench6 for a full sync, just to ensure nothing broken.

Copy link

@shidaxi shidaxi left a comment

Choose a reason for hiding this comment

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

.

This change makes use of uin256 to represent balance in state. It touches
primarily upon statedb, stateobject and state processing, trying to
avoid changes in transaction pools, core types, rpc and tracers.
core, core/txpool/blobpool: fix tests
core/vm: fix tests
core/txpool/legacypool: fix tests
eth/tracers: fix tests
trie: fix tests
core/state, core/vm/runtime: fix tests
eth: fix tests
miner: fix tests
@holiman holiman added this to the 1.13.11 milestone Jan 23, 2024
@holiman holiman merged commit a5a4fa7 into ethereum:master Jan 23, 2024
3 checks passed
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
This change makes use of uin256 to represent balance in state. It touches primarily upon statedb, stateobject and state processing, trying to avoid changes in transaction pools, core types, rpc and tracers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants