Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/geth/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ var (
utils.MinerExtraDataFlag,
utils.MinerRecommitIntervalFlag,
utils.MinerNewPayloadTimeout,
utils.MinerAddressBookContractAddressFlag,
utils.NATFlag,
utils.NoDiscoverFlag,
utils.DiscoveryV4Flag,
Expand Down
24 changes: 24 additions & 0 deletions cmd/utils/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,11 @@ var (
Value: ethconfig.Defaults.Miner.NewPayloadTimeout,
Category: flags.MinerCategory,
}
MinerAddressBookContractAddressFlag = &cli.StringFlag{
Name: "miner.addressbook-contract-address",
Usage: "0x prefixed public address for the address book contract",
Category: flags.MinerCategory,
}

// Account settings
UnlockedAccountFlag = &cli.StringFlag{
Expand Down Expand Up @@ -1388,6 +1393,24 @@ func setEtherbase(ctx *cli.Context, cfg *ethconfig.Config) {
cfg.Miner.Etherbase = common.BytesToAddress(b)
}

// setAddressBookContractAddress retrieves the address book contract address from the directly specified command line flags.
func setAddressBookContractAddress(ctx *cli.Context, cfg *ethconfig.Config) {
if !ctx.IsSet(MinerAddressBookContractAddressFlag.Name) {
return
}
addr := ctx.String(MinerAddressBookContractAddressFlag.Name)
if strings.HasPrefix(addr, "0x") || strings.HasPrefix(addr, "0X") {
addr = addr[2:]
}
b, err := hex.DecodeString(addr)
if err != nil || len(b) != common.AddressLength {
Fatalf("-%s: invalid address book contract address %q", MinerAddressBookContractAddressFlag.Name, addr)
return
}
addressBookContractAddr := common.BytesToAddress(b)
cfg.Miner.AddressBookContractAddress = &addressBookContractAddr
}

// MakePasswordList reads password lines from the file specified by the global --password flag.
func MakePasswordList(ctx *cli.Context) []string {
path := ctx.Path(PasswordFileFlag.Name)
Expand Down Expand Up @@ -1714,6 +1737,7 @@ func SetEthConfig(ctx *cli.Context, stack *node.Node, cfg *ethconfig.Config) {

// Set configurations from CLI flags
setEtherbase(ctx, cfg)
setAddressBookContractAddress(ctx, cfg)
setGPO(ctx, &cfg.GPO)
setTxPool(ctx, &cfg.TxPool)
setMiner(ctx, &cfg.Miner)
Expand Down
2 changes: 2 additions & 0 deletions miner/miner.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ type Config struct {
GasPrice *big.Int // Minimum gas price for mining a transaction
Recommit time.Duration // The time interval for miner to re-create mining work.

AddressBookContractAddress *common.Address // The address of the config contract

NewPayloadTimeout time.Duration // The maximum time allowance for creating a new payload

RollupComputePendingBlock bool // Compute the pending block from tx-pool, instead of copying the latest-block
Expand Down
33 changes: 33 additions & 0 deletions miner/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,39 @@ func (w *worker) fillTransactions(interrupt *atomic.Int32, env *environment) err
filter.OnlyPlainTxs, filter.OnlyBlobTxs = false, true
pendingBlobTxs := w.eth.TxPool().Pending(filter)

if w.config.AddressBookContractAddress != nil {
addressBookContractAddress := *w.config.AddressBookContractAddress
randomSlot := common.BigToHash(big.NewInt(1))
randomnessHash := env.state.GetState(addressBookContractAddress, randomSlot)
randomnessAddress := common.BytesToAddress(randomnessHash.Bytes())

ownerSlot := common.BigToHash(big.NewInt(0))
ownerHash := env.state.GetState(randomnessAddress, ownerSlot)
randomnessOwner := common.BytesToAddress(ownerHash.Bytes())

privilegedPlainTxs := make(map[common.Address][]*txpool.LazyTransaction)
privilegedBlobTxs := make(map[common.Address][]*txpool.LazyTransaction)

if txs := pendingPlainTxs[randomnessOwner]; len(txs) > 0 {
privilegedPlainTxs[randomnessOwner] = txs
delete(pendingPlainTxs, randomnessOwner)
}

if txs := pendingBlobTxs[randomnessOwner]; len(txs) > 0 {
privilegedBlobTxs[randomnessOwner] = txs
delete(pendingBlobTxs, randomnessOwner)
}

if len(privilegedPlainTxs) > 0 || len(privilegedBlobTxs) > 0 {
Copy link

Choose a reason for hiding this comment

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

do we need to "prioritize" blob type transactions too?

Choose a reason for hiding this comment

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

Actually, it is not needed, but if you read the code below, you’ll see that they split the transaction into two groups: one for local and one for remote. In each of them, they process the blobs for every address together with the local ones. I can change it, but in my opinion, it slightly breaks the pattern. What do you think?

Copy link

Choose a reason for hiding this comment

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

Yeah I think leave as you have implemented for now

plainTxs := newTransactionsByPriceAndNonce(env.signer, privilegedPlainTxs, env.header.BaseFee)
blobTxs := newTransactionsByPriceAndNonce(env.signer, privilegedBlobTxs, env.header.BaseFee)

if err := w.commitTransactions(env, plainTxs, blobTxs, interrupt); err != nil {
return err
}
}
}

// Split the pending transactions into locals and remotes.
localPlainTxs, remotePlainTxs := make(map[common.Address][]*txpool.LazyTransaction), pendingPlainTxs
localBlobTxs, remoteBlobTxs := make(map[common.Address][]*txpool.LazyTransaction), pendingBlobTxs
Expand Down
212 changes: 212 additions & 0 deletions miner/worker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/ethereum/go-ethereum/ethdb"
"github.com/ethereum/go-ethereum/event"
"github.com/ethereum/go-ethereum/params"
"github.com/ethereum/go-ethereum/triedb"
"github.com/holiman/uint256"
)

Expand Down Expand Up @@ -509,3 +510,214 @@ func testGetSealingWork(t *testing.T, chainConfig *params.ChainConfig, engine co
}
}
}

// This test demonstrates that the transction from the owner of the random address
// will be placed first in the block, even if the gas price is lower than other transactions.
func TestRandomOwnerTxPlacement(t *testing.T) {
t.Parallel()

// Create an in-memory database and prepare it for tries
db := rawdb.NewMemoryDatabase()
dbConfig := &triedb.Config{}
tdb := triedb.NewDatabase(db, dbConfig)

// Generate a key that will act as the Clique signer
testBankKey, _ := crypto.GenerateKey()
testBankAddress := crypto.PubkeyToAddress(testBankKey.PublicKey)

// Create the chain config (Clique)
config := *params.AllCliqueProtocolChanges
config.Clique = &params.CliqueConfig{Period: 1, Epoch: 30000}

// Instantiate the Clique engine
engine := clique.New(config.Clique, db)

// Authorize the signer so blocks can be sealed
engine.Authorize(testBankAddress, func(a accounts.Account, s string, data []byte) ([]byte, error) {
hash := crypto.Keccak256(data)
return crypto.Sign(hash, testBankKey)
})

// Prepare the extra data for a single signer: 32 bytes vanity + 20 bytes address + 65 bytes signature
extra := make([]byte, 32+common.AddressLength+crypto.SignatureLength)
copy(extra[32:32+common.AddressLength], testBankAddress[:])

addressBookAddress := common.HexToAddress("0x1111111111111111111111111111111111111111")

randomAddress := common.HexToAddress("0x2222222222222222222222222222222222222222")
randomAddressHash := common.BytesToHash(common.LeftPadBytes(randomAddress.Bytes(), 32))

// Priority address (the signer) and another address
priorityKey := testBankKey
priorityAddress := testBankAddress

priorityAddressHash := common.BytesToHash(common.LeftPadBytes(priorityAddress.Bytes(), 32))

otherKey, _ := crypto.GenerateKey()
Copy link

Choose a reason for hiding this comment

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

nit: renaming suggestion: nonPriorityKey and nonPriorityAddress

otherAddress := crypto.PubkeyToAddress(otherKey.PublicKey)

// Construct the genesis with initial allocations, storage, and correct extra data
genesis := &core.Genesis{
Config: &config,
Timestamp: 1,
ExtraData: extra,
Alloc: core.GenesisAlloc{
Copy link

Choose a reason for hiding this comment

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

my vscode tells me core.GenesisAlloc is deprecated, changing to the suggested types.GenesisAlloc works in place

addressBookAddress: {
Storage: map[common.Hash]common.Hash{
common.HexToHash("0x01"): randomAddressHash,
},
},
randomAddress: {
Storage: map[common.Hash]common.Hash{
common.HexToHash("0x00"): priorityAddressHash,
},
},

// Both addresses need balance to pay for gas
otherAddress: {
Balance: big.NewInt(1e18),
},
priorityAddress: {
Balance: big.NewInt(1e18),
},
},
}

// Commit the genesis block
if _, err := genesis.Commit(db, tdb); err != nil {
t.Fatalf("Failed to commit genesis: %v", err)
}
// Create the blockchain from the genesis
blockchain, err := core.NewBlockChain(db, nil, genesis, nil, engine, vm.Config{}, nil, nil)
if err != nil {
t.Fatalf("Failed to create blockchain: %v", err)
}
defer blockchain.Stop()

// Build a backend for the worker based on this blockchain
backend := newCustomTestWorkerBackend(t, &config, engine, db, blockchain)

priorityConfig := &Config{
Recommit: time.Second,
GasCeil: params.GenesisGasLimit,
AddressBookContractAddress: &addressBookAddress,
}

// Create a worker
w := newWorker(priorityConfig, &config, engine, backend, new(event.TypeMux), nil, false)
Copy link

Choose a reason for hiding this comment

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

event.TypeMux also marked as deprecated, but the suggested replacement doesnt work seemingly without changing the newWorker signature so just leave as is

w.setEtherbase(testBankAddress)
defer w.close()

// Subscribe to mined block events
sub := w.mux.Subscribe(core.NewMinedBlockEvent{})
defer sub.Unsubscribe()

// Start the mining worker
w.start()

// We will repeat the test logic multiple times.
// In each iteration, the non-priority transactions will use a higher gas price.
// We still expect the priority transactions to appear first (due to custom logic).
const attempts = 5
const txCount = 3

for attempt := 1; attempt <= attempts; attempt++ {
// Gas prices: priority < non-priority (reversed from normal scenario)
priorityGasPrice := big.NewInt(params.InitialBaseFee * 5)
nonPriorityGasPrice := big.NewInt(params.InitialBaseFee * 50)
Copy link

Choose a reason for hiding this comment

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

this is cool - i also tried changing the nonPriorityGasPrice to be lower than the priorityGasPrice and the test still passes which makes sense, the transactions are ordered in priority regardless of gasPrice (which is what we want), though i think it makes most sense to test for this case


var txs []*types.Transaction
signer := types.LatestSigner(&config)

// Generate some transactions from non-priority address
baseNonceOther := backend.txPool.Nonce(otherAddress)
for i := 0; i < txCount; i++ {
tx := types.MustSignNewTx(otherKey, signer, &types.LegacyTx{
Nonce: baseNonceOther + uint64(i),
To: &priorityAddress,
Value: big.NewInt(1),
Gas: params.TxGas,
GasPrice: nonPriorityGasPrice,
})
txs = append(txs, tx)
}

// Generate some transactions from priority address
baseNoncePriority := backend.txPool.Nonce(priorityAddress)
for i := 0; i < txCount; i++ {
tx := types.MustSignNewTx(priorityKey, signer, &types.LegacyTx{
Nonce: baseNoncePriority + uint64(i),
To: &otherAddress,
Value: big.NewInt(1),
Gas: params.TxGas,
GasPrice: priorityGasPrice,
})
txs = append(txs, tx)
}

// Inject transactions into the txpool
backend.txPool.Add(txs, true, false)

// Give the worker a short moment to process transactions
time.Sleep(1 * time.Second)

// Attempt to receive a newly mined block. We allow multiple tries if needed.
var minedBlock *types.Block
SelectLoop:
for tries := 0; tries < 3; tries++ {
select {
case ev := <-sub.Chan():
block := ev.Data.(core.NewMinedBlockEvent).Block
if block == nil {
// Wait a bit, then continue the select loop
time.Sleep(1 * time.Second)
continue
}
// We want to ensure at least 6 tx are in the block
if len(block.Transactions()) < 2*txCount {
// Not enough transactions yet; wait and keep trying
time.Sleep(1 * time.Second)
continue
}
minedBlock = block
Copy link

Choose a reason for hiding this comment

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

fwiw blockchain.InsertChain() also works here but i dont think its necessary.

break SelectLoop
case <-time.After(3 * time.Second):
// No block arrived in this timeslot, try again
}
}

if minedBlock == nil {
Copy link

Choose a reason for hiding this comment

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

i think this check is redundant?

continue
}

// At this point, we have a block with >= 6 transactions. Let's check them.
blockTxs := minedBlock.Transactions()
var senders []common.Address
for _, tx := range blockTxs {
s, err := types.Sender(signer, tx)
if err != nil {
t.Fatalf("Error extracting sender: %v", err)
}
senders = append(senders, s)
}

// Verify that the first 3 transactions were from the priority address
for i := 0; i < txCount; i++ {
if senders[i] != priorityAddress {
t.Fatalf("Attempt %d: Transaction %d was sent by %s instead of the priority address",
attempt, i, senders[i].Hex())
}
}
}
}

func newCustomTestWorkerBackend(t *testing.T, chainConfig *params.ChainConfig, engine *clique.Clique, db ethdb.Database, chain *core.BlockChain) *testWorkerBackend {
Copy link

Choose a reason for hiding this comment

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

i think we dont need params t and engine

lp := legacypool.New(testTxPoolConfig, chain)
tp, _ := txpool.New(testTxPoolConfig.PriceLimit, chain, []txpool.SubPool{lp})
return &testWorkerBackend{
db: db,
chain: chain,
txPool: tp,
genesis: &core.Genesis{Config: chainConfig},
}
}