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

fix statedb error during mempool #1702

Merged
merged 7 commits into from
Dec 15, 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
37 changes: 22 additions & 15 deletions go/enclave/evm/ethchainadapter/eth_chainadapter.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package ethchainadapter

import (
"fmt"
"math/big"

"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -83,13 +82,31 @@ func (e *EthChainAdapter) SubscribeChainHeadEvent(ch chan<- gethcore.ChainHeadEv

// GetBlock retrieves a specific block, used during pool resets.
func (e *EthChainAdapter) GetBlock(_ common.Hash, number uint64) *gethtypes.Block {
nbatch, err := e.storage.FetchBatchByHeight(number)
var batch *core.Batch

// to avoid a costly select to the db, check whether the batches requested are the last ones which are cached
headBatch, err := e.storage.FetchBatchBySeqNo(e.batchRegistry.HeadBatchSeq().Uint64())
if err != nil {
e.logger.Warn("unable to get batch by height", "number", number, log.ErrKey, err)
e.logger.Error("unable to get head batch", log.ErrKey, err)
return nil
}
if headBatch.Number().Uint64() == number {
batch = headBatch
} else if headBatch.Number().Uint64()-1 == number {
batch, err = e.storage.FetchBatch(headBatch.Header.ParentHash)
if err != nil {
e.logger.Error("unable to get parent of head batch", log.ErrKey, err, log.BatchHashKey, headBatch.Header.ParentHash)
return nil
}
} else {
batch, err = e.storage.FetchBatchByHeight(number)
if err != nil {
e.logger.Error("unable to get batch by height", log.BatchHeightKey, number, log.ErrKey, err)
return nil
}
}

nfromBatch, err := gethencoding.CreateEthBlockFromBatch(nbatch)
nfromBatch, err := gethencoding.CreateEthBlockFromBatch(batch)
if err != nil {
e.logger.Error("unable to convert batch to eth block", log.ErrKey, err)
return nil
Expand All @@ -104,17 +121,7 @@ func (e *EthChainAdapter) StateAt(root common.Hash) (*state.StateDB, error) {
return nil, nil //nolint:nilnil
}

currentBatchSeqNo := e.batchRegistry.HeadBatchSeq()
if currentBatchSeqNo == nil {
return nil, fmt.Errorf("not ready yet")
}
currentBatch, err := e.storage.FetchBatchBySeqNo(currentBatchSeqNo.Uint64())
if err != nil {
e.logger.Warn("unable to get batch by height", "currentBatchSeqNo", currentBatchSeqNo, log.ErrKey, err)
return nil, nil //nolint:nilnil
}

return e.storage.CreateStateDB(currentBatch.Hash())
return state.New(root, e.storage.StateDB(), nil)
}

func (e *EthChainAdapter) IngestNewBlock(batch *core.Batch) error {
Expand Down
3 changes: 3 additions & 0 deletions go/enclave/storage/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@ type Storage interface {

// TrieDB - return the underlying trie database
TrieDB() *trie.Database

// StateDB - return the underlying state database
StateDB() state.Database
}

type ScanStorage interface {
Expand Down
4 changes: 4 additions & 0 deletions go/enclave/storage/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ func (s *storageImpl) TrieDB() *trie.Database {
return s.stateDB.TrieDB()
}

func (s *storageImpl) StateDB() state.Database {
return s.stateDB
}

func (s *storageImpl) Close() error {
return s.db.GetSQLDB().Close()
}
Expand Down
4 changes: 4 additions & 0 deletions go/enclave/txpool/txpool_mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,3 +340,7 @@ func (m *mockStorage) TrieDB() *trie.Database {
// TODO implement me
panic("implement me")
}

func (m *mockStorage) StateDB() state.Database {
return m.stateDB
}
8 changes: 8 additions & 0 deletions integration/faucet/faucet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ const (
)

func TestFaucet(t *testing.T) {
t.Skip("Skipping because it is too flaky")

startPort := integration.StartPortFaucetUnitTest
createObscuroNetwork(t, startPort)
// This sleep is required to ensure the initial rollup exists, and thus contract deployer can check its balance.
Expand All @@ -60,6 +62,12 @@ func TestFaucet(t *testing.T) {
assert.NoError(t, err)

err = faucetContainer.Start()
defer func(faucetContainer *container.FaucetContainer) {
err := faucetContainer.Stop()
if err != nil {
fmt.Printf("Could not stop faucet %s", err.Error())
}
Comment on lines +65 to +69
Copy link

Choose a reason for hiding this comment

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

The addition of the defer function to stop the faucetContainer is a good practice to ensure resources are cleaned up after the test is done. However, the error handling within the defer function could be improved by using the testing object t to log the error instead of printing it to stdout, which is more idiomatic in test cases.

defer func(faucetContainer *container.FaucetContainer) {
	err := faucetContainer.Stop()
	if err != nil {
-		fmt.Printf("Could not stop faucet %s", err.Error())
+		t.Errorf("Could not stop faucet: %s", err.Error())
	}
}(faucetContainer)

Committable 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
defer func(faucetContainer *container.FaucetContainer) {
err := faucetContainer.Stop()
if err != nil {
fmt.Printf("Could not stop faucet %s", err.Error())
}
defer func(faucetContainer *container.FaucetContainer) {
err := faucetContainer.Stop()
if err != nil {
t.Errorf("Could not stop faucet: %s", err.Error())
}

}(faucetContainer)
assert.NoError(t, err)

initialFaucetBal, err := getFaucetBalance(faucetConfig.ServerPort)
Expand Down
2 changes: 1 addition & 1 deletion integration/obscurogateway/tengateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ func transferETHToAddress(client *ethclient.Client, wallet wallet.Wallet, toAddr
if err != nil {
return nil, err
}
return integrationCommon.AwaitReceiptEth(context.Background(), client, signedTx.Hash(), 2*time.Second)
return integrationCommon.AwaitReceiptEth(context.Background(), client, signedTx.Hash(), 20*time.Second)
Comment on lines 583 to +586
Copy link

Choose a reason for hiding this comment

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

The timeout duration for AwaitReceiptEth in the transferETHToAddress function has been increased from 2 seconds to 20 seconds. This change could potentially reduce flakiness in tests due to network delays or other issues. However, it's important to ensure that this increased timeout does not negatively impact the overall test suite's performance, especially if this function is called multiple times in a test run. If the longer timeout is necessary due to network conditions, consider making it configurable or documenting the reason for this specific value.

}

func subscribeToEvents(addresses []gethcommon.Address, topics [][]gethcommon.Hash, client *ethclient.Client, logs *[]types.Log) ethereum.Subscription { //nolint:unparam
Expand Down
2 changes: 1 addition & 1 deletion integration/simulation/simulation.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ func (s *Simulation) prefundL1Accounts() {
func (s *Simulation) checkHealthStatus() {
for _, client := range s.RPCHandles.ObscuroClients {
if healthy, err := client.Health(); !healthy || err != nil {
panic("Client is not healthy")
panic("Client is not healthy: " + err.Error())
Copy link

Choose a reason for hiding this comment

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

Potential nil pointer dereference when calling err.Error() if err is nil. Add a check to ensure err is not nil before using it.

-			panic("Client is not healthy: " + err.Error())
+			if err != nil {
+				panic("Client is not healthy: " + err.Error())
+			} else {
+				panic("Client is not healthy and no error was provided")
+			}

Committable 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
panic("Client is not healthy: " + err.Error())
if err != nil {
panic("Client is not healthy: " + err.Error())
} else {
panic("Client is not healthy and no error was provided")
}

}
}
}
Expand Down
1 change: 1 addition & 0 deletions tools/walletextension/test/wallet_extension_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type testHelper struct {
}

func TestWalletExtension(t *testing.T) {
t.Skip("Skipping because it is too flaky")
i := 0
for name, test := range map[string]func(t *testing.T, testHelper *testHelper){
"canInvokeSensitiveMethodsWithViewingKey": canInvokeSensitiveMethodsWithViewingKey,
Expand Down
Loading