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

PriceListByVersion #6766

Merged
merged 15 commits into from
Jul 22, 2021
Merged

PriceListByVersion #6766

merged 15 commits into from
Jul 22, 2021

Conversation

ZenGround0
Copy link
Contributor

Closes #6652

@@ -1054,14 +1054,15 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
return xerrors.Errorf("failed to load base state tree: %w", err)
}

pl := vm.PricelistByEpoch(baseTs.Height())
nv := syncer.sm.GetNtwkVersion(ctx, b.Header.Height)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm changing this from the base tipset's version to the block including the message's version which I think is the desired behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this could break consensus if there is a message below the min price in the nv6.5 epoch, but that's probably fine (we just need to make sure that there isn't one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out. I checked this against mainnet and this code syncs past the calico upgrade (265060 to 265241 so far) so we should be all good.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Looks better than what we had, obviously filecoin-project/go-state-types#30 should land first (probably with the bigger version number changes) (also need the api version bump)

chain/messagepool/check.go Outdated Show resolved Hide resolved
@@ -1054,14 +1054,15 @@ func (syncer *Syncer) checkBlockMessages(ctx context.Context, b *types.FullBlock
return xerrors.Errorf("failed to load base state tree: %w", err)
}

pl := vm.PricelistByEpoch(baseTs.Height())
nv := syncer.sm.GetNtwkVersion(ctx, b.Header.Height)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this could break consensus if there is a message below the min price in the nv6.5 epoch, but that's probably fine (we just need to make sure that there isn't one)

@@ -292,6 +296,18 @@ func (us UpgradeSchedule) Validate() error {
return nil
}

func (us UpgradeSchedule) GetNtwkVersion(e abi.ChainEpoch) (network.Version, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@magik6k I added this so that the mpool could get current network version without having to construct a new stmgr and depend on chainstore. If you think this redundancy is unacceptable I can try a deeper refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine with me

@@ -54,8 +54,8 @@ func VersionForType(nodeType NodeType) (Version, error) {

// semver versions of the rpc api exposed
var (
FullAPIVersion0 = newVer(1, 3, 0)
FullAPIVersion1 = newVer(2, 1, 0)
FullAPIVersion0 = newVer(1, 3, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stebalien @magik6k is there anything else needed to finish bumping the api version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is it.

@ZenGround0 ZenGround0 force-pushed the feat/fix-pricelist branch from 3ead2e2 to 5547aa4 Compare July 21, 2021 19:50
@magik6k
Copy link
Contributor

magik6k commented Jul 22, 2021

(marking as draft until we land filecoin-project/go-state-types#30)

@magik6k magik6k marked this pull request as draft July 22, 2021 13:29
@ZenGround0 ZenGround0 force-pushed the feat/fix-pricelist branch from 5547aa4 to ebbecca Compare July 22, 2021 13:42
@ZenGround0 ZenGround0 marked this pull request as ready for review July 22, 2021 13:42
@ZenGround0 ZenGround0 force-pushed the feat/fix-pricelist branch from ebbecca to c130d2c Compare July 22, 2021 13:50
@ZenGround0 ZenGround0 merged commit e6eb51b into master Jul 22, 2021
@ZenGround0 ZenGround0 deleted the feat/fix-pricelist branch July 22, 2021 14:22
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.

PricelistByEpoch Technical Debt
3 participants