Refactor the miner #1545

trianglesphere · 2021-05-12T20:27:18Z

Description

This refactors the miner with a couple goals

Simplify the block construction logic.
Enable pre-construciton / pre-sharing of blocks in the miner
Fix the pending state bug

I worked towards these goals by

Reducing the number of go-routines passing work to each other. There is now 1 main loop goroutine.
Make the block creation process as linear as possible (and use contexts to cancel instead of global interrupts).
- Each step in the block production cycle is now done in miner/block.go
- There is no longer a shared "current environment" for the in flight block, but a new environment is made for each request (which is the blockState struct).
- Block production is cancelable with contexts instead of global interrupts
- I have removed as much blocking as possible from engine.Prepare
  - It no longer sleeps until the block time - that is done in the worker
  - It does not wait until the next sequence for nodes that are not validating

Other changes

Removes proof of work specific miner interfaces.
Removes miner/unconfirmed.go
Includes some upstream changes made to miner/miner.go to simplify the start up logic.

Tested

Unit tests still pass.

Tested end to end on trianglesphere/miner_refactor_v2
TODO: test that this fixes the pending bug.

Related issues

Fixes RPC "pending" block can be behind "latest" block #1419
Partial fix of Implement pre-sharing of proposed block #1426

Backwards compatibility

Yes

This is b/c IPC does not play nice with long temp dirs.

Don't have a shared "environment", but create one per block production.

This reverts commit d5ec1e6.

Continuing to follow the convention that the receiver is modified while passed in parameters are not.

Now the miner only has one go-routine.

mcortesi · 2021-05-27T17:40:21Z

consensus/istanbul/backend/engine.go

+	// addParentSeal blocks for up to 500ms waiting for the core to reach the target sequence.
+	// Prepare is called from non-validators, so don't bother with the parent seal unless this
+	// block is to be proposed instead of for the local state.
+	if sb.IsValidating() {


this is awckward...

we have a method call addParentSeal() that does a sleep. As a reader, why would adding a seal implies sleeping?

And then i have a Prepare() than ask if we are validating...

I know this is not from this PR, and the ware only bringing up the awckarness to the surface. But i wonder if there's a clearer way to express this.

mcortesi · 2021-05-27T17:41:39Z

consensus/istanbul/backend/test_utils.go

@@ -223,6 +223,7 @@ func makeBlockWithoutSeal(chain *core.BlockChain, engine *Backend, parent *types
 	// The worker that calls Prepare is the one filling the Coinbase
 	header.Coinbase = engine.address
 	engine.Prepare(chain, header)
+	time.Sleep(time.Until(time.Unix(int64(header.Time), 0)))


do we need to sleep? would be ncie if we didn't as i've seen we are not really testing this behaviuor, but just need to create blocks on the test

The tests have issues if we don't sleep at all, but it's definitely possible to sleep for less time.

miner/miner.go

miner/worker.go

mcortesi · 2021-05-27T19:17:58Z

miner/block.go

+}
+
+// prepareBlock intializes a new blockState that is ready to have transaction included to.
+func prepareBlock(w *worker) (*blockState, error) {


i would say this is cheating: so, to avoid having a worker method in another file, you make it a standalone function...

but you access worker state is if where your private state.

for example, using the Lock to access internal state. This can be nasty, since the whole idea of having an object with "private state" is that you don't need to worry about anyone else when modifying it... that's encapsulation. But we don't have any of that here.

I can look to the other side for now, if we want to get this to master, but it is worth mentioning nonetheless

mcortesi · 2021-05-27T20:34:09Z

miner/block.go

+			cpy[i] = new(types.Log)
+			*cpy[i] = *l
+		}
+		w.pendingLogsFeed.Send(cpy)


to me it's ackward that we are accessing an internal feed object on the worker here

Yea. This is the same thing as above that the miner used to be so entangled in the block production that making it only depend on specific fields is a pain.

mcortesi · 2021-05-27T21:56:38Z

miner/worker.go

-			}
-		}
-		recommit = time.Duration(int64(next))
+	w.updatePendingBlock(b)


asking again, does it make sense to have a pending block with no transactions?

We need to make sure that the pending block is the latest head block

mcortesi · 2021-05-27T22:05:51Z

miner/worker.go

-		}
+	for {
+		select {
+		case <-w.startCh:


if we call start() twice that means we will cancel current work and start again, feature or bug?

That's correct. Unintended consequence that doesn't really matter. We want to cancel the pending block and start proposing blocks on start. It would be possible to gate sending to the start channel on the current miner state, but it doesn't seem like a big issue.

mcortesi · 2021-05-27T22:07:39Z

miner/worker.go


-	var coalescedLogs []*types.Log
+	txsCh := make(chan core.NewTxsEvent, txChanSize)


shouldn't we try to get txpool configured size to set this size?

We could, but this isn't regressing.

miner/worker.go

* Remove unused miner flags and configuration options * Remove threads flag. Mark GasPrice as deprecated - Removes MinerThreads flags, it was not used. - Marks GasPrice flag as deprecated - Maps miner.Start/Stop method on the web3js api * pr comment

mcortesi

LGTM! Great work!

Joshua Gutow added 30 commits May 11, 2021 13:48

Remove unused tests

e1e51cd

Disable the rest of the worker tests

0f8e175

Export templates in mycelo

d0c6de0

Add golang mycelo testnet

4f0c951

Fix contracts path

6d09400

Disable IPC in favor of ws

ab86aab

This is b/c IPC does not play nice with long temp dirs.

Adds more to the network

6a93759

Modify network config

fe4c813

Expose more of loadbot

5107233

Add sending and waiting for transactions

8e38a1b

Add block production test to the miner

a852cb0

Fix loadbot for replay protection

7172c42

Set a long timeout on network startup

498fda5

REVERT ME: Extra block validity check on writeWithState

8623e59

Remove unused miner methods

8a0ac2f

Remove unconfirmed

98da0c6

Pull out recover randomness function

965c634

Remove account manager

fbf9dc0

Use upstream channels instead of sync to manage the worker

0d4694c

Remove recommitting

152e24c

Remove side channel sub & timer

55e5304

Remove ancestors

e029737

Split worker into two

ec3ea4a

Remove tx sub channel

c29c60b

Operate on a separate block state

06d9136

Don't have a shared "environment", but create one per block production.

Remove interrupts from commitTransactions

00aebc6

Remove noempty

694c922

Remove the task loop

271a39c

Clean up fees calculation

1a153bb

Remove interrupts in the miner

0e42c75

Joshua Gutow added 6 commits May 18, 2021 13:00

Partial Revert "Pull subscriptions into the using function"

83affa3

This reverts commit d5ec1e6.

Drain txs sub channel

ed47233

Unsubscribe from the subscription at close

7567d7f

Fix typo

2466105

Merge branch 'master' into trianglesphere/new_miner_no_e2e_test

96fb0dc

Switch from receiver to first argument

9a2bc6b

Continuing to follow the convention that the receiver is modified while passed in parameters are not.

trianglesphere mentioned this pull request May 25, 2021

Removes the miner's pending cache #1555

Merged

Joshua Gutow added 3 commits May 27, 2021 12:06

Merge branch 'master' into trianglesphere/new_miner_no_e2e_test

0c89ebe

Merge validator and full node loop into one

bfa3dd9

Now the miner only has one go-routine.

Move unsubscribe to mainloop

1b44835

mcortesi reviewed May 27, 2021

View reviewed changes

miner/worker.go Show resolved Hide resolved

Remove Miner Flags (#1556)

345f10c

* Remove unused miner flags and configuration options * Remove threads flag. Mark GasPrice as deprecated - Removes MinerThreads flags, it was not used. - Marks GasPrice flag as deprecated - Maps miner.Start/Stop method on the web3js api * pr comment

trianglesphere requested review from hbandura and nategraf as code owners May 28, 2021 14:43

Joshua Gutow added 4 commits May 28, 2021 13:15

PR comment

26560bc

Add wait group

91d5178

Remove txFeeRecipient

57ceb42

Assert that transactions are included

90c2e5a

mcortesi approved these changes May 28, 2021

View reviewed changes

trianglesphere added the automerge label May 28, 2021

mergify bot merged commit 74b21c4 into master May 28, 2021

mergify bot deleted the trianglesphere/new_miner_no_e2e_test branch May 28, 2021 19:41

oneeman mentioned this pull request Aug 23, 2021

Remove unnecessary statedb copies and move the prepare inside Finalise #1673

Merged

nategraf mentioned this pull request Jan 19, 2022

Reinstate delay before signature aggregation in backend.Prepare #1798

Merged

piersy mentioned this pull request Feb 25, 2022

Ensure pending state is ready for tx execution #1858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the miner #1545

Refactor the miner #1545

trianglesphere commented May 12, 2021 •

edited

Loading

mcortesi May 27, 2021

mcortesi May 27, 2021

trianglesphere May 28, 2021

mcortesi May 27, 2021

mcortesi May 27, 2021

trianglesphere May 28, 2021

mcortesi May 27, 2021

trianglesphere May 28, 2021

mcortesi May 27, 2021

trianglesphere May 28, 2021

mcortesi May 27, 2021

trianglesphere May 28, 2021

mcortesi left a comment


		var coalescedLogs []*types.Log
		txsCh := make(chan core.NewTxsEvent, txChanSize)

Refactor the miner #1545

Refactor the miner #1545

Conversation

trianglesphere commented May 12, 2021 • edited Loading

Description

Other changes

Tested

Related issues

Backwards compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcortesi left a comment

Choose a reason for hiding this comment

trianglesphere commented May 12, 2021 •

edited

Loading