feat: address system contract consensus issues and comments (#1126)

* feat: address system contract consensus issues and comments

* fix Euclid header chain verification

consensus/clique/clique.go

@@ -191,6 +191,8 @@ type Clique struct {
 // New creates a Clique proof-of-authority consensus engine with the initial
 // signers set to the ones provided by the user.
 func New(config *params.CliqueConfig, db ethdb.Database) *Clique {
+	log.Info("Initializing clique consensus engine")
+
 	// Set any missing consensus parameters to their defaults
 	conf := *config
 	if conf.Epoch == 0 {

consensus/system_contract/api.go

@@ -2,17 +2,20 @@ package system_contract
 
 import (
 	"github.com/scroll-tech/go-ethereum/common"
-	"github.com/scroll-tech/go-ethereum/consensus"
-	"github.com/scroll-tech/go-ethereum/rpc"
 )
 
 // API is a user facing RPC API to allow controlling the signer and voting
 // mechanisms of the proof-of-authority scheme.
 type API struct {
-	chain consensus.ChainHeaderReader
+	system_contract *SystemContract
 }
 
 // GetSigners retrieves the list of authorized signers at the specified block.
-func (api *API) GetSigners(number *rpc.BlockNumber) ([]common.Address, error) {
-	return nil, nil
+func (api *API) GetLocalSigner() (common.Address, error) {
+	return api.system_contract.localSignerAddress(), nil
+}
+
+// GetSigners retrieves the list of authorized signers at the specified block.
+func (api *API) GetAuthorizedSigner() (common.Address, error) {
+	return api.system_contract.currentSignerAddressL1(), nil
 }

consensus/system_contract/consensus.go

@@ -36,20 +36,27 @@ var (
 	// errUnknownBlock is returned when the list of signers is requested for a block
 	// that is not part of the local blockchain.
 	errUnknownBlock = errors.New("unknown block")
+
 	// errNonceNotEmpty is returned if a nonce value is non-zero
 	errInvalidNonce = errors.New("nonce not empty nor zero")
+
 	// errMissingSignature is returned if a block's extra-data section doesn't seem
 	// to contain a 65 byte secp256k1 signature.
 	errMissingSignature = errors.New("extra-data 65 byte signature missing")
+
 	// errInvalidMixDigest is returned if a block's mix digest is non-zero.
 	errInvalidMixDigest = errors.New("non-zero mix digest")
+
 	// errInvalidUncleHash is returned if a block contains an non-empty uncle list.
 	errInvalidUncleHash = errors.New("non empty uncle hash")
+
 	// errInvalidDifficulty is returned if a difficulty value is non-zero
 	errInvalidDifficulty = errors.New("non-one difficulty")
+
 	// errInvalidTimestamp is returned if the timestamp of a block is lower than
-	// the previous block's timestamp + the minimum block period.
+	// the previous block's timestamp.
 	errInvalidTimestamp = errors.New("invalid timestamp")
+
 	// errInvalidExtra is returned if the extra data is not empty
 	errInvalidExtra = errors.New("invalid extra")
 )
@@ -335,10 +342,6 @@ func ecrecover(header *types.Header) (common.Address, error) {
 
 // SystemContractRLP returns the rlp bytes which needs to be signed for the system contract
 // sealing. The RLP to sign consists of the entire header apart from the ExtraData
-//
-// Note, the method requires the extra data to be at least 65 bytes, otherwise it
-// panics. This is done to avoid accidentally using both forms (signature present
-// or not), which could be abused to produce different hashes for the same header.
 func SystemContractRLP(header *types.Header) []byte {
 	b := new(bytes.Buffer)
 	encodeSigHeader(b, header)
@@ -354,8 +357,12 @@ func (s *SystemContract) CalcDifficulty(chain consensus.ChainHeaderReader, time
 // controlling the signer voting.
 func (s *SystemContract) APIs(chain consensus.ChainHeaderReader) []rpc.API {
 	return []rpc.API{{
-		Namespace: "system_contract",
-		Service:   &API{},
+		// note: cannot use underscore in namespace,
+		// but overlap with another module's name space works fine.
+		Namespace: "scroll",
+		Version:   "1.0",
+		Service:   &API{system_contract: s},
+		Public:    false,
 	}}
 }
 

consensus/system_contract/system_contract.go

@@ -16,10 +16,11 @@ const (
 	defaultSyncInterval = 10 * time.Second
 )
 
-// SystemContract
+// SystemContract is the proof-of-authority consensus engine that fetches
+// the authorized signer from the SystemConfig contract, starting from EuclidV2.
 type SystemContract struct {
 	config *params.SystemContractConfig // Consensus engine configuration parameters
-	client sync_service.EthClient
+	client sync_service.EthClient       // RPC client to fetch info from L1
 
 	signerAddressL1 common.Address // Address of the signer stored in L1 System Contract
 
@@ -32,8 +33,10 @@ type SystemContract struct {
 }
 
 // New creates a SystemContract consensus engine with the initial
-// signers set to the ones provided by the user.
+// authorized signer fetched from L1 (if available).
 func New(ctx context.Context, config *params.SystemContractConfig, client sync_service.EthClient) *SystemContract {
+	log.Info("Initializing system_contract consensus engine", "config", config)
+
 	ctx, cancel := context.WithCancel(ctx)
 
 	s := &SystemContract{
@@ -53,7 +56,7 @@ func New(ctx context.Context, config *params.SystemContractConfig, client sync_s
 // Authorize injects a private key into the consensus engine to mint new blocks
 // with.
 func (s *SystemContract) Authorize(signer common.Address, signFn SignerFn) {
-	log.Info("Authorizing system contract", "signer", signer.Hex())
+	log.Info("Authorizing system contract consensus", "signer", signer.Hex())
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
@@ -93,19 +96,17 @@ func (s *SystemContract) fetchAddressFromL1() error {
 
 	// Validate the address is not empty
 	if bAddress == (common.Address{}) {
-		return fmt.Errorf("retrieved empty signer address from L1 System Contract")
+		return fmt.Errorf("retrieved empty signer address from L1 System Contract: contract=%s, slot=%s", s.config.SystemContractAddress.Hex(), s.config.SystemContractSlot.Hex())
 	}
 
 	log.Debug("Read address from system contract", "address", bAddress.Hex())
 
-	s.lock.RLock()
-	addressChanged := s.signerAddressL1 != bAddress
-	s.lock.RUnlock()
+	s.lock.Lock()
+	defer s.lock.Unlock()
 
-	if addressChanged {
-		s.lock.Lock()
+	if s.signerAddressL1 != bAddress {
 		s.signerAddressL1 = bAddress
-		s.lock.Unlock()
+		log.Info("Updated new signer from L1 system contract", "signer", bAddress.Hex())
 	}
 
 	return nil
@@ -123,3 +124,10 @@ func (s *SystemContract) currentSignerAddressL1() common.Address {
 
 	return s.signerAddressL1
 }
+
+func (s *SystemContract) localSignerAddress() common.Address {
+	s.lock.RLock()
+	defer s.lock.RUnlock()
+
+	return s.signer
+}

consensus/wrapper/consensus.go

@@ -2,7 +2,7 @@ package wrapper
 
 import (
 	"math/big"
-	"sync"
+	"time"
 
 	"github.com/scroll-tech/go-ethereum/common"
 	"github.com/scroll-tech/go-ethereum/consensus"
@@ -10,13 +10,14 @@ import (
 	"github.com/scroll-tech/go-ethereum/consensus/system_contract"
 	"github.com/scroll-tech/go-ethereum/core/state"
 	"github.com/scroll-tech/go-ethereum/core/types"
+	"github.com/scroll-tech/go-ethereum/log"
 	"github.com/scroll-tech/go-ethereum/rpc"
 )
 
 // UpgradableEngine implements consensus.Engine and acts as a middleware to dispatch
 // calls to either Clique or SystemContract consensus.
 type UpgradableEngine struct {
-	// forkBlock is the block number at which the switchover to SystemContract occurs.
+	// isUpgraded takes a block timestamp, and returns true once the engine should be upgraded to SystemContract.
 	isUpgraded func(uint64) bool
 
 	// clique is the original Clique consensus engine.
@@ -28,6 +29,8 @@ type UpgradableEngine struct {
 
 // NewUpgradableEngine constructs a new upgradable consensus middleware.
 func NewUpgradableEngine(isUpgraded func(uint64) bool, clique consensus.Engine, system consensus.Engine) *UpgradableEngine {
+	log.Info("Initializing upgradable consensus engine")
+
 	return &UpgradableEngine{
 		isUpgraded: isUpgraded,
 		clique:     clique,
@@ -35,7 +38,7 @@ func NewUpgradableEngine(isUpgraded func(uint64) bool, clique consensus.Engine,
 	}
 }
 
-// chooseEngine returns the appropriate consensus engine based on the header's number.
+// chooseEngine returns the appropriate consensus engine based on the header's timestamp.
 func (ue *UpgradableEngine) chooseEngine(header *types.Header) consensus.Engine {
 	if ue.isUpgraded(header.Time) {
 		return ue.system
@@ -60,12 +63,12 @@ func (ue *UpgradableEngine) VerifyHeader(chain consensus.ChainHeaderReader, head
 // headers can only be all system, all clique, or start with clique and then switch once to system.
 func (ue *UpgradableEngine) VerifyHeaders(chain consensus.ChainHeaderReader, headers []*types.Header, seals []bool) (chan<- struct{}, <-chan error) {
 	abort := make(chan struct{})
-	out := make(chan error)
+	results := make(chan error, len(headers))
 
 	// If there are no headers, return a closed error channel.
 	if len(headers) == 0 {
-		close(out)
-		return nil, out
+		close(results)
+		return nil, results
 	}
 
 	// Choose engine for the first and last header.
@@ -97,42 +100,55 @@ func (ue *UpgradableEngine) VerifyHeaders(chain consensus.ChainHeaderReader, hea
 	systemHeaders := headers[splitIndex:]
 	systemSeals := seals[splitIndex:]
 
-	// Create a wait group to merge results.
-	var wg sync.WaitGroup
-	wg.Add(2)
+	log.Info("Verifying EuclidV2 transition header chain")
 
-	// Launch concurrent verifications.
+	// Do verification concurrently,
+	// but make sure to run Clique first, then SystemContract,
+	// so that the results are sent in the correct order.
 	go func() {
-		defer wg.Done()
-		_, cliqueResults := ue.clique.VerifyHeaders(chain, cliqueHeaders, cliqueSeals)
-		for err := range cliqueResults {
+		defer close(results)
+
+		// Verify clique headers.
+		log.Info("Start EuclidV2 transition verification in Clique section", "startBlockNumber", cliqueHeaders[0].Number, "endBlockNumber", cliqueHeaders[len(cliqueHeaders)-1].Number)
+		abortClique, cliqueResults := ue.clique.VerifyHeaders(chain, cliqueHeaders, cliqueSeals)
+
+		// Note: cliqueResults is not closed so we cannot directly iterate over it
+		for i := 0; i < len(cliqueHeaders); i++ {
 			select {
 			case <-abort:
+				close(abortClique)
+				log.Warn("Aborted EuclidV2 transition verification in Clique section")
 				return
-			case out <- err:
+			case err := <-cliqueResults:
+				results <- err
 			}
 		}
-	}()
 
-	go func() {
-		defer wg.Done()
-		_, systemResults := ue.system.VerifyHeaders(chain, systemHeaders, systemSeals)
-		for err := range systemResults {
+		// Not sure why we need this here, but without this we get err="unknown ancestor"
+		// at the 1st Euclid block. It seems that `VerifyHeaders` start processing the next
+		// header before the previous one was written into `chain`.
+		time.Sleep(2 * time.Second)
+
+		// Verify system contract headers.
+		log.Info("Start EuclidV2 transition verification in SystemContract section", "startBlockNumber", systemHeaders[0].Number, "endBlockNumber", systemHeaders[len(systemHeaders)-1].Number)
+		abortSystem, systemResults := ue.system.VerifyHeaders(chain, systemHeaders, systemSeals)
+
+		// Note: systemResults is not closed so we cannot directly iterate over it
+		for i := 0; i < len(systemHeaders); i++ {
 			select {
 			case <-abort:
+				close(abortSystem)
+				log.Info("Aborted EuclidV2 transition verification in SystemContract section")
 				return
-			case out <- err:
+			case err := <-systemResults:
+				results <- err
 			}
 		}
+
+		log.Info("Completed EuclidV2 transition verification")
 	}()
 
-	// Close the out channel when both verifications are complete.
-	go func() {
-		wg.Wait()
-		close(out)
-	}()
-
-	return abort, out
+	return abort, results
 }
 
 // Prepare prepares a block header for sealing.
@@ -167,27 +183,22 @@ func (ue *UpgradableEngine) VerifyUncles(chain consensus.ChainReader, block *typ
 
 // APIs returns any RPC APIs exposed by the consensus engine.
 func (ue *UpgradableEngine) APIs(chain consensus.ChainHeaderReader) []rpc.API {
-	// Determine the current chain head.
-	head := chain.CurrentHeader()
-	if head == nil {
-		// Fallback: return the clique APIs (or an empty slice) if we don't have a header.
-		return ue.clique.APIs(chain)
-	}
-
-	// Choose engine based on whether the chain head is before or after the fork block.
-	if ue.isUpgraded(head.Time) {
-		return ue.system.APIs(chain)
-	}
-	return ue.clique.APIs(chain)
+	return append(ue.clique.APIs(chain), ue.system.APIs(chain)...)
 }
 
 // Close terminates the consensus engine.
 func (ue *UpgradableEngine) Close() error {
 	// Always close both engines.
-	if err := ue.clique.Close(); err != nil {
-		return err
+	err1 := ue.clique.Close()
+	err2 := ue.system.Close()
+
+	if err1 != nil || err2 != nil {
+		log.Error("Error while closing upgradable engine", "cliqueError", err1, "systemContractError", err2)
 	}
-	return ue.system.Close()
+	if err1 != nil {
+		return err1
+	}
+	return err2
 }
 
 // SealHash returns the hash of a block prior to it being sealed.

core/types/block.go

@@ -86,10 +86,17 @@ type Header struct {
 	// BaseFee was added by EIP-1559 and is ignored in legacy headers.
 	BaseFee *big.Int `json:"baseFeePerGas" rlp:"optional"`
 
-	// BlockSignature was added by EuclidV2 to make Extra empty and is ignored during hashing
+	// Note: The subsequent fields are rlp:"optional". When encoding a struct with optional
+	// fields, the output RLP list contains all values up to the last non-zero optional field.
+
+	// BlockSignature was added by EuclidV2 to make Extra empty and is ignored during hashing.
+	// This field is stored in db but not included in messages sent on the network wire protocol,
+	// or in RPC responses. See also `PrepareForNetwork` and `PrepareFromNetwork`.
 	BlockSignature []byte `json:"-" rlp:"optional"`
 
-	// IsEuclidV2 was added by EuclidV2 to make Extra empty and is ignored during hashing
+	// IsEuclidV2 was added by EuclidV2 to make Extra empty and is ignored during hashing.
+	// This field is stored in db but not included in messages sent on the network wire protocol,
+	// or in RPC responses. See also `PrepareForNetwork` and `PrepareFromNetwork`.
 	IsEuclidV2 bool `json:"-" rlp:"optional"`
 
 	// WithdrawalsHash was added by EIP-4895 and is ignored in legacy headers.
@@ -108,7 +115,9 @@ type Header struct {
 	// Included for Ethereum compatibility in Scroll SDK
 	ParentBeaconRoot *common.Hash `json:"parentBeaconBlockRoot" rlp:"optional"`
 
-	//Hacky: used internally to mark the header as requested by the downloader at the deliver queue
+	// Hacky: used internally to mark the header as requested by the downloader at the deliver queue.
+	// Note: This is only used internally to mark a previously requested block, it is not included
+	// in db, on the network wire protocol, or in RPC responses.
 	Requested bool `json:"-" rlp:"-"`
 }
 

eth/protocols/eth/handlers.go

@@ -270,6 +270,7 @@ func handleNewBlock(backend Backend, msg Decoder, peer *Peer) error {
 
 	hCopy := ann.Block.Header()
 	if err := hCopy.NetworkCompatibleEuclidV2Fields(); err != nil {
+		log.Warn("Disconnecting peer in handleNewBlock", "peer", peer.id, "reason", err)
 		peer.Peer.Disconnect(p2p.DiscUselessPeer)
 		return err
 	}
@@ -309,6 +310,7 @@ func handleBlockHeaders66(backend Backend, msg Decoder, peer *Peer) error {
 	for _, header := range res.BlockHeadersPacket {
 		hCopy := types.CopyHeader(header)
 		if err := hCopy.NetworkCompatibleEuclidV2Fields(); err != nil {
+			log.Warn("Disconnecting peer in handleBlockHeaders66", "peer", peer.id, "reason", err)
 			peer.Peer.Disconnect(p2p.DiscUselessPeer)
 			return err
 		}

internal/web3ext/web3ext.go

@@ -975,6 +975,14 @@ web3._extend({
 			name: 'syncStatus',
 			getter: 'scroll_syncStatus',
 		}),
+		new web3._extend.Property({
+			name: 'authorizedSigner',
+			getter: 'scroll_getAuthorizedSigner',
+		}),
+		new web3._extend.Property({
+			name: 'localSigner',
+			getter: 'scroll_getLocalSigner',
+		}),
 	]
 });
 `

miner/scroll_worker.go

@@ -363,7 +363,10 @@ func (w *worker) mainLoop() {
 		if errors.As(err, &retryableCommitError) || errors.Is(err, system_contract.ErrUnauthorizedSigner) {
 			log.Warn("failed to commit to a block, retrying", "err", err)
 			if errors.Is(err, system_contract.ErrUnauthorizedSigner) {
-				time.Sleep(5 * time.Second) // half the time it takes for the system contract consensus to read and update the address locally.
+				// half the time it takes for the system contract consensus to read and update the address locally.
+				// note: a blocking wait here might be problematic, since it will prevent progress on
+				// `updateSnapshot` and other functionalities.
+				time.Sleep(5 * time.Second)
 			}
 			if _, err = w.tryCommitNewWork(time.Now(), w.current.header.ParentHash, w.current.reorging, w.current.reorgReason); err != nil {
 				continue
@@ -460,6 +463,7 @@ func (w *worker) collectPendingL1Messages(startIndex uint64) []types.L1MessageTx
 			l1MessagesV1 := rawdb.ReadL1MessagesV1From(w.eth.ChainDb(), startIndex, maxCount)
 			if len(l1MessagesV1) > 0 {
 				// backdate the block to the parent block's timestamp -> not yet EuclidV2
+				// TODO: might need to re-run Prepare here
 				w.current.header.Time = parent.Time
 				return l1MessagesV1
 			}

params/config.go

@@ -793,8 +793,12 @@ func (c *ChainConfig) String() string {
 	switch {
 	case c.Ethash != nil:
 		engine = c.Ethash
+	case c.Clique != nil && c.SystemContract != nil:
+		engine = "upgradable (clique + system_contract)"
 	case c.Clique != nil:
 		engine = c.Clique
+	case c.SystemContract != nil:
+		engine = c.SystemContract
 	default:
 		engine = "unknown"
 	}

params/version.go

@@ -24,7 +24,7 @@ import (
 const (
 	VersionMajor = 5         // Major version component of the current release
 	VersionMinor = 8         // Minor version component of the current release
-	VersionPatch = 14        // Patch version component of the current release
+	VersionPatch = 15        // Patch version component of the current release
 	VersionMeta  = "mainnet" // Version metadata to append to the version string
 )