Clean Read Only Geth #2

hrthaowang · 2022-01-26T16:46:36Z

In this PR, I removed the readOnlyNode and readOnlyBackend. Instead, I used a boolean variable to represent whether it's in the readOnly mode to make the code cleaner.

core/rawdb: do prefixed lookup first

When talking to an HTTP2 server, there are situations where it needs to "rewind" the Request.Body. To allow this, we have to set up the Request.GetBody function to return a brand new instance of the body. If not set, we can end up with the following error: http2: Transport: cannot retry err [http2: Transport received Server's graceful shutdown GOAWAY] after Request.Body was written; define Request.GetBody to avoid this error See this commit for more information: https://sourcegraph.com/github.com/golang/net/-/commit/cffdcf672aee934982473246bc7e9a8ba446aa9b?visible=2

danielmh0 · 2022-01-28T02:52:48Z

core/rawdb/accessors_state.go

-	// Try with the legacy code scheme first, if not then try with current
-	// scheme. Since most of the code will be found with legacy scheme.
-	//
-	// todo(rjl493456442) change the order when we forcibly upgrade the code


Why did you need this change, and are we sure it's safe?

danielmh0 · 2022-01-28T03:00:29Z

eth/backend.go

+		genesisHash = common.Hash{}
+		genesisErr = nil
+	} else {
+		chainDb, err = stack.OpenDatabaseWithFreezer("chaindata", config.DatabaseCache, config.DatabaseHandles, config.DatabaseFreezer, "eth/db/chaindata/", false)


If the database cannot be opened read-only with a freezer, why does OpenDatabaesWithFreezer have a readonly arg? Please comment this for the reader.

eth/backend.go

danielmh0 · 2022-01-28T03:06:52Z

eth/backend.go

-	if s.config.LightServ > 0 {
-		if s.config.LightPeers >= s.p2pServer.MaxPeers {
-			return fmt.Errorf("invalid peer config: light peer count (%d) >= total peer count (%d)", s.config.LightPeers, s.p2pServer.MaxPeers)
+	if s.readOnly {


Shouldn't this be !s.readOnly?

danielmh0 · 2022-01-28T03:10:20Z

eth/backend.go

+	if !s.readOnly {
+		// Stop all the peer-related stuff first.
+		s.ethDialCandidates.Close()
+		s.snapDialCandidates.Close()
+		s.handler.Stop()
+	}


I don't know if this is easy to code, but it'd be nice not to branch on !s.readOnly in particular, but rather have the Close()/Stop() functions handle a never-opened/never-started case more gracefully.

Analogous to how the destructor of a unique_ptr just does the right thing even if the unique_ptr is null.

danielmh0 · 2022-01-28T03:13:35Z

node/node.go

 	// Creates an empty AccountManager with no backends. Callers (e.g. cmd/geth)
 	// are required to add the backends later on.
 	node.accman = accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: conf.InsecureUnlockAllowed})


Can we try to keep all the !node.readOnly logic contained in fewer if blocks? It seems like we could merge above and below, if this assignment was moved up?

danielmh0 · 2022-01-28T03:18:35Z

node/node.go

@@ -342,50 +352,52 @@ func (n *Node) startRPC() error {
 	if err := n.startInProc(); err != nil {


I think it's cleaner to easier to rename this function startExternalRPC, remove n.startInProc() from it entirely, and avoid extra layer of branching on !n.readOnly.

Overall: You have a lot of if !n.readOnly branches, which is a bad code smell -- try to refactor the code so that the read-only special case is self-contained.

danielmh0 · 2022-01-28T03:19:29Z

node/node.go

@@ -396,9 +408,11 @@ func (n *Node) wsServerForPort(port int) *httpServer {
 }

 func (n *Node) stopRPC() {


Similar to above, I think this should be stopExternalRPC.

danielmh0 · 2022-01-28T03:22:49Z

node/node.go

-	n.http.stop()
-	n.ws.stop()
-	n.ipc.stop()
+	if !n.readOnly {


Here and elsewhere, you're conflating "read-only" with "used-as-a-library" in a few places. You could imagine starting HTTP/WS/IPC services on a read-only geth process. Maybe localLibrary should be a separate option in Node?

And readOnlyGeth/ should be renamed readOnlyLib/?

* tests: add ipsilon/evm-benchmarks git submodule * tests: plug-in evm-benchmarks

* eth/tracers: add initial native prestate tracer * fix balance hex * handle prestate for tx from and to * drop created contract from prestate * fix sender balance * use switch instead Co-authored-by: Martin Holst Swende <[email protected]> * minor fix * lookup create2 account * mv code around a bit * check stackLen for create2 * fix transfer tx for js prestate tracer * fix create2 addr * track extcodehash in js prestate tracer Co-authored-by: Martin Holst Swende <[email protected]>

* build: append GOARM to arm lint download URL otherwise it fails with: downloading from https://github.com/golangci/golangci-lint/releases/download/v1.42.0/golangci-lint-1.42.0-linux-arm.tar.gz ci.go:347: download error: status 404 * build: increase timeout for lint Otherwise it times out on a pi * Increase timeout even further saw longer build times

* all: seperate catalyst package * eth/catalyst: moved some methods, added docs * eth/catalyst, les/catalyst: add method docs * core, eth, les, miner: move common function to beacon package * eth/catalyst: goimported * cmd/utils, miner/stress/beacon: naming nitpicks Co-authored-by: Marius van der Wijden <[email protected]> Co-authored-by: Péter Szilágyi <[email protected]>

I believe the sentence is attempting to explain that the URL is "[used] by upper layers to define a sorting order over all wallets from multiple backends."

* eth/tracers: clean-up tracer collection * Rm test for dropped tracer

This change makes it so WaitMined no longer logs an error when the receipt is unavailable. It also changes the simulated backend to return NotFound for unavailable receipts, just like ethclient does.

cmd: auto-enable beacon APIs if TTD is defined

This also contains some changes to the protocol handler to make the tests pass.

…o-ethereum into clean-read-only

holiman and others added 6 commits January 25, 2022 10:51

core/rawdb: do prefixed lookup first

0e35192

Merge pull request ethereum#24288 from holiman/prefer_prefixed

78f13a3

core/rawdb: do prefixed lookup first

core/rawdb: fix typo (ethereum#24289)

29cb5de

eth/tracers: avoid using blockCtx concurrently (ethereum#24286)

015fde9

test

91a1962

clean code

9c5d99c

hrthaowang requested a review from danielmh0 January 26, 2022 16:47

s1na and others added 3 commits January 27, 2022 10:06

cmd/utils: open db in readonly when dev datadir exists (ethereum#24298)

e282246

internal/web3ext: add eth.getLogs wrapper (ethereum#24297)

a988550

danielmh0 suggested changes Jan 28, 2022

View reviewed changes

chfast and others added 19 commits January 28, 2022 08:47

tests: external evm benchmarks (ethereum#24050)

64c53ed

* tests: add ipsilon/evm-benchmarks git submodule * tests: plug-in evm-benchmarks

cmd/geth: make test pass on a pi4 by using lightkdf (ethereum#24314)

0c1bd22

accounts: correct spelling mistake (ethereum#24323)

b868ca1

I believe the sentence is attempting to explain that the URL is "[used] by upper layers to define a sorting order over all wallets from multiple backends."

eth/tracers: clean-up tracer collection (ethereum#24320)

c5436c8

* eth/tracers: clean-up tracer collection * Rm test for dropped tracer

accounts/abi/bind: improve WaitMined error handling (ethereum#24321)

afe344b

This change makes it so WaitMined no longer logs an error when the receipt is unavailable. It also changes the simulated backend to return NotFound for unavailable receipts, just like ethclient does.

cmd: auto-enable beacon APIs if TTD is defined

d99e759

Merge pull request ethereum#24328 from karalabe/uke-catalyst

1a7e345

cmd: auto-enable beacon APIs if TTD is defined

go.mode: bump graphql-go dependency to v1.3.0 (ethereum#24324)

aaca58a

cmd/devp2p: implement snap protocol testing (ethereum#24276)

6ce4670

This also contains some changes to the protocol handler to make the tests pass.

miner: avoid data race in miner (ethereum#24349)

2d20fed

test

4e3fdaa

clean code

33ca88f

add LocalLib option

8535422

fix style

e3d8699

fix style

8256512

expose ServeRawRequset Under rpc.Server

5cf2f5f

haowang0402 and others added 5 commits July 5, 2022 16:18

expose ServeRawRequset Under rpc.Server

9df4438

add back debug logger

f356b69

test stuff

d1002d2

Merge branch 'clean-read-only' of https://github.com/hudson-trading/g…

fb5baec

…o-ethereum into clean-read-only

fix merge bug

0558e7e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean Read Only Geth #2

Clean Read Only Geth #2

hrthaowang commented Jan 26, 2022 •

edited

Loading

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

danielmh0 Jan 28, 2022

		@@ -342,50 +352,52 @@ func (n *Node) startRPC() error {
		if err := n.startInProc(); err != nil {

		@@ -396,9 +408,11 @@ func (n Node) wsServerForPort(port int) httpServer {
		}

		func (n *Node) stopRPC() {

Clean Read Only Geth #2

Are you sure you want to change the base?

Clean Read Only Geth #2

Conversation

hrthaowang commented Jan 26, 2022 • edited Loading

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

hrthaowang commented Jan 26, 2022 •

edited

Loading