Support for syncing on mergemock #3174

Giulio2002 · 2021-12-26T19:01:56Z

Things Added

Code for Block Proposing
Validators can now be ran on Erigon
Private testnet with proof-of-stake enabled can be ran on Erigon
mergemock can be ran on Erigon
All test vectors from EF pass successfully

Prerequisite

Erigon-lib: updated to latest merge spec ledgerwatch/erigon-lib#217
Interfaces : update to latest merge spec interfaces#87

Above PRs must be merged for this PR to not be a draft @AskAlexSharov

Things missing before moving to M2 Kinstugi Milestones:

Disable P2P Gossip accordingly to spec
EIP-2124

Giulio2002 · 2021-12-26T21:17:41Z

More things to add: if ran against mergemock VerifyHeader could fail, probably a bug in mergemock, for now i commented the verifyHeader statement. other thing to say, if we are on the tip of the chain and receive the new head, i made it so we process the bodies in stage headers, i will change it in a next PR, this one is too big already honestly.

.gitignore

eth/stagedsync/stage_mining_finish.go

Giulio2002 · 2021-12-27T10:03:05Z

We use it to communicate the mined block to ForkchoiceUpdated, so that we keep mining separate from ethbackend

yperbasis · 2021-12-27T10:05:16Z

We use it to communicate the mined block to ForkchoiceUpdated, so that we keep mining separate from ethbackend

But where's the code that reads MiningFinishCfg.assembledBlock?

Giulio2002 · 2021-12-27T10:25:07Z

here https://github.com/ledgerwatch/erigon/blob/45cfc7ed83aafd4d55f4a0c728fed0674aa5ae79/ethdb/privateapi/ethbackend.go#L308 s.assembledBlock is the same pointer as in the mining staged sync

yperbasis · 2021-12-27T10:32:19Z

https://github.com/ledgerwatch/erigon/blob/45cfc7ed83aafd4d55f4a0c728fed0674aa5ae79/ethdb/privateapi/ethbackend.go#L308

s.assembledBlock is the same pointer as in the mining staged sync

Hmm. I'm afraid I'm still not following. Where is the code that ensures that MiningFinishCfg.assembledBlock is the same as EthBackendServer.assembledBlock?

Giulio2002 · 2021-12-27T10:37:05Z

https://github.com/ledgerwatch/erigon/blob/45cfc7ed83aafd4d55f4a0c728fed0674aa5ae79/ethdb/privateapi/ethbackend.go#L308

s.assembledBlock is the same pointer as in the mining staged sync

Hmm. I'm afraid I'm still not following. Where is the code that ensures that MiningFinishCfg.assembledBlock is the same as EthBackendServer.assembledBlock?

https://github.com/ledgerwatch/erigon/blob/45cfc7ed83aafd4d55f4a0c728fed0674aa5ae79/eth/backend.go#L138

https://github.com/ledgerwatch/erigon/blob/45cfc7ed83aafd4d55f4a0c728fed0674aa5ae79/eth/backend.go#L402

https://github.com/ledgerwatch/erigon/blob/45cfc7ed83aafd4d55f4a0c728fed0674aa5ae79/eth/backend.go#L410

It is the same pointer

yperbasis · 2021-12-27T10:50:19Z

It is the same pointer

Ah, OK, I see. The problem is that you need a syncronization mechanism to prevent data races since the data will be accessed from multiple goroutines. You should use something like atomic.Value (or a channel or a mutex perhaps).

yperbasis · 2021-12-27T10:57:07Z

Something like "Concurrency in Go" by Katherine Cox–buday is a nice read :)

yperbasis · 2021-12-28T11:31:10Z

eth/stagedsync/stage_headers.go

 ) error {
-	atomic.StoreUint32(cfg.waitingPosHeaders, 1)
+	// We need to have another write transaction for the miner in case we want to validate.


Not sure I'm following. Why do you commit the transaction?

we are closing it, in case we receive the request to mine. Mining stages uses a write transaction so we would stall when trying to propose new blocks

Hmm. My gut feeling is that the transaction commitment here might be problematic. I don't understand the current interplay between waiting for PoS blocks and the mining. Probably need to pick up @AskAlexSharov brain for that.

I do not think it is problematic, if any error occur we rollback them, so if the tx has not been killed and has been kept by another stage it should not give problems. if any stage does not rollback on error then the issue is not in the scope of stage headers. I can explain the interplay, mining stages uses a write transaction and block proposing happens only if we are waiting the beacon chain, thus when mining is requested the stagedsync must be in stage headers waiting for payloads. now if we did not close it prior waiting, the mining stage will never start unless we close the write transaction in stage headers. because you cannot have more than 2 write transactions on MDBX.

Note also that external transactions are usually used for unit tests and not for production

And when the Beacon node sends us a engine_forkchoiceupdatedv1 request with a payload, ethbackend should send a message to SkipCycleHack to interrupt the wait in the header stage.

okay, so to answer all the points:

Beacon chain calls are synchronous

if external transactions are used in production then ending stage headers simply wont't be enough

I noticed that there is an actual benefit of doing how i did it as of now, before we mined block every 3 seconds on a goroutine constantly, now we can do the same without having a goroutine constantly on and only when requested.

I think it makes more sense logically to do it like this, also tests are passing so it should not break any other component

yes, must just wait until we timeout

The thing is that Beacon mining ("payload build process") is supposed to be asynchronous. engine_forkchoiceUpdatedV1 with payload should only initiate the mining, not to wait for its completion. So IMHO more work is still required.

But this PR is too big already. In order to make progress, I suggest the following:

Leave only a rudimentary block build process in this PR, without any transactions (probably w/o correct state root either). Then we continue the work on proper PoS mining with transactions and everything in a subsequent PR.

Restore the useExternalTx logic in HeaderPoS and don't do this tx.Commit hack.

eth/stagedsync/stage_mining_create_block.go

eth/stagedsync/stage_headers.go

eth/stagedsync/stage_mining_create_block.go

ethdb/privateapi/ethbackend.go

yperbasis · 2021-12-29T11:45:06Z

eth/backend.go

@@ -404,9 +404,29 @@ func New(stack *node.Node, config *ethconfig.Config, logger log.Logger) (*Ethere
 	if casted, ok := backend.engine.(*ethash.Ethash); ok {
 		ethashApi = casted.APIs(nil)[1].Service.(*ethash.API)
 	}
-	atomic.StoreUint32(&backend.waitingForPOSHeaders, 0)
+	atomic.StoreUint32(&backend.waitingForBeaconChain, 0)
+	startAssembleFunc := func(timestamp uint64, random common.Hash, suggestedFeeRecipient common.Address) (types.Block, error) {


We should not perform complete mining here, only to initiate it. Per the spec: "if payloadAttributes is not null and the client is not SYNCING, and MUST begin a payload build process building on top of forkchoiceState.headBlockHash". And the build process should start concurrently and last for up to 12 seconds. See https://github.com/ethereum/execution-apis/blob/v1.0.0-alpha.5/src/engine/specification.md#payload-build-process

Currently startAssembleFunc confusingly not only starts the build process, but waits for it to finish.

…into mining-pos

ethdb/privateapi/ethbackend.go

yperbasis · 2021-12-29T14:39:48Z

ethdb/privateapi/ethbackend.go

+		return nil, err
+	}
+
+	if atomic.LoadUint32(s.waitingForBeaconChain) == 0 || headHeader == nil {


The previous check parent != rawdb.ReadHeadBlockHash(tx) is better since we might have the header in the DB but be on a different fork.

we are reading the header by hash, which means it looks into blockhashes which does not do only canonical headers

Yeah, that's my point. When our latest canonical header doesn't match headBlockHash, we should return "SYNCING" status and initiate a re-org.

enrique will add it

ethdb/privateapi/ethbackend.go

Giulio2002 added 5 commits December 24, 2021 19:06

block proposing

28287c7

standard finalized

40d61ea

mergemock execution

2a33c4c

private chain can now be ran yay

f31df32

perfectioned

bdb44eb

Giulio2002 marked this pull request as draft December 26, 2021 19:02

Giulio2002 added the proof-of-stake label Dec 26, 2021

Giulio2002 added 2 commits December 26, 2021 21:12

polished

2a0020c

more polishing

d4eab6f

Giulio2002 marked this pull request as ready for review December 26, 2021 21:18

Giulio2002 added 5 commits December 27, 2021 00:46

resize PR

5900107

resize PR

230efd8

resize PR

718e856

simplifications

3ad01be

fixed tests

45cfc7e

Giulio2002 requested review from yperbasis, AskAlexSharov and enriavil1 December 27, 2021 02:48

yperbasis mentioned this pull request Dec 27, 2021

[TheMerge] Kintsugi tracking #3078

Closed

12 tasks

yperbasis reviewed Dec 27, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

yperbasis reviewed Dec 27, 2021

View reviewed changes

eth/stagedsync/stage_mining_finish.go Outdated Show resolved Hide resolved

better syncronous communication

71e47db

yperbasis reviewed Dec 28, 2021

View reviewed changes

feeRecipient into preset

107c967

enriavil1 reviewed Dec 28, 2021

View reviewed changes

eth/stagedsync/stage_mining_create_block.go Outdated Show resolved Hide resolved

useExternal

7df70ab

yperbasis reviewed Dec 28, 2021

View reviewed changes

eth/stagedsync/stage_headers.go Show resolved Hide resolved

yperbasis reviewed Dec 28, 2021

View reviewed changes

eth/stagedsync/stage_headers.go Show resolved Hide resolved

todo

964cfdc

yperbasis reviewed Dec 29, 2021

View reviewed changes

eth/stagedsync/stage_mining_create_block.go Outdated Show resolved Hide resolved

yperbasis reviewed Dec 29, 2021

View reviewed changes

ethdb/privateapi/ethbackend.go Outdated Show resolved Hide resolved

fixed comment for forkchoiceUpdateV1

873910a

yperbasis reviewed Dec 29, 2021

View reviewed changes

Giulio2002 added 2 commits December 29, 2021 14:13

smaller

eb6671d

Merge branch 'mining-pos' of https://www.github.com/ledgerwatch/erigon …

3ae5c95

…into mining-pos

Giulio2002 changed the title ~~Added block proposal to Proof-of-stake.~~ Support for syncing on mergemock Dec 29, 2021

Giulio2002 and others added 3 commits December 29, 2021 14:24

smaller

8a55a2d

Revert changes to miner frequency

506736d

Restore useExternalTx

e24b375

yperbasis reviewed Dec 29, 2021

View reviewed changes

ethdb/privateapi/ethbackend.go Outdated Show resolved Hide resolved

yperbasis and others added 2 commits December 29, 2021 14:42

Fix headerLoadFunc

0a89c98

do not reset payloadId

f4526f6

yperbasis reviewed Dec 29, 2021

View reviewed changes

ethdb/privateapi/ethbackend.go Outdated Show resolved Hide resolved

rename

a7e2c3a

yperbasis reviewed Dec 29, 2021

View reviewed changes

ethdb/privateapi/ethbackend.go Outdated Show resolved Hide resolved

extra is empty

08617ba

yperbasis reviewed Dec 29, 2021

View reviewed changes

ethdb/privateapi/ethbackend.go Show resolved Hide resolved

yperbasis approved these changes Dec 29, 2021

View reviewed changes

Giulio2002 merged commit 864b744 into devel Dec 29, 2021

Giulio2002 deleted the mining-pos branch December 29, 2021 16:25

VBulikov mentioned this pull request Feb 5, 2025

Proof of stake #13695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for syncing on mergemock #3174

Support for syncing on mergemock #3174

Giulio2002 commented Dec 26, 2021 •

edited

Loading

Giulio2002 commented Dec 26, 2021

Giulio2002 commented Dec 27, 2021

yperbasis commented Dec 27, 2021

Giulio2002 commented Dec 27, 2021

yperbasis commented Dec 27, 2021

Giulio2002 commented Dec 27, 2021

yperbasis commented Dec 27, 2021

yperbasis commented Dec 27, 2021

yperbasis Dec 28, 2021

Giulio2002 Dec 28, 2021

yperbasis Dec 28, 2021 •

edited

Loading

Giulio2002 Dec 28, 2021

Giulio2002 Dec 28, 2021

yperbasis Dec 29, 2021

Giulio2002 Dec 29, 2021

Giulio2002 Dec 29, 2021

Giulio2002 Dec 29, 2021

yperbasis Dec 29, 2021

yperbasis Dec 29, 2021

yperbasis Dec 29, 2021

Giulio2002 Dec 29, 2021

yperbasis Dec 29, 2021

Giulio2002 Dec 29, 2021

Support for syncing on mergemock #3174

Support for syncing on mergemock #3174

Conversation

Giulio2002 commented Dec 26, 2021 • edited Loading

Things Added

Prerequisite

Things missing before moving to M2 Kinstugi Milestones:

Giulio2002 commented Dec 26, 2021

Giulio2002 commented Dec 27, 2021

yperbasis commented Dec 27, 2021

Giulio2002 commented Dec 27, 2021

yperbasis commented Dec 27, 2021

Giulio2002 commented Dec 27, 2021

yperbasis commented Dec 27, 2021

yperbasis commented Dec 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yperbasis Dec 28, 2021 • edited Loading

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

Choose a reason for hiding this comment

Giulio2002 commented Dec 26, 2021 •

edited

Loading

yperbasis Dec 28, 2021 •

edited

Loading