maindb v4 #868

libotony · 2024-10-29T08:20:42Z

Description

This PR upgrades maindb to v4. It greatly reduces the storage space occupied and significantly reduces the This PR upgrades maindb to v4. It greatly reduces the storage space occupied and significantly reduces the synchronization time.

For MPT
- The encoding and storage format of nodes are comprehensively improved, reducing the storage space used to store the state trie by 30%. Moreover, the speed of encoding and decoding nodes has been greatly improved.
- Introducing the concept of Version to identify nodes, which simplifies the implementation code.
- Optimize the trie interface to make it easier to maintain.
- Remove leafbank stuff.
- Improve generation-based node cache to reduce the pressure of GC.
- Improve node storage key encoding.
Optimize block indexing and storage format.

Check out commits for more detail.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

libotony · 2024-10-29T08:22:04Z

This PR is a successor of PR #635.

darrenvechain · 2024-10-29T08:27:21Z

@libotony could you sync the branch with feat/db so that the CI runs? I made that change in the GHA, which got merged into feat/db just now

libotony · 2024-10-29T08:28:35Z

@libotony could you sync the branch with feat/db so that the CI runs? I made that change in the GHA, which got merged into feat/db just now

Done.

muxdb/cache_test.go

cmd/thor/pruner/pruner.go

muxdb/cache.go

libotony · 2024-11-01T03:42:03Z

Wired that tests won't run with this PR only, might be caused by the on-pull-request.yaml wasn't updated in the master branch, but after PR #871 was opened, CI was trigger for targeting master then with the same commit hash of this PR, the workflow was also linked here.

darrenvechain · 2024-11-05T13:26:57Z

@libotony looks like GET /block/{revision} is returning null in the transactions field, according to the e2e tests

-   "transactions": Any<Array>,
+   "transactions": null,

libotony · 2024-11-06T09:25:42Z

@libotony looks like GET /block/{revision} is returning null in the transactions field, according to the e2e tests
-   "transactions": Any<Array>,
+   "transactions": null,

Should be fixed.

darrenvechain · 2024-11-06T12:48:05Z

go.mod

@@ -24,6 +24,7 @@ require (
 	github.com/prometheus/client_model v0.5.0
 	github.com/prometheus/common v0.45.0
 	github.com/qianbin/directcache v0.9.7
+	github.com/qianbin/drlp v0.0.0-20240102101024-e0e02518b5f9


Seems the dependency is quite small, should we just copy the files over to this repo?

wdyt？ @qianbin

darrenvechain · 2024-11-06T14:51:58Z

muxdb/cache.go

+}
+
+// AddNodeBlob adds encoded node blob into the cache.
+func (c *cache) AddNodeBlob(keyBuf *[]byte, name string, path []byte, ver trie.Version, blob []byte, isCommitting bool) {


Looks like most of these methods are exported but not used outside the package. Maybe I'm wrong ? Probably doesn't matter too much anyhow

Itself can be treated a standalone "class" to other ones, even it's inside the same package. IMHO, it's a good practice to define public/private functions even it's internal only.

I'm good with either, but what if the other structs used an interface (like Cache) instead of *cache. Would that help, or make will it make it into future-proving land ?

chain/repository.go

otherview · 2024-11-08T14:47:37Z

api/accounts/accounts_test.go

+	if err := repo.AddBlock(b, receipts, 0, false); err != nil {
 		t.Fatal(err)
 	}
 	if err := repo.SetBestBlockID(b.Header().ID()); err != nil {


Likely worth removing the repo.SetBestBlockID and setting repo.AddBlock(b, receipts, 0, true)

Removed repo.SetBestBlockID from most test files in commit ebed07a, but there is still a usage for it in https://github.com/vechain/thor/blob/tony/maindb-v4/bft/engine_test.go#L164, which I think is not avoidable.

Agreed 👍

There's a tester pattern that can be applied in such cases where

type RepositoryTest struct { Repository } func (r *RepositoryTest) SetBestBlockID(...) {}

I don't think there are a lot of other testing methods no point in using this, but wanted to highlight it if needed in the future!

api/debug/debug_test.go

api/transactions/transactions.go

chain/chain.go

chain/persist.go

chain/repository.go

otherview · 2024-11-11T14:19:14Z

chain/chain_test.go

+	{
+		tx, meta, err := c.GetTransaction(tx1.ID())
+		assert.Nil(t, err)
+		assert.Equal(t, tx1Meta, meta)
+		assert.Equal(t, tx1.ID(), tx.ID())
+	}
+	{
+		r, err := c.GetTransactionReceipt(tx1.ID())
+		assert.Nil(t, err)
+		got, _ := rlp.EncodeToBytes(r)
+		want, _ := rlp.EncodeToBytes(tx1Receipt)
+		assert.Equal(t, want, got)
+	}
+


Nit - Perhaps the implode func M can be reused here ? Or changed the M so it's kinda the same type test approach ?

otherview · 2024-11-11T14:24:54Z

muxdb/engine/leveldb.go

I think it might be worth changing the error check with errors.Is

func (ldb *levelEngine) IsNotFound(err error) bool { return err == leveldb.ErrNotFound }

My understanding here is comparing to the specific error and errors.Is will have more checks including wrapped errors.

Yes, it's comparing to the instance of leveldb.ErrNotFound.

return err == leveldb.ErrNotFound will fail iferr := fmt.Errorf("this be error: %w", errNotFound)

errors.Is is meant to unwrap errors until it finds that type. So if for any reason the error is wrapped upstream, this won't be caught.

otherview · 2024-11-11T14:38:41Z

chain/repository.go

+	if err := bulk.Write(); err != nil {
 		return nil, err
 	}
 	r.caches.summaries.Add(id, &summary)
-	return &summary, bulk.Write()
+	if asBest {
+		r.bestSummary.Store(&summary)
+		r.tick.Broadcast()
+	}
+	return &summary, nil


It looks like the saveBlock is also used during bootstrap - Could it be detrimental to cache those summaries at bootstrap time ?

Bootstrapping can be ignored here, as it's not a common case. And the behavior is not detrimental as LRU will evict the oldest entry.

Yes, I was considering if it could be an improvement to the bootstrap phase.
It looks like it's adding stuff to cache that will get (almost) immediately booted off ( given it bootstraps a lot of blocks).

Perhaps it's of negligible impact. I wish we had a benchmark test for bootstrapping

otherview · 2024-11-11T14:43:13Z

chain/repository.go

-	repo.caches.txs = newCache(2048)
-	repo.caches.receipts = newCache(2048)


I probably missed the why, but it's no longer caching txs and receipts ?

You probably are right, based on the perf test results.

otherview · 2024-11-11T14:46:37Z

chain/repository.go

+func (r *Repository) getTransaction(key []byte) (*tx.Transaction, error) {
+	var tx tx.Transaction
+	if err := loadRLP(r.bodyStore, key, &tx); err != nil {


Did we miss the caching, or is it upstream ?

otherview · 2024-11-11T15:57:01Z

muxdb/cache.go

+	if now-last > int64(time.Second*20) {
+		log1, ok1 := c.nodeStats.ShouldLog("node cache stats")
+		log2, ok2 := c.rootStats.ShouldLog("root cache stats")
+
+		if ok1 || ok2 {
+			log1()
+			log2()
+		}


I think this is fine, but can be simplified to just log at 20 seconds interval.
Looks like ShouldLog only logs when the hitrate has changed and in this case both nodeStats and rootStats should have changed their hitrate.

I might be missing it, but I don't see the added value of checking if it should log or not. I'm starting to think it could be simpler to log in demand ?

muxdb/cache.go

muxdb/trie.go

muxdb/backend.go

* feat: add thorclient * refactor: remove roundTripper * refactor: change null check * clean: remove commented code * feat: add account revision and pending tx * fix: add licence headers and fix linter issue * refactor: rename package * refactor: change revision type to string * refactor: rename GetLogs and GetTransfers to FilterEvents and FilterTransfers * refactor: change FilterEvents and FilterTransactions request type to EventFilter * Adding common.EventWrapper to handle channel errors * tweak * update rawclient + update account tests * tidy up names * update tests * pr comments * adding raw tx * Tidy up method names and calls * options client * tweaks * pr comments * Update thorclient/common/common.go Co-authored-by: libotony <[email protected]> * pr comments * Adding Subscriptions * Pr comments * adjust func orders * pr comments * changing subscribe to use the channel close vs multiple channels * adding go-doc * no error after unsubscribe * pr comments * checking status code is 2xx * fix: change FilterTransfers argument --------- Co-authored-by: otherview <[email protected]> Co-authored-by: libotony <[email protected]>

* Refactor thor node * thorchain allows insertion of blocks * remove thorNode, added testchain * clean up + comments * adding license headers * adding templating tests for thorclient * Remove test event hacks * remove types * removed chain_builder + added logdb to testchain * pr comments * Update test/testchain/chain.go Co-authored-by: libotony <[email protected]> --------- Co-authored-by: libotony <[email protected]>

libotony · 2024-11-12T07:22:05Z

Rebased branch feat/db with master then rebased tony/maindb-v4 with feat/db.

muxdb/trie.go

otherview · 2024-11-18T14:51:15Z

api/accounts/accounts_test.go

+	if err := repo.AddBlock(b, receipts, 0, false); err != nil {
 		t.Fatal(err)
 	}
 	if err := repo.SetBestBlockID(b.Header().ID()); err != nil {


Agreed 👍

There's a tester pattern that can be applied in such cases where

type RepositoryTest struct { Repository } func (r *RepositoryTest) SetBestBlockID(...) {}

I don't think there are a lot of other testing methods no point in using this, but wanted to highlight it if needed in the future!

chain/chain.go

otherview · 2024-11-18T15:54:39Z

muxdb/engine/leveldb.go

Yes, it's comparing to the instance of leveldb.ErrNotFound.

return err == leveldb.ErrNotFound will fail iferr := fmt.Errorf("this be error: %w", errNotFound)

errors.Is is meant to unwrap errors until it finds that type. So if for any reason the error is wrapped upstream, this won't be caught.

otherview · 2024-11-18T15:59:07Z

chain/repository.go

+	if err := bulk.Write(); err != nil {
 		return nil, err
 	}
 	r.caches.summaries.Add(id, &summary)
-	return &summary, bulk.Write()
+	if asBest {
+		r.bestSummary.Store(&summary)
+		r.tick.Broadcast()
+	}
+	return &summary, nil


Yes, I was considering if it could be an improvement to the bootstrap phase.
It looks like it's adding stuff to cache that will get (almost) immediately booted off ( given it bootstraps a lot of blocks).

Perhaps it's of negligible impact. I wish we had a benchmark test for bootstrapping

muxdb/cache.go

muxdb/trie.go

otherview · 2024-11-18T16:07:53Z

Sorry, not sure what I did for the above comments to not stay in the thread 🙏

* change * reverted to previous format

* Add empty cache for inmem ops * changing name to dummyCache

…pace (#888) * first commit * first commit

* changes * removed log * sleeping any way * pr review

libotony requested a review from a team as a code owner October 29, 2024 08:20

libotony changed the title ~~Tony/maindb v4~~ maindb v4 Oct 29, 2024

libotony marked this pull request as draft October 29, 2024 08:20

libotony added new feature resync-required labels Oct 29, 2024

libotony force-pushed the tony/maindb-v4 branch from b43bbfb to ef814c0 Compare October 29, 2024 08:28

libotony marked this pull request as ready for review October 29, 2024 08:55

leszek-vechain reviewed Oct 30, 2024

View reviewed changes

muxdb/cache_test.go Outdated Show resolved Hide resolved

leszek-vechain reviewed Oct 30, 2024

View reviewed changes

cmd/thor/pruner/pruner.go Show resolved Hide resolved

leszek-vechain reviewed Oct 30, 2024

View reviewed changes

muxdb/cache.go Show resolved Hide resolved

libotony closed this Nov 1, 2024

libotony reopened this Nov 1, 2024

libotony closed this Nov 1, 2024

libotony reopened this Nov 1, 2024

darrenvechain reviewed Nov 6, 2024

View reviewed changes

darrenvechain approved these changes Nov 6, 2024

View reviewed changes

darrenvechain reviewed Nov 6, 2024

View reviewed changes

libotony mentioned this pull request Nov 7, 2024

Add LogIndex and TxIndex into logs/event response body #862

Merged

14 tasks

otherview reviewed Nov 8, 2024

View reviewed changes

otherview reviewed Nov 11, 2024

View reviewed changes

paologalligit and others added 4 commits November 12, 2024 15:19

cmd/thor: update instance dir to v4

4ef2f8a

trie: implement varint-prefix coder

47534e2

qianbin and others added 14 commits November 12, 2024 15:19

build: fix test cases

d355820

lint: fix lint error

40795d9

muxdb: fix ver encoding in node blob cache

1f06c5e

muxdb: add test cases for cache

35abfef

runtime: fix test compile error

ce3c5ec

make build and test pass after rebase

7fbc4a1

add back pruner tests

217a54e

add tests for node encoding

1be7687

minor typo

2641337

update named store space prefix

d48f3cc

add more tests

34cb622

fix block summary in repo

20e5104

make build and test pass after rebase

8eff09c

add back pruner tests

df4dc18

libotony force-pushed the feat/db branch from 368e5d2 to f471d95 Compare November 12, 2024 07:19

libotony force-pushed the tony/maindb-v4 branch from 3e5f529 to df4dc18 Compare November 12, 2024 07:20

riikolilover approved these changes Nov 12, 2024

View reviewed changes

libotony and others added 4 commits November 12, 2024 15:43

remove SetBestBlockID from tests

ebed07a

minor improvement

8e0bc5d

pr comments

14a0ac1

Merge branch 'feat/db' into tony/maindb-v4

87e1ff7

otherview reviewed Nov 18, 2024

View reviewed changes

otherview and others added 6 commits November 18, 2024 16:16

adding a comment

0c2d638

Merge branch 'feat/db' into tony/maindb-v4

ab114b1

Metrics: Cache hit/miss (#886)

61aa676

* change * reverted to previous format

Add dummy cache for inmem ops (#883)

eab4398

* Add empty cache for inmem ops * changing name to dummyCache

Metrics: Reuse shouldLog so we get cache hit/miss data at the same …

c3f3bc4

…pace (#888) * first commit * first commit

Metrics: Disk IO reads and writes (#890)

75e437e

* changes * removed log * sleeping any way * pr review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maindb v4 #868

maindb v4 #868

libotony commented Oct 29, 2024 •

edited

Loading

libotony commented Oct 29, 2024

darrenvechain commented Oct 29, 2024

libotony commented Oct 29, 2024

libotony commented Nov 1, 2024

darrenvechain commented Nov 5, 2024 •

edited

Loading

libotony commented Nov 6, 2024

darrenvechain Nov 6, 2024

libotony Nov 7, 2024

qianbin Nov 12, 2024

darrenvechain Nov 6, 2024

libotony Nov 7, 2024

otherview Nov 11, 2024

otherview Nov 8, 2024

libotony Nov 12, 2024

otherview Nov 18, 2024

otherview Nov 11, 2024

otherview Nov 11, 2024

libotony Nov 12, 2024

otherview Nov 18, 2024

otherview Nov 11, 2024

libotony Nov 13, 2024

otherview Nov 18, 2024

otherview Nov 11, 2024

libotony Nov 12, 2024

otherview Nov 11, 2024

otherview Nov 11, 2024

libotony commented Nov 12, 2024

otherview Nov 18, 2024

otherview Nov 18, 2024

otherview Nov 18, 2024

otherview commented Nov 18, 2024

		repo.caches.txs = newCache(2048)
		repo.caches.receipts = newCache(2048)

maindb v4 #868

Are you sure you want to change the base?

maindb v4 #868

Conversation

libotony commented Oct 29, 2024 • edited Loading

Description

Type of change

Checklist:

libotony commented Oct 29, 2024

darrenvechain commented Oct 29, 2024

libotony commented Oct 29, 2024

libotony commented Nov 1, 2024

darrenvechain commented Nov 5, 2024 • edited Loading

libotony commented Nov 6, 2024

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

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

libotony commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otherview commented Nov 18, 2024

libotony commented Oct 29, 2024 •

edited

Loading

darrenvechain commented Nov 5, 2024 •

edited

Loading