Skip to content

Miner Collator#23278

Closed
jwasinger wants to merge 13 commits intoethereum:masterfrom
jwasinger:miner_interface
Closed

Miner Collator#23278
jwasinger wants to merge 13 commits intoethereum:masterfrom
jwasinger:miner_interface

Conversation

@jwasinger
Copy link
Copy Markdown
Contributor

Introduces the concept of a Collator which implements a strategy for selecting and ordering transactions from the pool into a pending block.

Includes implementation of a default collator which implements the current miner block collation behavior: choose transactions ordered descending by price, preferencing local over remote transactions when sealing.

@holiman
Copy link
Copy Markdown
Contributor

holiman commented Jul 27, 2021 via email

…ult block collator which has current miner block collation behavior.

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Jared Wasinger <j-wasinger@hotmail.com>
@jwasinger
Copy link
Copy Markdown
Contributor Author

jwasinger commented Jul 27, 2021

I think I rebased off the wrong fork. Now it's fixed. Currently failing one of the clique tests though.

Comment thread miner/collator.go Outdated

if len(localTxs) > 0 {
if err := w.submit(bs, types.NewTransactionsByPriceAndNonce(bs.Signer(), localTxs, bs.BaseFee()), interrupt); err != nil {
return err
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops. I think this should should not return in the case that the recommit timer elapses (at least that is the behvior in the current miner/worker.go)

Comment thread miner/collator.go
Comment thread miner/collator.go
Comment thread miner/collator.go
Comment thread miner/collator.go Outdated
Comment thread miner/collator.go Outdated
Comment thread miner/collator.go Outdated
Comment thread miner/collator.go Outdated
Comment thread miner/collator.go Outdated
Comment thread miner/collator.go Outdated

// Collator is something that can assemble a block.
type Collator interface {
CollateBlock(bs BlockState, pool Pool, interrupt *int32, isSealing bool) error
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Both interrupt and isSealing are for the recommit feedback. Can we somehow nuke them out from the plugin interface? So that the plugin implementers only need to take care of the transaction selection and ordering.

Actually after The Merge, the recommit is totally meaningless. Since in the PoS, we can only generate a single block for attestation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding isSealing, I think it can be removed. When not sealing, we can always use the standard strategy for constructing the pending block when the newTxs event fires. So then CollateBlock only would get called during sealing.

Copy link
Copy Markdown
Contributor Author

@jwasinger jwasinger Jul 28, 2021

Choose a reason for hiding this comment

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

Regarding the interrupt. I am thinking to abstract it inside BlockState so that the collator doesn't actually need to know/care about the details of it (i.e. having AddTransactions fail when the interrupt goes off).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yup, looks good to me.

@jwasinger
Copy link
Copy Markdown
Contributor Author

jwasinger commented Jul 28, 2021

We may also want an additional collator hook (maybe when newHead event fires). Reasoning being that Collators will want to be aware of chain reorgs to adjust whatever custom data they are storing/tracking accordingly. mev-geth doesn't return bundles to the pool of eligible bundles when blocks with already-included bundles are reorged away. So to at least provide an interface which enables a custom collator compatible with the behavior of mev-geth, just the CollateBlock hook should be fine for now.

@jwasinger
Copy link
Copy Markdown
Contributor Author

This is superseded by #23383

@jwasinger jwasinger closed this Aug 11, 2021
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.

3 participants