wip: adding some implementation of the finalizer, defining needed int… by Psykepro · Pull Request #1481 · 0xPolygon/zkevm-node

Psykepro · 2022-12-22T16:17:33Z

…erfaces by the finalizer from the dbmanager, and implementing the remaining resourses Sub.

Signed-off-by: Nikolay Nedkov nikolai_nedkov@yahoo.com

Related to issue #1478.

What does this PR do?

Adding some of the implementations (still WIP). Mostly to define needed interfaces by the dbManager to unblock @ToniRamirezM. I've added to the proto the TouchedAddresses and generated the proto files.
I got carried away and implemented the remainingResources.Sub method but I think the implementation is very clean. :)

Reviewers

Main reviewers:

…erfaces by the finalizer from the dbmanager, and implementing the remaining resourses Sub. Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

arnaubennassar

Sorry for too many comments saying You can do this in a later PR as here the priority is to define interfaces ASAP. It was to help identifying what needs to be done now and what can be done later... Was lazy so just copy pasted it 🙃
Also not to be done now, but in some parts of the code there already too many identation levels (at least I've counted 5...). As a rule of thumb when this happens it's time to start thinking about splitting into multiple functions

arnaubennassar · 2022-12-22T16:57:31Z

sequencer/worker.go

+	r.remainingBytes -= other.remainingBytes
+	r.remainingGas -= other.remainingGas


This also needs underflow check

It has it in line 112 this check everything. It is first checking every field for underflow before doing any changes.

arnaubennassar · 2022-12-22T16:58:22Z

sequencer/worker.go

+	return nil
+}
+
+func (r *RemainingResources) checkForResourcesOverflow(other RemainingResources) error {


Suggested change

func (r *RemainingResources) checkForResourcesOverflow(other RemainingResources) error {

func (r *RemainingResources) checkForResourcesUnderflow(other RemainingResources) error {

Ops, yep mistake here.

arnaubennassar · 2022-12-22T17:02:26Z

pool/transaction.go

 }

+// SubZkCounters subtract zk counters with passed zk counters (not safe)
+func (zkc *ZkCounters) SubZkCounters(txZkCounters ZkCounters) {


I'm not very happy about this (because for how it used to be, not your changes). Things that IMO could imporve current situation:

Move the struct to state package

Ideally reuse the counters from the proto interface so we don't need to reparse (maybe by wrapping)

ZkCounters => ZKCounters 🙏

Include the underflow logic already in the SubZkCounters method

SubZkCounters => Sub

The only actual request here is the renamings of the functions, otherwise it's just suggestions up to you to implement if you consider them legit

arnaubennassar · 2022-12-22T17:07:11Z

sequencer/finalizer.go

+		GER:                   lastBatch.GlobalExitRoot,
+		initialStateRoot:      lastBatch.StateRoot,
+		intermediaryStateRoot: lastBatch.StateRoot,
+		txs:                   make([]TxTracker, 0, f.maxTxsPerBatch),


Exisitng transactions in the batch should be also loaded from DB. You can do this in a later PR as here the priority is to define interfaces ASAP

This I had in mind just wanted to make quick PR for the interfaces.

arnaubennassar · 2022-12-22T17:08:09Z

sequencer/finalizer.go

-	wipBatch := wipBatch{}
+	wipBatch := wipBatch{
+		GER:                   lastBatch.GlobalExitRoot,
+		initialStateRoot:      lastBatch.StateRoot,


The initial state root is actually yhe state root stored in the previous batch (the last closed batch). You can do this in a later PR as here the priority is to define interfaces ASAP

This I had in mind just wanted to make quick PR for the interfaces.

arnaubennassar · 2022-12-22T17:12:05Z

sequencer/finalizer.go

 			tx := f.worker.GetBestFittingTx(wipBatch.remainingResources)
 			if tx != nil {
+				lenOfTxs := len(wipBatch.txs)
+				isFirstTx := lenOfTxs == 1


Suggested change

isFirstTx := lenOfTxs == 1

isFirstTx := lenOfTxs == 0

At this point, tx is not added yet into wipBatch.txs. If wipBatch.txs == 1 it means that in previous iterations there has been a tx added into the batch, and that tx was actually the first one.

Beyond that we may consider to execute a batch with no txs at all to produce the GER update, and here use always empty hash. We can talk about this later. You can do this in a later PR as here the priority is to define interfaces ASAP

Ops, yep mistake here.. from hurrying :D

arnaubennassar · 2022-12-22T17:13:48Z

sequencer/finalizer.go

+					from, err := types.Sender(types.NewEIP155Signer(processedTx.Tx.ChainId()), &processedTx.Tx)
+					if err != nil {
+						from, err = types.Sender(types.HomesteadSigner{}, &processedTx.Tx)
+					}


We should be able to get the from from the TxTracker. You can do this in a later PR as here the priority is to define interfaces ASAP

So this I guess to be provided by the DbManager to the struct form the database?

arnaubennassar · 2022-12-22T17:17:15Z

sequencer/finalizer.go

+					processBatchRequest.OldAccInputHash = result.NewAccInputHash
+					// We store the processed transaction, add it to the batch and delete from the pool atomically.
+					go f.dbManager.StoreProcessedTxAndDeleteFromPool(ctx, wipBatch.batchNumber, processedTx)
+					go f.worker.UpdateAfterSingleSuccessfulTxExecution(from, result.TouchedAddresses)


Suggested change

go f.worker.UpdateAfterSingleSuccessfulTxExecution(from, result.TouchedAddresses)

f.worker.UpdateAfterSingleSuccessfulTxExecution(from, result.TouchedAddresses)

We decided to do this synchronously. The idea is to compromise a bit of performance but to gain more security (improve "hit ratio")

Yep, sorry mistake here.

arnaubennassar · 2022-12-22T17:24:19Z

sequencer/finalizer.go

+						AccInputHash:  processBatchRequest.OldAccInputHash,
+						StateRoot:     wipBatch.intermediaryStateRoot,
+						LocalExitRoot: processBatchRequest.GlobalExitRoot,
+						Txs:           ethTxs,


I would like to review this to see if we can directly pass wipBatch.txs here. Also it would be nice if we can stop working with types.Transaction in the first place and use the TxTracker for the sequencer and "raw txs" (just the encoded tx as []byte) in the state.

I'm not sure about how feasible is this, but at least is worth exploring

Okay, will check if possible to use everywhere the raw tx.

arnaubennassar · 2022-12-22T17:29:33Z

state/types.go

+	TxData           []byte
+	SequencerAddress common.Address
+	Timestamp        uint64
+	IsFirstTx        bool


I think this is not needed here. This should be used for the GER empty hack, but we're already taking into consideration this in the finalizer.

With that being said, since this is something related to how the executor works, it will make a lot of sense to take care bout this using the isFirstTx, but then we don't need to do it in the finalizer. Also if it's the state who needs to do that, then it also needs to know if the inteeractions is meant to be done in the tx by tx mode or in the full batch mode...

Up to you, essentially you have to either:

Remove IsFirstTx from the struct

Remoe the GER = empty hash from finalizer, and add a tx by tx flag in the process request struct

agnusmor · 2022-12-23T11:48:42Z

sequencer/interfaces.go

@@ -43,11 +47,17 @@ type txManager interface {

 type workerInterface interface {


We need to add also "func (w *Worker) AddTx(tx TxTracker)" function that is used by the Pool Loader to load txs from the Pool to the Worker

This PR is just for the interfaces needed in the dbManager by the Finalizer. But after I get back from the vacation, because you will be still on vacation I can take a look at adding some of the needed in the Worker no probs! :))

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

ToniRamirezM · 2023-01-03T08:56:31Z

sequencer/interfaces.go

 }

+// The dbManager will need to handle the errors inside the functions which don't return error as they will be used async in the other abstractions.
+// Also if dbTx is missing this needs also to be handled ion the dbManager


Suggested change

// Also if dbTx is missing this needs also to be handled ion the dbManager

// Also if dbTx is missing this needs also to be handled in the dbManager

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

* WIP * WIP * WIP

…ng small refactor. Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

wip: adding some implementation of the finalizer, defining needed int…

b19bf97

…erfaces by the finalizer from the dbmanager, and implementing the remaining resourses Sub. Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro added sequencer improvement labels Dec 22, 2022

Psykepro requested review from ToniRamirezM and arnaubennassar December 22, 2022 16:17

Psykepro self-assigned this Dec 22, 2022

Psykepro requested a review from tclemos as a code owner December 22, 2022 16:17

cla-bot bot added the cla-signed label Dec 22, 2022

ToniRamirezM approved these changes Dec 22, 2022

View reviewed changes

arnaubennassar suggested changes Dec 22, 2022

View reviewed changes

arnaubennassar requested a review from agnusmor December 22, 2022 17:32

agnusmor reviewed Dec 23, 2022

View reviewed changes

Psykepro requested review from agnusmor and arnaubennassar December 23, 2022 15:55

Psykepro force-pushed the feature/newSequencer-finalizer branch 2 times, most recently from 37c0052 to a2ecd09 Compare December 23, 2022 16:31

Psykepro requested review from ARR552 and cool-develope as code owners December 23, 2022 16:31

wip:2

ada278d

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro force-pushed the feature/newSequencer-finalizer branch from a2ecd09 to ada278d Compare December 23, 2022 16:54

ToniRamirezM suggested changes Jan 3, 2023

View reviewed changes

fix: fixing comment

6d2d345

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro requested a review from ToniRamirezM January 5, 2023 08:29

wip: #4

27bf5c2

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro force-pushed the feature/newSequencer-finalizer branch 2 times, most recently from 969fa8a to 86a7241 Compare January 9, 2023 12:36

fix: fixing bootstrap method in sequencer

bfa53aa

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro force-pushed the feature/newSequencer-finalizer branch from 86a7241 to bfa53aa Compare January 9, 2023 12:37

fix: fixing log in finalizer.reopenBatch

b3c3ccf

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro force-pushed the feature/newSequencer-finalizer branch from 5cab1b3 to b3c3ccf Compare January 9, 2023 12:39

DBManager WIP (#1512)

201ea32

* WIP * WIP * WIP

ToniRamirezM approved these changes Jan 9, 2023

View reviewed changes

wip: adding implementation for .isCurrBatchAboveLimitWindow', and doi…

ec6ee2e

…ng small refactor. Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro force-pushed the feature/newSequencer-finalizer branch from 975f1d5 to ec6ee2e Compare January 10, 2023 08:25

Psykepro added 3 commits January 10, 2023 13:34

fix: fixing handling of deadlines and events

ff3b27d

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

WIP: fixing logic.

59e6914

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

WIP: fixing logic.

eda9053

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

Psykepro merged commit 59dbd49 into feature/newSequencer Jan 10, 2023

		r.remainingBytes -= other.remainingBytes
		r.remainingGas -= other.remainingGas

	func (r *RemainingResources) checkForResourcesOverflow(other RemainingResources) error {
	func (r *RemainingResources) checkForResourcesUnderflow(other RemainingResources) error {

	go f.worker.UpdateAfterSingleSuccessfulTxExecution(from, result.TouchedAddresses)
	f.worker.UpdateAfterSingleSuccessfulTxExecution(from, result.TouchedAddresses)

		@@ -43,11 +47,17 @@ type txManager interface {

		type workerInterface interface {

	// Also if dbTx is missing this needs also to be handled ion the dbManager
	// Also if dbTx is missing this needs also to be handled in the dbManager

Conversation

Psykepro commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Reviewers

Uh oh!

arnaubennassar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Psykepro Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Psykepro Dec 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToniRamirezM Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Psykepro commented Dec 22, 2022 •

edited

Loading

Psykepro Dec 23, 2022 •

edited

Loading

Psykepro Dec 23, 2022 •

edited

Loading

ToniRamirezM Jan 3, 2023 •

edited

Loading