Skip to content

Conversation

@m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Apr 4, 2025

Work towards Pectra upgrade 🚀

dahu33 and others added 24 commits February 25, 2025 09:54
This pull request removes the node copy operation to reduce memory
allocation. Key Changes as below:

**(a) Use `decodeNodeUnsafe` for decoding nodes retrieved from the trie
node reader**

In the current implementation of the MPT, once a trie node blob is
retrieved, it is passed to `decodeNode` for decoding. However,
`decodeNode` assumes the supplied byte slice might be mutated later, so
it performs a deep copy internally before parsing the node.

Given that the node reader is implemented by the path database and the
hash database, both of which guarantee the immutability of the returned
byte slice. By restricting the node reader interface to explicitly
guarantee that the returned byte slice will not be modified, we can
safely replace `decodeNode` with `decodeNodeUnsafe`. This eliminates the
need for a redundant byte copy during each node resolution.

**(b) Modify the trie in place**

In the current implementation of the MPT, a copy of a trie node is
created before any modifications are made. These modifications include:
- Node resolution: Converting the value from a hash to the actual node.
- Node hashing: Tagging the hash into its cache.
- Node commit: Replacing the children with its hash.
- Structural changes: For example, adding a new child to a fullNode or
replacing a child of a shortNode.

This mechanism ensures that modifications only affect the live tree,
leaving all previously created copies unaffected.

Unfortunately, this property leads to a huge memory allocation
requirement. For example, if we want to modify the fullNode for n times,
the node will be copied for n times.

In this pull request, all the trie modifications are made in place. In
order to make sure all previously created copies are unaffected, the
`Copy` function now will deep-copy all the live nodes rather than the
root node itself.

With this change, while the `Copy` function becomes more expensive, it's
totally acceptable as it's not a frequently used one. For the normal
trie operations (Get, GetNode, Hash, Commit, Insert, Delete), the node
copy is not required anymore.
This removes DB schema for LES related db entries. LES has been non-functional
since the merge.
Simple bugfix to include the access-list in the gas-estimation step of
the ABI bindings code.
Add support for state overrides in eth_createAccessList. This will make the method consistent
with other execution methods.

---------

Co-authored-by: Sina Mahmoodi <[email protected]>
eth/catalyst: force sync of txpool before clearing subpools in Rollback
…thereum#31500)

This PR changes log indexer error handling so that if an indexing error
happens then it disables the indexer and reverts to unindexed more
without resetting the database (except in case of a failed database
init).
Resetting the database on the first error would probably be overkill as
a client update might fix this without having to reindex the entire
history. It would also make debugging very hard. On the other hand,
these errors do not resolve themselves automatically so constantly
retrying makes no sense either. With these changes a new attempt to
resume indexing is made every time the client is restarted.
The PR also fixes ethereum#31491
which originated from the tail indexer trying to resume processing a
failed map renderer.

---------

Co-authored-by: Felix Lange <[email protected]>
Ignores all hand-built binaries (built with go build, everything built
with make is already ignored)
This PR adds `rawdb.SafeDeleteRange` and uses it for range deletion in
`core/filtermaps`. This includes deleting the old bloombits database,
resetting the log index database and removing index data for unindexed
tail epochs (which previously weren't properly implemented for the
fallback case).
`SafeDeleteRange` either calls `ethdb.DeleteRange` if the node uses the
new path based state scheme or uses an iterator based fallback method
that safely skips trie nodes in the range if the old hash based state
scheme is used. Note that `ethdb.DeleteRange` also has its own iterator
based fallback implementation in `ethdb/leveldb`. If a path based state
scheme is used and the backing db is pebble (as it is on the majority of
new nodes) then `rawdb.SafeDeleteRange` uses the fast native range
delete.
Also note that `rawdb.SafeDeleteRange` has different semantics from
`ethdb.DeleteRange`, it does not automatically return if the operation
takes a long time. Instead it receives a `stopCallback` that can
interrupt the process if necessary. This is because in the safe mode
potentially a lot of entries are iterated without being deleted (this is
definitely the case when deleting the old bloombits database which has a
single byte prefix) and therefore restarting the process every time a
fixed number of entries have been iterated would result in a quadratic
run time in the number of skipped entries.

When running in safe mode, unindexing an epoch takes about a second,
removing bloombits takes around 10s while resetting a full log index
might take a few minutes. If a range delete operation takes a
significant amount of time then log messages are printed. Also, any
range delete operation can be interrupted by shutdown (tail uinindexing
can also be interrupted by head indexing, similarly to how tail indexing
works). If the last unindexed epoch might have "dirty" index data left
then the indexed map range points to the first valid epoch and
`cleanedEpochsBefore` points to the previous, potentially dirty one. At
startup it is always assumed that the epoch before the first fully
indexed one might be dirty. New tail maps are never rendered and also no
further maps are unindexed before the previous unindexing is properly
cleaned up.

---------

Co-authored-by: Gary Rong <[email protected]>
Co-authored-by: Felix Lange <[email protected]>
@janezpodhostnik
Copy link

I only reviewed this last one, since if this one is ok, they all are.

I found it looks ok, but there are some files in our repo that are gone upstream. One of those is p2p/simulations/simulation.go it looks like it was removed here:
ethereum#30250

can you take a look

@m-Peter
Copy link
Collaborator Author

m-Peter commented Apr 7, 2025

I only reviewed this last one, since if this one is ok, they all are.

I found it looks ok, but there are some files in our repo that are gone upstream. One of those is p2p/simulations/simulation.go it looks like it was removed here: ethereum#30250

can you take a look

@janezpodhostnik Good catch 💯 I have removed the entire directory in 51e8259 . This wouldn't cause a problem anyway, because it's not a module that we use anywhere, and it's actually only a CLI tool. Nevertheless, I removed it so we are on par with the upstream Geth.

@janezpodhostnik
Copy link

Yeah, it wouldn't be a problem, but it might cause confusion later. Thanks for fixing it!

Another file seems to have been deleted with v1.14.9 https://github.com/ethereum/go-ethereum/blob/v1.14.9/core/forkchoice.go. Can you also remove that one? the rest looks ok.

@m-Peter
Copy link
Collaborator Author

m-Peter commented Apr 7, 2025

Yeah, it wouldn't be a problem, but it might cause confusion later. Thanks for fixing it!

Another file seems to have been deleted with v1.14.9 https://github.com/ethereum/go-ethereum/blob/v1.14.9/core/forkchoice.go. Can you also remove that one? the rest looks ok.

Good point 👍 Removed this one as well in fb036e1

Copy link

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

🚀

Base automatically changed from mpeter/update-go-ethereum-v1.15.6 to master April 10, 2025 06:56
@m-Peter m-Peter merged commit 4d02203 into master Apr 10, 2025
@m-Peter m-Peter deleted the mpeter/update-go-ethereum-v1.15.7 branch April 10, 2025 06:59
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.