Estimation reverted user operations are not excluded from the bundle transaction?

[wanliqun]

I think we may need to exclude the removed user operation from consideration for next estimation loop.

file /pkg/modules/relay/relayer.go :

// SendUserOperation returns a BatchHandler that is used by the Bundler to send batches in a regular EOA
// transaction.
func (r *Relayer) SendUserOperation() modules.BatchHandlerFunc {
	return func(ctx *modules.BatchHandlerCtx) error {
		opts := transaction.Opts{
			EOA:         r.eoa,
			Eth:         r.eth,
			ChainID:     ctx.ChainID,
			EntryPoint:  ctx.EntryPoint,
			Batch:       ctx.Batch,
			Beneficiary: r.beneficiary,
			BaseFee:     ctx.BaseFee,
			Tip:         ctx.Tip,
			GasPrice:    ctx.GasPrice,
			GasLimit:    0,
			WaitTimeout: r.waitTimeout,
		}
		// Estimate gas for handleOps() and drop all userOps that cause unexpected reverts.
		estRev := []string{}
		for len(ctx.Batch) > 0 {
			est, revert, err := transaction.EstimateHandleOpsGas(&opts)

			if err != nil {
				return err
			} else if revert != nil {
				ctx.MarkOpIndexForRemoval(revert.OpIndex)
				estRev = append(estRev, revert.Reason)

				// Exclude the removed user operation from consideration for next estimation loop.
				opts.Batch =  ctx.Batch
			} else {
				opts.GasLimit = est
				break
			}
		}
		ctx.Data["estimate_revert_reasons"] = estRev

		...
	}
}```

[hazim-j]

Nice catch! Any context on how you came across this? Are you able to reproduce it in a test?

[wanliqun]

Well, I am facing a gas estimation problem and try to figure out how this estimation works. So take a few moment to check the codes, but I didn't try to reproduce it in a test yet. Maybe you can try it.

[hazim-j]

On second pass, this may not be an issue since opts.Batch is already initialised to point to ctx.Batch. ctx.MarkOpIndexForRemoval will mutate ctx.Batch which mean opts.Batch should reflect that.

[wanliqun]

Yes, opts.Batch is intialized to be ctx.Batch at fist, andctx.Batch then is assigned to a new slice after ctx.MarkOpIndexForRemoval, so ctx.Batch is updated with new user operations, but I'm pretty sure that opts.Batch would not be updated accordingly.