Skip to content
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

Store witness #174

Closed

Conversation

anshalshukla
Copy link

No description provided.

Copy link

cla-bot bot commented Mar 5, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

@cffls cffls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Left a few comments in the code.

@@ -61,6 +61,11 @@ func SpawnBlockHashStage(s *StageState, tx kv.RwTx, cfg BlockHashesCfg, ctx cont
return nil
}

// Move 100 block at a time, required for witness generation stage
if headNumber-s.BlockNumber > 100 {
headNumber = s.BlockNumber + 100
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if you can explain why we need to move 100 block at a time for witness stage. Also, is it possible not to use a hard coded number?

zk/stages/stage_witness.go Show resolved Hide resolved
return nil
}

const chunkSize = 100000 // 100KB
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to the top of the file.

Comment on lines +98 to +110
{
ID: stages2.Witness,
Description: "Generates and stores witness of batches",
Forward: func(firstCycle bool, badBlockUnwind bool, s *stages.StageState, u stages.Unwinder, tx kv.RwTx, quiet bool) error {
return SpawnWitnessStage(s, u, tx, ctx, witnessCfg, firstCycle, quiet)
},
Unwind: func(firstCycle bool, u *stages.UnwindState, s *stages.StageState, tx kv.RwTx) error {
return UnwindWitnessStage(u, s, tx, ctx, witnessCfg, firstCycle)
},
Prune: func(firstCycle bool, p *stages.PruneState, tx kv.RwTx) error {
return PruneWitnessStage(p, tx, witnessCfg, ctx, firstCycle)
},
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a flag to cli which users can enable/disable this stage?

Comment on lines 345 to 357
{
// First cycle 1-100
// First batch - 1-104
// Second cycle - witness generation
ID: stages2.Witness,
Description: "Generates and stores witness of batches",
Forward: func(firstCycle bool, badBlockUnwind bool, s *stages.StageState, u stages.Unwinder, tx kv.RwTx, quiet bool) error {
return SpawnWitnessStage(s, u, tx, ctx, witnessCfg, firstCycle, quiet)
},
Unwind: func(firstCycle bool, u *stages.UnwindState, s *stages.StageState, tx kv.RwTx) error {
return UnwindWitnessStage(u, s, tx, ctx, witnessCfg, firstCycle)
},
Prune: func(firstCycle bool, p *stages.PruneState, tx kv.RwTx) error {
return PruneWitnessStage(p, tx, witnessCfg, ctx, firstCycle)
},
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link

cla-bot bot commented Mar 11, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

4 similar comments
Copy link

cla-bot bot commented Mar 14, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

cla-bot bot commented Mar 14, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

cla-bot bot commented Mar 14, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

cla-bot bot commented Mar 14, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

}

// Removes `_chuck_ + chunk_id` that we add while storing it in db
batch := key[0:9]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wil be you can refer(https://github.com/0xPolygonHermez/cdk-erigon/blob/zkevm/zk/hermez_db/utils.go#L18), also I found out there is a function already in the same file which convert bytesToUint64 so I can remove the function, but the logic there is pretty much the same.

Copy link

cla-bot bot commented Mar 14, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

@anshalshukla anshalshukla requested a review from cffls March 14, 2024 12:18
startBlock := rpc.BlockNumberOrHashWithNumber(rpc.BlockNumber(blocks[len(blocks)-1]))
return api.getBlockRangeWitness(ctx, api.db, startBlock, endBlock, false)
hermezDb := hermez_db.NewHermezDbReader(tx)
return hermezDb.GetWitnessByBatchNo(batchNumber)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it fall back to witness calculation if the witness is not found in db, e.g. when witness for a batch is pruned?

Comment on lines 178 to 188
// Only storeNBatches witness in db
// Won't enforce a hard count as batches aren't contiguous
// Cannot use cursor count to get the exact number as batch
// witnesses are stored in chunks (number will vary from batch to batch)
if (b - oldestBatch + 1) > cfg.storeNBatches {
hermezDb.DeleteWitnessByBatchNo(oldestBatch)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using batch number, we can't control the exact max number of witnesses stored in db, because batch number is not contiguous as you mentioned. If we use block number instead, we will have a more accurate control of how many blocks' witnesses we want to keep in the db. Basically, changing storeNBatches to storeNBlocks, and check if the batch number of the lower bound block (currentBlockN - storeNBatches - 1) is stored in db. If so, delete the batch.

Copy link

cla-bot bot commented Mar 18, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

@anshalshukla anshalshukla requested a review from cffls March 18, 2024 05:08
@cffls
Copy link

cffls commented Mar 18, 2024

LGTM, thanks! Could you rebase the branch?

Copy link

cla-bot bot commented Mar 19, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

cla-bot bot commented Mar 19, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

cla-bot bot commented Mar 19, 2024

We require contributors/corporates @anshalshukla to read our Contributor License Agreement, please check the Individual CLA document/Corporate CLA document

Copy link

sonarcloud bot commented Mar 19, 2024

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
3.4% Duplication on New Code

See analysis details on SonarCloud

@revitteth
Copy link
Collaborator

@anshalshukla great contribution, please could you comment something like 'I accept the terms of the CLA' and I will summon the CLA bot :D

@anshalshukla
Copy link
Author

Ah, I thought I had to add it on cla repo. I had create a separate PR for that 😆

@anshalshukla
Copy link
Author

I accept the terms of the CLA

@anshalshukla
Copy link
Author

PR#200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants