-
Notifications
You must be signed in to change notification settings - Fork 878
perf(app): parallelize per-tx gas classification in checkTotalBlockGas #3399
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
base: main
Are you sure you want to change the base?
Changes from all commits
2809bf1
753e1bb
f8f2841
b3d58a1
8c7fd31
2cbdf0a
e48eb0e
6cb54a7
016d8f7
edf945e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import ( | |
| "runtime/debug" | ||
| "strings" | ||
| "sync" | ||
| "sync/atomic" | ||
| "time" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
|
|
@@ -2508,104 +2509,173 @@ func RegisterSwaggerAPI(rtr *mux.Router) { | |
| rtr.PathPrefix("/swagger/").Handler(http.StripPrefix("/swagger/", staticServer)) | ||
| } | ||
|
|
||
| // txGasResult holds the per-tx gas classification outcome for checkTotalBlockGas. | ||
| type txGasResult struct { | ||
| gasWanted uint64 | ||
| gasContrib uint64 // effective gas: estimate when valid (>= MinGasEVMTx and <= gasWanted), else gasWanted | ||
| skip bool // tx contributes no gas (nil, gasless, associate, decode error) | ||
| malicious bool // panic inside IsTxGasless — reject the entire proposal | ||
| } | ||
|
|
||
| // txGasClassifyJob is one unit of work for the checkTotalBlockGas worker pool. | ||
| type txGasClassifyJob struct { | ||
| idx int | ||
| tx sdk.Tx | ||
| } | ||
|
|
||
| // checkTotalBlockGas checks that the block gas limit is not exceeded by our best estimate of | ||
| // the total gas by the txs in the block. The gas of a tx is either the gas estimate if it's an EVM tx, | ||
| // or the gas wanted if it's a Cosmos tx. typedTxs must align with proposal order (nil = decode failure). | ||
| func (app *App) checkTotalBlockGas(ctx sdk.Context, typedTxs []sdk.Tx) (result bool) { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| logger.Error("panic recovered in checkTotalBlockGas", "panic", r) | ||
| result = false // Reject proposal if panic occurs | ||
| result = false | ||
| } | ||
| }() | ||
|
|
||
| totalGas, totalGasWanted := uint64(0), uint64(0) | ||
| nonzeroTxsCnt := 0 | ||
| for _, decodedTx := range typedTxs { | ||
| if decodedTx == nil { | ||
| // such tx will not be processed and thus won't consume gas. Skipping | ||
| results := make([]txGasResult, len(typedTxs)) | ||
|
|
||
| // ctx (CacheMultiStore) is not goroutine-safe; IsTxGasless is the only caller that | ||
| // reads from it. EVM txs skip that path entirely, so ctxMu is uncontested on | ||
| // typical EVM-heavy blocks. | ||
| var ctxMu sync.Mutex | ||
|
|
||
| const maxConcurrent = 32 | ||
| jobs := make([]txGasClassifyJob, 0, len(typedTxs)) | ||
| for i, tx := range typedTxs { | ||
| if tx == nil { | ||
| results[i].skip = true | ||
| continue | ||
| } | ||
| isEVM, evmErr := evmante.IsEVMMessage(decodedTx) | ||
| // MsgEVMTransaction cannot be gasless under IsTxGasless (only oracle vote / MsgAssociate). | ||
| // Skip keeper-backed IsTxGasless for valid single-message EVM txs; still run it when the tx | ||
| // is not EVM or EVM classification failed (e.g. multi-msg with an EVM message). | ||
| skipGaslessCheck := evmErr == nil && isEVM | ||
| if !skipGaslessCheck && app.couldBeGaslessTransaction(decodedTx) { | ||
| isGasless, err := antedecorators.IsTxGasless(decodedTx, ctx, app.OracleKeeper, &app.EvmKeeper) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "panic in IsTxGasless") { | ||
| // This is a unexpected panic, reject the entire proposal | ||
| logger.Error("malicious transaction detected in gasless check", "err", err) | ||
| return false | ||
| } | ||
| // Other business logic errors (like duplicate votes) - continue processing but tx is not gasless | ||
| logger.Info("transaction failed gasless check but not malicious", "err", err) | ||
| continue | ||
| } | ||
| if isGasless { | ||
| continue | ||
| } | ||
| jobs = append(jobs, txGasClassifyJob{idx: i, tx: tx}) | ||
| } | ||
| if len(jobs) > 0 { | ||
| var maliciousSeen atomic.Bool | ||
| nWorkers := maxConcurrent | ||
| if nWorkers > len(jobs) { | ||
| nWorkers = len(jobs) | ||
| } | ||
| // Check whether it's associate tx | ||
| gasWanted := uint64(0) | ||
| if evmErr != nil { | ||
| continue | ||
| jobCh := make(chan txGasClassifyJob, len(jobs)) | ||
| for _, job := range jobs { | ||
| jobCh <- job | ||
| } | ||
| if isEVM { | ||
| msg := evmtypes.MustGetEVMTransactionMessage(decodedTx) | ||
| if msg.IsAssociateTx() { | ||
| continue | ||
| } | ||
| etx, _ := msg.AsTransaction() | ||
| gasWanted = etx.Gas() | ||
| } else { | ||
| feeTx, ok := decodedTx.(sdk.FeeTx) | ||
| if !ok { | ||
| // such tx will not be processed and thus won't consume gas. Skipping | ||
| continue | ||
| } | ||
|
|
||
| // Check for overflow before adding | ||
| gasWanted = feeTx.GetGas() | ||
| close(jobCh) | ||
| var wg sync.WaitGroup | ||
| wg.Add(nWorkers) | ||
| for range nWorkers { | ||
| go func() { | ||
| defer wg.Done() | ||
| for job := range jobCh { | ||
| if maliciousSeen.Load() { | ||
| break | ||
| } | ||
| r := app.classifyTxForGas(ctx, job.tx, &ctxMu) | ||
| results[job.idx] = r | ||
| if r.malicious { | ||
| maliciousSeen.Store(true) | ||
| } | ||
| } | ||
| }() | ||
| } | ||
|
|
||
| if int64(gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(gasWanted) { // nolint:gosec | ||
| wg.Wait() | ||
| if maliciousSeen.Load() { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| if gasWanted > 0 { | ||
| nonzeroTxsCnt++ | ||
| // Serial phase: malicious check + overflow-safe accumulation + limit enforcement. | ||
| totalGas, totalGasWanted := uint64(0), uint64(0) | ||
| for _, r := range results { | ||
| if r.malicious { | ||
| return false | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why proceed with the remaining concurrent processing if a malicious result is found? Have a look at err group. that primitive SDK type allows you to then return an error and as a result cancel all remaining concurrent work, which would make a more effective optimisation given that the point of this PR is exactly that. |
||
| } | ||
|
|
||
| totalGasWanted += gasWanted | ||
|
|
||
| // If the gas estimate is set and at least 21k (the minimum gas needed for an EVM tx) | ||
| // and less than or equal to the tx gas limit, use the gas estimate. Otherwise, use gasWanted. | ||
| useEstimate := false | ||
| if decodedTx.GetGasEstimate() >= MinGasEVMTx { | ||
| if decodedTx.GetGasEstimate() <= gasWanted { | ||
| useEstimate = true | ||
| } | ||
| if r.skip { | ||
| continue | ||
| } | ||
| if useEstimate { | ||
| totalGas += decodedTx.GetGasEstimate() | ||
| } else { | ||
| totalGas += gasWanted | ||
| if int64(r.gasWanted) < 0 || int64(totalGas) > math.MaxInt64-int64(r.gasWanted) { //nolint:gosec | ||
| return false | ||
| } | ||
|
|
||
| totalGasWanted += r.gasWanted | ||
| totalGas += r.gasContrib | ||
| if totalGasWanted > uint64(ctx.ConsensusParams().Block.MaxGasWanted) { //nolint:gosec | ||
| return false | ||
| } | ||
|
|
||
| if totalGas > uint64(ctx.ConsensusParams().Block.MaxGas) { //nolint:gosec | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| // classifyTxForGas computes the gas contribution of a single tx for checkTotalBlockGas. | ||
| // Safe to call concurrently: each invocation reads a fixed ctx snapshot with no writes, so | ||
| // results are order-independent. ctxMu serializes IsTxGasless calls because sdk.Context | ||
| // (CacheMultiStore) is not goroutine-safe even for reads. | ||
| func (app *App) classifyTxForGas(ctx sdk.Context, tx sdk.Tx, ctxMu *sync.Mutex) (res txGasResult) { | ||
| // Worker goroutines are not covered by the recover in checkTotalBlockGas (different goroutine). | ||
| // Catch panics here so a malformed tx (e.g. MustGetEVMTransactionMessage on bad shape) rejects | ||
| // the proposal instead of crashing the process. | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| logger.Error("panic recovered in classifyTxForGas", "panic", r) | ||
| res = txGasResult{malicious: true} | ||
| } | ||
| }() | ||
| isEVM, evmErr := evmante.IsEVMMessage(tx) | ||
| // MsgEVMTransaction cannot be gasless under IsTxGasless (only oracle vote / MsgAssociate). | ||
| // Skip keeper-backed IsTxGasless for valid single-message EVM txs; still run it when the tx | ||
| // is not EVM or EVM classification failed (e.g. multi-msg with an EVM message). | ||
| skipGaslessCheck := evmErr == nil && isEVM | ||
|
|
||
| if !skipGaslessCheck && app.couldBeGaslessTransaction(tx) { | ||
| ctxMu.Lock() | ||
| isGasless, err := func() (bool, error) { | ||
| defer ctxMu.Unlock() | ||
| return antedecorators.IsTxGasless(tx, ctx, app.OracleKeeper, &app.EvmKeeper) | ||
| }() | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "panic in IsTxGasless") { | ||
| logger.Error("malicious transaction detected in gasless check", "err", err) | ||
| return txGasResult{malicious: true} | ||
| } | ||
| logger.Info("transaction failed gasless check but not malicious", "err", err) | ||
| return txGasResult{skip: true} | ||
| } | ||
| if isGasless { | ||
| return txGasResult{skip: true} | ||
| } | ||
| } | ||
|
|
||
| if evmErr != nil { | ||
| return txGasResult{skip: true} | ||
| } | ||
|
|
||
| var gasWanted uint64 | ||
| if isEVM { | ||
| msg := evmtypes.MustGetEVMTransactionMessage(tx) | ||
| if msg.IsAssociateTx() { | ||
| return txGasResult{skip: true} | ||
| } | ||
| etx, _ := msg.AsTransaction() | ||
| gasWanted = etx.Gas() | ||
| } else { | ||
| feeTx, ok := tx.(sdk.FeeTx) | ||
| if !ok { | ||
| return txGasResult{skip: true} | ||
| } | ||
| gasWanted = feeTx.GetGas() | ||
| } | ||
|
|
||
| // Use the gas estimate if it's set and within the tx gas limit; otherwise fall back to gasWanted. | ||
| gasContrib := gasWanted | ||
| if est := tx.GetGasEstimate(); est >= MinGasEVMTx && est <= gasWanted { | ||
| gasContrib = est | ||
| } | ||
|
|
||
| return txGasResult{gasWanted: gasWanted, gasContrib: gasContrib} | ||
| } | ||
|
|
||
| func isExpectedGaslessMetricsError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document the rationale for 32; if there is none please either paremeterise it in config or make it a factor of CPU cores with capped min and max.