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

chore: merge release/v1.18.0 into master #9597

Merged
merged 172 commits into from
Nov 7, 2022
Merged

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Nov 6, 2022

Related Issues

This was requested by @jennijuju for users wanting to sync the reset calibration testnet with master.

Proposed Changes

Merges the release/v1.18.0 branch into master, updating the build version to 1.19.0-dev.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

arajasek and others added 30 commits September 20, 2022 15:56
* Allow builtin-actors to return a map of methods

* go mod

* Fix tests

* Fix tests, check carefully please
feat: refactor: remove NewestNetworkVersion
* Integrate datacap actor

* Implement datacap actor in chain/builtin
feat: API:  support typed errors over RPC
feat: wdpost: Add ability to only have single partition per msg for partitions with recovery sectors
…-for-beneficiary

fix: state: Return beneficiary info from miner state Info()
@arajasek arajasek requested a review from a team as a code owner November 6, 2022 19:31
@arajasek arajasek force-pushed the asr/merge-release-into-master branch 2 times, most recently from a728636 to 2892ce0 Compare November 6, 2022 19:39
@arajasek arajasek force-pushed the asr/merge-release-into-master branch from 2892ce0 to c0b7343 Compare November 6, 2022 19:40
@@ -1,5 +1,92 @@
# Lotus changelog

# 1.18.0-rc5 / 2022-11-1
Copy link
Member

Choose a reason for hiding this comment

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

not meant to be here ?

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 think it is? Because the new version is v1.19.0-dev, so it's a bit odd if changelog is still 1.17.2?

So i would say merge in 1.18.0-rc5 changelog now, and when we have the final release, update to 1.18.0?

return bs.Get(ctx, c)
return nil, ipld.ErrNotFound{Cid: c}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this technically go to backingBs again?

Copy link
Member

Choose a reason for hiding this comment

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

@@ -56,7 +56,7 @@ var UpgradeOhSnapHeight = abi.ChainEpoch(-18)

var UpgradeSkyrHeight = abi.ChainEpoch(-19)

var UpgradeV17Height = abi.ChainEpoch(99999999999999)
var UpgradeSharkHeight = abi.ChainEpoch(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this consistent with other upgrades in 2k devnets and set this to -20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually? I like starting on the prev version, and upgrading early as we're testing a new nv. After Shark sharks, I would support bumping devnet to starting from v17 by default.

chain/actors/builtin/datacap/actor.go.template Outdated Show resolved Hide resolved
// method names always match the field names in the
// `builtin.Method*` structs (tested in the specs-actors
// tests).
fnName := runtime.FuncForPC(ev.Pointer()).Name()
Copy link
Contributor

Choose a reason for hiding this comment

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

That's mildly scary, but I guess fiine? Would be really nice to have a unit test in lotus making sure we notice when this breaks

Copy link
Member

Choose a reason for hiding this comment

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

Really, we should get rid of this. This is lotus-vm legacy stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have integ tests that break (cuz they depend on the MethodMeta being setup correctly)...which is also the only reason anyone notices when we break this...

Network: network.Version17,
Migration: UpgradeActorsV9,
PreMigrations: []stmgr.PreMigration{{
PreMigration: PreUpgradeActorsV9,
StartWithin: 180,
StartWithin: 240,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have numbers for premigrations? I'm assuming that we do, but want to sanity check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we're looking at roughly 40 mins for the premigration, but we really want this to finish, so 2 hours is the window I'm going with.

@@ -769,6 +771,135 @@ func (m *StateModule) StateMarketStorageDeal(ctx context.Context, dealId abi.Dea
return stmgr.GetStorageDeal(ctx, m.StateManager, dealId, ts)
}

func (a *StateAPI) StateGetAllocationForPendingDeal(ctx context.Context, dealId abi.DealID, tsk types.TipSetKey) (*verifregtypes.Allocation, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something to do now, but looking at this code it really looks like we could make it a lot less duplicated by sprinkling some helpers with generics here

@@ -86,6 +86,7 @@ require (
github.com/filecoin-project/go-bitfield v0.2.4 // indirect
github.com/filecoin-project/go-cbor-util v0.0.1 // indirect
github.com/filecoin-project/go-commp-utils v0.1.3 // indirect
github.com/filecoin-project/go-commp-utils/nonffi v0.0.0-20220905160352-62059082a837 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Not tagged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeee, good catch.

return bs.Get(ctx, c)
return nil, ipld.ErrNotFound{Cid: c}
Copy link
Member

Choose a reason for hiding this comment

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

@@ -25,7 +25,7 @@ func TestRegistration(t *testing.T) {
require.True(t, found)
require.True(t, manifestCid.Defined())

for _, key := range actors.GetBuiltinActorsKeys() {
for _, key := range actors.GetBuiltinActorsKeys(actorstypes.Version8) {
Copy link
Member

Choose a reason for hiding this comment

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

hm. latest version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeee

// method names always match the field names in the
// `builtin.Method*` structs (tested in the specs-actors
// tests).
fnName := runtime.FuncForPC(ev.Pointer()).Name()
Copy link
Member

Choose a reason for hiding this comment

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

Really, we should get rid of this. This is lotus-vm legacy stuff.

epoch := curTs.Height() + 1
minGas := vm.PricelistByEpoch(epoch).OnChainMessage(m.ChainLength())

if err := m.VMMessage().ValidForBlockInclusion(minGas.Total(), build.NewestNetworkVersion); err != nil {
if err := m.VMMessage().ValidForBlockInclusion(minGas.Total(), mp.api.StateNetworkVersion(ctx, epoch)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

So, the old code here was to avoid adding message to the pool that would be invalid very shortly. Is there a good reason to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting dropping the check? I'd be okay with that.

The reason it's changing is because we're getting rid of NewestNetworkVersion, which is an overall terrible variable.

@arajasek arajasek merged commit 65ee059 into master Nov 7, 2022
@arajasek arajasek deleted the asr/merge-release-into-master branch November 7, 2022 20:41
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.

8 participants