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

remove check performed in the mempool #1708

Merged
merged 3 commits into from
Dec 19, 2023
Merged

remove check performed in the mempool #1708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
046efc2
remove check performed in the mempool
tudor-malene Dec 19, 2023
c976da8
remove check performed in the mempool
tudor-malene Dec 19, 2023
3b43599
fix flakyness
tudor-malene Dec 19, 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
16 changes: 0 additions & 16 deletions go/enclave/enclave.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,10 +499,6 @@ func (e *enclaveImpl) SubmitTx(tx common.EncryptedTx) (*responses.RawTx, common.
if e.crossChainProcessors.Local.IsSyntheticTransaction(*decryptedTx) {
return responses.AsPlaintextError(responses.ToInternalError(fmt.Errorf("synthetic transaction coming from external rpc"))), nil
}
if err = e.checkGas(decryptedTx); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this check is performed again in the geth mempool, and returns better errors

e.logger.Info("gas check failed", log.ErrKey, err.Error())
return responses.AsEncryptedError(err, vkHandler), nil
}

if err = e.service.SubmitTransaction(decryptedTx); err != nil {
e.logger.Debug("Could not submit transaction", log.TxKey, decryptedTx.Hash(), log.ErrKey, err)
Comment on lines 503 to 504
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 the SubmitTransaction call within the SubmitTx method logs the error but also returns an encrypted error response. It is crucial to ensure that the error returned does not leak sensitive information and that it is properly encrypted using the viewing key handler.

Confirm that the error encryption process is secure and that the error messages are generic enough to not reveal details that could be exploited by an attacker.

Comment on lines 499 to 504
Copy link

Choose a reason for hiding this comment

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

The removal of the gas check logic in the SubmitTx method aligns with the PR's objective to standardize error messages with Ethereum tooling. Ensure that the removal does not introduce any security or functional issues, especially considering that gas checks are a critical part of transaction validation in Ethereum-like networks.

Expand Down Expand Up @@ -1436,18 +1432,6 @@ func (e *revertError) ErrorData() interface{} {
return e.reason
}

func (e *enclaveImpl) checkGas(tx *types.Transaction) error {
txGasPrice := tx.GasPrice()
if txGasPrice == nil {
return fmt.Errorf("rejected transaction %s. No gas price was set", tx.Hash())
}
minGasPrice := e.config.MinGasPrice
if txGasPrice.Cmp(minGasPrice) == -1 {
return fmt.Errorf("rejected transaction %s. Gas price was only %d, wanted at least %d", tx.Hash(), txGasPrice, minGasPrice)
}
return nil
}

// Returns the params extracted from an eth_getLogs request.
func extractGetLogsParams(paramList []interface{}) (*filters.FilterCriteria, *gethcommon.Address, error) {
// We extract the first param, the filter for the logs.
Expand Down
2 changes: 1 addition & 1 deletion go/enclave/nodetype/sequencer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (s *sequencer) createGenesisBatch(block *common.L1Block) error {

// 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)),
big.NewInt(0).Add(batch.Header.SequencerOrderNo, big.NewInt(1)),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was the major source of all flakyness.
This call was mutating the sequencer number of the first batch.

block.Hash(),
batch.Hash(),
common.L2Transactions{msgBusTx},
Expand Down
Loading