core/txpool/blobpool: return all reinject-addresses

[holiman]

This PR reverts part of #30437.

I failed to take note of this at the time, but now I am unsure if the "avoid reinject adding empty value" part was correct.

Previously, reinject would contains an empty list for the address addr, and the blobpool Reset function would then call p.recheck on that addr

	if reinject, inclusions := p.reorg(oldHead, newHead); reinject != nil {
		var adds []*types.Transaction
		for addr, txs := range reinject {
			// Blindly push all the lost transactions back into the pool
			for _, tx := range txs {
				if err := p.reinject(addr, tx.Hash()); err == nil {
					adds = append(adds, tx.WithoutBlobTxSidecar())
				}
			}
			// Recheck the account's pooled transactions to drop included and
			// invalidated ones
			p.recheck(addr, inclusions)
		}
		if len(adds) > 0 {
			p.insertFeed.Send(core.NewTxsEvent{Txs: adds})
		}
	}

I think the new code is wrong: it may very well happen that we reorg out a transaction, but it is not eligible for re-injection, because the filtering fails:

			if p.Filter(tx) {
				lost = append(lost, tx)
			}

// Filter returns whether the given transaction can be consumed by the blob pool.
func (p *BlobPool) Filter(tx *types.Transaction) bool {
	return tx.Type() == types.BlobTxType
}

That is: we reorg-out a regular transaction, it is not eligible for re-injection (in the blob pool), and thus the blob pool becomes off: it's should have invoked recheck on the account.

---

This PR restores the earlier behaviour. I'm still not 100% sure which is correct, opening for discussion.

[karalabe]

This is interesting. On one hand, I would have expected myself to check empty-ness and not add a useless field to the map (i.e. which would make me believe it was intentional); on the other hand, I would expect myself to document such a subtle API interface somewhere to avoid these refactors.

I.e. Completely unsure about it :D

[karalabe]

		for addr, txs := range reinject {
			// Blindly push all the lost transactions back into the pool
			for _, tx := range txs {
				if err := p.reinject(addr, tx.Hash()); err == nil {
					adds = append(adds, tx.WithoutBlobTxSidecar())
				}
			}
			// Recheck the account's pooled transactions to drop included and
			// invalidated ones
			p.recheck(addr, inclusions)
		}

Ugh, yeah, so this uses the reinject to iterate the addresses to recheck.

The failure is not if Filter somehow failed in reorg, the problem if no txs were lost at all. Then lost will be empty and not added at all, so recheck will not run for the included address, so it won't process the txs being included either. Pool goes out of sync with the chain.

I'll try to make a test for this.

[mask-pp]

inclusions from reorg is always not null, and if txs is null it won't touch reinject func and in recheck function inclusions != nil && txs == nil check is true and then return. So I think this PR is right.

[karalabe]

Pushed a test that reproes the bug this PR fixes. Also, FYI, the blobpool currently is 100% totally broken without this fix. So we need a hotfix release.

[karalabe]

FWIW, the test is very ugly because previously we didn't have to have a live chain, but this reorg functionality does depend on a bunch of chain ops. We can emrge as is for now and then see if we can clean up the tests soemhow with a live chain. Gonna be annoying though.

[karalabe]

LGTM