From 9410ec5fc127596ae49bf981ef2e03f6b94fe066 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Garamv=C3=B6lgyi?= Date: Fri, 28 Feb 2025 16:44:05 +0100 Subject: [PATCH 1/2] feat: address system contract consensus issues and comments --- consensus/clique/clique.go | 2 + consensus/system_contract/api.go | 13 +++-- consensus/system_contract/consensus.go | 21 ++++--- consensus/system_contract/system_contract.go | 30 ++++++---- consensus/wrapper/consensus.go | 61 ++++++++------------ core/types/block.go | 15 ++++- eth/protocols/eth/handlers.go | 2 + internal/web3ext/web3ext.go | 8 +++ miner/scroll_worker.go | 5 +- params/config.go | 4 ++ params/version.go | 2 +- 11 files changed, 98 insertions(+), 65 deletions(-) diff --git a/consensus/clique/clique.go b/consensus/clique/clique.go index 448cf75efab0..bbc90195092b 100644 --- a/consensus/clique/clique.go +++ b/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 { diff --git a/consensus/system_contract/api.go b/consensus/system_contract/api.go index c975347f1b99..e73467c36acc 100644 --- a/consensus/system_contract/api.go +++ b/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 } diff --git a/consensus/system_contract/consensus.go b/consensus/system_contract/consensus.go index 456cf0d7ef42..ce33b84887f5 100644 --- a/consensus/system_contract/consensus.go +++ b/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, }} } diff --git a/consensus/system_contract/system_contract.go b/consensus/system_contract/system_contract.go index ebd753661788..8e99c9cfa3d6 100644 --- a/consensus/system_contract/system_contract.go +++ b/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 +} diff --git a/consensus/wrapper/consensus.go b/consensus/wrapper/consensus.go index 040de0ef467f..bb7c8eb09dc9 100644 --- a/consensus/wrapper/consensus.go +++ b/consensus/wrapper/consensus.go @@ -2,7 +2,6 @@ package wrapper import ( "math/big" - "sync" "github.com/scroll-tech/go-ethereum/common" "github.com/scroll-tech/go-ethereum/consensus" @@ -10,13 +9,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 +28,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 +37,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 +62,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 +99,38 @@ 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.Warn("Verifying EuclidV2 transition header chain", "startBlockNumber", headers[0].Number, "endBlockNumber", headers[len(headers)-1].Number, "transitionBlockNumber", headers[splitIndex].Number) - // 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) + defer close(results) + + // Verify clique headers. + abortClique, cliqueResults := ue.clique.VerifyHeaders(chain, cliqueHeaders, cliqueSeals) for err := range cliqueResults { select { case <-abort: + close(abortClique) return - case out <- err: + case results <- err: } } - }() - go func() { - defer wg.Done() - _, systemResults := ue.system.VerifyHeaders(chain, systemHeaders, systemSeals) + // Verify system contract headers. + abortSystem, systemResults := ue.system.VerifyHeaders(chain, systemHeaders, systemSeals) for err := range systemResults { select { case <-abort: + close(abortSystem) return - case out <- err: + case results <- err: } } }() - // 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,18 +165,7 @@ 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. diff --git a/core/types/block.go b/core/types/block.go index 644291c52992..28b81244b490 100644 --- a/core/types/block.go +++ b/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:"-"` } diff --git a/eth/protocols/eth/handlers.go b/eth/protocols/eth/handlers.go index 766bc1370fff..fd1072596871 100644 --- a/eth/protocols/eth/handlers.go +++ b/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 } diff --git a/internal/web3ext/web3ext.go b/internal/web3ext/web3ext.go index 8de02ae6f57e..325510034186 100644 --- a/internal/web3ext/web3ext.go +++ b/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', + }), ] }); ` diff --git a/miner/scroll_worker.go b/miner/scroll_worker.go index 4ae5413f60e2..aea1e754b364 100644 --- a/miner/scroll_worker.go +++ b/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 diff --git a/params/config.go b/params/config.go index af6d82377273..1684a1bc3f8c 100644 --- a/params/config.go +++ b/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" } diff --git a/params/version.go b/params/version.go index b5ef4640b6e3..932a969d2156 100644 --- a/params/version.go +++ b/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 ) From f32fb17b06770a2afc9f74d62ab512280e651994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Garamv=C3=B6lgyi?= Date: Sun, 2 Mar 2025 20:38:22 +0100 Subject: [PATCH 2/2] fix Euclid header chain verification --- consensus/wrapper/consensus.go | 40 +++++++++++++++++++++++++++------- miner/scroll_worker.go | 1 + 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/consensus/wrapper/consensus.go b/consensus/wrapper/consensus.go index bb7c8eb09dc9..5feaed61a47d 100644 --- a/consensus/wrapper/consensus.go +++ b/consensus/wrapper/consensus.go @@ -2,6 +2,7 @@ package wrapper import ( "math/big" + "time" "github.com/scroll-tech/go-ethereum/common" "github.com/scroll-tech/go-ethereum/consensus" @@ -99,7 +100,7 @@ func (ue *UpgradableEngine) VerifyHeaders(chain consensus.ChainHeaderReader, hea systemHeaders := headers[splitIndex:] systemSeals := seals[splitIndex:] - log.Warn("Verifying EuclidV2 transition header chain", "startBlockNumber", headers[0].Number, "endBlockNumber", headers[len(headers)-1].Number, "transitionBlockNumber", headers[splitIndex].Number) + log.Info("Verifying EuclidV2 transition header chain") // Do verification concurrently, // but make sure to run Clique first, then SystemContract, @@ -108,26 +109,43 @@ func (ue *UpgradableEngine) VerifyHeaders(chain consensus.ChainHeaderReader, hea 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) - for err := range cliqueResults { + + // 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 results <- err: + case err := <-cliqueResults: + results <- err } } + // 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) - for err := range systemResults { + + // 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 results <- err: + case err := <-systemResults: + results <- err } } + + log.Info("Completed EuclidV2 transition verification") }() return abort, results @@ -171,10 +189,16 @@ func (ue *UpgradableEngine) APIs(chain consensus.ChainHeaderReader) []rpc.API { // 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) + } + if err1 != nil { + return err1 } - return ue.system.Close() + return err2 } // SealHash returns the hash of a block prior to it being sealed. diff --git a/miner/scroll_worker.go b/miner/scroll_worker.go index aea1e754b364..2818abc761bc 100644 --- a/miner/scroll_worker.go +++ b/miner/scroll_worker.go @@ -463,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 }