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

unbound feature and version/height #2768

Merged
merged 15 commits into from
Sep 22, 2021
Merged

Conversation

maswalker
Copy link
Contributor

@maswalker maswalker requested a review from a team as a code owner August 23, 2021 04:20
@todo
Copy link

todo bot commented Aug 23, 2021

define features name here instead of hardfork

// TODO: define features name here instead of hardfork
FEATURE_PACIFIC = "pacific"
)
// Default contains the default genesis config
var Default = defaultConfig()


This comment was generated by todo based on a TODO comment in 5744cbc in #2768. cc @maswalker.

@@ -134,6 +194,10 @@ type (
Rewarding `yaml:"rewarding"`
Staking `yaml:"staking"`
}
HardFork struct {
Copy link
Member

Choose a reason for hiding this comment

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

add // Hardfork
pls run make fmt, make lint before submitting PR

@@ -134,6 +194,10 @@ type (
Rewarding `yaml:"rewarding"`
Staking `yaml:"staking"`
}
HardFork struct {
Height uint64 `yaml:"height"`
EnableFeatures []string `yaml:"enableFeatures"`
Copy link
Member

Choose a reason for hiding this comment

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

EnabledFeatures

}
for _, v := range hardForks {
for _, feature := range v.EnableFeatures {
_, ok := g.FeatureHeightMap[feature]
Copy link
Member

Choose a reason for hiding this comment

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

no need to check if feature exist
same feature cannot appear in 2 hardforks

return ok && ht >= height
}

func (g *Blockchain) SupportPacific(height uint64) bool {
Copy link
Member

@dustinxie dustinxie Aug 23, 2021

Choose a reason for hiding this comment

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

should be EnabledFeature(feature string, height uint64) bool

@codecov
Copy link

codecov bot commented Sep 7, 2021

Codecov Report

Merging #2768 (1424fa9) into master (c1cea44) will decrease coverage by 0.17%.
The diff coverage is 42.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2768      +/-   ##
==========================================
- Coverage   58.06%   57.88%   -0.18%     
==========================================
  Files         245      245              
  Lines       22363    22437      +74     
==========================================
+ Hits        12985    12988       +3     
- Misses       7729     7799      +70     
- Partials     1649     1650       +1     
Impacted Files Coverage Δ
action/protocol/context.go 36.36% <0.00%> (-63.64%) ⬇️
action/protocol/poll/consortium.go 0.00% <ø> (ø)
action/protocol/poll/staking_command.go 9.19% <0.00%> (ø)
action/protocol/staking/read_state.go 5.47% <0.00%> (ø)
action/protocol/execution/evm/evm.go 47.18% <50.00%> (+0.01%) ⬆️
action/protocol/poll/slasher.go 47.01% <60.00%> (+0.13%) ⬆️
action/protocol/account/transfer.go 62.35% <75.00%> (ø)
action/protocol/poll/staking_committee.go 39.01% <80.00%> (ø)
action/protocol/poll/util.go 51.80% <85.71%> (ø)
action/protocol/staking/handlers.go 64.44% <89.28%> (-0.35%) ⬇️
... and 9 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 c1cea44...1424fa9. Read the comment docs.

FeatureWithHeightCtx struct {
GetUnproductiveDelegates CheckFunc
EnableSMStorage CheckFunc
ReadStateFromDB CheckFunc
Copy link
Member

@dustinxie dustinxie Sep 7, 2021

Choose a reason for hiding this comment

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

we could move these 3 into FeatureCtx?
so no need for an extra FeatureWithHeightCtx

Copy link
Member

Choose a reason for hiding this comment

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

also, EnableSMStorage is the same flag as ReadStateFromDB, combine to 1

RefundAllDeposit: g.IsPacific(height),
AddChainIDToConfig: g.IsIceland(height),
UseV2Storage: g.IsGreenland(height),
CheckUnstaked: g.IsGreenland(height),
Copy link
Member

Choose a reason for hiding this comment

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

CannotUnstakeAgain

CheckUnstaked: g.IsGreenland(height),
SkipStakingIndexer: !g.IsFairbank(height),
ReturnFetchError: !g.IsGreenland(height),
CanNotTranferToSelf: g.IsHawaii(height),
Copy link
Member

Choose a reason for hiding this comment

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

Cannot instead of CanNot

SkipStakingIndexer: !g.IsFairbank(height),
ReturnFetchError: !g.IsGreenland(height),
CanNotTranferToSelf: g.IsHawaii(height),
PostFairbankMigration: g.IsFbkMigration(height),
Copy link
Member

Choose a reason for hiding this comment

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

NewStakingReceiptFormat

FixGetHashFnHeight: g.IsHawaii(height),
UsePendingNonceOption: g.IsHawaii(height),
AsyncContractTrie: g.IsGreenland(height),
StoreOutOfGasToReceipt: !g.IsGreenland(height),
Copy link
Member

Choose a reason for hiding this comment

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

AddOutOfGasToTransactionLog

UsePendingNonceOption: g.IsHawaii(height),
AsyncContractTrie: g.IsGreenland(height),
StoreOutOfGasToReceipt: !g.IsGreenland(height),
RefundAllDeposit: g.IsPacific(height),
Copy link
Member

Choose a reason for hiding this comment

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

RefundAllDeposit is the same fix as DepositGasLast
rename to FixDoubleChargeGas

@@ -70,6 +69,7 @@ func (sc *stakingCommand) Start(ctx context.Context, sr protocol.StateReader) (i
}

func (sc *stakingCommand) CreatePreStates(ctx context.Context, sm protocol.StateManager) error {
ctx = protocol.WithFeatureCtx(protocol.WithFeatureWithHeightCtx(ctx))
Copy link
Member

@dustinxie dustinxie Sep 8, 2021

Choose a reason for hiding this comment

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

is this needed?
inside useV2, it will add protocol.WithFeatureWithHeightCtx() into ctx?

@@ -148,6 +148,7 @@ func (sc *stakingCommand) NextCandidates(ctx context.Context, sr protocol.StateR
}

func (sc *stakingCommand) ReadState(ctx context.Context, sr protocol.StateReader, method []byte, args ...[]byte) ([]byte, uint64, error) {
ctx = protocol.WithFeatureCtx(protocol.WithFeatureWithHeightCtx(ctx))
Copy link
Member

@dustinxie dustinxie Sep 8, 2021

Choose a reason for hiding this comment

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

is this needed? see above

@@ -247,22 +251,27 @@ func (sc *stakingCommittee) CalculateUnproductiveDelegates(
}

func (sc *stakingCommittee) Delegates(ctx context.Context, sr protocol.StateReader) (state.CandidateList, error) {
ctx = protocol.WithFeatureWithHeightCtx(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to find a common place to add WithFeatureWithHeightCtx,
instead of doing it for every API?

@@ -28,7 +28,7 @@ const TransferSizeLimit = 32 * 1024
func (p *Protocol) handleTransfer(ctx context.Context, act action.Action, sm protocol.StateManager) (*action.Receipt, error) {
actionCtx := protocol.MustGetActionCtx(ctx)
blkCtx := protocol.MustGetBlockCtx(ctx)
DepositGasLast := protocol.MustGetFeatureCtx(ctx).DepositGasLast
FixDoubleChargeGas := protocol.MustGetFeatureCtx(ctx).FixDoubleChargeGas
Copy link
Member

Choose a reason for hiding this comment

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

fixDoubleChargeGas

@@ -59,6 +59,7 @@ func TestProtocol_HandleTransfer(t *testing.T) {
config.Default.Genesis,
)
ctx := protocol.WithBlockCtx(chainCtx, protocol.BlockCtx{})
ctx = protocol.WithFeatureCtx(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

you can make test local to make sure all test pass, instead of pushing/fixing multiple times

@maswalker maswalker merged commit 0459a1f into iotexproject:master Sep 22, 2021
millken added a commit that referenced this pull request Oct 8, 2021
* saving, switch to linux

* tracing grpc api

* saving, switch to linux

* tracing grpc api

* update tracer.go for test

* small change

* fix comments

* Squashed commit of the following:

commit 77a5a15
Author: dustinxie <dahuaxie@gmail.com>
Date:   Tue Oct 5 23:34:52 2021 -0700

    [api] add cache for ReadContract/State() (#2827)

commit 7d9bf96
Author: Haaai <55118568+Liuhaai@users.noreply.github.com>
Date:   Tue Oct 5 17:31:37 2021 -0700

    bump Go to 1.17 (#2784)

commit 09ef379
Author: dustinxie <dahuaxie@gmail.com>
Date:   Tue Oct 5 16:46:49 2021 -0700

    [ioctl] display chainID and encoding (#2820)

commit c2eab8d
Author: Haaai <55118568+Liuhaai@users.noreply.github.com>
Date:   Tue Oct 5 15:45:45 2021 -0700

    fix ioctl accountDelete test (#2825)

commit 9489b57
Author: dustinxie <dahuaxie@gmail.com>
Date:   Tue Oct 5 15:14:31 2021 -0700

    remove unnecessary WithFeatureCtx() (#2823)

commit 3933ae9
Author: dayuanc <34013965+dayuanc@users.noreply.github.com>
Date:   Tue Oct 5 07:40:32 2021 -0700

    limit the pagination size for all API calls to 5000 -- 2781 (#2800)

    Co-authored-by: dayuanc <wentaicui@WENTAIs-MacBook-Pro.local>
    Co-authored-by: CoderZhi <thecoderzhi@gmail.com>
    Co-authored-by: Raullen Chai <raullenchai@gmail.com>

commit 88d273d
Author: dayuanc <34013965+dayuanc@users.noreply.github.com>
Date:   Mon Oct 4 10:51:54 2021 -0700

    Add unit tests to cover the functions in action/signedaction.go (#2824)

commit ebe895c
Author: dustinxie <dahuaxie@gmail.com>
Date:   Thu Sep 30 15:32:52 2021 -0700

    [reward] extend foundation bonus (#2785)

commit baa0a92
Author: CoderZhi <thecoderzhi@gmail.com>
Date:   Tue Sep 28 14:09:44 2021 -0700

    Fix codecov (#2813)

commit 84f06d3
Author: dayuanc <34013965+dayuanc@users.noreply.github.com>
Date:   Tue Sep 28 12:17:47 2021 -0700

    reduce unnecessary logs for mainnet (#2808)

    * downgrade some unnecessary error and info logs

    * fix comments

    Co-authored-by: Raullen Chai <raullenchai@gmail.com>

commit a245185
Author: dustinxie <dahuaxie@gmail.com>
Date:   Mon Sep 27 15:27:59 2021 -0700

    [evm] panic in AccessList API (#2816)

commit 8eec1b7
Author: dustinxie <dahuaxie@gmail.com>
Date:   Mon Sep 27 11:01:34 2021 -0700

    set Jutland to activate at 10-11-2021 3pm PDT (#2812)

    Co-authored-by: Raullen Chai <raullenchai@gmail.com>

commit e89be88
Author: CoderZhi <thecoderzhi@gmail.com>
Date:   Sat Sep 25 22:51:54 2021 -0700

    set target height (#2807)

    Co-authored-by: dustinxie <dahuaxie@gmail.com>

commit dba4993
Author: CoderZhi <thecoderzhi@gmail.com>
Date:   Thu Sep 23 11:44:26 2021 -0700

    change commit block failure log level to error (#2810)

commit 0459a1f
Author: mas walker <handsome.void@gmail.com>
Date:   Wed Sep 22 20:18:23 2021 +0800

    unbound feature and version/height (#2768)

    * unbound feature and version/height

    Co-authored-by: Raullen Chai <raullenchai@gmail.com>

commit c1cea44
Author: millken <millken@gmail.com>
Date:   Tue Sep 21 06:31:57 2021 +0800

    update circleci config, using golang 1.16.6 #2789 (#2791)

    Co-authored-by: dustinxie <dahuaxie@gmail.com>

commit f245109
Author: Dustin Xie <dahuaxie@gmail.com>
Date:   Mon Sep 20 09:54:38 2021 -0700

    [evm] enable opCall fix at Jutland height

commit 3eea5b7
Author: Dustin Xie <dahuaxie@gmail.com>
Date:   Mon Sep 20 12:11:36 2021 -0700

    [evm] fix datacopy.json for TestIstanbul

commit 0641db0
Author: CoderZhi <thecoderzhi@gmail.com>
Date:   Fri Sep 17 14:37:28 2021 -0700

    [evm] fix snapshot bug (#2802)

    * fix snapshot bug

    * add unit test

    Co-authored-by: Raullen Chai <raullenchai@gmail.com>

commit e0683e8
Author: Haaai <55118568+Liuhaai@users.noreply.github.com>
Date:   Sat Sep 18 01:34:12 2021 +0800

    add datacopy contract test (#2788)

    * add datacopy test

    * modify contract

    * remove debug log

    * update testdata

    * add attack bytecode

    * update datacopy.json

    * correct code format

    * remove printStore() in datacopy.sol

    * update contract test data

    Co-authored-by: dustinxie <dahuaxie@gmail.com>

commit 44e0a68
Author: Haaai <55118568+Liuhaai@users.noreply.github.com>
Date:   Fri Sep 17 15:31:29 2021 +0800

    Allow ioctl to show the list of actions for an account (#2750)

    * [ioctl] allow ioctl to show the list of actions for an account

commit d10dabe
Author: Haaai <55118568+Liuhaai@users.noreply.github.com>
Date:   Fri Sep 17 12:08:33 2021 +0800

    [api] fix gas estimation calc bug (#2786)

    * fixed gas estimation bug in api.go

* Tracing (#2)

* [reward] extend foundation bonus (#2785)

* Add unit tests to cover the functions in action/signedaction.go (#2824)

* limit the pagination size for all API calls to 5000 -- 2781 (#2800)

Co-authored-by: dayuanc <wentaicui@WENTAIs-MacBook-Pro.local>
Co-authored-by: CoderZhi <thecoderzhi@gmail.com>
Co-authored-by: Raullen Chai <raullenchai@gmail.com>

* remove unnecessary WithFeatureCtx() (#2823)

* fix ioctl accountDelete test (#2825)

* [ioctl] display chainID and encoding (#2820)

* bump Go to 1.17 (#2784)

* [api] add cache for ReadContract/State() (#2827)

Co-authored-by: dustinxie <dahuaxie@gmail.com>
Co-authored-by: dayuanc <34013965+dayuanc@users.noreply.github.com>
Co-authored-by: dayuanc <wentaicui@WENTAIs-MacBook-Pro.local>
Co-authored-by: CoderZhi <thecoderzhi@gmail.com>
Co-authored-by: Raullen Chai <raullenchai@gmail.com>
Co-authored-by: Haaai <55118568+Liuhaai@users.noreply.github.com>

Co-authored-by: Raullen Chai <raullenchai@gmail.com>
Co-authored-by: dustinxie <dahuaxie@gmail.com>
Co-authored-by: dayuanc <34013965+dayuanc@users.noreply.github.com>
Co-authored-by: dayuanc <wentaicui@WENTAIs-MacBook-Pro.local>
Co-authored-by: CoderZhi <thecoderzhi@gmail.com>
Co-authored-by: Haaai <55118568+Liuhaai@users.noreply.github.com>
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.

4 participants