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

markets: OnDealExpiredOrSlashed - get deal by proposal instead of deal ID #5431

Merged
merged 4 commits into from
Aug 27, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jan 26, 2021

Fixes #5414
Depends on #5309 filecoin-project/go-fil-markets#476
Also move common code for OnDealExpiredOrSlashed from storage adapter client and provider into one place.

@dirkmc dirkmc changed the base branch from master to feat/deal-batch-publish January 26, 2021 14:48
Base automatically changed from feat/deal-batch-publish to master February 4, 2021 00:19
markets/storageadapter/ondealexpired.go Outdated Show resolved Hide resolved
}

prop := market.DealProposal(proposal)
res, err := mgr.demAPI.GetCurrentDealInfo(ctx, head.Key().Bytes(), &prop, publishCid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically we can reorg after getting this info

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 can, but in order to fix that we would need to change the way the state change API works.
I suggest we merge this improvement first and then tackle the state change API changes.

@arajasek
Copy link
Contributor

arajasek commented Apr 5, 2021

@dirkmc What's up with this PR?

@dirkmc
Copy link
Contributor Author

dirkmc commented Apr 6, 2021

I think it was just waiting for approval - if we want to get this in let me know and I'll rebase

@jennijuju jennijuju marked this pull request as ready for review April 12, 2021 16:33
@jennijuju
Copy link
Member

@dirkmc we opened it for review - could you please resolve the conflict?

@dirkmc dirkmc requested a review from arajasek as a code owner May 7, 2021 01:26
@jennijuju jennijuju added the P2 P2: Should be resolved label May 17, 2021
@jennijuju jennijuju requested review from nonsense and raulk and removed request for whyrusleeping and Kubuxu June 14, 2021 16:08
@jennijuju jennijuju added this to the v1.11.x milestone Jun 14, 2021
@dirkmc dirkmc force-pushed the refactor/mkts-deal-exp-params branch from 99c8c5e to 9696ea6 Compare June 18, 2021 08:17
@dirkmc
Copy link
Contributor Author

dirkmc commented Jun 18, 2021

@jennijuju I've resolved the conflict now, please review when ready

@BlocksOnAChain BlocksOnAChain added P1 P1: Must be resolved and removed P2 P2: Should be resolved labels Jun 28, 2021
@jennijuju
Copy link
Member

still waiting filecoin-project/go-fil-markets#476 and will be reviewed by the ignite team soon.

@jacobheun jacobheun added the team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs label Jul 27, 2021
@jennijuju jennijuju modified the milestones: 1.11.1 , v1.11.2 Jul 27, 2021
@Stebalien Stebalien marked this pull request as draft July 30, 2021 03:55
@dirkmc dirkmc force-pushed the refactor/mkts-deal-exp-params branch from 9696ea6 to a1baea7 Compare August 4, 2021 12:06
@raulk
Copy link
Member

raulk commented Aug 16, 2021

Here's a start for a test. Unfortunately the sector start and end are set too far out in the future (epoch 23k), so it takes a long while to run. What worried me is that despite the expiry epoch elapsing, the deal did not appear to be set as "expired" on the client side. I might have a bug in my test, or there's another problem here. Abstaining from merging until we get to the bottom of this.

itests/deal_expiry_test.go:

func TestDealExpiry(t *testing.T) {
	kit.QuietMiningLogs()

	// reset minimum deal duration to 0, so we can make very short-lived deals.
	// NOTE: this will need updating with every new specs-actors version.
	market2.DealMinDuration = 0
	market3.DealMinDuration = 0
	market4.DealMinDuration = 0
	market5.DealMinDuration = 0

	ctx := context.Background()

	var (
		client kit.TestFullNode
		miner1 kit.TestMiner
	)

	ens := kit.NewEnsemble(t, kit.MockProofs())
	ens.FullNode(&client)
	ens.Miner(&miner1, &client, kit.WithAllSubsystems())
	bm := ens.Start().InterconnectAll().BeginMining(50 * time.Millisecond)

	dh := kit.NewDealHarness(t, &client, &miner1, &miner1)

	client.WaitTillChain(ctx, kit.HeightAtLeast(5))

	_, _, _ = dh.MakeOnlineDeal(ctx, kit.MakeFullDealParams{
		Rseed:             0,
		FastRet:           true,
		StartEpoch:        0,
		MinBlocksDuration: 50,
	})

	go func() {
		ch, _ := client.ChainNotify(ctx)
		for range ch {
			bm[0].InjectNulls(10)
		}
	}()

	for {
		// sectors, err := miner.SectorsList(ctx)
		// require.NoError(t, err)

		minerDeals, err := miner1.DealsList(ctx)
		require.NoError(t, err)

		for _, d := range minerDeals {
			spew.Dump(d)
		}

		ts, err := client.ChainHead(ctx)
		require.NoError(t, err)

		fmt.Println(ts.Height())

		deals, err := client.ClientListDeals(ctx)
		require.NoError(t, err)

		fmt.Println(storagemarket.DealStates[deals[0].State])
		time.Sleep(1 * time.Second)

		fmt.Printf("%+v\n", deals[0])
	}
}

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

See comment above.

@jennijuju jennijuju added the kind/enhancement Kind: Enhancement label Aug 17, 2021
@jennijuju jennijuju added kind/bug Kind: Bug and removed kind/enhancement Kind: Enhancement labels Aug 17, 2021
@jennijuju jennijuju modified the milestones: v1.11.2, v1.11.3 Aug 17, 2021
@raulk raulk added P2 P2: Should be resolved and removed P1 P1: Must be resolved labels Aug 20, 2021
@dirkmc dirkmc self-assigned this Aug 25, 2021
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

We would benefit from two more tests to verify:

  • behaviour on miner-initiated sector termination
  • behaviour on protocol-initiated sector termination (aka. slashing)

But happy merging this as-is, as it's already strictly better than what we have in master. @dirkmc should we open issues to track the remaining tests?

@dirkmc
Copy link
Contributor Author

dirkmc commented Aug 27, 2021

Created issues to track

@dirkmc dirkmc force-pushed the refactor/mkts-deal-exp-params branch from 78b9929 to 77a1977 Compare August 27, 2021 07:05
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #5431 (ac51b8e) into master (f33c02e) will increase coverage by 0.04%.
The diff coverage is 54.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5431      +/-   ##
==========================================
+ Coverage   39.04%   39.09%   +0.04%     
==========================================
  Files         607      608       +1     
  Lines       64565    64542      -23     
==========================================
+ Hits        25212    25230      +18     
+ Misses      34963    34929      -34     
+ Partials     4390     4383       -7     
Impacted Files Coverage Δ
node/modules/storageminer.go 62.13% <0.00%> (ø)
markets/storageadapter/ondealexpired.go 43.39% <43.39%> (ø)
chain/events/events_called.go 81.32% <71.42%> (ø)
itests/kit/deals.go 87.95% <100.00%> (+0.14%) ⬆️
markets/storageadapter/client.go 49.68% <100.00%> (+7.18%) ⬆️
markets/storageadapter/provider.go 49.10% <100.00%> (+6.86%) ⬆️
extern/sector-storage/sched_worker.go 76.53% <0.00%> (-1.45%) ⬇️
chain/vm/vm.go 59.47% <0.00%> (-1.12%) ⬇️
storage/wdpost_changehandler.go 97.64% <0.00%> (-0.95%) ⬇️
chain/sync_manager.go 66.77% <0.00%> (-0.63%) ⬇️
... 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 f33c02e...ac51b8e. Read the comment docs.

@raulk raulk merged commit a8bd8d1 into master Aug 27, 2021
@raulk raulk deleted the refactor/mkts-deal-exp-params branch August 27, 2021 08:26
magik6k added a commit that referenced this pull request Aug 30, 2021
…nges

revert changes to OnDealExpiredOrChanged in #5431 #7201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Kind: Bug P2 P2: Should be resolved team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lookup current deal ID in OnDealExpiredOrSlashed
7 participants