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

Jump to bottom

remove UseInboxContract and optimize getTxSucceed #59

Merged
merged 3 commits into from
Sep 24, 2024
Merged

remove UseInboxContract and optimize getTxSucceed #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e76c3c4
remove UseInboxContract
blockchaindevsh Sep 15, 2024
a4af765
optimize getTxSucceed to return a smaller map
blockchaindevsh Sep 17, 2024
8613b74
clear cache in recordFailedTx
blockchaindevsh Sep 22, 2024
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
13 changes: 5 additions & 8 deletions op-batcher/batcher/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,21 +516,18 @@ func (l *BatchSubmitter) sendTransaction(ctx context.Context, txdata txData, que
if *candidate.To != l.RollupConfig.BatchInboxAddress {
return fmt.Errorf("candidate.To is not inbox")
}
if l.RollupConfig.UseInboxContract && l.inboxIsEOA == nil {
if l.inboxIsEOA == nil {
var code []byte
code, err = l.L1Client.CodeAt(ctx, *candidate.To, nil)
if err != nil {
return fmt.Errorf("CodeAt failed:%w", err)
}
isEOA := len(code) == 0
if !isEOA {
return fmt.Errorf("UseInboxContract is enabled but BatchInboxAddress is an EOA")
}
l.inboxIsEOA = &isEOA
}

// Don't set GasLimit when UseInboxContract is enabled so that later on `EstimateGas` will be called
if !l.RollupConfig.UseInboxContract {
// Don't set GasLimit when inbox is contract so that later on `EstimateGas` will be called
if !*l.inboxIsEOA {
Copy link

Choose a reason for hiding this comment

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

If the inbox is changed from an EOA to a smart contract, would we need to restart the batch, or is the system able to handle this transition smoothly?

Copy link
Author

@blockchaindevsh blockchaindevsh Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a great question!

(Note we don't consider dynamic inbox for this PR, instead we assume it's a fixed address that never changes, but the content can change for the reason here.)

I think in theory we can not reliably cache the result, but in reality the chance it actually happens is small, so maybe it's good enough if we just clear the cache(set inboxIsEOA to nil) when a transaction failure is spotted afterwards?

Copy link
Author

Choose a reason for hiding this comment

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

@qzhodl Please take another look at the latest commit, which applies the above change.

intrinsicGas, err := core.IntrinsicGas(candidate.TxData, nil, false, true, true, false)
if err != nil {
// we log instead of return an error here because txmgr can do its own gas estimation
Expand Down Expand Up @@ -573,8 +570,8 @@ func (l *BatchSubmitter) handleReceipt(r txmgr.TxReceipt[txID]) {
if r.Err != nil {
l.recordFailedTx(r.ID, r.Err)
} else {
// check tx status if UseInboxContract
if l.RollupConfig.UseInboxContract && r.Receipt.Status == types.ReceiptStatusFailed {
// check tx status
if r.Receipt.Status == types.ReceiptStatusFailed {
l.recordFailedTx(r.ID, ErrInboxTransactionFailed)
return
}
Expand Down
3 changes: 0 additions & 3 deletions op-chain-ops/genesis/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,9 +298,6 @@ type DeployConfig struct {
// IsSoulBackedByNative is a flag that indicates if the SoulGasToken is backed by native.
// Only effective when UseSoulGasToken is true.
IsSoulBackedByNative bool `json:"isSoulBackedByNative"`

// UseInboxContract is a flag that indicates if the inbox is a contract
UseInboxContract bool `json:"use_inbox_contract"`
}

// Copy will deeply copy the DeployConfig. This does a JSON roundtrip to copy
Expand Down
1 change: 0 additions & 1 deletion op-node/cmd/genesis/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,6 @@ var Subcommands = cli.Commands{
if err != nil {
return err
}
rollupConfig.UseInboxContract = config.UseInboxContract
if err := rollupConfig.Check(); err != nil {
return fmt.Errorf("generated rollup config does not pass validation: %w", err)
}
Expand Down
15 changes: 5 additions & 10 deletions op-node/rollup/derive/blob_data_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,8 @@ func (ds *BlobDataSource) Next(ctx context.Context) (eth.Data, error) {
return data, nil
}

// getTxSucceedIfUseInboxContract returns nil if !useInboxContract;
// otherwise it returns a non-nil map which contains all successful tx hashes
func getTxSucceedIfUseInboxContract(ctx context.Context, useInboxContract bool, fetcher L1Fetcher, hash common.Hash) (txSucceeded map[common.Hash]bool, err error) {
if !useInboxContract {
return
}
// getTxSucceed returns a non-nil map which contains all successful tx hashes
func getTxSucceed(ctx context.Context, fetcher L1Fetcher, hash common.Hash) (txSucceeded map[common.Hash]bool, err error) {
_, receipts, err := fetcher.FetchReceipts(ctx, hash)
if err != nil {
return nil, NewTemporaryError(fmt.Errorf("failed to fetch L1 block info and receipts: %w", err))
Expand All @@ -105,7 +101,7 @@ func (ds *BlobDataSource) open(ctx context.Context) ([]blobOrCalldata, error) {
}
return nil, NewTemporaryError(fmt.Errorf("failed to open blob data source: %w", err))
}
txSucceeded, err := getTxSucceedIfUseInboxContract(ctx, ds.dsCfg.useInboxContract, ds.fetcher, ds.ref.Hash)
txSucceeded, err := getTxSucceed(ctx, ds.fetcher, ds.ref.Hash)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -144,9 +140,8 @@ func dataAndHashesFromTxs(txs types.Transactions, config *DataSourceConfig, batc
var hashes []eth.IndexedBlobHash
blobIndex := 0 // index of each blob in the block's blob sidecar
for _, tx := range txs {
// skip any non-batcher transactions or failed transactions if useInboxContract
// note: txSucceeded will only be nil when !useInboxContract
if !(isValidBatchTx(tx, config.l1Signer, config.batchInboxAddress, batcherAddr) && (txSucceeded == nil || txSucceeded[tx.Hash()])) {
// skip any non-batcher transactions or failed transactions
if !(isValidBatchTx(tx, config.l1Signer, config.batchInboxAddress, batcherAddr) && txSucceeded[tx.Hash()]) {
blobIndex += len(tx.BlobHashes())
continue
}
Expand Down
7 changes: 3 additions & 4 deletions op-node/rollup/derive/calldata_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func NewCalldataSource(ctx context.Context, log log.Logger, dsCfg DataSourceConf
batcherAddr: batcherAddr,
}
}
txSucceed, err := getTxSucceedIfUseInboxContract(ctx, dsCfg.useInboxContract, fetcher, ref.Hash)
txSucceed, err := getTxSucceed(ctx, fetcher, ref.Hash)
if err != nil {
return &CalldataSource{
open: false,
Expand All @@ -67,7 +67,7 @@ func NewCalldataSource(ctx context.Context, log log.Logger, dsCfg DataSourceConf
func (ds *CalldataSource) Next(ctx context.Context) (eth.Data, error) {
if !ds.open {
if _, txs, err := ds.fetcher.InfoAndTxsByHash(ctx, ds.ref.Hash); err == nil {
txSucceeded, err := getTxSucceedIfUseInboxContract(ctx, ds.dsCfg.useInboxContract, ds.fetcher, ds.ref.Hash)
txSucceeded, err := getTxSucceed(ctx, ds.fetcher, ds.ref.Hash)
if err != nil {
return nil, err
}
Expand All @@ -94,8 +94,7 @@ func (ds *CalldataSource) Next(ctx context.Context) (eth.Data, error) {
func DataFromEVMTransactions(dsCfg DataSourceConfig, batcherAddr common.Address, txs types.Transactions, log log.Logger, txSucceeded map[common.Hash]bool) []eth.Data {
out := []eth.Data{}
for _, tx := range txs {
// note: txSucceeded will only be nil when !useInboxContract
if isValidBatchTx(tx, dsCfg.l1Signer, dsCfg.batchInboxAddress, batcherAddr) && (txSucceeded == nil || txSucceeded[tx.Hash()]) {
if isValidBatchTx(tx, dsCfg.l1Signer, dsCfg.batchInboxAddress, batcherAddr) && txSucceeded[tx.Hash()] {
out = append(out, tx.Data())
}
}
Expand Down
2 changes: 0 additions & 2 deletions op-node/rollup/derive/data_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ func NewDataSourceFactory(log log.Logger, cfg *rollup.Config, fetcher L1Fetcher,
l1Signer: cfg.L1Signer(),
batchInboxAddress: cfg.BatchInboxAddress,
plasmaEnabled: cfg.PlasmaEnabled(),
useInboxContract: cfg.UseInboxContract,
}
return &DataSourceFactory{
log: log,
Expand Down Expand Up @@ -93,7 +92,6 @@ type DataSourceConfig struct {
l1Signer types.Signer
batchInboxAddress common.Address
plasmaEnabled bool
useInboxContract bool
}

// isValidBatchTx returns true if:
Expand Down
3 changes: 0 additions & 3 deletions op-node/rollup/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,6 @@ type Config struct {
LegacyUsePlasma bool `json:"use_plasma,omitempty"`

L2BlobConfig *L2BlobConfig `json:"l2_blob_config,omitempty"`

// UseInboxContract is a flag that indicates if the inbox is a contract
UseInboxContract bool `json:"use_inbox_contract"`
}

type L2BlobConfig struct {
Expand Down