cmd/geth: implement dev mode for post-merge #27327

jwasinger · 2023-05-23T13:21:28Z

Opening this as a draft to get eyes on it in its current state, and get feedback if there are obvious things that should be changed.

Currently works with the command: ./build/bin/geth --dev

holiman · 2023-05-24T10:18:17Z

Currently works with the command: ./build/bin/geth --http --http.api "engine" --nodiscover --authrpc.jwtsecret $(pwd)/jwt_secret --dev

Haven't looked at the code, but why would one want to pass jwtsecret? Shouldn't it just be geth --dev and done?

jwasinger · 2023-05-24T11:25:14Z

@holiman yup. That's something I've noted in todo list above. Trying to address those today.

s1na · 2023-05-25T12:45:29Z

core/genesis.go

 		GasLimit:   gasLimit,
 		BaseFee:    big.NewInt(params.InitialBaseFee),
-		Difficulty: big.NewInt(1),


I find it a bit weird that the genesis block has a non-zero difficulty:

> eth.getBlock(0).difficulty 131072

it was very difficult genecizing this genesis block

The difficulty formula is undefined for difficulties <131072. There is a minimum difficulty, and below that, the spec can be interpreted in two different ways, depending on at what exact step one decides to apply the minimum threshold.

params/config.go

s1na

For testing use-cases it would be nice to also have a way of setting safe and finalized blocks through clmock. As well as pushing withdrawals. Possibly adding a dev namespace in the RPC for this purpose?

clmock/engine_api.go

clmock/clmock.go

jwasinger · 2023-06-01T12:41:11Z

Okay. This is getting closer.

Two items still need to be addressed:

configurability of dev mode gas limit (override genesis configuration if --dev.gaslimit flag is present and there is no pre-existing chain)
currently panics if started up using an incompatible datadir.

gballet

Left a few questions

clmock/clmock.go

core/genesis.go

gballet · 2023-06-01T13:15:19Z

core/genesis.go

 		GasLimit:   gasLimit,
 		BaseFee:    big.NewInt(params.InitialBaseFee),
-		Difficulty: big.NewInt(1),


it was very difficult genecizing this genesis block

gballet · 2023-06-01T13:20:08Z

params/config.go

-	Clique *CliqueConfig `json:"clique,omitempty"`
+	Ethash *EthashConfig        `json:"ethash,omitempty"`
+	Clique *CliqueConfig        `json:"clique,omitempty"`
+	Dev    *DeveloperModeConfig `json:"dev,omitempty"`


Just my 2 cents here: I feel this is overkill, because you can just pass it as a command line option, and use that parameter when instantiating the CLMock. So you could reduce the footprint of that PR by not adding more config here, since it's really only used in one place.

Yes but it must be persisted in the case where dev mode is not being used ephemerally. Hence I add DeveloperModeConfig so that the period will be stored.

must it, though? if you need it to remain active, all you need to do is add --dev to your command line. Not going to die on that hill, just feel the PR could be simpler.

In dev mode: When you specify --datadir and --dev together it persists the chain. if the datadir is omitted, the chain is ephemeral (cleared on shutdown).

I also feel this is overkill? Is there ever really a use case to have a dev mode genesis? ChainConfig is used quite a bit and so we should be confident in the addition here.

In DeveloperModeConfig , there is only a single field period. It's totally fine to use another value in the next restart I think. We can have a default value for it, e.g. 12s or so, and users can change it by CLI flag.

clmock/clmock.go

jwasinger · 2023-06-02T10:47:59Z

Alright, I've addressed the remaining issues that I listed above. Adding a few unit tests and then hopefully, this should be ready for merge.

jwasinger · 2023-06-02T11:25:28Z

It occurs to me that it probably makes sense to implement a way to shim-in support for withdrawals. I'll try to think about how this will work.

clmock/api.go

clmock/clmock.go

clmock/api.go

clmock/clmock.go

jwasinger · 2023-06-12T22:06:28Z

Okay, I've addressed most of the feedback that @lightclient has provided (thanks for that!). The only remaining question is where to put the simulated_beacon package?

It can either live under internal or in cmd/geth as suggested by @lightclient .

lightclient · 2023-06-13T09:03:27Z

I think I lean towards just putting the files into cmd/geth and not adding a new package. It's relatively lightweight and only ever instantiated in cmd/geth/config.go. If it is a package though, you should drop the underscore in the name.

holiman · 2023-06-13T10:03:01Z

Some UX quirks...
It opens port 8551 -- which seems unnecessary ?

INFO [06-13|11:56:39.169] IPC endpoint opened                      url=/tmp/geth.ipc
INFO [06-13|11:56:39.171] WebSocket enabled                        url=ws://127.0.0.1:8551
INFO [06-13|11:56:39.171] HTTP server started                      endpoint=127.0.0.1:8551 auth=true prefix= cors=localhost vhosts=localhost

When running it, it keep spitting out this:

INFO [06-13|11:59:40.087] Starting work on payload                 id=0x1a411adabba04e97
INFO [06-13|11:59:40.087] Updated payload                          id=0x1a411adabba04e97 number=1 hash=3970ba..f2fc01 txs=0 gas=0 fees=0 root=e642b9..90f354 elapsed="263.675µs"
INFO [06-13|11:59:52.088] Stopping work on payload                 id=0x1a411adabba04e97 reason=timeout
INFO [06-13|11:59:52.089] Starting work on payload                 id=0xa0f6fb4c31da930b
INFO [06-13|11:59:52.089] Updated payload                          id=0xa0f6fb4c31da930b number=1 hash=051b8f..e98ddc txs=0 gas=0 fees=0 root=e642b9..90f354 elapsed="60.977µs"
INFO [06-13|12:00:04.089] Stopping work on payload                 id=0xa0f6fb4c31da930b reason=timeout
INFO [06-13|12:00:04.090] Starting work on payload                 id=0x7c0f68cd6ee81a44
INFO [06-13|12:00:04.090] Updated payload                          id=0x7c0f68cd6ee81a44 number=1 hash=5cfc3d..3ca163 txs=0 gas=0 fees=0 root=e642b9..90f354 elapsed="99.294µs"
INFO [06-13|12:00:16.091] Stopping work on payload                 id=0x7c0f68cd6ee81a44 reason=timeout

And that's while I'm not doing anything, just letting it idle. Seems unnecessary?

node/node.go

holiman · 2023-06-13T10:21:42Z

simulated_beacon/api.go

+func (api *API) AddWithdrawal(ctx context.Context, withdrawal *types.Withdrawal) error {
+	api.simBeacon.mu.Lock()
+	defer api.simBeacon.mu.Unlock()
+	return api.simBeacon.withdrawals.add(withdrawal)
+}


This is IMO a bit odd. I guess you wanted to split this up into a standalone API in order to make the distinction more clear about what is the public API-surface and what is the actual implementation?

The thing which is a bit odd, IMO, is that you have this external wrapper which reaches in and locks/unlocks an internal mutex inside teh simBeacon. Either move the mutex to this API, or, probably better, move the locking so the SimulatedBeacon controls it's own locks.

So this type would be a 'pure' wrapper:

Suggested change

func (api *API) AddWithdrawal(ctx context.Context, withdrawal *types.Withdrawal) error {

api.simBeacon.mu.Lock()

defer api.simBeacon.mu.Unlock()

return api.simBeacon.withdrawals.add(withdrawal)

}

func (api *API) AddWithdrawal(ctx context.Context, withdrawal *types.Withdrawal) error {

return api.simBeacon.withdrawals.add(withdrawal)

}

You could even (maybe?) turn this whole file into

type API SimulatedBeacon func (api *API) AddWithdrawal(ctx context.Context, withdrawal *types.Withdrawal) error { return api.addWithdrawal(withdrawal) } func (api *API) SetFeeRecipient(ctx context.Context, feeRecipient *common.Address) { api.feeRecipient = *feeRecipient }

Depends on your preference I guess, but it's less code at least

The last change also requires a change here:

--- a/simulated_beacon/simulated_beacon.go +++ b/simulated_beacon/simulated_beacon.go @@ -216,7 +216,7 @@ func RegisterAPIs(stack *node.Node, c *SimulatedBeacon) { stack.RegisterAPIs([]rpc.API{ { Namespace: "dev", - Service: &API{c}, + Service: (*API)(c), Version: "1.0", }, })

I've moved the mutex-handling for controlling access for fee recipient and the withdrawals list into SimulatedBeacon/withdrawals and pulled them out of the api. This should make things a bit cleaner.

simulated_beacon/simulated_beacon.go

…ullNode. rename feeTarget->feeRecipient and remove getter function

…d in the next payload

…Beacon fields into constructor. Co-authored-by: rjl493456442 <[email protected]>

Co-authored-by: lightclient <[email protected]>

…aneously

holiman

LGTM

hadv · 2023-08-15T04:53:21Z

Hi,

A random, pre-allocated developer account will be available and unlocked as
eth.coinbase, which can be used for testing. The random dev account is temporary,
stored on a ramdisk, and will be lost if your machine is restarted.

Using random account for testing is not a good idea, especially with automation test environment.
Can you add a flag to pre-allocate fund for specific accounts?

jwasinger · 2023-08-15T08:40:43Z

@hadv if you want to fund specific accounts, transfer ether from the coinbase account. There isn't a need for the ability to pre-allocate specific accounts at genesis.

hadv · 2023-08-16T03:05:54Z

@hadv if you want to fund specific accounts, transfer ether from the coinbase account. There isn't a need for the ability to pre-allocate specific accounts at genesis.

When using web3j for automation testing, we need Credentials to send fund; but we didn't have Credentials of coinbase account

v1.12.1 geth changes the default ChainConfig returned with DeveloperGenesisBlock from AllCliqueProtocolChanges to AllDevChainProtocolChanges. Since the dev chain config is not set to clique mode and is set as post-TTD with TTD=0, there needs to be a non-ethhash based consensus engine running in order for blocks to be produced. See the upstream geth change here: ethereum/go-ethereum#27327 This commit copies the initailization code for how geth is started up in dev mode with the simulated beacon node to our test L1 setup code. It also enables FullSync mode on the L1, since otherwise SimluatedBeacon.sealBlock would fail when calling the ConsensusAPI.ForkChoiceUpdatedV2 as it would still be syncing if it was in SnapSync mode. UseMergeFinality was also disabled on the TestDelayedSequencerConfig since the SimulatedBeacon seems to have different finalization behavior.

rmlearney-digicatapult · 2023-11-01T13:27:52Z

I've just opened issue #28449 related to this issue - is it possible to run a private Shanghai-EVM-compatible PoA network without --dev?

This change adds back the 'geth --dev' mode of operation, using a cl-mocker. --------- Co-authored-by: Martin Holst Swende <[email protected]> Co-authored-by: rjl493456442 <[email protected]> Co-authored-by: lightclient <[email protected]>

This reverts commit 7818ec3.

s1na reviewed May 25, 2023

View reviewed changes

params/config.go Outdated Show resolved Hide resolved

s1na reviewed May 25, 2023

View reviewed changes

clmock/engine_api.go Outdated Show resolved Hide resolved

clmock/clmock.go Outdated Show resolved Hide resolved

s1na reviewed May 25, 2023

View reviewed changes

clmock/clmock.go Outdated Show resolved Hide resolved

s1na mentioned this pull request May 26, 2023

local geth --dev network with shanghai evm support #27365

Closed

lightclient reviewed May 28, 2023

View reviewed changes

clmock/clmock.go Outdated Show resolved Hide resolved

gballet reviewed Jun 1, 2023

View reviewed changes

MariusVanDerWijden reviewed Jun 1, 2023

View reviewed changes

clmock/clmock.go Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

jwasinger marked this pull request as ready for review June 2, 2023 11:05

jwasinger requested review from fjl, karalabe, holiman and rjl493456442 as code owners June 2, 2023 11:05

lightclient reviewed Jun 8, 2023

View reviewed changes

jwasinger force-pushed the dev-mode-clmock branch 2 times, most recently from 23ee876 to 4645e03 Compare June 12, 2023 21:43

holiman reviewed Jun 13, 2023

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

holiman reviewed Jun 13, 2023

View reviewed changes

simulated_beacon/simulated_beacon.go Outdated Show resolved Hide resolved

holiman reviewed Jun 13, 2023

View reviewed changes

simulated_beacon/simulated_beacon.go Outdated Show resolved Hide resolved

holiman reviewed Jun 13, 2023

View reviewed changes

simulated_beacon/simulated_beacon.go Outdated Show resolved Hide resolved

jwasinger and others added 14 commits July 2, 2023 18:30

cmd/geth: prefer utils.Fatalf over returning an error object in makeF…

8d3330a

…ullNode. rename feeTarget->feeRecipient and remove getter function

only clear as many pending withdrawals from the queue as were include…

e21dc29

…d in the next payload

change dev.period flag to uint64 flag type

e3ab2db

don't register catalyst service if in light sync mode

714bf98

better naming for feeRecipientMutex. move initialization of Simulated…

4fd9197

…Beacon fields into constructor. Co-authored-by: rjl493456442 <[email protected]>

Update eth/catalyst/simulated_beacon.go

5eac860

Co-authored-by: lightclient <[email protected]>

wip: prepare for using new miner GetFullPayload api

1340352

eth/catalyst, miner: add GetFullPayload API

c6b50e9

somewhat works... but is mistakenly tracking multiple payloads simult…

e83d22d

…aneously

refactor simulated_beacon based on miner API changes

68608ca

remove logging. add comment

3a16d22

remove period from developer genesis configuration

c592736

comments

a504a80

cmd, eth/catalyst, miner: terminate block building by ResolveFull

ba286e5

jwasinger force-pushed the dev-mode-clmock branch from 26cc87c to ba286e5 Compare July 2, 2023 16:31

holiman approved these changes Jul 6, 2023

View reviewed changes

holiman merged commit ea78280 into ethereum:master Jul 6, 2023

jwasinger deleted the dev-mode-clmock branch August 15, 2023 05:10

rmlearney-digicatapult mentioned this pull request Nov 1, 2023

Run a private PoA network that supports Shanghai/PUSH0 without --dev #28449

Closed

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "cmd/geth: implement dev mode for post-merge (ethereum#27327)"

a975273

This reverts commit 7818ec3.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "cmd/geth: implement dev mode for post-merge (ethereum#27327)"

61581cd

This reverts commit 7818ec3.

konradkonrad mentioned this pull request Nov 23, 2023

Upgrade go-ethereum to match shlib's version shutter-network/rolling-shutter#422

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/geth: implement dev mode for post-merge #27327

cmd/geth: implement dev mode for post-merge #27327

jwasinger commented May 23, 2023 •

edited

Loading

holiman commented May 24, 2023

jwasinger commented May 24, 2023

s1na May 25, 2023

gballet Jun 1, 2023

holiman Jun 13, 2023

s1na left a comment

jwasinger commented Jun 1, 2023

gballet left a comment

gballet Jun 1, 2023

gballet Jun 1, 2023

jwasinger Jun 1, 2023

gballet Jun 1, 2023

jwasinger Jun 2, 2023 •

edited

Loading

lightclient Jun 20, 2023

rjl493456442 Jun 20, 2023

This comment was marked as resolved.

jwasinger commented Jun 2, 2023

jwasinger commented Jun 2, 2023 •

edited

Loading

jwasinger commented Jun 12, 2023

lightclient commented Jun 13, 2023

holiman commented Jun 13, 2023

holiman Jun 13, 2023

holiman Jun 13, 2023 •

edited

Loading

jwasinger Jun 13, 2023

holiman left a comment

hadv commented Aug 15, 2023

jwasinger commented Aug 15, 2023

hadv commented Aug 16, 2023

rmlearney-digicatapult commented Nov 1, 2023

cmd/geth: implement dev mode for post-merge #27327

cmd/geth: implement dev mode for post-merge #27327

Conversation

jwasinger commented May 23, 2023 • edited Loading

holiman commented May 24, 2023

jwasinger commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na left a comment

Choose a reason for hiding this comment

jwasinger commented Jun 1, 2023

gballet left a 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

jwasinger Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

jwasinger commented Jun 2, 2023

jwasinger commented Jun 2, 2023 • edited Loading

jwasinger commented Jun 12, 2023

lightclient commented Jun 13, 2023

holiman commented Jun 13, 2023

Choose a reason for hiding this comment

holiman Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

hadv commented Aug 15, 2023

jwasinger commented Aug 15, 2023

hadv commented Aug 16, 2023

rmlearney-digicatapult commented Nov 1, 2023

jwasinger commented May 23, 2023 •

edited

Loading

jwasinger Jun 2, 2023 •

edited

Loading

jwasinger commented Jun 2, 2023 •

edited

Loading

holiman Jun 13, 2023 •

edited

Loading