Skip to content

miner: allow for extradata in post-merge blocks #26387

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 28, 2022
Merged

miner: allow for extradata in post-merge blocks #26387

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d6a31b
miner: allow for extradata in post-merge blocks
MariusVanDerWijden Dec 27, 2022
f5ee8ee
miner: nits
MariusVanDerWijden Dec 27, 2022
a835275
miner: remove extradata checks
MariusVanDerWijden Dec 27, 2022
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
11 changes: 7 additions & 4 deletions miner/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,13 +959,13 @@ func (w *worker) commitTransactions(env *environment, txs *types.TransactionsByP

// generateParams wraps various of settings for generating sealing task.
type generateParams struct {
timestamp uint64 // The timstamp for sealing task
timestamp uint64 // The timestamp for sealing task
forceTime bool // Flag whether the given timestamp is immutable or not
parentHash common.Hash // Parent block hash, empty means the latest chain head
coinbase common.Address // The fee recipient address for including transaction
random common.Hash // The randomness generated by beacon chain, empty before the merge
noUncle bool // Flag whether the uncle block inclusion is allowed
noExtra bool // Flag whether the extra field assignment is allowed
limitExtra bool // Flag whether the extra field should be limited to 32 byte
noTxs bool // Flag whether an empty block without any transaction is expected
}

Expand Down Expand Up @@ -1002,7 +1002,10 @@ func (w *worker) prepareWork(genParams *generateParams) (*environment, error) {
Coinbase: genParams.coinbase,
}
// Set the extra field if it's allowed.
if !genParams.noExtra && len(w.extra) != 0 {
if genParams.limitExtra && len(w.extra) > 32 {
return nil, fmt.Errorf("invalid extra data length: %v", w.extra)
Copy link
Member

@karalabe karalabe Dec 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This seems a bit aggressive to fail on mining. IMHO we should either refuse to start up, or if we already reached this point, we should print a warning log and continue either truncating or leaving the field empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm kinda agree that we should fail earlier.
Not too aggressive though since len(extradata) <= 32 is part of the block verification rules post merge

Copy link
Contributor

Choose a reason for hiding this comment

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

Not too aggressive though

Well, if a user happened to specify 35 bytes of extradata via command line params, or API, and somehow we missed capping it earlier, it is too aggressive to punish him by disabling mining, when we can just cap it at 32 and let him off with a stern warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just saw we already have the check on minerAPI.SetExtra()
So I think we can just remove the limitExtra field

}
if len(w.extra) != 0 {
header.Extra = w.extra
}
// Set the randomness field from the beacon chain if it's available.
Expand Down Expand Up @@ -1225,7 +1228,7 @@ func (w *worker) getSealingBlock(parent common.Hash, timestamp uint64, coinbase
coinbase: coinbase,
random: random,
noUncle: true,
noExtra: true,
limitExtra: true,
noTxs: noTxs,
},
result: make(chan *newPayloadResult, 1),
Expand Down
2 changes: 1 addition & 1 deletion miner/worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ func testGetSealingWork(t *testing.T, chainConfig *params.ChainConfig, engine co
}
_, isClique := engine.(*clique.Clique)
if !isClique {
if len(block.Extra()) != 0 {
if len(block.Extra()) != 2 {
Copy link
Member

Choose a reason for hiding this comment

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

2?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test was wrong in that it required the resulting block to have no extradata set for ethash and post-merge even if we set the extradata to w.setExtra([]byte{0x01, 0x02})

t.Error("Unexpected extra field")
}
if block.Coinbase() != coinbase {
Expand Down