core/txpool/legacypool: locals-tracker (second attempt)

[holiman]

This is a re-run of #27535

I still think the idea is solid. Basically, our nodes transaction pool should have no concept of locals. Therefore, it is also a very good simulation/approximation of how our peers' pools behave.

This PR implements a locals-tracker, which basically is a little thing which, from time to time, asks the pool "did you forget this transaction?". If it did, the tracker resubmits it.

If the txpool had forgotten it, chances are that the peers had also forgotten it. It will be propagated again.

Doing this change means that we can simplify the pool internals, quite a lot (not included in this PR).
This PR as is, does not integrate with worker: hence, block building will pay no special attention to local transactions. If we want to use the approach in this PR, we can discuss if / how to do such an integration (now that mining-pools no longer function like they did earlier, during ethash, I'm not sure that local txs in block-building is as important any longer) .

cc @lightclient since we discussed this a bit

[lightclient]

Looks nice to me. I found it a little surprising though that TxTracker is in legacypool and only operates on a single subpool? Curious your reasoning here?

I imagined that the tracker would hold all different types of txs and check/resubmit them to the correct pool.

[lightclient]

Suggested change

	nStales = 0
	nOk = 0
	numStales = 0
	numOk = 0

nit: extra two chars add decent readability

[namiloh]

Looks nice to me. I found it a little surprising though that TxTracker is in legacypool and only operates on a single subpool? Curious your reasoning here?

Yeah, the implementation is old, I just rebased and fixed it up to compile. You are right that it should not be 'legacy only'

[holiman]

Marking this for triage again, would like to have a discussion about if it's an approach that we think is good or not. If not, then I'll drop it. Otherwise, I'll fix it up proper, which means (possibly) integrating it with blobpool/subpools in general, and remove hacks.

[SonuG56]

SonuG56

[holiman]

Comments addressed -- it now interacts with the mail pool instead of the legacy subpool. Package-wise, it resides in the legacypool package, because it uses some utilities from that package: sortedMap and journal. The latter could be moved, along with the tracker, into a standalone package, e.g. locals. The sortedMap however, would have to be placed in some shared utilities-package.

This PR so far only contains the locals-tracker and disables the locals-tracking in the pools. It does not delete the actual code. I'm thinking that it's easier to review this PR as is. And then do a follow-up PR which just wipes the old locals-handling, and moves the tx_tracker.go and journal into a separate subbpackage.

[holiman]

This needs to be considered:

This PR as is, does not integrate with worker: hence, block building will pay no special attention to local transactions. If we want to use the approach in this PR, we can discuss if / how to do such an integration (now that mining-pools no longer function like they did earlier, during ethash, I'm not sure that local txs in block-building is as important any longer) .

[holiman]

About locals and mining

Historically, we've always had a special handling of locals when mining.

• Early on, one could spin up a bit of mining on one's laptop. It was a somewhat convenient way to make a transaction, or perhaps a sequence of transactions into the chain. Mining was somewhat widespread among casual users.
• By special-casing local transactions, these could be free of charge. Note: that although the block producer would have been the fee recipient, arguably it would have been free of charge anyway,
  • but an additional aspect is that there was a high orphan-rate, and an uncled or orphaned block was picked apart, and the transactions included in other blocks.
  • all transactions, even local, were submitted to the network, and potentially includable by anyone.
• Anything submitted via RPC was/is considered local
• Command-line parameters can be used to mark certain addresses as local address, thus special-casing transactions that enter via the network as locals.
• It is a bit murky what happens. The command-line parameter marks address as local, but RPC marks individual transactions. How contagious is the local:ness? From tx to address? If so, should geth search through the entire pool once a new address has been localed?

In the end, in order to ensure that your transactions were included, would have been to either special-case them in locals, or pay sufficiently to win a spot in a block by someone else.

There is one scenario for which this was extra important: pool payouts. A pool payout would typically be performed by an in-pool block, where the opportunity-cost of including a free-of-charge transaction is borne by the pool members. As opposed to paid to potentially non-pool-miners by the pool operator (signer).

The case for dropping the special case

I believe that special-casing local transactions from the miner perspective is now less relevant.

• There is no longer any 'free' transactions, since they all need to pay the basefee. This makes the difference between "mine it ourselves" and "let someone else mine it" becomes smaller .
• There is no longer any 'casual user miner'. Miners are dedicated machines, surrounded by rather high security. The usecase of "convenience" is no longer accurate, the production-system RPC-API is usually behind several protection layers.
• Assembling the blocks is usually performed by MEV, which do not care about local transactions at all.

Note: we would still have the notion of locals. The meaning would be:

I want my local node (which is probably not mining) to remember my transaction indefinitely, and from time to time remind the network that the tx still exists.

If we still want it

If we were to decide that it is still important to support the following usecase:

I want this (valid) transaction included to be top-prio for my miner

Then I think we should use the command-line parameter --txpool.locals: make it mark those transactions as local and prio. The local means it will not be evicted, the prio means it will be prioritized by miner.

Then it becomes semantically clear:

• A sender address can be marked as prio. That sender will have it's txs prioritized when block building
• A transaction can be marked as local. Those transactions will not be forgotten, and sometimes broadcast to the network.

[MariusVanDerWijden]

I think one area where local transaction handling might be interesting in the future is once we implement some flavor of inclusion lists, where it would make sense for a node to add their local transactions to the inclusion list in order to force the block builders to include them.

[holiman]

For inclusion lists, that will be a whole new API method, I'm sure we'll figure it out later.

For now, I have reused the --txpool.locals command, and those are used to set the miners priority-addresses. So this PR essentially separates two notions local and prio. The former is a tx management-concern (don't let the network forget it), the latter is a miner-concern (special-case these senders).

It means that the semantics offered by this PR is slightly different than the current way things work. Off the top of my head, I can't think of any obvious drawback or downside with the changes.

[MariusVanDerWijden]

Looks like one of the tests panics

--- FAIL: TestSimulatedBeaconSendWithdrawals (0.02s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x8c239bd]

goroutine 9411 [running]:
testing.tRunner.func1.2({0x8e91800, 0x9be7230})
	/home/devops/main-go-ethereum/main-go-ethereum/_tool/go/1.23.0/x64/src/testing/testing.go:1632 +0x283
testing.tRunner.func1()
	/home/devops/main-go-ethereum/main-go-ethereum/_tool/go/1.23.0/x64/src/testing/testing.go:1635 +0x3fd
panic({0x8e91800, 0x9be7230})
	/home/devops/main-go-ethereum/main-go-ethereum/_tool/go/1.23.0/x64/src/runtime/panic.go:785 +0x103
github.com/ethereum/go-ethereum/core/txpool/legacypool.(*journal).insert(0x0, 0xb35a330)
	/home/devops/main-go-ethereum/main-go-ethereum/go-ethereum/go-ethereum/core/txpool/legacypool/journal.go:122 +0x1d
github.com/ethereum/go-ethereum/core/txpool/legacypool.(*TxTracker).TrackAll(0xb070640, {0xb8a9

[holiman]

Looks like one of the tests

Thanks, it's fixed. The fix was already applied on "pt2". I don't really expect tests to pass in this PR: I do expect the tests to pass in that second PR. But that one is so large, I felt it's better to disuss the crux of the new mecanism here. Maybe that's not a great idea, and I should just close this one?

[holiman]

Closing in favour of #30559