remove config.EVMNetworkID() #3460

millken · 2022-06-21T08:51:32Z

Description

Type of change

Please delete options that are not relevant.

[] Bug fix (non-breaking change which fixes an issue)
[] New feature (non-breaking change which adds functionality)
Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

codecov · 2022-06-21T11:03:57Z

Codecov Report

Merging #3460 (90d2209) into master (388be8c) will decrease coverage by 75.12%.
The diff coverage is n/a.

❗ Current head 90d2209 differs from pull request most recent head f51f142. Consider uploading reports for the commit f51f142 to get more accurate results

@@            Coverage Diff             @@
##           master   #3460       +/-   ##
==========================================
- Coverage   75.12%       0   -75.13%     
==========================================
  Files         243       0      -243     
  Lines       22514       0    -22514     
==========================================
- Hits        16914       0    -16914     
+ Misses       4677       0     -4677     
+ Partials      923       0      -923

Impacted Files	Coverage Δ
blockchain/block/block_deserializer.go
blockchain/block/blockstore.go
blockchain/blockdao/blockdao.go
blockchain/filedao/filedao.go
blockchain/filedao/filedao_legacy.go
blockchain/filedao/filedao_v2.go
blockchain/filedao/filedao_v2_util.go
blockchain/filedao/staging_buffer.go
blockchain/filedao/testing.go
consensus/scheme/rolldpos/blockproposal.go
... and 233 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 388be8c...f51f142. Read the comment docs.

CoderZhi · 2022-06-21T17:30:11Z

blockchain/block/blockstore.go

-func (in *Store) FromProto(pb *iotextypes.BlockStore) error {
- // TODO: pass the correct EVM network ID at the time of newFileDAOv2()
- blk, err := (&Deserializer{}).SetEvmNetworkID(config.EVMNetworkID()).FromBlockProto(pb.Block)
+func (in *Store) FromProto(evmNetworkID uint32, pb *iotextypes.BlockStore) error {


if a store only contains the data of the same evmNetworkID, then the value should be stored in Store.

Yes，added EVMNetworkID to Store

CoderZhi · 2022-06-21T17:34:19Z

blockchain/block/blockstore.go

- // TODO: pass the correct EVM network ID at the time of newFileDAOv2()
- blk, err := (&Deserializer{}).SetEvmNetworkID(config.EVMNetworkID()).FromBlockProto(pb.Block)
+func (in *Store) FromProto(evmNetworkID uint32, pb *iotextypes.BlockStore) error {
+ blk, err := (&Deserializer{}).SetEvmNetworkID(evmNetworkID).FromBlockProto(pb.Block)


moreover, I don't see the value of creating a new Deserializer in each function call

Liuhaai · 2022-06-21T21:19:16Z

blockchain/filedao/filedao_v2.go

 }
 )

 // newFileDAOv2 creates a new v2 file
-func newFileDAOv2(bottom uint64, cfg db.Config) (*fileDAOv2, error) {
+func newFileDAOv2(bottom uint64, cfg ModuleConfig) (*fileDAOv2, error) {


Get the deserializer from chainbuilder when a new filedao is created .

we can have an offline discussion if you have a better solution here

Liuhaai · 2022-06-21T21:24:25Z

blockchain/filedao/staging_buffer.go

@@ -40,7 +40,7 @@ func (s *stagingBuffer) Put(pos uint64, blkBytes []byte) (bool, error) {
 return false, ErrNotSupported
 }
 blk := &block.Store{}
- if err := blk.Deserialize(blkBytes); err != nil {
+ if err := blk.Deserialize(0, blkBytes); err != nil {


CoderZhi

@millken My last comment about store is wrong. Sorry about that.

CoderZhi · 2022-06-22T02:55:56Z

blockchain/block/blockstore.go

+// NewStore creates a new Store
+func NewStore(evmNetworkID uint32) *Store {
+ return &Store{
+ EVMNetworkID: evmNetworkID,


I am sorry about my last comment. I was mislead by the BAD naming.

How could it be named as store?!

You mean it should be renamed to a more understandable name?

let's leave the Store as it is. Do not change anything else in blockdao and block. Let's focus on deleting config.EVMNetworkID in this PR.
So, ignore my previous comments, your implementation is fine.

blockchain/blockdao/blockdao.go

blockchain/filedao/filedao.go

blockchain/filedao/filedao_v2_util.go

dustinxie · 2022-06-22T05:20:12Z

blockchain/filedao/filedao_v2_util.go

-func extractBlockStore(pbStores *iotextypes.BlockStores, height uint64) (*block.Store, error) {
- info := &block.Store{}
+func extractBlockStore(evmNetworkID uint32, pbStores *iotextypes.BlockStores, height uint64) (*block.Store, error) {
+ info := block.NewStore(evmNetworkID)


add a common deserializer into fileDAOv2, and here:

info, err := fd.deser.FromBlockStoreProto(v)

blockchain/block/blockstore.go

CoderZhi · 2022-06-22T17:16:56Z

chainservice/builder.go

@@ -251,8 +251,8 @@ func (builder *Builder) buildBlockDAO(forTest bool) error {
 dbConfig := builder.cfg.DB
 dbConfig.DbPath = builder.cfg.Chain.ChainDBPath
 dbConfig.CompressLegacy = builder.cfg.Chain.CompressBlock
-
- builder.cs.blockdao = blockdao.NewBlockDAO(indexers, dbConfig)
+ blockDAOConfig, _ := blockdao.CreateBlockdaoConfig(dbConfig, blockdao.EVMNetworkIDOption(builder.cfg.Chain.EVMNetworkID))


In product, EVMNetworkID will always be assigned to initialize blockdao. Therefore, it is not an option, but a parameter. Accordingly, we may define
blockdao.NewBlockDAO(indexers, evmNetworkID, dbConfig) and the data structs XXXConfig are useless.

dustinxie · 2022-06-23T06:08:02Z

blockchain/block/block_deserializer.go

+ receipt.ConvertFromReceiptPb(receiptPb)
+ in.Receipts = append(in.Receipts, receipt)
+ }
+


remove empty line

CoderZhi

the others look fine

CoderZhi · 2022-06-23T06:02:13Z

blockchain/filedao/filedao.go

 // NewFileDAO creates an instance of FileDAO
-func NewFileDAO(cfg db.Config) (FileDAO, error) {
+func NewFileDAO(cfg Config) (FileDAO, error) {


similar to blockdao, filedao's config may not be essential.

CoderZhi · 2022-06-23T06:02:33Z

blockchain/filedao/filedao_legacy.go

@@ -51,7 +50,7 @@ type (
 fileDAOLegacy struct {
 compressBlock bool
 lifecycle lifecycle.Lifecycle
- cfg db.Config
+ cfg Config


CoderZhi · 2022-06-23T06:03:02Z

blockchain/filedao/filedao_v2.go

 }
 )

 // newFileDAOv2 creates a new v2 file
-func newFileDAOv2(bottom uint64, cfg db.Config) (*fileDAOv2, error) {
+func newFileDAOv2(bottom uint64, cfg Config) (*fileDAOv2, error) {


CoderZhi · 2022-06-23T06:07:10Z

blockchain/block/blockstore.go

@@ -42,7 +42,6 @@ func (in *Store) ToProto() *iotextypes.BlockStore {

 // FromProto converts from proto message
 func (in *Store) FromProto(pb *iotextypes.BlockStore) error {
- // TODO: pass the correct EVM network ID at the time of newFileDAOv2()


CoderZhi · 2022-06-23T06:09:18Z

blockchain/block/blockstore.go

@@ -42,7 +42,6 @@ func (in *Store) ToProto() *iotextypes.BlockStore {

 // FromProto converts from proto message
 func (in *Store) FromProto(pb *iotextypes.BlockStore) error {


are this function and the deserialize function still in use? if not, we can delete them

dustinxie · 2022-06-23T06:10:01Z

blockchain/block/blockstore.go

@@ -42,7 +42,6 @@ func (in *Store) ToProto() *iotextypes.BlockStore {

 // FromProto converts from proto message
 func (in *Store) FromProto(pb *iotextypes.BlockStore) error {


still need to make it FromProto(pb *iotextypes.BlockStore, evmid uint32) so we can remove the config.EVMNetworkID() in line 45

or like @CoderZhi commented, remove it (and fix test)

dustinxie · 2022-06-23T06:12:54Z

blockchain/filedao/filedao_legacy.go

@@ -61,11 +60,11 @@ type (
 )


similar to filedao_v2, add deser *Deserializer here

dustinxie · 2022-06-23T06:13:10Z

blockchain/filedao/filedao_legacy.go

 return &fileDAOLegacy{
 compressBlock: cfg.CompressLegacy,
 cfg: cfg,
- kvStore: db.NewBoltDB(cfg),
+ kvStore: db.NewBoltDB(cfg.Config),
 kvStores: cache.NewThreadSafeLruCache(0),


and initialize deser here

dustinxie · 2022-06-23T06:13:42Z

blockchain/filedao/filedao_legacy.go

@@ -263,8 +262,7 @@ func (fd *fileDAOLegacy) body(h hash.Hash256) (*block.Body, error) {
 // block body could be empty
 return &block.Body{}, nil
 }
- // TODO: pass the correct EVM network ID at the time of newFileDAOLegacy()
- return (&block.Deserializer{}).SetEvmNetworkID(config.EVMNetworkID()).DeserializeBody(value)
+ return (&block.Deserializer{}).SetEvmNetworkID(fd.cfg.evmNetworkID).DeserializeBody(value)


so here just fd.deser.DeserializeBody()

dustinxie · 2022-06-23T06:14:52Z

blockchain/filedao/filedao_v2.go

@@ -48,11 +48,13 @@ type (
 hashStore db.CountingIndex // store block hash
 blkStore db.CountingIndex // store raw blocks
 sysStore db.CountingIndex // store transaction log
+ cfg Config


we don't need to add cfg into the struct? the only new thing needed should be deser *block.Deserializer below

dustinxie · 2022-06-23T06:16:06Z

blockchain/filedao/staging_buffer.go

- buffer []*block.Store
+ evmNetworkID uint32
+ size uint64
+ buffer []*block.Store


similar to filedao_v2, add deser *block.Deserializer

dustinxie · 2022-06-23T06:16:25Z

blockchain/filedao/staging_buffer.go

- buffer: make([]*block.Store, size),
+ evmNetworkID: evmNetworkID,
+ size: size,
+ buffer: make([]*block.Store, size),


and initialize deser here

dustinxie · 2022-06-23T06:16:55Z

blockchain/filedao/staging_buffer.go

- blk := &block.Store{}
- if err := blk.Deserialize(blkBytes); err != nil {
+ deser := (&block.Deserializer{}).SetEvmNetworkID(s.evmNetworkID)
+ blk, err := deser.DeserializeBlockStore(blkBytes)


and s.deser.DeserializeBlockStore() here

dustinxie · 2022-06-23T06:19:04Z

blockchain/filedao/filedao_v2_util.go

- }
- return info, nil
+func extractBlockStore(deser *block.Deserializer, pbStores *iotextypes.BlockStores, height uint64) (*block.Store, error) {
+ return deser.FromBlockStoreProto(pbStores.BlockStores[height])


can remove this func, b/c it is just 1 line code

CoderZhi · 2022-06-24T17:52:20Z

blockchain/filedao/filedao.go

+ currFd BaseFileDAO
+ legacyFd FileDAO
+ v2Fd *FileV2Manager // a collection of v2 db files
+ deser *block.Deserializer


not initialized? if so, is it in use? if yes, why the unit test doesn't cover it?

right, can remove, added a quick patch to fix it

CoderZhi · 2022-06-24T17:55:05Z

blockchain/blockchain.go

- bc.dao = blockdao.NewBlockDAO(indexers, cfg.DB)
+ bc.dao = blockdao.NewBlockDAO(indexers, cfg.Chain.EVMNetworkID, cfg.DB)


it seems EVMNetworkID is only for deserializer, therefore, we may just pass deserializer into dao in next PR.

CoderZhi · 2022-06-27T16:24:46Z

fix unit test

Liuhaai · 2022-06-28T04:25:55Z

blockchain/filedao/filedao.go

+ currFd BaseFileDAO
+ legacyFd FileDAO
+ v2Fd *FileV2Manager // a collection of v2 db files
+ deser *block.Deserializer


deser -> blockDeserializer

* upstream/master: (24 commits) account type with zero init nonce (iotexproject#3387) [api] Separate Server and Server Handler (iotexproject#3485) [ioctl] Build hdwallet derive command line into new ioctl (iotexproject#3418) [ioctl] Build hdwallet create command line into new ioctl (iotexproject#3470) [makefile] add go mod tidy (iotexproject#3471) [api] update chain metrics (iotexproject#3484) remove config.EVMNetworkID() (iotexproject#3460) [filedao] remove checkMasterChainDBFile() (iotexproject#3463) [api] add crashlog (iotexproject#3456) [api] Move generateBlockMeta to grpcserver.go (iotexproject#3303) [ioctl] Build action hash command line into new ioctl (iotexproject#3425) [ioctl] Build hdwallet export command line into new ioctl (iotexproject#3423) [ioctl] Refactor nodereward command in new ioctl (iotexproject#3416) [ioctl] Cleanup TestNewNodeDelegateCmd (iotexproject#3421) [blockchain] Remove BoltDBDaoOption (iotexproject#3465) remove InMemDaoOption (iotexproject#3464) [action] add evm london test (iotexproject#3402) [ioctl] create main for ioctl/newcmd (iotexproject#3296) [ioctl] Build block bucket command line into new ioctl (iotexproject#3386) [ioctl] Build hdwallet import command line into new ioctl (iotexproject#3419) ...

millken added 3 commits June 20, 2022 15:16

save work

24376b2

save work

2244ef0

fix issue3452

dc2261a

millken requested a review from a team as a code owner June 21, 2022 08:51

millken added 3 commits June 21, 2022 16:52

Merge branch 'master' into issue3452

7d8043a

fix golint

b62ddcc

fix test

58f8cb0

CoderZhi reviewed Jun 21, 2022

View reviewed changes

Liuhaai requested changes Jun 21, 2022

View reviewed changes

millken added 4 commits June 22, 2022 09:52

only db.config in blockdao moduleconfig

0292443

fix comments

0a432a4

fix comments

0746936

fix comments

d3dfdb4

CoderZhi reviewed Jun 22, 2022

View reviewed changes