Skip to content

miner, core/txpool: refactor processing of pending transactions #29025

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Feb 19, 2024

This PR is work in progress, based on the comment made here: #29008 (comment)

This PR changes the format of Pending from subpool. It now returns

type pendingSet struct {
	Tails map[common.Address][]*LazyTransaction // Per account nonce-sorted list of transactions
	Heads FeeList                               // Next transaction for each unique account (price heap)
}

The Tails is simply all pending transactions, in a map.
The Heads is a list of address, fee, denoting the fee of the first tx from each account. It is a heap.

The pendingSet supports operations that were previously implemented by transactionsByPriceAndNonce :

type Pending interface {
	// Shift replaces the current best head with the next one from the same account.
	Shift()
	// Peek returns the next transaction by price.
	Peek() (*LazyTransaction, *uint256.Int)

	// Pop removes the best transaction, *not* replacing it with the next one from
	// the same account. This should be used when a transaction cannot be executed
	// and hence all subsequent ones should be discarded from the same account.
	Pop()

	// Empty returns true if the set is empty.
	Empty() bool

	// Clears the set
	Clear()
}

There are several TODOs

  • Implement PendingHashes (used in simulated and eth/sync
  • Handle incoming txs in worker.mainLoop
  • Add some mechanism to filter out blob-txs / disable blobpool pendings from miner
  • Re-add locals-handling
  • Benchmark block building vs master

@holiman holiman force-pushed the refactor_pending branch 2 times, most recently from c3e05fd to 0d07ab8 Compare February 19, 2024 15:28
Copy link
Contributor Author

@holiman holiman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter.OnlyXXX ought to be a single boolean, to denote always one xor the other.

@holiman holiman force-pushed the refactor_pending branch 2 times, most recently from c0308d1 to 9c6b402 Compare February 20, 2024 09:43
Comment on lines +50 to +60
type PendingFilter struct {
MinTip *uint256.Int // Minimum miner tip required to include a transaction
BaseFee *uint256.Int // Minimum 1559 basefee needed to include a transaction
BlobFee *uint256.Int // Minimum 4844 blobfee needed to include a blob transaction

OnlyPlainTxs bool // Return only plain EVM transactions (peer-join announces, block space filling)
OnlyBlobTxs bool // Return only blob transactions (block blob-space filling)

OnlyLocals bool // Return only txs from 'local' addresses.
NoLocals bool // Remove all txs from 'local' addresses
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing is getting kind of big. Perhaps, instead of passing around a PendingFilter, we should pass around *PendingFilter, which in turn could have uint256.Int fields instead of *uint256.Int?

core/txpool: make heads into smaller struct

core/txpool miner: handle incoming txs in worker

core/txpool: implement PendingHashes

core/txpool: unexport types, use interface + minor tweaks

core/txpool, miner: post-rebase fixup

core/txpool, miner: add back handling of locals

core/txpool, miner: move new types to pending.go

core/txpool: testing

core/txpool: make tests pass

core/txpool, eth: fix up more tests

core/txpool: fix pendingset empty-check

core/txpool: add back sorting by time
@holiman holiman force-pushed the refactor_pending branch from 91e3d6d to 5ccec6f Compare March 7, 2024 13:32
@@ -258,6 +258,25 @@ func (m *sortedMap) Flatten() types.Transactions {
return txs
}

// AppendHashes uses the flattened slice of transactions and appends the hashes
// to the destination slice.
func (m *sortedMap) AppendHashes(dst []common.Hash) []common.Hash {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this function. It serves as backend for PendingHashes, which is used e.g. when we meet another peer on the network, and need to exchange hashes. This happens quite often, so I figured it might be worth optimizing a bit.
With this change, we just allocate the slice of hashes once, and then we just iterate the flattened cached list and append them.
The method Flatten copies the cache in an intermediate step, to prevent accidental modification. So this allows us to skip that step.

"github.com/holiman/uint256"
)

type Pending interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, I had two types implementing this. Now there's only pendingSet. I'm torn if we should keep this as an interface or not. Open to suggestions.

}
pending := make(map[common.Address][]*txpool.LazyTransaction, len(pool.pending))
baseFeeBig := baseFee.ToBig()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you dropped the if filter.BaseFee != nil branch?

NoLocals bool // Remove all txs from 'local' addresses
}

type TxTips struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably want some docs for these?

@MariusVanDerWijden
Copy link
Member

This seems like a nice idea, but the PR has bitrotted over time, so I'm wondering if you want to pick this up again @holiman or if we should close this and open an issue with the idea so we don't forget about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants