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

Improve deal making efficiency & Lotus v1.1.3 update & fixes #705

Merged
merged 16 commits into from
Nov 13, 2020

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Nov 12, 2020

This PR:

  • Update Lotus client & docker image to v1.1.3
  • Leverage a PR included in 1.1.3 I made in Lotus some time ago that allows removing the replace directive we had in go.mod; GO clients will be happier.
  • Leverage new APIs to only calculate once deal size and CommP. This makes Job execution load on the Lotus node be independent of the replication factor. That is if the replication factor was 5, this PR decreases the total cumulative load in the Job execution by 5x in Lotus.
  • The TotalActiveDeals is removed from the miner's index. Unfortunately, this was derived from an API that lists all the sectors of miners to do some extra calculation, but for big miners, this can have a huge output in the API call which isn't paged thus trigger OOMs. Total active deals were nice metrics, but honestly is very easy to fake anyway so not really convincing for decisions anyway. If we come up to think this was important later, we should research/escalate a more efficient way to derive this. Unblocks Consider making Powergate miner index data public textile#397.
  • Make the index raw json output bind-address be configurable, and bind to dynamic free ports to avoid collisions with other running things in the same GH runner.

@jsign jsign added enhancement New feature or request patch labels Nov 12, 2020
@jsign jsign self-assigned this Nov 12, 2020
@jsign jsign changed the title Improve deal making efficiency & Lotus v1.1.3 update Improve deal making efficiency & Lotus v1.1.3 update & fixes Nov 12, 2020
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@jsign jsign requested a review from asutula November 13, 2020 14:58
@jsign jsign marked this pull request as ready for review November 13, 2020 14:58
@jsign jsign added minor and removed patch labels Nov 13, 2020
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Comment on lines +149 to +150
PieceCid: &pieceCid,
PieceSize: pieceSize.Unpadded(),
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 delegate to the client to provide pieceSize and pieceCid. This allows Lotus to avoid calculating these values, and just trusting they're correct. This is important for clients such as FFS, which create multiple deals with the same data.

Comment on lines -185 to +186
dsz, err := lapi.ClientDealSize(ctx, c)
dsz, err := lapi.ClientDealPieceCID(ctx, c)
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 use a new API that calculates both size and PieceCID.

@@ -81,14 +81,14 @@ func (fc *FilCold) Fetch(ctx context.Context, pyCid cid.Cid, piCid *cid.Cid, wad
return ffs.FetchInfo{RetrievedMiner: miner, FundsSpent: fundsSpent}, nil
}

func (fc *FilCold) calculatePieceSize(ctx context.Context, c cid.Cid) (api.DataSize, error) {
func (fc *FilCold) calculateDealPiece(ctx context.Context, c cid.Cid) (abi.PaddedPieceSize, cid.Cid, 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.

I preferred to avoid using api namespace here since is a Lotus package.
Also, I decided using abi.PaddedPieceSize since using uint64 is ambiguous regarding if the size includes padding or not. It's a technical detail that matters in using the right size type for calculating price. For other minor things, we can still use uint64. It's just a type-safety mesaure.

Saying it differently: abi.PaddedPieceSize always indicates padded data which is the real size it will take in the miner's sector.

require.Equal(t, "failed to start deal: cannot propose a deal whose piece size (4096) is greater than sector size (2048)", de.Message)
require.Equal(t, "data doesn't fit in a sector", de.Message)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lotus API changed error string.

@@ -60,5 +61,3 @@ require (
)

replace github.com/dgraph-io/badger/v2 => github.com/dgraph-io/badger/v2 v2.2007.2

replace github.com/filecoin-project/filecoin-ffi => github.com/filecoin-project/filecoin-ffi v0.30.4-0.20200910194244-f640612a1a1f
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gone.

score := 50*faultsScore + 20*powerScore + 20*externalScore + 10*askScore + 1000*float64(miner.ActiveDeals)
score := 50*faultsScore + 20*powerScore + 20*externalScore + 100*askScore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still arbitrary.

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Copy link
Member

@asutula asutula left a comment

Choose a reason for hiding this comment

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

Awesome!

@jsign jsign merged commit fd1029c into master Nov 13, 2020
@jsign jsign deleted the jsign/upandeff branch November 13, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants