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.

Sign up for GitHub

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 the msg bus deploy at genesis #1652

Merged
merged 5 commits into from
Nov 21, 2023
Merged

Fix the msg bus deploy at genesis #1652

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55da99f
Fix the msg bus deploy at genesis
otherview Nov 17, 2023
1a12801
tests
otherview Nov 17, 2023
4645f03
remove ports on same test
otherview Nov 17, 2023
e63b6fc
remove more test ports
otherview Nov 17, 2023
8869515
lint
otherview Nov 17, 2023
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: 4 additions & 7 deletions go/enclave/components/batch_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext, fail
return nil, fmt.Errorf("failed adding cross chain data to batch. Cause: %w", err)
}

executor.populateHeader(&copyBatch, allReceipts(txReceipts, ccReceipts))
allReceipts := append(txReceipts, ccReceipts...)
executor.populateHeader(&copyBatch, allReceipts)
if failForEmptyBatch &&
len(txReceipts) == 0 &&
len(ccReceipts) == 0 &&
Expand All @@ -220,7 +221,7 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext, fail
}

// the logs and receipts produced by the EVM have the wrong hash which must be adjusted
for _, receipt := range txReceipts {
for _, receipt := range allReceipts {
Copy link
Collaborator

Choose a reason for hiding this comment

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

qq. Was this change required to fix the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly. This was required to make sure the message bus contract address hadn't changed when I was debugging the issue.
Right now everywhere that we refer to the message bus contract address it's hardcoded, which is kinda ok if you know ahead of time, otherwise you'll be looping around trying to find the address output somewhere like I did.
Left it there because I think it's better to be dynamic and potentially it enables Matt's info endpoint and allows Moray to read the address from the logs.

I understand that we were not initially showing the synthetic tx receipts, don't know if there was a major reason behind it, can revert back to the old behavior if this is no buenos!

receipt.BlockHash = copyBatch.Hash()
for _, l := range receipt.Logs {
l.BlockHash = copyBatch.Hash()
Expand All @@ -229,7 +230,7 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext, fail

return &ComputedBatch{
Batch: &copyBatch,
Receipts: txReceipts,
Receipts: allReceipts,
Commit: func(deleteEmptyObjects bool) (gethcommon.Hash, error) {
executor.stateDBMutex.Lock()
defer executor.stateDBMutex.Unlock()
Expand Down Expand Up @@ -429,10 +430,6 @@ func (executor *batchExecutor) processTransactions(
return executedTransactions, excludedTransactions, txReceipts, nil
}

func allReceipts(txReceipts []*types.Receipt, depositReceipts []*types.Receipt) types.Receipts {
return append(txReceipts, depositReceipts...)
}

type sortByTxIndex []*types.Receipt

func (c sortByTxIndex) Len() int { return len(c) }
Expand Down
44 changes: 36 additions & 8 deletions go/enclave/nodetype/sequencer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,33 @@ func (s *sequencer) createGenesisBatch(block *common.L1Block) error {
return err
}

// make sure the mempool queuing system is initialized before adding the msg bus tx to it
time.Sleep(time.Second)
// produce batch #2 which has the message bus and any other system contracts
cb, err := s.produceBatch(
batch.Header.SequencerOrderNo.Add(batch.Header.SequencerOrderNo, big.NewInt(1)),
block.Hash(),
batch.Hash(),
common.L2Transactions{msgBusTx},
uint64(time.Now().Unix()),
false,
)
if err != nil {
if errors.Is(err, components.ErrNoTransactionsToProcess) {
// skip batch production when there are no transactions to process
// todo: this might be a useful event to track for metrics (skipping batch production because empty batch)
s.logger.Debug("Skipping batch production, no transactions to execute")
return nil
}
return fmt.Errorf(" failed producing batch. Cause: %w", err)
}

if err = s.mempool.Add(msgBusTx); err != nil {
return fmt.Errorf("failed to queue message bus creation transaction to genesis - %w", err)
if len(cb.Receipts) == 0 || cb.Receipts[0].TxHash.Hex() != msgBusTx.Hash().Hex() {
err = fmt.Errorf("message Bus contract not minted - no receipts in batch")
s.logger.Error(err.Error())
return err
}

s.logger.Info("Message Bus Contract minted successfully", "address", cb.Receipts[0].ContractAddress.Hex())

return nil
}
otherview marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -232,7 +253,14 @@ func (s *sequencer) createNewHeadBatch(l1HeadBlock *common.L1Block, skipBatchIfE
return nil
}

func (s *sequencer) produceBatch(sequencerNo *big.Int, l1Hash common.L1BlockHash, headBatch common.L2BatchHash, transactions common.L2Transactions, batchTime uint64, failForEmptyBatch bool) (*core.Batch, error) {
func (s *sequencer) produceBatch(
sequencerNo *big.Int,
l1Hash common.L1BlockHash,
headBatch common.L2BatchHash,
transactions common.L2Transactions,
batchTime uint64,
failForEmptyBatch bool,
) (*components.ComputedBatch, error) {
cb, err := s.batchProducer.ComputeBatch(&components.BatchExecutionContext{
BlockPtr: l1Hash,
ParentPtr: headBatch,
Expand Down Expand Up @@ -268,7 +296,7 @@ func (s *sequencer) produceBatch(sequencerNo *big.Int, l1Hash common.L1BlockHash
return nil, fmt.Errorf("unable to remove tx from mempool - %w", err)
}

return cb.Batch, nil
return cb, nil
otherview marked this conversation as resolved.
Show resolved Hide resolved
}

// StoreExecutedBatch - stores an executed batch in one go. This can be done for the sequencer because it is guaranteed
Expand Down Expand Up @@ -359,11 +387,11 @@ func (s *sequencer) duplicateBatches(l1Head *types.Block, nonCanonicalL1Path []c
}
sequencerNo = sequencerNo.Add(sequencerNo, big.NewInt(1))
// create the duplicate and store/broadcast it, recreate batch even if it was empty
b, err := s.produceBatch(sequencerNo, l1Head.ParentHash(), currentHead, orphanBatch.Transactions, orphanBatch.Header.Time, false)
cb, err := s.produceBatch(sequencerNo, l1Head.ParentHash(), currentHead, orphanBatch.Transactions, orphanBatch.Header.Time, false)
if err != nil {
return fmt.Errorf("could not produce batch. Cause %w", err)
}
currentHead = b.Hash()
currentHead = cb.Batch.Hash()
s.logger.Info("Duplicated batch", log.BatchHashKey, currentHead)
}

Expand Down
8 changes: 4 additions & 4 deletions integration/obscuroscan/obscuroscan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,12 @@ func TestObscuroscan(t *testing.T) {
statusCode, body, err := fasthttp.Get(nil, fmt.Sprintf("%s/count/contracts/", serverAddress))
assert.NoError(t, err)
assert.Equal(t, 200, statusCode)
assert.Equal(t, "{\"count\":1}", string(body))
assert.Equal(t, "{\"count\":2}", string(body))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Msg bus contract wasn't being minted previously, it is now.


statusCode, body, err = fasthttp.Get(nil, fmt.Sprintf("%s/count/transactions/", serverAddress))
assert.NoError(t, err)
assert.Equal(t, 200, statusCode)
assert.Equal(t, "{\"count\":5}", string(body))
assert.Equal(t, "{\"count\":6}", string(body))

statusCode, body, err = fasthttp.Get(nil, fmt.Sprintf("%s/items/batch/latest/", serverAddress))
assert.NoError(t, err)
Expand Down Expand Up @@ -120,8 +120,8 @@ func TestObscuroscan(t *testing.T) {
publicTxsObj := publicTxsRes{}
err = json.Unmarshal(body, &publicTxsObj)
assert.NoError(t, err)
assert.Equal(t, 5, len(publicTxsObj.Result.TransactionsData))
assert.Equal(t, uint64(5), publicTxsObj.Result.Total)
assert.Equal(t, 6, len(publicTxsObj.Result.TransactionsData))
assert.Equal(t, uint64(6), publicTxsObj.Result.Total)

statusCode, body, err = fasthttp.Get(nil, fmt.Sprintf("%s/items/batches/?offset=0&size=10", serverAddress))
assert.NoError(t, err)
Expand Down
42 changes: 18 additions & 24 deletions tools/walletextension/test/wallet_extension_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ import (
const (
errFailedDecrypt = "could not decrypt bytes with viewing key"
dummyParams = "dummyParams"
jsonKeyTopics = "topics"
_hostWSPort = integration.StartPortWalletExtensionUnitTest
_testOffset = 100 // offset each test by a multiplier of the offset to avoid port colision. ie: hostPort := _hostWSPort + _testOffset*2
)
Comment on lines 20 to 22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These offsets were never required as same test files run tests sequentially.
Removing them because they were making the ci flaky


type testHelper struct {
Expand All @@ -41,14 +39,13 @@ func TestWalletExtension(t *testing.T) {
"canRegisterViewingKeyAndMakeRequestsOverWebsockets": canRegisterViewingKeyAndMakeRequestsOverWebsockets,
} {
t.Run(name, func(t *testing.T) {
hostPort := _hostWSPort + i*_testOffset
dummyAPI, shutDownHost := createDummyHost(t, hostPort)
shutdownWallet := createWalExt(t, createWalExtCfg(hostPort, hostPort+1, hostPort+2))
dummyAPI, shutDownHost := createDummyHost(t, _hostWSPort)
shutdownWallet := createWalExt(t, createWalExtCfg(_hostWSPort, _hostWSPort+1, _hostWSPort+2))

h := &testHelper{
hostPort: hostPort,
walletHTTPPort: hostPort + 1,
walletWSPort: hostPort + 2,
hostPort: _hostWSPort,
walletHTTPPort: _hostWSPort + 1,
walletWSPort: _hostWSPort + 2,
hostAPI: dummyAPI,
}

Expand Down Expand Up @@ -173,14 +170,13 @@ func canRegisterViewingKeyAndMakeRequestsOverWebsockets(t *testing.T, testHelper
}

func TestCannotInvokeSensitiveMethodsWithoutViewingKey(t *testing.T) {
hostPort := _hostWSPort + _testOffset*7
walletHTTPPort := hostPort + 1
walletWSPort := hostPort + 2
walletHTTPPort := _hostWSPort + 1
walletWSPort := _hostWSPort + 2
Comment on lines +173 to +174
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These offsets were never required as same test files run tests sequentially.
Removing them because they were making the ci flaky


_, shutdownHost := createDummyHost(t, hostPort)
_, shutdownHost := createDummyHost(t, _hostWSPort)
defer shutdownHost() //nolint: errcheck

shutdownWallet := createWalExt(t, createWalExtCfg(hostPort, walletHTTPPort, walletWSPort))
shutdownWallet := createWalExt(t, createWalExtCfg(_hostWSPort, walletHTTPPort, walletWSPort))
defer shutdownWallet() //nolint: errcheck

conn, err := openWSConn(walletWSPort)
Expand All @@ -202,13 +198,12 @@ func TestCannotInvokeSensitiveMethodsWithoutViewingKey(t *testing.T) {
}

func TestKeysAreReloadedWhenWalletExtensionRestarts(t *testing.T) {
hostPort := _hostWSPort + _testOffset*8
walletHTTPPort := hostPort + 1
walletWSPort := hostPort + 2
walletHTTPPort := _hostWSPort + 1
walletWSPort := _hostWSPort + 2

dummyAPI, shutdownHost := createDummyHost(t, hostPort)
dummyAPI, shutdownHost := createDummyHost(t, _hostWSPort)
defer shutdownHost() //nolint: errcheck
walExtCfg := createWalExtCfg(hostPort, walletHTTPPort, walletWSPort)
walExtCfg := createWalExtCfg(_hostWSPort, walletHTTPPort, walletWSPort)
shutdownWallet := createWalExt(t, walExtCfg)

addr, viewingKeyBytes, signature := simulateViewingKeyRegister(t, walletHTTPPort, walletWSPort, false)
Expand Down Expand Up @@ -278,13 +273,12 @@ func TestKeysAreReloadedWhenWalletExtensionRestarts(t *testing.T) {
//}

func TestGetStorageAtForReturningUserID(t *testing.T) {
hostPort := _hostWSPort + _testOffset*8
walletHTTPPort := hostPort + 1
walletWSPort := hostPort + 2
walletHTTPPort := _hostWSPort + 1
walletWSPort := _hostWSPort + 2

createDummyHost(t, hostPort)
walExtCfg := createWalExtCfg(hostPort, walletHTTPPort, walletWSPort)
createWalExtCfg(hostPort, walletHTTPPort, walletWSPort)
createDummyHost(t, _hostWSPort)
walExtCfg := createWalExtCfg(_hostWSPort, walletHTTPPort, walletWSPort)
createWalExtCfg(_hostWSPort, walletHTTPPort, walletWSPort)
Comment on lines +276 to +281
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This still doesn't make a lot of sense, but I didn't want to refactor these methods in this PR.

createWalExt(t, walExtCfg)

// create userID
Expand Down
Loading