Skip to content
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

miner: support custom block collation strategies #23421

Closed
wants to merge 16 commits into from

Conversation

jwasinger
Copy link
Contributor

@jwasinger jwasinger commented Aug 19, 2021

depends on #23256.

This PR changes Geth's miner to allow for implementation of custom block collation strategies as Go plugins. A block collator receives empty pending blocks from the miner which it can fill with transactions from the pool (or another source), and commit to be the current sealing/pending block. Plugins can also register custom RPC methods.

Usage

./geth --miner.collator /path/to/plugin --miner.collatorconfig /path/to/custom/config.toml

See https://github.com/jwasinger/geth-mev-plugin for an example which re-implements https://github.com/flashbots/mev-geth miner changes as a plugin.

Collator Plugins

Collator plugins are Go plugins. Geth expects a plugin to export a plugin constructor method:

func PluginConstructor(config *map[string]interface{}, stack *node.Node) (Collator, error)

config represents an optional custom plugin toml configuration file. A Collator implements custom block-filling strategies:

type Collator interface {
    CollateBlocks(miner MinerState, pool Pool, blockCh <-chan BlockCollatorWork, exitCh <-chan struct{})
    CollateBlock(bs BlockState, pool Pool)
}

CollateBlocks is a function that should run for the lifetime of the client (until it sees that exitCh is closed). It is invoked in its own go-routine. Every time a new empty pending block is created, it will appear for the collator to read from blockCh. The BlockCollatorWork object read from blockCh has an empty BlockState for constructing and committing new blocks for sealing as well as a Context that the client will use to communicate when that block is stale due to the arrival of a new canonical chain head block. A block is constructed by adding transactions from the pool (or another custom source) to a BlockState object and calling Commit (which sets the block as a the currently-sealing block).

CollateBlock is the hook for post-merge proposal of a single block. The final call to Commit on the passed BlockState instance (or one of its copies) during the invocation of CollateBlock chooses the block that will be proposed. Post-merge, CollateBlocks is used as a hook for receiving empty pending blocks. Post-merge, calling Commit on the BlockState instances passed to it is a no-op.

The BlockState API allows for adding transactions to a block, inspecting the state between transactions, deep-copying, and a Commit method.

Custom Plugin RPC Methods

The plugin constructor can choose to register custom rpc methods using the given node.Node object's RegisterAPIs method.

@jwasinger jwasinger marked this pull request as ready for review August 24, 2021 11:49
@jwasinger jwasinger requested a review from gballet as a code owner August 24, 2021 11:49
@jwasinger jwasinger marked this pull request as draft August 24, 2021 11:53
headerView := bs.Header()
for {
// If we don't have enough gas for any further transactions then we're done
available := headerView.GasLimit() - headerView.GasUsed()
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 it's wrong to me. Since header is immutable, the GasUsed should be modified at all and this check is meaningless.

miner/collator.go Outdated Show resolved Hide resolved
miner/collator.go Outdated Show resolved Hide resolved
miner/collator.go Outdated Show resolved Hide resolved
miner/worker.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
core/vm/interface.go Show resolved Hide resolved
miner/collator.go Outdated Show resolved Hide resolved
miner/collator.go Outdated Show resolved Hide resolved
miner/collator.go Outdated Show resolved Hide resolved
miner/collator.go Outdated Show resolved Hide resolved
}

// TODO can this error also be returned by commitTransaction below?
_, err := tx.EffectiveGasTip(bs.env.header.BaseFee)
Copy link
Member

Choose a reason for hiding this comment

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

@jwasinger
Copy link
Contributor Author

jwasinger commented Sep 15, 2021

The goal with these shared contextual values for the given work cycle was to give the following properties:

  • calling Commit on a BlockState instance or its copies only interrupts the current sealing block, sets the pending block if the call to CollateBlock where the instance was originally passed to has not returned.
  • if a new canon chain head is received and triggers the miner interrupt while CollateBlock is executing, subsequent calls to Commit() or AddTransactions() for the passed BlockState instance (or any copies of it) for the given work cycle will fail and return an error.
  • if the recommit interval elapses and triggers the miner interrupt while CollateBlock is executing, subsequent calls to AddTransactions() for the passed BlockState instance (or copies of it) will fail and return an error.
  • The recommit interval adjustment should only be triggered once per work cycle.

But as I think about it, the implementation seems really hacky:

  • Each call to Commit overwrites resultEnv , and its value becomes the pending block after CollateBlock returns when sealing. But it seems unintuitive for it to be part of the blockState.
  • The "shared contextual values" mentioned above really belong to the environment for the current work-cycle and not to blockState.
  • Some block-level mutable values (txs, state, tcount, header, receipts) are in environment whereas others are in blockState which is not intuitive. Anything that is unique to a given block should be in blockState. The pending block should belong to the environment for that cycle.

These are just my high-level thoughts about making the changes more intuitive. Going to see if I encounter issues trying to do a refactor.

I started a refactor to make the changes in this PR more intuitive... but it's touching/changing so much of the miner worker that I really think I need to halt until we can do a review call for this.

miner/collator.go Outdated Show resolved Hide resolved

// If mining is running resubmit a new work cycle periodically to pull in
// higher priced transactions. Disable this overhead for pending blocks.
if c.miner.IsRunning() && (chainConfig.Clique == nil || chainConfig.Clique.Period > 0) {
Copy link
Contributor Author

@jwasinger jwasinger Oct 13, 2021

Choose a reason for hiding this comment

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

So I expose this minerState which exposes the chain config and a flag to see if the miner is running to the collator implementation. But I'm thinking for the sake of making the API clean, to remove the MinerState and always do the recommit (eating the added overhead if the miner is not running).

Originally was thinking about the case of 0-period clique where you don't want to commit if there aren't any txs. But this detail should be abstracted away from the collator implementation.

… miner is running and what the consensus engine is
@jwasinger jwasinger closed this Oct 16, 2021
@jwasinger jwasinger reopened this Oct 20, 2021
@jwasinger jwasinger marked this pull request as ready for review October 20, 2021 20:27
@jwasinger jwasinger requested a review from holiman as a code owner October 20, 2021 20:27
@holiman
Copy link
Contributor

holiman commented Mar 8, 2023

This was a good project, but ultimately did not gain the traction we had hoped -- having it be the base used for MEV so that MEV would not have to fork geth, but instead only build plugins.
We're in a different situation now, and I think this PR is no longer relevant. Time to close?

@jwasinger
Copy link
Contributor Author

Yep

@jwasinger jwasinger closed this Mar 9, 2023
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.

5 participants