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

Host: Monitor and serve important contract addresses #1645

Merged
merged 5 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions go/common/host/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,8 @@ type L1Publisher interface {
InitializeSecret(attestation *common.AttestationReport, encSecret common.EncryptedSharedEnclaveSecret) error
// RequestSecret will send a management contract transaction to request a secret from the enclave, returning the L1 head at time of sending
RequestSecret(report *common.AttestationReport) (gethcommon.Hash, error)
// ExtractSecretResponses will return all secret response tx from an L1 block
ExtractSecretResponses(block *types.Block) []*ethadapter.L1RespondSecretTx
// ExtractRollupTxs will return all rollup txs from an L1 block
ExtractRollupTxs(block *types.Block) []*ethadapter.L1RollupTx
// ExtractObscuroRelevantTransactions will return all Obscuro relevant tx from an L1 block
ExtractObscuroRelevantTransactions(block *types.Block) ([]*ethadapter.L1RespondSecretTx, []*ethadapter.L1RollupTx, []*ethadapter.L1SetImportantContractsTx)
// PublishRollup will create and publish a rollup tx to the management contract - fire and forget we don't wait for receipt
// todo (#1624) - With a single sequencer, it is problematic if rollup publication fails; handle this case better
PublishRollup(producedRollup *common.ExtRollup)
Expand All @@ -114,6 +112,11 @@ type L1Publisher interface {
FetchLatestPeersList() ([]string, error)

FetchLatestSeqNo() (*big.Int, error)

// GetImportantContracts returns a (cached) record of addresses of the important network contracts
GetImportantContracts() map[string]gethcommon.Address
// ResyncImportantContracts will fetch the latest important contracts from the management contract, update the cache
ResyncImportantContracts() error
}

// L2BatchRepository provides an interface for the host to request L2 batch data (live-streaming and historical)
Expand Down
1 change: 1 addition & 0 deletions go/common/query_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,5 @@ type ObscuroNetworkInfo struct {
L1StartHash common.Hash
SequencerID common.Address
MessageBusAddress common.Address
ImportantContracts map[string]common.Address // map of contract name to address
}
5 changes: 5 additions & 0 deletions go/ethadapter/l1_transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ type L1RespondSecretTx struct {
HostAddress string
}

type L1SetImportantContractsTx struct {
Key string
NewAddress gethcommon.Address
}

// Sign signs the payload with a given private key
func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx {
var data []byte
Comment on lines 42 to 43
Copy link

Choose a reason for hiding this comment

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

The Sign function for L1RespondSecretTx is provided, but it seems to be incomplete. The data variable is declared but not initialized with the Secret, RequesterID, AttesterID, and HostAddress fields of L1RespondSecretTx. This could lead to a runtime panic due to a nil slice. The code should be updated to properly initialize data with the relevant fields before appending.

- var data []byte
+ data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret))

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx {
var data []byte
func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx {
data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret))

Comment on lines 33 to 43
Copy link

Choose a reason for hiding this comment

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

The Sign function for L1RespondSecretTx has been updated to include the HostAddress in the data that is signed. However, the initialization of the data slice is missing, which could lead to a runtime panic due to a nil slice dereference when append is called. The data slice should be initialized with an appropriate size before appending elements to it. Here's a proposed change:

- var data []byte
+ data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret))

This will allocate a slice with enough capacity to hold all the bytes that will be appended, avoiding potential reallocations during the append operations.


Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
HostAddress string
}
type L1SetImportantContractsTx struct {
Key string
NewAddress gethcommon.Address
}
// Sign signs the payload with a given private key
func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx {
var data []byte
HostAddress string
}
type L1SetImportantContractsTx struct {
Key string
NewAddress gethcommon.Address
}
// Sign signs the payload with a given private key
func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx {
data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret))

Expand Down
13 changes: 8 additions & 5 deletions go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the mgm contract sol contract be updated too ?

Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@ package mgmtcontractlib
import "github.com/ten-protocol/go-ten/contracts/generated/ManagementContract"

const (
AddRollupMethod = "AddRollup"
RespondSecretMethod = "RespondNetworkSecret"
RequestSecretMethod = "RequestNetworkSecret"
InitializeSecretMethod = "InitializeNetworkSecret" //#nosec
GetHostAddressesMethod = "GetHostAddresses"
AddRollupMethod = "AddRollup"
RespondSecretMethod = "RespondNetworkSecret"
RequestSecretMethod = "RequestNetworkSecret"
InitializeSecretMethod = "InitializeNetworkSecret" //#nosec
GetHostAddressesMethod = "GetHostAddresses"
GetImportantContractKeysMethod = "GetImportantContractKeys"
SetImportantContractsMethod = "SetImportantContractAddress"
GetImportantAddressMethod = "importantContractAddresses"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this lower case intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's the method that solidity creates magically for the public field

)

var MgmtContractABI = ManagementContract.ManagementContractMetaData.ABI
161 changes: 147 additions & 14 deletions go/ethadapter/mgmtcontractlib/mgmt_contract_lib.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,23 @@ type MgmtContractLib interface {
CreateRequestSecret(tx *ethadapter.L1RequestSecretTx) types.TxData
CreateRespondSecret(tx *ethadapter.L1RespondSecretTx, verifyAttester bool) types.TxData
CreateInitializeSecret(tx *ethadapter.L1InitializeSecretTx) types.TxData
GetHostAddresses() (ethereum.CallMsg, error)

// DecodeTx receives a *types.Transaction and converts it to an common.L1Transaction
DecodeTx(tx *types.Transaction) ethadapter.L1Transaction
// DecodeCallResponse unpacks a call response into a slice of strings.
DecodeCallResponse(callResponse []byte) ([][]string, error)
GetContractAddr() *gethcommon.Address

// The methods below are used to create call messages for mgmt contract data and unpack the responses

GetHostAddressesMsg() (ethereum.CallMsg, error)
DecodeHostAddressesResponse(callResponse []byte) ([]string, error)

SetImportantContractMsg(key string, address gethcommon.Address) (ethereum.CallMsg, error)

GetImportantContractKeysMsg() (ethereum.CallMsg, error)
DecodeImportantContractKeysResponse(callResponse []byte) ([]string, error)

GetImportantAddressCallMsg(key string) (ethereum.CallMsg, error)
DecodeImportantAddressResponse(callResponse []byte) (gethcommon.Address, error)
}

type contractLibImpl struct {
Expand Down Expand Up @@ -93,6 +103,14 @@ func (c *contractLibImpl) DecodeTx(tx *types.Transaction) ethadapter.L1Transacti

case InitializeSecretMethod:
return c.unpackInitSecretTx(tx, method, contractCallData)

case SetImportantContractsMethod:
tx, err := c.unpackSetImportantContractsTx(tx, method, contractCallData)
if err != nil {
c.logger.Warn("could not unpack set important contracts tx", log.ErrKey, err)
return nil
}
return tx
}

return nil
Comment on lines 103 to 116
Copy link

Choose a reason for hiding this comment

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

The DecodeTx method now includes a case for SetImportantContractsMethod. The error handling has been improved by logging a warning instead of panicking, which is a good practice to avoid service disruption. However, ensure that all possible error paths are handled correctly and that the system can recover from these errors gracefully.

// Replace the panic calls with proper error handling
func (c *contractLibImpl) DecodeTx(tx *types.Transaction) ethadapter.L1Transaction {
    // ... existing code ...

    method, err := c.contractABI.MethodById(tx.Data()[:methodBytesLen])
    if err != nil {
        c.logger.Warn("could not find method by ID", log.ErrKey, err)
        return nil
    }

    // ... existing switch case ...

    default:
        c.logger.Warn("unknown method", "methodName", method.Name)
        return nil
    }
}

Expand Down Expand Up @@ -180,31 +198,116 @@ func (c *contractLibImpl) CreateInitializeSecret(tx *ethadapter.L1InitializeSecr
}
}

func (c *contractLibImpl) GetHostAddresses() (ethereum.CallMsg, error) {
func (c *contractLibImpl) GetHostAddressesMsg() (ethereum.CallMsg, error) {
data, err := c.contractABI.Pack(GetHostAddressesMethod)
if err != nil {
return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err)
}
return ethereum.CallMsg{To: c.addr, Data: data}, nil
}

func (c *contractLibImpl) DecodeCallResponse(callResponse []byte) ([][]string, error) {
func (c *contractLibImpl) DecodeHostAddressesResponse(callResponse []byte) ([]string, error) {
unpackedResponse, err := c.contractABI.Unpack(GetHostAddressesMethod, callResponse)
if err != nil {
return nil, fmt.Errorf("could not unpack call response. Cause: %w", err)
}

// We convert the returned interfaces to strings.
unpackedResponseStrings := make([][]string, 0, len(unpackedResponse))
for _, obj := range unpackedResponse {
str, ok := obj.([]string)
if !ok {
return nil, fmt.Errorf("could not convert interface in call response to string")
}
unpackedResponseStrings = append(unpackedResponseStrings, str)
// We expect the response to be a list containing one element, that element is a list of address strings
if len(unpackedResponse) != 1 {
return nil, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse)
}
addresses, ok := unpackedResponse[0].([]string)
if !ok {
return nil, fmt.Errorf("could not convert element in call response to list of strings")
}

return addresses, nil
}

func (c *contractLibImpl) GetContractNamesMsg() (ethereum.CallMsg, error) {
data, err := c.contractABI.Pack(GetImportantContractKeysMethod)
if err != nil {
return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err)
}
return ethereum.CallMsg{To: c.addr, Data: data}, nil
}

func (c *contractLibImpl) DecodeContractNamesResponse(callResponse []byte) ([]string, error) {
unpackedResponse, err := c.contractABI.Unpack(GetImportantContractKeysMethod, callResponse)
if err != nil {
return nil, fmt.Errorf("could not unpack call response. Cause: %w", err)
}

// We expect the response to be a list containing one element, that element is a list of address strings
if len(unpackedResponse) != 1 {
return nil, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse)
}
contractNames, ok := unpackedResponse[0].([]string)
if !ok {
return nil, fmt.Errorf("could not convert element in call response to list of strings")
}

return contractNames, nil
}
Comment on lines +227 to +251
Copy link

Choose a reason for hiding this comment

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

The DecodeContractNamesResponse method appears to be a duplicate of the DecodeImportantContractKeysResponse method (lines 269-285). If this is not intentional and the functionality is indeed the same, one of the methods should be removed to avoid redundancy and potential confusion.

- func (c *contractLibImpl) DecodeContractNamesResponse(callResponse []byte) ([]string, error) {
-     // ... existing code ...
- }

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func (c *contractLibImpl) GetContractNamesMsg() (ethereum.CallMsg, error) {
data, err := c.contractABI.Pack(GetImportantContractKeysMethod)
if err != nil {
return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err)
}
return ethereum.CallMsg{To: c.addr, Data: data}, nil
}
func (c *contractLibImpl) DecodeContractNamesResponse(callResponse []byte) ([]string, error) {
unpackedResponse, err := c.contractABI.Unpack(GetImportantContractKeysMethod, callResponse)
if err != nil {
return nil, fmt.Errorf("could not unpack call response. Cause: %w", err)
}
// We expect the response to be a list containing one element, that element is a list of address strings
if len(unpackedResponse) != 1 {
return nil, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse)
}
contractNames, ok := unpackedResponse[0].([]string)
if !ok {
return nil, fmt.Errorf("could not convert element in call response to list of strings")
}
return contractNames, nil
}


func (c *contractLibImpl) SetImportantContractMsg(key string, address gethcommon.Address) (ethereum.CallMsg, error) {
data, err := c.contractABI.Pack(SetImportantContractsMethod, key, address)
if err != nil {
return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err)
}
return ethereum.CallMsg{To: c.addr, Data: data}, nil
}

return unpackedResponseStrings, nil
func (c *contractLibImpl) GetImportantContractKeysMsg() (ethereum.CallMsg, error) {
data, err := c.contractABI.Pack(GetImportantContractKeysMethod)
if err != nil {
return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err)
}
return ethereum.CallMsg{To: c.addr, Data: data}, nil
}

func (c *contractLibImpl) DecodeImportantContractKeysResponse(callResponse []byte) ([]string, error) {
unpackedResponse, err := c.contractABI.Unpack(GetImportantContractKeysMethod, callResponse)
if err != nil {
return nil, fmt.Errorf("could not unpack call response. Cause: %w", err)
}

// We expect the response to be a list containing one element, that element is a list of address strings
if len(unpackedResponse) != 1 {
return nil, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse)
}
contractNames, ok := unpackedResponse[0].([]string)
if !ok {
return nil, fmt.Errorf("could not convert element in call response to list of strings")
}

return contractNames, nil
}

func (c *contractLibImpl) GetImportantAddressCallMsg(key string) (ethereum.CallMsg, error) {
data, err := c.contractABI.Pack(GetImportantAddressMethod, key)
if err != nil {
return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err)
}
return ethereum.CallMsg{To: c.addr, Data: data}, nil
}

func (c *contractLibImpl) DecodeImportantAddressResponse(callResponse []byte) (gethcommon.Address, error) {
unpackedResponse, err := c.contractABI.Unpack(GetImportantAddressMethod, callResponse)
if err != nil {
return gethcommon.Address{}, fmt.Errorf("could not unpack call response. Cause: %w", err)
}

// We expect the response to be a list containing one element, that element is a list of address strings
if len(unpackedResponse) != 1 {
return gethcommon.Address{}, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse)
}
address, ok := unpackedResponse[0].(gethcommon.Address)
if !ok {
return gethcommon.Address{}, fmt.Errorf("could not convert element in call response to list of strings")
}

return address, nil
}

func (c *contractLibImpl) unpackInitSecretTx(tx *types.Transaction, method *abi.Method, contractCallData map[string]interface{}) *ethadapter.L1InitializeSecretTx {
Expand Down Expand Up @@ -297,6 +400,36 @@ func (c *contractLibImpl) unpackRespondSecretTx(tx *types.Transaction, method *a
}
}

func (c *contractLibImpl) unpackSetImportantContractsTx(tx *types.Transaction, method *abi.Method, contractCallData map[string]interface{}) (*ethadapter.L1SetImportantContractsTx, error) {
err := method.Inputs.UnpackIntoMap(contractCallData, tx.Data()[methodBytesLen:])
if err != nil {
return nil, fmt.Errorf("could not unpack transaction. Cause: %w", err)
}

keyData, found := contractCallData["key"]
if !found {
return nil, fmt.Errorf("call data not found for key")
}
keyString, ok := keyData.(string)
if !ok {
return nil, fmt.Errorf("could not decode key data")
}

contractAddressData, found := contractCallData["newAddress"]
if !found {
return nil, fmt.Errorf("call data not found for newAddress")
}
contractAddress, ok := contractAddressData.(gethcommon.Address)
if !ok {
return nil, fmt.Errorf("could not decode newAddress data")
}

return &ethadapter.L1SetImportantContractsTx{
Key: keyString,
NewAddress: contractAddress,
}, nil
}

// base64EncodeToString encodes a byte array to a string
func base64EncodeToString(bytes []byte) string {
return base64.StdEncoding.EncodeToString(bytes)
Expand Down
19 changes: 12 additions & 7 deletions go/host/enclave/guardian.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,7 @@ func (g *Guardian) provideSecret() error {
if err != nil {
return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err)
}
secretRespTxs := g.sl.L1Publisher().ExtractSecretResponses(nextBlock)
if err != nil {
return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err)
}
secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock)
for _, scrt := range secretRespTxs {
if scrt.RequesterID.Hex() == g.hostData.ID.Hex() {
err = g.enclaveClient.InitEnclave(scrt.Secret)
Comment on lines 276 to 282
Copy link

Choose a reason for hiding this comment

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

The error handling for g.enclaveClient.InitEnclave(scrt.Secret) is missing. If the initialization fails, the loop should continue to the next secret response or exit the retry loop if there are no more responses. The current implementation will exit the retry loop even if there are more secret responses to try, which could lead to missed opportunities for successful enclave initialization.

-				err = g.enclaveClient.InitEnclave(scrt.Secret)
+				if err := g.enclaveClient.InitEnclave(scrt.Secret); err != nil {
+					g.logger.Error("Could not initialize enclave with received secret response", log.ErrKey, err)
+					continue // try the next secret response in the block if there are more
+				}

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if err != nil {
return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err)
}
secretRespTxs := g.sl.L1Publisher().ExtractSecretResponses(nextBlock)
if err != nil {
return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err)
}
secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock)
for _, scrt := range secretRespTxs {
if scrt.RequesterID.Hex() == g.hostData.ID.Hex() {
err = g.enclaveClient.InitEnclave(scrt.Secret)
if err != nil {
return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err)
}
secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock)
for _, scrt := range secretRespTxs {
if scrt.RequesterID.Hex() == g.hostData.ID.Hex() {
if err := g.enclaveClient.InitEnclave(scrt.Secret); err != nil {
g.logger.Error("Could not initialize enclave with received secret response", log.ErrKey, err)
continue // try the next secret response in the block if there are more
}

Expand Down Expand Up @@ -435,13 +432,12 @@ func (g *Guardian) submitL1Block(block *common.L1Block, isLatest bool) (bool, er

func (g *Guardian) processL1BlockTransactions(block *common.L1Block) {
// if there are any secret responses in the block we should refresh our P2P list to re-sync with the network
respTxs := g.sl.L1Publisher().ExtractSecretResponses(block)
if len(respTxs) > 0 {
secretRespTxs, rollupTxs, contractAddressTxs := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(block)
if len(secretRespTxs) > 0 {
// new peers may have been granted access to the network, notify p2p service to refresh its peer list
go g.sl.P2P().RefreshPeerList()
}
Comment on lines 433 to 439
Copy link

Choose a reason for hiding this comment

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

The asynchronous call to g.sl.P2P().RefreshPeerList() is not handling potential errors. It is important to handle errors even in goroutines to ensure that the system can react appropriately, such as retrying the operation or alerting an operator.

-		go g.sl.P2P().RefreshPeerList()
+		go func() {
+			if err := g.sl.P2P().RefreshPeerList(); err != nil {
+				g.logger.Error("Failed to refresh P2P peer list", log.ErrKey, err)
+			}
+		}()

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func (g *Guardian) processL1BlockTransactions(block *common.L1Block) {
// if there are any secret responses in the block we should refresh our P2P list to re-sync with the network
respTxs := g.sl.L1Publisher().ExtractSecretResponses(block)
if len(respTxs) > 0 {
secretRespTxs, rollupTxs, contractAddressTxs := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(block)
if len(secretRespTxs) > 0 {
// new peers may have been granted access to the network, notify p2p service to refresh its peer list
go g.sl.P2P().RefreshPeerList()
}
func (g *Guardian) processL1BlockTransactions(block *common.L1Block) {
// if there are any secret responses in the block we should refresh our P2P list to re-sync with the network
secretRespTxs, rollupTxs, contractAddressTxs := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(block)
if len(secretRespTxs) > 0 {
// new peers may have been granted access to the network, notify p2p service to refresh its peer list
go func() {
if err := g.sl.P2P().RefreshPeerList(); err != nil {
g.logger.Error("Failed to refresh P2P peer list", log.ErrKey, err)
}
}()
}


rollupTxs := g.sl.L1Publisher().ExtractRollupTxs(block)
for _, rollup := range rollupTxs {
r, err := common.DecodeRollup(rollup.Rollup)
if err != nil {
Expand All @@ -456,6 +452,15 @@ func (g *Guardian) processL1BlockTransactions(block *common.L1Block) {
}
}
}

if len(contractAddressTxs) > 0 {
go func() {
err := g.sl.L1Publisher().ResyncImportantContracts()
if err != nil {
g.logger.Error("Could not resync important contracts", log.ErrKey, err)
Comment on lines +456 to +460
Copy link
Contributor

Choose a reason for hiding this comment

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

This kinda breaks the security of the block ingestion, if we go back and do an eth_call someone malicious can interject. Should follow the same pattern of decoding the tx that it's known for a fact is "safe" ?

}
}()
Comment on lines +456 to +462
Copy link

Choose a reason for hiding this comment

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

The previous comment about error handling within the goroutine for ResyncImportantContracts is still valid. There should be a retry mechanism or a way to alert the system that the resync did not succeed. This is crucial for maintaining consistency and ensuring that the system remains up-to-date.

-		go func() {
-			err := g.sl.L1Publisher().ResyncImportantContracts()
+		go retry.Do(func() error {
+			if err := g.sl.L1Publisher().ResyncImportantContracts(); err != nil {
+				g.logger.Error("Could not resync important contracts", log.ErrKey, err)
+				return err // return error to trigger a retry
+			}
+			return nil // success, no need to retry
+		}, retry.NewExponentialBackoffStrategy(5, 100*time.Millisecond, 2*time.Second))

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if len(contractAddressTxs) > 0 {
go func() {
err := g.sl.L1Publisher().ResyncImportantContracts()
if err != nil {
g.logger.Error("Could not resync important contracts", log.ErrKey, err)
}
}()
if len(contractAddressTxs) > 0 {
go retry.Do(func() error {
if err := g.sl.L1Publisher().ResyncImportantContracts(); err != nil {
g.logger.Error("Could not resync important contracts", log.ErrKey, err)
return err // return error to trigger a retry
}
return nil // success, no need to retry
}, retry.NewExponentialBackoffStrategy(5, 100*time.Millisecond, 2*time.Second))

}
}

func (g *Guardian) publishSharedSecretResponses(scrtResponses []*common.ProducedSecretResponse) error {
Expand Down
5 changes: 3 additions & 2 deletions go/host/host.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,9 @@ func (h *host) ObscuroConfig() (*common.ObscuroNetworkInfo, error) {
ManagementContractAddress: h.config.ManagementContractAddress,
L1StartHash: h.config.L1StartHash,

SequencerID: h.config.SequencerID,
MessageBusAddress: h.config.MessageBusAddress,
SequencerID: h.config.SequencerID,
MessageBusAddress: h.config.MessageBusAddress,
ImportantContracts: h.services.L1Publisher().GetImportantContracts(),
}, nil
Comment on lines 204 to 210
Copy link

Choose a reason for hiding this comment

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

The implementation of ObscuroConfig does not handle the potential error that could be returned by GetImportantContracts. This could lead to a runtime panic if GetImportantContracts encounters an error. It is important to handle this error to ensure the robustness of the code. Here is a suggested change:

func (h *host) ObscuroConfig() (*common.ObscuroNetworkInfo, error) {
	importantContracts, err := h.services.L1Publisher().GetImportantContracts()
	if err != nil {
		return nil, fmt.Errorf("failed to get important contracts: %w", err)
	}
	return &common.ObscuroNetworkInfo{
		ManagementContractAddress: h.config.ManagementContractAddress,
		L1StartHash:               h.config.L1StartHash,
		SequencerID:               h.config.SequencerID,
		MessageBusAddress:         h.config.MessageBusAddress,
		ImportantContracts:        importantContracts,
	}, nil
}

This change ensures that any errors encountered when retrieving important contracts are properly handled and reported, preventing potential runtime issues.

}

Expand Down
Loading
Loading