-
Notifications
You must be signed in to change notification settings - Fork 21.1k
core/state: implement optional BAL construction in statedb #31959
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
Conversation
Oh wow, I'm surprised that it takes that little code to built this. I guess we have a pretty good abstraction with the statedb |
1e6266f
to
cebf143
Compare
accounts/abi/unpack_test.go
Outdated
@@ -1014,128 +1014,134 @@ func TestPackAndUnpackIncompatibleNumber(t *testing.T) { | |||
cases := []struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue why this file is showing diffs. I've compared locally with master and there is no diff. Might be a github bug.
@@ -681,7 +681,7 @@ func DeveloperGenesisBlock(gasLimit uint64, faucet *common.Address) *Genesis { | |||
}, | |||
} | |||
if faucet != nil { | |||
genesis.Alloc[*faucet] = types.Account{Balance: new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 256), big.NewInt(9))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BAL max balance is limited to 12 bytes. Geth will panic if an attempt is made to record a larger balance in the BAl.
I use dev mode to test changes here so I reduced the allocation here to allow for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now 16 bytes in the spec.
b13d49d
to
e42ed68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why you need to add a tracer, and simply not use the witness to build the BAL?
@@ -35,6 +35,7 @@ type Config struct { | |||
ExtraEips []int // Additional EIPS that are to be enabled | |||
|
|||
StatelessSelfValidation bool // Generate execution witnesses and self-check against them (testing purpose) | |||
BALConstruction bool // Generate BAL for executed blocks (testing purposes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you define an eip field in the rules (see eips.go) and use isEIPWhatever
instead of adding another new field here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BAL construction is only performed when creating a block, not verifying. So it's not possible to activate based on fork alone.
go.sum
Outdated
@@ -116,6 +118,8 @@ github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= | |||
github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= | |||
github.com/ferranbt/fastssz v0.1.2 h1:Dky6dXlngF6Qjc+EfDipAkE83N5I5DE68bY6O0VLNPk= | |||
github.com/ferranbt/fastssz v0.1.2/go.mod h1:X5UPrE2u1UJjxHA8X54u04SBwdAQjG2sFtWs39YxyWs= | |||
github.com/ferranbt/fastssz v0.1.4 h1:OCDB+dYDEQDvAgtAGnTSidK1Pe2tW3nFV40XyMkTeDY= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Peter's lib not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really like to keep fastszz out, it's really tricky to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've tentatively removed SSZ from the spec in favor of RLP. This branch hasn't been updated against #31948 . I will rebase this as soon as that's merged.
CancunTime *uint64 `json:"cancunTime,omitempty"` // Cancun switch time (nil = no fork, 0 = already on cancun) | ||
PragueTime *uint64 `json:"pragueTime,omitempty"` // Prague switch time (nil = no fork, 0 = already on prague) | ||
OsakaTime *uint64 `json:"osakaTime,omitempty"` // Osaka switch time (nil = no fork, 0 = already on osaka) | ||
GlamsterdamTime *uint64 `json:"glamsterdamTime,omitempty"` // Glamsterdam switch time (nil = no fork, 0 = already on glamsterdam) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make this another PR
core/types/bal/bal.go
Outdated
@@ -0,0 +1,227 @@ | |||
package bal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, and for the sake of future maintainers, I think it's worth spelling the whole "block access list" out. yes it's long, but bal is just too non-descriptive for people who are not involved in BALs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just hate the way blockaccesslist
looks...
IsEIP2929, IsEIP4762 bool | ||
IsByzantium, IsConstantinople, IsPetersburg, IsIstanbul bool | ||
IsBerlin, IsLondon bool | ||
IsMerge, IsShanghai, IsCancun, IsPrague, IsOsaka, IsGlamsterdam bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you did create that field, so I understand your need to do BALConstruction
go.mod
Outdated
@@ -24,7 +24,7 @@ require ( | |||
github.com/ethereum/c-kzg-4844/v2 v2.1.0 | |||
github.com/ethereum/go-verkle v0.2.2 | |||
github.com/fatih/color v1.16.0 | |||
github.com/ferranbt/fastssz v0.1.2 | |||
github.com/ferranbt/fastssz v0.1.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh crap we already had it...
@@ -111,7 +111,7 @@ type btHeaderMarshaling struct { | |||
ExcessBlobGas *math.HexOrDecimal64 | |||
} | |||
|
|||
func (t *BlockTest) Run(snapshotter bool, scheme string, witness bool, tracer *tracing.Hooks, postCheck func(error, *core.BlockChain)) (result error) { | |||
func (t *BlockTest) Run(snapshotter bool, scheme string, witness bool, bal bool, tracer *tracing.Hooks, postCheck func(error, *core.BlockChain)) (result error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the issue I have with this BALConstruction
: we will have to add a flag all over the place, when it's a glamsterdam thing (actually maybe not, but it's a well-identified eip which activation can be checked with rules)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above: bal construction is not strictly dependent on fork activation, it will be activated when we are creating a block (and glamsterdam is active)
34a7a2b
to
9ef8dcb
Compare
Beacon root contract update and withdrawals are not properly handled here. However, the BAL spec does not specify how state mutations which occur before/after execution of block txs are handled. Trying to get this spec'd and then will fix in this branch. |
f5d6d01
to
b5948ee
Compare
closing this as it has become stale. I'm not sure where things will land after #32263 is completed and refactored. |
This PR implements changes to Geth which allow for the creation of block-level access lists, generated when executing blocks for import.
Ultimately, block access lists will be created from within the miner and validated as part of block execution. However, being able to generate them from historical blocks is useful for collecting access lists across large ranges of blocks. So the changes in this PR aim to facilitate data collection which will inform the continued development of the spec.