Update Astria-geth to use geth 1.14.3#21
Merged
Conversation
4b8ecbe to
b757f5b
Compare
2b28ac2 to
1fbd448
Compare
bharath-123
commented
Jun 4, 2024
Comment on lines
-136
to
-149
| if merger := eth.Merger(); !merger.PoSFinalized() { | ||
| merger.FinalizePoS() | ||
| } |
Contributor
Author
There was a problem hiding this comment.
There is some legacy code being removed in geth.
Contributor
Author
There was a problem hiding this comment.
bharath-123
commented
Jun 4, 2024
| require.True(t, balanceDiff.Cmp(big.NewInt(1000000000000000000)) == 0, "Chain destination address balance is not correct") | ||
| balanceDiff := new(uint256.Int).Sub(chainDestinationAddressBalanceAfter, chainDestinationAddressBalanceBefore) | ||
| fmt.Printf("Balance diff: %s\n", balanceDiff.String()) | ||
| require.True(t, balanceDiff.Cmp(uint256.NewInt(1000000000000000000)) == 0, "Chain destination address balance is not correct") |
Contributor
Author
There was a problem hiding this comment.
examples of some changes to uint256.Int. This is because the stateDB is now returning only uint256s.
bharath-123
commented
Jun 4, 2024
core/state_transition.go
Outdated
| @@ -364,7 +381,7 @@ func (st *StateTransition) preCheck() error { | |||
| func (st *StateTransition) TransitionDb() (*ExecutionResult, error) { | |||
| // if this is a deposit tx, we only need to mint funds and no gas is used. | |||
| if st.msg.IsDepositTx { | |||
| st.state.AddBalance(st.msg.From, st.msg.Value) | |||
| st.state.AddBalance(st.msg.From, uint256.MustFromBig(st.msg.Value), tracing.BalanceIncreaseAstriaDepositTx) | |||
Contributor
Author
There was a problem hiding this comment.
example of requirement of uint256 and adding tracing reason
bharath-123
commented
Jun 4, 2024
|
|
||
| // collect base fee instead of burn | ||
| if rules.IsLondon && st.evm.Context.Coinbase.Cmp(common.Address{}) != 0 { | ||
| baseFee := new(big.Int).SetUint64(st.gasUsed()) | ||
| baseFee.Mul(baseFee, st.evm.Context.BaseFee) | ||
| st.state.AddBalance(st.evm.Context.Coinbase, baseFee) | ||
| st.state.AddBalance(st.evm.Context.Coinbase, uint256.MustFromBig(baseFee), tracing.BalanceIncreaseRewardTransactionFee) |
Contributor
Author
There was a problem hiding this comment.
Update from big.int to uint256
bharath-123
commented
Jun 4, 2024
| @@ -175,7 +176,10 @@ func (payload *Payload) ResolveFull() *engine.ExecutionPayloadEnvelope { | |||
| } | |||
|
|
|||
| // buildPayload builds the payload according to the provided parameters. | |||
| func (w *worker) buildPayload(args *BuildPayloadArgs) (*Payload, error) { | |||
| func (miner *Miner) buildPayload(args *BuildPayloadArgs) (*Payload, error) { | |||
Contributor
Author
There was a problem hiding this comment.
key change: buildPayload is now based on Miner rather than worker.
bharath-123
commented
Jun 4, 2024
| @@ -20,57 +20,18 @@ import ( | |||
| "errors" | |||
Contributor
Author
There was a problem hiding this comment.
the worker.go file is now very simplified.
Member
|
I think we might need some fresh updates after Elizabeth PR merge, but otherwise LGTM |
Co-authored-by: Felix Lange <fjl@twurst.com>
* node: fix test if directory already exists * node: remove test
…tion (#29169) * eth: drop support for forward sync triggers and head block packets * consensus, eth: enforce always merged network * eth: fix tx looper startup and shutdown * cmd, core: fix some tests * core: remove notion of future blocks * core, eth: drop unused methods and types
EIP-1559 burns the base fee. This doesn't make sense for rollups. The set fee recipient should collect it.
e8ceb69 to
14a96f6
Compare
a7ab4e4 to
5f9724b
Compare
joroshiba
reviewed
Jun 12, 2024
joroshiba
approved these changes
Jun 19, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR rebases astria-geth on top of geth 1.14.3. Below are the main changes which affect astria-geth:
There seems to be an overall shift in the Geth code to prefer
uint256.NewIntoverbig.NewInt. There are some places in our codebase where I had to typecastbigInttouint256. But this usage also seems to be pretty inconsistent overall. I suspect as geth updates, most of the codebase will eventually useuint256.The block building process under the miner directory used to have a worker struct in pre 1.14 geth. The payloadBuilding process was as method on the
workerstruct. With geth 1.14 there is a major refactor to simplify the codebase by removing theworkerstruct. Now we need to define all the methods under theMinerstruct. This required some modifications tofillAstriaTransactionsandcommitAstriaTransactionsto be implemented on theMinerstruct.Files Changed
miner/payload_building.go
miner/worker.go
Methods like
AddBalanceandSubBalancewhich update the balance of the user in the stateDB now requires to add an extra parameter indicating the reason of change of balance. This is used for live tracing introduced in geth 1.14. This is basically an enum. I added a new value calledBalanceIncreaseAstriaDepositTxwhich indicates a balance increase due to an astria deposit tx.Files Changed:
core/state_transition.go
tracing/hooks.go
Geth re-worked their testing infra. Integration tests use a
SimulatedBackendwhich mocked the geth RPC. It simulates api calls such assendTransaction. Pre geth 1.14, whensendTransactionwas called withSimulatedBackend, txs bypassed the mempool and were directly added to the block. Post geth 1.14, this is not the case anymore and the txs are sent to the mempool. This breaks certain tests for us, since we rely on txs to come from the Execution API. I have commented out the tests for now but it ll be worth adding a simulated backend which bypasses the tx for our purposes. It ll be useful to test certain codepaths.The following tests have been commented out:
a. TestWaitDeployed
b. TestGolangBindings
c. TestEthClient/AtFunctions
d. TestForkResendTx
Testing:
make testruns without problems. I deployed this astria-evm locally using the monorepo charts and was able to run spamooor scenarios successfully against it.