State trie garbage collection #15903

zsfelfoldi · 2018-01-17T00:30:03Z

This is an experimental implementation of my garbage collection proposal. Note that LES and fast sync are not working yet on this branch. This is a PoC, actual implementation details might still change considerably.
Automatical background GC is implemented. It also does db compaction for every GC'd database section so it can keep the db size low while syncing blocks one by one. When processing a block, GC is not running so that it does not hurt block processing performance. This means that when full syncing a longer chain, GC can not keep up with the rate of new trie nodes created (they will be collected later but until the chain is synced, the db grows). This is not going to be a serious problem with fast sync. Also, committing every Nth block is hopefully going to help the situation considerably when full syncing.

The two original proposals this PR is based on:
https://github.com/zsfelfoldi/ethereum-docs/blob/master/geth/gc_proposal.md
https://github.com/zsfelfoldi/ethereum-docs/blob/master/geth/gc_proposal_2.md

The final data format uses both position prefixes and separate data/reference entries. For the exact description see:
https://github.com/zsfelfoldi/go-ethereum/blob/gc/core/hashtree/hashtree.go#L19
https://github.com/zsfelfoldi/go-ethereum/blob/gc/trie/encoding.go#L122
https://github.com/zsfelfoldi/go-ethereum/blob/gc/core/state/statedb.go#L611

To do list:

store the latest GC block and iterator pointer in database
if a reorg goes back before the latest GC block, handle it gracefully (either stop syncing with an error message or automatically start a complete resync/fast sync)
make the GC work together with core, trie: intermediate mempool between trie and database #15857
make the currently disabled parts work again (fast sync, LES)
extend fast sync protocol to include position information

GitCop · 2018-01-17T00:30:04Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: d3760fb
Commits must be prefixed with the package(s) they modify
Commit: 5395aa3
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

holiman · 2018-01-17T07:18:44Z

I know there was a gist describing the GC somewhere, could you please either link to it from this PR, or alternatively paste it in full in the PR description?

holiman · 2018-01-17T07:23:37Z

core/blockchain.go

 	// Take ownership of this particular state
 	go bc.update()
 	return bc, nil
 }

+func (bc *BlockChain) hasDataCallback(version uint64) func(position, hash []byte) bool {
+	header := bc.GetHeaderByNumber(version)


While I don't yet understand the full context of how this method is used, I do find it a bit odd that number is used to resolve a header, since a number can be ambiguous.

So what I'm wondering is if whatever uses this method, does it handle reorgs without breaking?

Definitely not. This function verifies a piece of data (identified by hash) being present at the given position in a certain block. This block is the "GC block" which is the oldest block whose state we still want to remember. This block is also the earliest one where we can roll back to. If a longer reorg happens, we should resync the entire chain from the beginning (or do a fast sync). Handling this corner case is not implemented yet.

holiman · 2018-01-17T07:27:38Z

core/blockchain.go

 		return NonStatTy, err
 	}
+	bc.gc.UnlockWrite()


This is a bit playing with fire; holding on to one mutex (bc.mu) while obtaining another mutex. Could lead to race conditions.

For example, the bc.mu.RLock() is called in CurrentBlock, which is called from BackgroundGC, which is a separate thread.

You mean deadlock? It cannot cause a deadlock because the only thing we do under this lock is write or delete state data. Chain mutex is never used while holding this one. Still, I know that using such a lock in Blockchain is critical, but so is the GC. If we keep using it like this, we should document it very well what it does and why it does that. We should somehow avoid deleting trie nodes that reappeared just when GC removed the old entry. I am open to other suggestions though.

Note: my original proposal had an inherently safe db structure:
https://github.com/zsfelfoldi/ethereum-docs/blob/master/geth/gc_proposal.md
Unfortunately this also means that we don't know the exact key when reading and we would need a db iterator for reading every trie node. So far I could not find a db format that is both concurrency-safe and provides good performance. Using this mutex and not starting a GC while processing a block ensures that they usually don't collide and if they do anyway, it won't cause any trouble. From a practical point of view this seems to be a good solution to me if it is properly documented.

holiman · 2018-01-17T07:36:45Z

core/hashtree/gc.go

+				gcCounter = wc - 10000
+				diff = 10000
+			}
+			if diff >= 100 && atomic.LoadInt32(processing) == 0 {


I don't understand.. You verify that processing is not happening here, but it there's nothing preventing it from being 1 immediately after this if-statement has been evaluated. Is that ok?

It is. Processing can start while doing GC, it is usually prevented in order to not hinder block processing. If new state trie nodes are written while collecting keys to be deleted (between the two instances of writeLock being locked in GC) then the collected node entries are not deleted in order to avoid collision. The references still are, but those are concurrency-safe.

holiman · 2018-01-17T07:38:40Z

core/state/statedb.go

+		if lp == 33 && position[32] == 5 {
+			return bytes.Equal(hash, data.CodeHash)
+		}
+		if position[32] != 6 {


Many magic values here, please define them as some sort of consts or enum (iota?) or whatever. Hard to understand otherwise.

holiman · 2018-01-17T07:39:22Z

core/state/statedb.go

@@ -602,3 +605,39 @@ func (s *StateDB) CommitTo(dbw trie.DatabaseWriter, deleteEmptyObjects bool) (ro
 	log.Debug("Trie cache stats after commit", "misses", trie.CacheMisses(), "unloads", trie.CacheUnloads())
 	return root, err
 }
+
+func HasDataCallback(root common.Hash, dbr hashtree.DatabaseReader) func(position, hash []byte) bool {


Documentation, what is the meaning of HasDataCallback?

holiman · 2018-01-17T07:41:21Z

trie/secure_trie.go

-var secureKeyPrefix = []byte("secure-key-")
-
-const secureKeyLength = 11 + 32 // Length of the above prefix + 32byte hash
+const secureKeyLength = 100 //???


holiman · 2018-01-17T07:42:25Z

trie/sync.go

@@ -104,7 +105,8 @@ func (s *TrieSync) AddSubTrie(root common.Hash, depth int, parent common.Hash, c
 		return
 	}
 	key := root.Bytes()
-	blob, _ := s.database.Get(key)
+	panic(nil) // add position


Is this intentional or a leftover from some debugging?

Intentional, until trie syncing is updated for the new structure. Right now I wanted to immediately fail if anyone tried to use it.

holiman · 2018-01-17T07:42:30Z

trie/sync.go

@@ -138,7 +140,8 @@ func (s *TrieSync) AddRawEntry(hash common.Hash, depth int, parent common.Hash)
 	if _, ok := s.membatch.batch[hash]; ok {
 		return
 	}
-	if ok, _ := s.database.Has(hash.Bytes()); ok {
+	panic(nil) // add position


Is this intentional or a leftover from some debugging?

holiman · 2018-01-17T07:42:36Z

trie/sync.go

@@ -220,7 +223,8 @@ func (s *TrieSync) Process(results []SyncResult) (bool, int, error) {
 func (s *TrieSync) Commit(dbw DatabaseWriter) (int, error) {
 	// Dump the membatch into a database dbw
 	for i, key := range s.membatch.order {
-		if err := dbw.Put(key[:], s.membatch.batch[key]); err != nil {
+		panic(nil) // add position


Is this intentional or a leftover from some debugging?

holiman · 2018-01-17T07:42:42Z

trie/sync.go

@@ -296,7 +300,8 @@ func (s *TrieSync) children(req *request, object node) ([]*request, error) {
 			if _, ok := s.membatch.batch[hash]; ok {
 				continue
 			}
-			if ok, _ := s.database.Has(node); ok {
+			panic(nil) // add position


Is this intentional or a leftover from some debugging?

karalabe · 2018-01-17T16:24:19Z

Zsolt, you might want to check out my state trie PR. It reorgs a lot of trie handling, which will probably break your code hard.

…

On Jan 17, 2018 18:18, "Felföldi Zsolt" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/blockchain.go <#15903 (comment)> : > return NonStatTy, err } + bc.gc.UnlockWrite() You mean deadlock? It cannot cause a deadlock because the only thing we do under this lock is write or delete state data. Chain mutex is never used while holding this one. Still, I know that using such a lock in Blockchain is critical, but so is the GC. If we keep using it like this, we should document it very well what it does and why it does that. We should somehow avoid deleting trie nodes that reappeared just when GC removed the old entry. I am open to other suggestions though. Note: my original proposal had an inherently safe db structure: https://github.com/zsfelfoldi/ethereum-docs/blob/master/ geth/gc_proposal.md Unfortunately this also means that we don't know the exact key when reading and we would need a db iterator for reading every trie node. So far I could not find a db format that is both concurrency-safe and provides good performance. Using this mutex and not starting a GC while processing a block ensures that they usually don't collide and if they do anyway, it won't cause any trouble. From a practical point of view this seems to be a good solution to me if it is properly documented. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#15903 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAH6GWYPGmMhKcO0Z-87PQzneH4xbosZks5tLh1ogaJpZM4RgoaE> .

zsfelfoldi · 2018-01-17T16:30:27Z

@karalabe sure, will do. Most of this code will change anyway in the final version, that's why I'm not investing any more time into making the rest of client functions (or tests) work, just documenting my approach. We can discuss the final form of this code after your state trie changes took their more or less final shape.

GitCop · 2018-01-18T01:20:16Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: d3760fb
Commits must be prefixed with the package(s) they modify
Commit: 5395aa3
Commits must be prefixed with the package(s) they modify
Commit: 8f6e4e9
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

zsfelfoldi · 2018-01-18T01:34:56Z

@holiman I did some cleanup and added comments and docs to make the code easier to understand.

zsfelfoldi added 3 commits January 16, 2018 06:18

removed les, light, ethstats

d3760fb

removed state sync

5395aa3

core, trie: new state trie database format with garbage collection

b0c3134

zsfelfoldi added type:feature in progress labels Jan 17, 2018

zsfelfoldi requested review from holiman and karalabe as code owners January 17, 2018 00:30

holiman reviewed Jan 17, 2018

View reviewed changes

docs and cleanup

8f6e4e9

zsfelfoldi closed this Jun 8, 2018

adamschmideg removed in progress labels Dec 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State trie garbage collection #15903

State trie garbage collection #15903

zsfelfoldi commented Jan 17, 2018 •

edited

Loading

GitCop commented Jan 17, 2018

holiman commented Jan 17, 2018

holiman Jan 17, 2018

zsfelfoldi Jan 17, 2018

holiman Jan 17, 2018

zsfelfoldi Jan 17, 2018

holiman Jan 17, 2018

zsfelfoldi Jan 17, 2018

holiman Jan 17, 2018

holiman Jan 17, 2018

holiman Jan 17, 2018

holiman Jan 17, 2018

zsfelfoldi Jan 17, 2018

holiman Jan 17, 2018

holiman Jan 17, 2018

holiman Jan 17, 2018

karalabe commented Jan 17, 2018 via email

zsfelfoldi commented Jan 17, 2018

GitCop commented Jan 18, 2018

zsfelfoldi commented Jan 18, 2018

State trie garbage collection #15903

State trie garbage collection #15903

Conversation

zsfelfoldi commented Jan 17, 2018 • edited Loading

GitCop commented Jan 17, 2018

holiman commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe commented Jan 17, 2018 via email

zsfelfoldi commented Jan 17, 2018

GitCop commented Jan 18, 2018

zsfelfoldi commented Jan 18, 2018

zsfelfoldi commented Jan 17, 2018 •

edited

Loading