-
Notifications
You must be signed in to change notification settings - Fork 20k
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: allow for extradata in post-merge blocks #26387
Conversation
miner/worker.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2?
There was a problem hiding this comment.
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})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* miner: allow for extradata in post-merge blocks * miner: nits * miner: remove extradata checks
closes #26376