miner: support multiple custom block collation strategies evaluated concurrently#23383
miner: support multiple custom block collation strategies evaluated concurrently#23383jwasinger wants to merge 26 commits intoethereum:masterfrom
Conversation
This PR removes the duplicated code in the catalyst package, reuse the miner package code instead.
holiman
left a comment
holiman
left a comment
There was a problem hiding this comment.
A lot to review here, it's coming along nicely. I'll need to dive more deeply into it later on. Found a few nits though
| @@ -0,0 +1,394 @@ | |||
| // Copyright 2015 The go-ethereum Authors | |||
There was a problem hiding this comment.
| // Copyright 2015 The go-ethereum Authors | |
| // Copyright 2021 The go-ethereum Authors |
| ) | ||
|
|
||
| const ( | ||
| Uint64Max = 18446744073709551615 |
There was a problem hiding this comment.
We have math.MaxUint64 already
| txReceipts = bs.work.env.receipts[startTCount:tcount] | ||
| } | ||
| // TODO: deep copy the tx receipts here or add a disclaimer to implementors not to modify them? | ||
| shouldRevert := cb(err, txReceipts) |
There was a problem hiding this comment.
I had kind of thought that you'd call the cb after each transaction. If you do that, then we might abort earlier, in case there's a sequence of txs in which the first "succeed-fails" (gets into the block but leads to an undesired state, e.g. fails execution).
Any particular reasons for preferring one way over the other? Haven't thought about it too deeply myself.
There was a problem hiding this comment.
Yeah. I think calling cb after every transaction makes sense.
| for i := startTCount; i < tcount; i++ { | ||
| bs.work.env.txs[i] = nil | ||
| bs.work.env.receipts[i] = nil | ||
| } |
There was a problem hiding this comment.
This looks strange. No need to nil the slice values when you just crop it right below here anyway.
| func (m *MultiCollator) Close() { | ||
| for _, c := range m.collators { | ||
| select { | ||
| case c.exitCh <- struct{}{}: |
There was a problem hiding this comment.
For exit channels, what usually works pretty good is just to close them when it's time to exit. Then it doesn't matter if there's one listening or zero or 100. Whereas if you send a struct{}{}, then you need exactly one listener -- or, do as you've done here, and have a default-case.
So unless you have some compelling reason, just close(c.exitCh) IMO
| select { | ||
| case c.newWorkCh <- collatorWork{env: work.copy(), counter: m.counter, interrupt: interrupt}: | ||
| m.responsiveCollatorCount++ | ||
| } |
There was a problem hiding this comment.
This is a select with only one case -- I think it's equivalent to:
| select { | |
| case c.newWorkCh <- collatorWork{env: work.copy(), counter: m.counter, interrupt: interrupt}: | |
| m.responsiveCollatorCount++ | |
| } | |
| c.newWorkCh <- collatorWork{env: work.copy(), counter: m.counter, interrupt: interrupt} | |
| m.responsiveCollatorCount++ | |
Is that the intent? Or did you mean to have a default-case too?
There was a problem hiding this comment.
Yeah. Missing a default case (shouldn't block when newWorkCh is full).
| shouldIgnore := false | ||
| for _, finishedCollator := range finishedCollators { | ||
| if i == finishedCollator { | ||
| shouldIgnore = true |
There was a problem hiding this comment.
| shouldIgnore = true | |
| shouldIgnore = true | |
| break |
?
jwasinger
changed the title
|
Superseded by #23421 |
4 participants
depends on #23256