refactor(tm2): make rpc client not depend on goleveldb #1603

thehowl · 2024-01-30T15:57:54Z

The PR depends on #1602 but it's good for review. For ease of reviewing I put that PR's branch as the base branch; once it is merged I will rebase on master.

A bunch of changes to get there:

Split proxy into appconn and proxy. proxy has a dependency on kvstore, which then depends on pkg/db and goleveldb.
Tools for inspecting dependency chains:
- go list -f '{{ join .Deps "\n" }}' github.com/gnolang/gno/tm2/your/pkg
- depth

The original point of the PR was to not include an entire key-store database as part of cmd/gno, but really the better side-effect is that the dependency is no longer in the rpc package. Sadly, gnoclient still imports goleveldb, but I'll leave understanding why that is the case for another refactor-investigation :)

…-leveldb

codecov · 2024-01-30T16:01:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.41%. Comparing base (2bb80df) to head (4561ba3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1603      +/-   ##
==========================================
- Coverage   47.46%   47.41%   -0.05%     
==========================================
  Files         385      384       -1     
  Lines       61355    61240     -115     
==========================================
- Hits        29123    29040      -83     
+ Misses      29797    29771      -26     
+ Partials     2435     2429       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…-leveldb

zivkovicmilos

Thank you for resolving this coupling 🙏

Looks great 💯

tm2/pkg/bft/consensus/wal_test.go

tm2/pkg/bft/types/keys.go

tm2/pkg/bft/rpc/client/localclient.go

…no-no-leveldb

![image](https://github.com/gnolang/gno/assets/4681308/761f1109-3e0d-4846-b331-3c60b6e803c0) ![image](https://github.com/gnolang/gno/assets/4681308/312b9226-49fa-48ac-b280-f4d704a341df) The PR depends on gnolang#1602 but it's good for review. For ease of reviewing I put that PR's branch as the base branch; once it is merged I will rebase on master. A bunch of changes to get there: - Split `proxy` into `appconn` and `proxy`. `proxy` has a dependency on `kvstore`, which then depends on pkg/db and goleveldb. - Tools for inspecting dependency chains: - `go list -f '{{ join .Deps "\n" }}' github.com/gnolang/gno/tm2/your/pkg` - [`depth`](https://github.com/KyleBanks/depth) The original point of the PR was to not include an entire key-store database as part of `cmd/gno`, but really the better side-effect is that the dependency is no longer in the rpc package. Sadly, `gnoclient` still imports `goleveldb`, but I'll leave understanding why that is the case for another refactor-investigation :)

piux2 · 2024-03-11T18:00:33Z

The original TM2 allows for in-process app and RPC versus separate server configurations. With this change, can we still deploy TM2, the app, and RPC as separate services in production?

thehowl · 2024-03-11T18:05:09Z

@piux2 The change only affects the RPC client. No breaking change was introduced, and as you mentioned, we can still have separate tm2/app/rpc server.

piux2 · 2024-03-12T01:46:58Z

@piux2 The change only affects the RPC client. No breaking change was introduced, and as you mentioned, we can still
have separate tm2/app/rpc server.

Thank you for the clarification. I'm not sure I fully understand the intentions behind this PR for refactoring with all these changes

Could you help to clarify a few points?

Why do we want to change this signature?

func NewLocal(node *nm.Node) *Local {

https://github.com/gnolang/gno/pull/1603/files#diff-40f9edc97cafeef3b08870b23f9c1726a7ca1f624b949194b9dfac2188f58346L39

Why was this function removed?

func (ps *PeerState) ToJSON()

https://github.com/gnolang/gno/pull/1603/files#diff-cbf5dcdca399dbb992b6443cfec33d11f3dd043771025ce250df2398c3113a5bL933

Regarding the following line of code:

rpccore.SetGetFastSync(n.consensusReactor.FastSync)

Why do we set code FastSync and remove setting the consensusReactor for the RPC?

https://github.com/gnolang/gno/pull/1603/files#diff-83913b6c6e92b6851951c0f50d813df7becd09b5cd235dabfcbc584be15941f8R700

The state of consensusReactor.FastSync() could change after the chain starts.

if consensusReactor.FastSync() { // could have different result

The current implementation only use the on-start state of the consensusReactor

if getFastSync() { // this does not reflect the latest states of consensusReactor

https://github.com/gnolang/gno/pull/1603/files#diff-93f50c3c46fcb420642932a285a68fa1cd3dc696578a95084115896c1d1bc93cL77

Why do we make these changes?
https://github.com/gnolang/gno/pull/1603/files#diff-630ab5b18ba3a6424db79d28b5d6ffd639ef63acc3041140e05d0ec26f57e4d8L220

jaekwon · 2024-03-25T00:52:12Z

tm2/pkg/bft/node/node.go

@@ -588,6 +589,16 @@ func (n *Node) OnStart() error {
 		time.Sleep(genTime.Sub(now))
 	}

+	// Set up the GLOBAL variables in rpc/core which refer to this node.


jaekwon · 2024-03-25T00:53:21Z

tm2/pkg/bft/rpc/client/localclient.go

+// functions on a given node, without going through any network connection.
+//
+// As this connects directly to a Node instance, a Local client only works
+// after the Node has been started. Note that the way this works is (alas)


jaekwon · 2024-03-25T00:57:43Z

Thank you Morgan, good work!

thehowl · 2024-03-26T10:19:01Z

@piux2 please add inline comments next time, it makes answering your questions easier :)

Could you help to clarify a few points?
1. Why do we want to change this signature?
func NewLocal(node *nm.Node) *Local {
https://github.com/gnolang/gno/pull/1603/files#diff-40f9edc97cafeef3b08870b23f9c1726a7ca1f624b949194b9dfac2188f58346L39

Removes the dependency from rpc/client to github.com/gnolang/gno/tm2/pkg/bft/node. pkg/bft/node depends on goleveldb in this way:

$ depth -explain github.com/gnolang/gno/tm2/pkg/db/goleveldb .
. -> github.com/gnolang/gno/tm2/pkg/bft/consensus -> github.com/gnolang/gno/tm2/pkg/bft/proxy -> github.com/gnolang/gno/tm2/pkg/bft/abci/example/kvstore -> github.com/gnolang/gno/tm2/pkg/db/goleveldb

Practically, we're separating concerns between what client/rpc should do. Rather than also set up the connections to make the local client work, rpc/client only provides the client itself, and the responsibility to wire everything up is with the caller.

2. Why was this function removed?
func (ps *PeerState) ToJSON()
https://github.com/gnolang/gno/pull/1603/files#diff-cbf5dcdca399dbb992b6443cfec33d11f3dd043771025ce250df2398c3113a5bL933

See rpc/core/consensus.go. rpc/core is imported by the RPC client. The function DumpConsensusState will print the peer state for each peer. There are other places where we get PeerStateKey, so I didn't want to completely refactor it.

However, since only rpc/core with this function will need the function to print it as JSON, I added a separate type in bft/consensus/types with the fields that should, indeed, be exposed. This avoids the import on bft/consensus, which eventually imports goleveldb (see the above depth result).

thehowl · 2024-03-26T10:26:20Z

@piux2

3. Regarding the following line of code:
rpccore.SetGetFastSync(n.consensusReactor.FastSync)
Why do we set code FastSync and remove setting the consensusReactor for the RPC?

https://github.com/gnolang/gno/pull/1603/files#diff-83913b6c6e92b6851951c0f50d813df7becd09b5cd235dabfcbc584be15941f8R700

Note that in rpccore.SetGetFastSync(n.consensusReactor.FastSync), n.consensusReactor.FastSync is a method (a getter, specifically). The reasoning here is similar to the above: rpc/core should not depend on bft/consensus, but because it is also the hub through which things are exposed in RPC it should have access to the node's properties.

To make it have access to the properties, while not importing bft/consensus, we change the consensusReactor to be a closure: getFastSync func() bool. This also should help answer your next question...

4. The state of `consensusReactor.FastSync()` could change after the chain starts.
if consensusReactor.FastSync() { // could have different result

The current implementation only use the on-start state of the consensusReactor

if getFastSync() { // this does not reflect the latest states of consensusReactor

https://github.com/gnolang/gno/pull/1603/files#diff-93f50c3c46fcb420642932a285a68fa1cd3dc696578a95084115896c1d1bc93cL77

Not really.

getFastSync is a getter. Because it is a method with a pointer receiver, it will return the value of fastSync for the node at the given pointer. Since this is the only place we used consensusReactor, using the getter will return the up-to-date value.

I was considering making it a property, but set for making it a getter for the precise reason it could be updated during a node's lifespan.

5. Why do we make these changes?
   https://github.com/gnolang/gno/pull/1603/files#diff-630ab5b18ba3a6424db79d28b5d6ffd639ef63acc3041140e05d0ec26f57e4d8L220

See answer number 2.

thehowl added 11 commits January 30, 2024 13:05

refactor(tm2): split pkg/db into sub-packages

06cd45d

refactor(tm2): do not have goleveldb as a transitive dependency

cec92f6

mod tidy

3bf09e5

Merge branch 'dev/morgan/split-db-package' into dev/morgan/cmd-gno-no…

d1cab27

…-leveldb

port to 1.20

a63e6b0

linter fixes

ea39113

Merge branch 'dev/morgan/split-db-package' into dev/morgan/cmd-gno-no…

b47e51c

…-leveldb

fix old references

0f3e12c

attempt to fix GH actions

8d7fe9f

attempt to fix ci

1bc8081

Merge branch 'dev/morgan/split-db-package' into dev/morgan/cmd-gno-no…

cd4069e

…-leveldb

thehowl requested review from gfanton, jaekwon, piux2, moul and zivkovicmilos as code owners January 30, 2024 15:57

github-actions bot assigned thehowl Jan 30, 2024

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jan 30, 2024

thehowl added 4 commits January 30, 2024 17:06

dont test db in others

56c1d6b

add db to makefile

bb4a3b2

Merge branch 'dev/morgan/split-db-package' into dev/morgan/cmd-gno-no…

c32f2bc

…-leveldb

modtidy

be58dd6

thehowl requested a review from a team as a code owner January 30, 2024 16:35

zivkovicmilos mentioned this pull request Feb 12, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

zivkovicmilos approved these changes Feb 15, 2024

View reviewed changes

tm2/pkg/bft/consensus/wal_test.go Show resolved Hide resolved

tm2/pkg/bft/types/keys.go Show resolved Hide resolved

zivkovicmilos requested a review from ajnavarro February 15, 2024 11:45

gfanton approved these changes Feb 15, 2024

View reviewed changes

tm2/pkg/bft/rpc/client/localclient.go Outdated Show resolved Hide resolved

albttx approved these changes Feb 15, 2024

View reviewed changes

unexport ConfigureRPC

ff80b78

thehowl changed the title ~~refactor(tm2): don't make rpc client depend on pkg/db~~ refactor(tm2): make rpc client not depend on pkg/db Feb 19, 2024

ajnavarro approved these changes Feb 26, 2024

View reviewed changes

Base automatically changed from dev/morgan/split-db-package to master March 1, 2024 11:53

thehowl requested review from a team as code owners March 1, 2024 11:53

thehowl added 3 commits March 1, 2024 13:21

Merge branch 'master' of github.com:gnolang/gno into dev/morgan/cmd-g…

f967e94

…no-no-leveldb

remove node.reset in gnodev

b630e57

xx

4561ba3

thehowl force-pushed the dev/morgan/cmd-gno-no-leveldb branch from d8c4555 to 4561ba3 Compare March 1, 2024 12:27

thehowl changed the title ~~refactor(tm2): make rpc client not depend on pkg/db~~ refactor(tm2): make rpc client not depend on goleveldb Mar 1, 2024

thehowl merged commit 6c5b4cf into master Mar 1, 2024
182 of 184 checks passed

thehowl deleted the dev/morgan/cmd-gno-no-leveldb branch March 1, 2024 12:34

michelleellen mentioned this pull request Mar 21, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

jaekwon reviewed Mar 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tm2): make rpc client not depend on goleveldb #1603

refactor(tm2): make rpc client not depend on goleveldb #1603

thehowl commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

zivkovicmilos left a comment •

edited

Loading

piux2 commented Mar 11, 2024

thehowl commented Mar 11, 2024 •

edited

Loading

piux2 commented Mar 12, 2024 •

edited

Loading

jaekwon Mar 25, 2024

jaekwon Mar 25, 2024

jaekwon commented Mar 25, 2024

thehowl commented Mar 26, 2024 •

edited

Loading

thehowl commented Mar 26, 2024

refactor(tm2): make rpc client not depend on goleveldb #1603

refactor(tm2): make rpc client not depend on goleveldb #1603

Conversation

thehowl commented Jan 30, 2024 • edited Loading

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

zivkovicmilos left a comment • edited Loading

Choose a reason for hiding this comment

piux2 commented Mar 11, 2024

thehowl commented Mar 11, 2024 • edited Loading

piux2 commented Mar 12, 2024 • edited Loading

jaekwon Mar 25, 2024

Choose a reason for hiding this comment

jaekwon Mar 25, 2024

Choose a reason for hiding this comment

jaekwon commented Mar 25, 2024

thehowl commented Mar 26, 2024 • edited Loading

thehowl commented Mar 26, 2024

thehowl commented Jan 30, 2024 •

edited

Loading

codecov bot commented Jan 30, 2024 •

edited

Loading

zivkovicmilos left a comment •

edited

Loading

thehowl commented Mar 11, 2024 •

edited

Loading

piux2 commented Mar 12, 2024 •

edited

Loading

thehowl commented Mar 26, 2024 •

edited

Loading