Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ethclient/simulated: implement new sim backend #28202

Merged
merged 32 commits into from
Jan 10, 2024

Conversation

MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Sep 26, 2023

This is a rewrite of the 'simulated backend', an implementation of the ethclient interfaces
which is backed by a simulated blockchain. It was getting annoying to maintain the old
version of the simulated backend feature because there was a lot of code duplication with
the main client.

The new version is built using parts that we already have: an in-memory geth node instance
running in developer mode provides the chain, while the Go API is provided by ethclient.
A backwards-compatibility wrapper is provided, but the simulated backend has also moved to
a more sensible import path: github.com/ethereum/go-ethereum/ethclient/simulated

closes #28573
closes #21457
closes #28431
closes #28144

@MariusVanDerWijden
Copy link
Member Author

One thing I'm not quite sure about is that I actually slightly changed the interface of the constructor from
NewSimulatedBackend() (*SimulatedBackend) to NewSimulatedBackend() (*SimulatedBackend, error) since way more stuff can go wrong. I could just return nil and expect the caller to make sure that the backend is initialized correctly, but I don't know, would be good to discuss a bit

@MariusVanDerWijden MariusVanDerWijden marked this pull request as ready for review November 27, 2023 11:26
@MariusVanDerWijden
Copy link
Member Author

This PR actually changes the way AdjustTime works. Previously the simulated backend would adjust the time of the pending block (as long as it was empty). Now AdjustTime will create a new block. This behavior change is okay in my opinion since the old way required callers to have an empty block anyway when calling adjust time.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much experience, in general, with using the simulated backend, so can't really review the "ux" of these changes.

accounts/abi/bind/backends/simulated.go Outdated Show resolved Hide resolved
accounts/abi/bind/backends/simulated.go Outdated Show resolved Hide resolved
accounts/abi/bind/backends/simulated.go Outdated Show resolved Hide resolved
accounts/abi/bind/backends/simulated.go Outdated Show resolved Hide resolved
accounts/abi/bind/backends/simulated.go Outdated Show resolved Hide resolved
accounts/abi/bind/backends/simulated.go Outdated Show resolved Hide resolved
@fjl
Copy link
Contributor

fjl commented Dec 4, 2023

I think we should create more interfaces in the top-level package to cover these methods. We should probably also review the ethclient API for anything we missed that isn't covered by tests.

@fjl
Copy link
Contributor

fjl commented Dec 7, 2023

So my proposal is:

we move the simulated backend implementation to a new package ethclient/simulated, and rename it to something like Backend. So users of the package would see it as simulated.Backend.

In package accounts/abi/bind, we create a wrapper struct and mark it deprecated.

For the new type, one thing I would like to figure out is whether we should separate the SimChainManagement methods from the simulated backend somehow. At the moment, the simulated backend object implements all of the ethclient methods and also Commit, Fork, etc.

I think it might be better to have these separated, i.e. the simulated backend object should have Commit, Fork, Close etc. and also a method Client to retrieve another object that implements the ethclient interfaces. The advantage would be, if we introduce ethclient v2 later, we could add another method to return the v2 as well.

@MariusVanDerWijden
Copy link
Member Author

I moved the Sim implementation to its new package now and added a stump & deprecation notice

@ARR4N
Copy link
Contributor

ARR4N commented Dec 8, 2023

One thing I'm not quite sure about is that I actually slightly changed the interface of the constructor from NewSimulatedBackend() (*SimulatedBackend) to NewSimulatedBackend() (*SimulatedBackend, error) since way more stuff can go wrong. I could just return nil and expect the caller to make sure that the backend is initialized correctly, but I don't know, would be good to discuss a bit

I think you should just return the errors if they occur. The return of an error in a function signature signals to the user that they need to check it, whereas the lack of one suggests that failure is impossible. Expecting users to know to check the backend initialisation is more error prone and, IMO, will frustrate users who experience unreported errors that they need to search for.

@fjl
Copy link
Contributor

fjl commented Dec 8, 2023

The constructor really cannot fail since no I/O will occur. And it is way more convenient to avoid the error in tests.

accounts/abi/bind/util_test.go Outdated Show resolved Hide resolved
ethclient/simulated/backend.go Outdated Show resolved Hide resolved
@@ -117,6 +126,10 @@ func (c *SimulatedBeacon) setFeeRecipient(feeRecipient common.Address) {
func (c *SimulatedBeacon) Start() error {
if c.period == 0 {
go c.loopOnDemand()
} else if c.period == math.MaxUint64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to supplant the "loopOnDemand" logic with your commit format. Right now, we have two special periods for SimulatedBeacon that do kind of the same thing: mine blocks arbitrarily.

Since dev mode is really the only thing that consumes this, you could wrap SimulatedBeacon in another object which retains the original behavior for period == 0, it just does so by first sending the txs / wxs and then calls Commit().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mean all transactions would need to be cached in this other object, and we would only pass them to the txpool on Commit. The issue with that would be that all the tx verification logic would need to be duplicated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need to cache them? This is what I had in mind (that wrapper already existed, it's called "api"):

diff --git a/eth/catalyst/simulated_beacon.go b/eth/catalyst/simulated_beacon.go
index 291f91874..7012a53e1 100644
--- a/eth/catalyst/simulated_beacon.go
+++ b/eth/catalyst/simulated_beacon.go
@@ -19,14 +19,12 @@ package catalyst
 import (
 	"crypto/rand"
 	"errors"
-	"math"
 	"math/big"
 	"sync"
 	"time"
 
 	"github.com/ethereum/go-ethereum/beacon/engine"
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/eth"
 	"github.com/ethereum/go-ethereum/log"
@@ -125,9 +123,7 @@ func (c *SimulatedBeacon) setFeeRecipient(feeRecipient common.Address) {
 // Start invokes the SimulatedBeacon life-cycle function in a goroutine.
 func (c *SimulatedBeacon) Start() error {
 	if c.period == 0 {
-		go c.loopOnDemand()
-	} else if c.period == math.MaxUint64 {
-		// if period is set to MaxUint, do not mine at all
+		// if period is set to 0, do not mine at all
 		// this is used in the simulated backend where blocks
 		// are explicitly mined via Commit, AdjustTime and Fork
 	} else {
@@ -204,32 +200,6 @@ func (c *SimulatedBeacon) sealBlock(withdrawals []*types.Withdrawal, timestamp u
 	return nil
 }
 
-// loopOnDemand runs the block production loop for "on-demand" configuration (period = 0)
-func (c *SimulatedBeacon) loopOnDemand() {
-	var (
-		newTxs = make(chan core.NewTxsEvent)
-		sub    = c.eth.TxPool().SubscribeTransactions(newTxs, true)
-	)
-	defer sub.Unsubscribe()
-
-	for {
-		select {
-		case <-c.shutdownCh:
-			return
-		case w := <-c.withdrawals.pending:
-			withdrawals := append(c.withdrawals.gatherPending(9), w)
-			if err := c.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil {
-				log.Warn("Error performing sealing work", "err", err)
-			}
-		case <-newTxs:
-			withdrawals := c.withdrawals.gatherPending(10)
-			if err := c.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil {
-				log.Warn("Error performing sealing work", "err", err)
-			}
-		}
-	}
-}
-
 // loop runs the block production loop for non-zero period configuration
 func (c *SimulatedBeacon) loop() {
 	timer := time.NewTimer(0)
@@ -318,10 +288,12 @@ func (c *SimulatedBeacon) AdjustTime(adjustment time.Duration) error {
 }
 
 func RegisterSimulatedBeaconAPIs(stack *node.Node, sim *SimulatedBeacon) {
+	api := &api{sim}
+	go api.loop()
 	stack.RegisterAPIs([]rpc.API{
 		{
 			Namespace: "dev",
-			Service:   &api{sim},
+			Service:   api,
 			Version:   "1.0",
 		},
 	})
diff --git a/eth/catalyst/simulated_beacon_api.go b/eth/catalyst/simulated_beacon_api.go
index 93670257f..73d0a5921 100644
--- a/eth/catalyst/simulated_beacon_api.go
+++ b/eth/catalyst/simulated_beacon_api.go
@@ -18,19 +18,44 @@ package catalyst
 
 import (
 	"context"
+	"time"
 
 	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
+	"github.com/ethereum/go-ethereum/log"
 )
 
 type api struct {
-	simBeacon *SimulatedBeacon
+	sim *SimulatedBeacon
+}
+
+func (a *api) loop() {
+	var (
+		newTxs = make(chan core.NewTxsEvent)
+		sub    = a.sim.eth.TxPool().SubscribeTransactions(newTxs, true)
+	)
+	defer sub.Unsubscribe()
+
+	for {
+		select {
+		case <-a.sim.shutdownCh:
+			return
+		case w := <-a.sim.withdrawals.pending:
+			withdrawals := append(a.sim.withdrawals.gatherPending(9), w)
+			if err := a.sim.sealBlock(withdrawals, uint64(time.Now().Unix())); err != nil {
+				log.Warn("Error performing sealing work", "err", err)
+			}
+		case <-newTxs:
+			a.sim.Commit()
+		}
+	}
 }
 
 func (a *api) AddWithdrawal(ctx context.Context, withdrawal *types.Withdrawal) error {
-	return a.simBeacon.withdrawals.add(withdrawal)
+	return a.sim.withdrawals.add(withdrawal)
 }
 
 func (a *api) SetFeeRecipient(ctx context.Context, feeRecipient common.Address) {
-	a.simBeacon.setFeeRecipient(feeRecipient)
+	a.sim.setFeeRecipient(feeRecipient)
 }

Basically just moved "loopOnDemand" into the api struct so it only runs when in dev mode. Now if period is 0, you have to manually call commit (as desired when using MaxUint before).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that makes sense to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied your patch (and some more fixes) pls check

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look correct to me, but i haven't run in the on-demand devmode with the update yet. I noticed there are tests for devmode, but only if the period is non-zero.

eth/catalyst/simulated_beacon.go Outdated Show resolved Hide resolved
eth/catalyst/simulated_beacon.go Show resolved Hide resolved
ethclient/simulated/backend.go Show resolved Hide resolved
ethclient/simulated/backend.go Show resolved Hide resolved
ethclient/simulated/backend.go Outdated Show resolved Hide resolved
interfaces.go Show resolved Hide resolved
@fjl fjl changed the title eth/catalyst: accounts/abi/bind/backend: implement new sim backend ethclient/simulated: implement new sim backend Jan 9, 2024
@MariusVanDerWijden MariusVanDerWijden modified the milestones: 1.13.8, 1.13.9 Jan 10, 2024
@fjl fjl merged commit 2d08c99 into ethereum:master Jan 10, 2024
1 of 3 checks passed
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
This is a rewrite of the 'simulated backend', an implementation of the ethclient interfaces
which is backed by a simulated blockchain. It was getting annoying to maintain the old
version of the simulated backend feature because there was a lot of code duplication with
the main client. 

The new version is built using parts that we already have: an in-memory geth node instance
running in developer mode provides the chain, while the Go API is provided by ethclient.
A backwards-compatibility wrapper is provided, but the simulated backend has also moved to
a more sensible import path: github.com/ethereum/go-ethereum/ethclient/simulated

---------

Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Gary Rong <[email protected]>
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Apr 29, 2024
This is a rewrite of the 'simulated backend', an implementation of the ethclient interfaces
which is backed by a simulated blockchain. It was getting annoying to maintain the old
version of the simulated backend feature because there was a lot of code duplication with
the main client.

The new version is built using parts that we already have: an in-memory geth node instance
running in developer mode provides the chain, while the Go API is provided by ethclient.
A backwards-compatibility wrapper is provided, but the simulated backend has also moved to
a more sensible import path: github.com/ethereum/go-ethereum/ethclient/simulated

---------

Co-authored-by: Felix Lange <[email protected]>
Co-authored-by: Gary Rong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants