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

Add SimultaneousTransfers limitation for incoming data transfers from Storage Deals #7030

Closed
jennijuju opened this issue Aug 11, 2021 · 6 comments · Fixed by #7405
Closed
Assignees
Labels
area/data-transfer area/markets/storage area/ux Area: UX impact/test-flakiness Impact: Test Flakiness kind/enhancement Kind: Enhancement P1 P1: Must be resolved
Milestone

Comments

@jennijuju
Copy link
Member

jennijuju commented Aug 11, 2021

A miner node got OOM , due to too many transfers (had around 30+ incoming and not stalled, and 2 outgoing) coinciding with the miner doing wdPoSt, resulting in the miner losing all it's power.

As a Storage Service provider, instead of rejecting the deals, ideally, they can have the ability to open the services to the clients, accept the deals and queue them up for processing in order.

Related: Problem #2 in #6861 (comment)

@jennijuju
Copy link
Member Author

@magik6k suspected this is a graphsync bug @hannahhoward

We should make sure that test TestSimultanenousTransferLimit passes and not flaky after fixing this issue.

@jennijuju jennijuju added the P2 P2: Should be resolved label Aug 11, 2021
@jennijuju jennijuju changed the title SimultaneousTransfers is not limiting the amount of DataTransfer going Add SimultaneousTransfers limitation for incoming Storage Deals Aug 16, 2021
@jennijuju
Copy link
Member Author

Retitled the issue.
Related to the correction in #7066 SimultaneousTransfers is only applying to the retrieval deals currently - However, at the moment, storage provider is more impacted by the incoming storage deal transfers resource wise, therefore, some configuration to control the amount of incoming data tranfer is needed.

@jennijuju jennijuju added P1 P1: Must be resolved and removed P2 P2: Should be resolved area/markets/retrieval labels Aug 16, 2021
@jennijuju jennijuju changed the title Add SimultaneousTransfers limitation for incoming Storage Deals Add SimultaneousTransfers limitation for incoming data transfers from Storage Deals Aug 16, 2021
@jennijuju
Copy link
Member Author

jennijuju commented Aug 16, 2021

from #7031 (comment)

Storage Providers can get a lot of deals from one client (i.e Esturay) simultaneously, that can/will stay in a transferring state for a long time. If the Storage Provider then has a low/normal MaxSimultaneousTransfers=20, and the amount of storage-deals from that client are equal to this, another client (i.e web3.storage) could potentially be blocked from retrieving a file they have previously stored with the storage provider earlier.

a better storage deal data transfer incoming queue up control for storage providers to be able to receive large amount of deal transfer without crashing the node would be essential when the demand increases in the newtork

@jennijuju jennijuju added kind/enhancement Kind: Enhancement and removed kind/bug Kind: Bug labels Aug 16, 2021
@jennijuju jennijuju added this to the v1.11.3 milestone Aug 16, 2021
@Stebalien Stebalien added the impact/test-flakiness Impact: Test Flakiness label Aug 18, 2021
@hannahhoward
Copy link
Contributor

filecoin-project/go-data-transfer#243 is a stop gap solution for this problem. It's not the ideal design but it may be what we can ship for 1.11.3.

@jennijuju jennijuju modified the milestones: v1.11.3, 1.11.4 Sep 2, 2021
@jennijuju jennijuju added the need/community-input Hint: Needs Community Input label Sep 2, 2021
@jennijuju
Copy link
Member Author

filecoin-project/go-data-transfer#243 is a stop gap solution for this problem. It's not the ideal design but it may be what we can ship for 1.11.3.

Let's design and implement an ideal solution after gathering community input - postpone to v1.11.4.

@jennijuju
Copy link
Member Author

gathering community input in #7259

@jennijuju jennijuju removed this from the v1.13.0 milestone Sep 21, 2021
@jennijuju jennijuju modified the milestones: v1.13.1, v1.13.0 Sep 21, 2021
@TippyFlitsUK TippyFlitsUK removed the need/community-input Hint: Needs Community Input label Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/data-transfer area/markets/storage area/ux Area: UX impact/test-flakiness Impact: Test Flakiness kind/enhancement Kind: Enhancement P1 P1: Must be resolved
Projects
None yet
4 participants