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

batch publish deal messages #5309

Merged
merged 7 commits into from
Feb 4, 2021
Merged

batch publish deal messages #5309

merged 7 commits into from
Feb 4, 2021

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jan 8, 2021

Fixes #4878

TODO:


ctx := context.Background()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the changes in this file are just refactoring some common code into functions

PublishMsg: PublishMsgConfig{
PublishPeriod: Duration(time.Hour),
MaxDealsPerMsg: 8,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are placeholder defaults for PublishPeriod and MaxDealsPerMsg, open to suggestions for these values

Copy link

Choose a reason for hiding this comment

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

What is the MaxDealsPerMsg supported by the network ?

Copy link
Contributor

Choose a reason for hiding this comment

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

i have seen up to 30 on the chain already -but this should be proper documented with the max values

@dirkmc dirkmc force-pushed the feat/deal-batch-publish branch from b4dd2ac to d65467d Compare January 14, 2021 09:33
@dirkmc dirkmc force-pushed the feat/deal-batch-publish branch 3 times, most recently from ce6320d to e38ca0c Compare January 14, 2021 15:38
@s0nik42
Copy link

s0nik42 commented Jan 15, 2021

@dirkmc how deal batching will be exposed to miners via lotus ? specific command line or automatically ?

@dirkmc
Copy link
Contributor Author

dirkmc commented Jan 18, 2021

It will be automatic, with two config values:

  • The maximum number of deals to include in a publish message
  • The amount of time to wait for more deals after the first deal arrives, before publishing the deal

@dirkmc dirkmc force-pushed the feat/deal-batch-publish branch 3 times, most recently from b78e389 to 1dbef15 Compare January 25, 2021 10:21
@dirkmc dirkmc force-pushed the feat/deal-batch-publish branch from 1dbef15 to 09c463b Compare January 25, 2021 13:15
@dirkmc dirkmc changed the base branch from master to feat/sealing-handle-batch-publish January 25, 2021 13:16
@dirkmc dirkmc force-pushed the feat/deal-batch-publish branch from 09c463b to c48a069 Compare January 25, 2021 13:18
@dirkmc dirkmc force-pushed the feat/deal-batch-publish branch from c48a069 to adac340 Compare January 25, 2021 13:26
@dirkmc dirkmc marked this pull request as ready for review January 25, 2021 14:31
Base automatically changed from feat/sealing-handle-batch-publish to staging/1.5.x January 28, 2021 18:33
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.

Just a couple of nits, generally looks good.

Would also be great to have commands to see pending deals / force send a publish message

node/config/def.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher.go Outdated Show resolved Hide resolved
markets/storageadapter/dealpublisher.go Show resolved Hide resolved
Base automatically changed from staging/1.5.x to master January 30, 2021 09:57
@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 1, 2021

Would also be great to have commands to see pending deals / force send a publish message

Good idea, I created a ticket: #5491

@jennijuju
Copy link
Member

jennijuju commented Feb 2, 2021

Seems like if I’m sending a message for deals 1,2,3,4,5, with start epoch increasingly and when the time of sending the message has passed the 3’s start epoch, the message will fail for all deals.

will one be able to republish the valid deals, say for deals 4,5?

assuming we add some checks to run the message locally first before sending it out, what will happen if by the time of the message landed on chain, one of the deal expired? @dirkmc @magik6k

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 2, 2021

#5505 implements a fix for the case where the deal proposal start epoch has already elapsed by the time the publish message is about to be sent. The fix is that we will filter this deal out of the publish message.

I'm not sure if we can do much in lotus about the case where the publish message takes a few epochs to land on chain, and in the mean time the start epoch of one of the deals expires. I believe we would need a change in specs-actors to account for this case:

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.

LGTM. Also did some manual testing on my mining setup, and it seems to work well

@magik6k magik6k merged commit ba57179 into master Feb 4, 2021
@magik6k magik6k deleted the feat/deal-batch-publish branch February 4, 2021 00:19
@f8-ptrk
Copy link
Contributor

f8-ptrk commented Feb 4, 2021

can you give a quick overview on the config params until they are properly documented?

@dirkmc
Copy link
Contributor Author

dirkmc commented Feb 4, 2021

@PatrickDeuse I just opened a PR against filecoin docs here: filecoin-project/filecoin-docs#658
Please let me know if that explanation is clear.

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.

Make PublishStorageDeals logic clever
5 participants