How to gracefully remove the limit on the number of selected transactions in mining

[zeech3]

See above code. 200 txns a block is hard code and not user friendly. cfg.txPool2.Best(200, &txSlots, poolTx)

[PeaStew]

https://github.com/ledgerwatch/erigon/blob/68cc6e461264cb63c4e8fb332ae528478db74279/eth/stagedsync/stage_mining_create_block.go#L133 for reference

[hexoscott]

There is a similar issue around this #5451, which suggest an unlimited cap. I'd put the question forwards on discord about defaulting to unlimited or using a flag to set the cap which should satisfy both cases. I'm just unsure about the implications of increasing this number, surely there is a cap to how large a new block can be so we'd need code to ensure we don't go about this which I don't think we currently have.

[zeech3]

We already adjust it to unlimited cap(Uint64.MAX) for our test.

[hexoscott]

Ok brilliant, and no issues?

[zeech3]

Ok brilliant, and no issues?

bad idea. The gas of the mined block will exceed the GasLimit.

Verify header fail:                      height=1342 diffculty=1 err="invalid gasUsed: have 204564298, gasLimit 200000000"

[hexoscott]

That was my worry, there is no "stop here" switch in the code, other than checking for an individual transaction having a high gas fee there is nothing to stop it just adding 1000s of transactions to a block if they were available.

Not to say we couldn't add this, but I'd need guidance from core team on how best to achieve this.

[hexoscott]

The way I'd see it would be:

• no flag set, unlimited transactions allowed but capped at gas limit
• flag set, transactions capped at gas limit or by the flag set by the user

^ unlimited in this case would be uint16 maximum number

[zeech3]

That was my worry, there is no "stop here" switch in the code, other than checking for an individual transaction having a high gas fee there is nothing to stop it just adding 1000s of transactions to a block if they were available.

Not to say we couldn't add this, but I'd need guidance from core team on how best to achieve this.

		// Start executing the transaction
		logs, err := miningCommitTx(txn, coinbase, vmConfig, chainConfig, ibs, current)

		if errors.Is(err, core.ErrGasLimitReached) {
			// Pop the env out-of-gas transaction without shifting in the next from the account
			log.Debug(fmt.Sprintf("[%s] Gas limit exceeded for env block", logPrefix), "hash", txn.Hash(), "sender", from)
			txs.Pop()
		} else if errors.Is(err, core.ErrNonceTooLow) {
			// New head notification data race between the transaction pool and miner, shift
			log.Debug(fmt.Sprintf("[%s] Skipping transaction with low nonce", logPrefix), "hash", txn.Hash(), "sender", from, "nonce", txn.GetNonce())
			txs.Shift()
		} else if errors.Is(err, core.ErrNonceTooHigh) {
			// Reorg notification data race between the transaction pool and miner, skip account =
			log.Debug(fmt.Sprintf("[%s] Skipping transaction with high nonce", logPrefix), "hash", txn.Hash(), "sender", from, "nonce", txn.GetNonce())
			txs.Pop()
		} else if err == nil {
			// Everything ok, collect the logs and shift in the next transaction from the same account
			log.Debug(fmt.Sprintf("[%s] addTransactionsToMiningBlock Successful", logPrefix), "sender", from, "nonce", txn.GetNonce())
			coalescedLogs = append(coalescedLogs, logs...)
			tcount++
			txs.Shift()
		} else {
			// Strange error, discard the transaction and get the next in line (note, the
			// nonce-too-high clause will prevent us from executing in vain).
			log.Debug(fmt.Sprintf("[%s] Skipping transaction", logPrefix), "hash", txn.Hash(), "sender", from, "err", err)
			txs.Shift()
		}

It seems that the txn will not pop out when Gas limit reached during the mining exec. Geth will pop out. So i think it's a bug

[zeech3]

The way I'd see it would be:

• no flag set, unlimited transactions allowed but capped at gas limit
• flag set, transactions capped at gas limit or by the flag set by the user

^ unlimited in this case would be uint16 maximum number

it can pick more txns from txpool, but in mining exec, Txns should be filtered when GasLimit reached. Obviously erigon didn't.

[INFO] [09-22|08:40:52.548] [5/5 MiningFinish] block ready for seal  block_num=1342 transactions=3462 gas_used=283505560 gas_limit=200000000 difficulty=1

[zeech3]

The way I'd see it would be:

• no flag set, unlimited transactions allowed but capped at gas limit
• flag set, transactions capped at gas limit or by the flag set by the user

^ unlimited in this case would be uint16 maximum number

it can pick more txns from txpool, but in mining exec, Txns should be filtered when GasLimit reached. Obviously erigon didn't.

[INFO] [09-22|08:40:52.548] [5/5 MiningFinish] block ready for seal  block_num=1342 transactions=3462 gas_used=283505560 gas_limit=200000000 difficulty=1

Maybe @AskAlexSharov can help us?

[AskAlexSharov]

In my head:

• mined block does apply to current state
• after apply it send event to txpool 
• at this time txpool pop txs (which are in this mined block) - see removeMined() func

If you want to pop transactions earlier - right after block construction - likely you need some method in TxPool for this

[zeech3]

In my head:

• mined block does apply to current state
• after apply it send event to txpool
• at this time txpool pop txs (which are in this mined block) - see removeMined() func

If you want to pop transactions earlier - right after block construction - likely you need some method in TxPool for this

I find the reason why it occurs.

	// BSC always gave gas bailout due to system transactions that set 2^256/2 gas limit and
	// for Parlia consensus this flag should be always be set
	if st.isParlia {
		gasBailout = true
	}

....
	if err := st.gp.SubGas(st.msg.Gas()); err != nil {
		if !gasBailout {
			return err
		}
	}

It will not return err in bsc.

then in mining exec stage, it will not pop the txns.

		// Start executing the transaction
		logs, err := miningCommitTx(txn, coinbase, vmConfig, chainConfig, ibs, current)

		if errors.Is(err, core.ErrGasLimitReached) {
			// Pop the env out-of-gas transaction without shifting in the next from the account
			log.Debug(fmt.Sprintf("[%s] Gas limit exceeded for env block", logPrefix), "hash", txn.Hash(), "sender", from)
			txs.Pop()
		}

[hexoscott]

So I've got some code changes in flight from Erigon to take in a new flag to control maximum amount of TXs in a block, nothing exciting other than these lines in stage_mining_create_block.go which use the config value if set or just use the default 1000 TX limit hard coded:

	if err = cfg.txPool2DB.View(context.Background(), func(poolTx kv.Tx) error {
		txSlots := types2.TxsRlp{}
		txLimit := maxTransactions
		if cfg.miner.MiningConfig.TxLimit != 0 {
			txLimit = uint16(cfg.miner.MiningConfig.TxLimit)
		}
		if err := cfg.txPool2.Best(txLimit, &txSlots, poolTx); err != nil {
			return err
		}

In Erigon lib the call to Best in the txpool looks like it could be a good place to handle gas limits, we're already skipping transactions with really large gas here anyway, edited code:

func (p *TxPool) Best(n uint16, txs *types.TxsRlp, tx kv.Tx) error {
	p.lock.RLock()
	defer p.lock.RUnlock()

	txs.Resize(uint(cmp.Min(int(n), len(p.pending.best.ms))))

	var gasCum uint64 = 0
	best := p.pending.best
	j := 0
	for i := 0; j < int(n) && i < len(best.ms); i++ {
		mt := best.ms[i]
		if mt.Tx.Gas >= p.blockGasLimit.Load() {
			// Skip transactions with very large gas limit
			continue
		}
		if gasCum+mt.Tx.Gas >= p.blockGasLimit.Load() {
			// transactions would exceed the gas limit so continue, we might find a smaller
			// transaction that would fit so need to continue rather than break
			continue
		}
		gasCum += mt.Tx.Gas
		rlpTx, sender, isLocal, err := p.getRlpLocked(tx, mt.Tx.IDHash[:])
		if err != nil {
			return err
		}
		if len(rlpTx) == 0 {
			p.pending.Remove(mt)
			continue
		}
		txs.Txs[j] = rlpTx
		copy(txs.Senders.At(j), sender)
		txs.IsLocal[j] = isLocal
		j++
	}
	txs.Resize(uint(j))
	return nil
}

basically take a cumulative count of gas as we grab transactions and when getting close to exceeding the limit we continue on trying to find smaller transactions that would fit into the block. So in theory get as close to the gas limit as we can but don't exceed the maximum number of transactions.....

Any thoughts @AskAlexSharov?

[zeech3]

But we can't get the real gas used in Txpool, except in the execution stage. @hexoscott

[hexoscott]

OK, that shows my lack of understanding there... I saw the line checking for a TX with gas over the gas limit and continuing and made an incorrect assumption, makes sense now though considering multiple transactions for the same account and other logic in the execution phase. Will continue having a look into the chicken/egg problem.

[hexoscott]

Raised PR #6160 to tackle this

[hexoscott]

Hi @fynn-z, a long time in the making, but in devel there is a fix for this hopefully now. Transactions are pulled 50 at a time in the execution phase and run until the block is out of gas or the CL wants the block back, whichever comes first. We're looking at testing this our side, but if you wanted to check it our yourself it's there for you.

[zeech3]

Hi @fynn-z, a long time in the making, but in devel there is a fix for this hopefully now. Transactions are pulled 50 at a time in the execution phase and run until the block is out of gas or the CL wants the block back, whichever comes first. We're looking at testing this our side, but if you wanted to check it our yourself it's there for you.

Good job @hexoscott. I'll check later based on your PR.

[hexoscott]

There were some follow up PRs to fix some issues in the original work so worth checking those out too. All in devel now.