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

Limit maximum number of DAG links to traverse #7427

Merged
merged 2 commits into from
Oct 5, 2021

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Oct 2, 2021

Goals

Set reasonable limits on number of links traversed while calculating commP & doing go-graphsync transfers

Blockers:

Implementation

  • update go-graphsync and go-car and go-fil-markets
  • set a config.MaxTraversalLinks
  • allow it to be overridden via ENV
  • pass to initialization code for markets & go-graphsync

@hannahhoward hannahhoward requested a review from a team as a code owner October 2, 2021 00:24
@jennijuju jennijuju added this to the v1.13.0 milestone Oct 2, 2021
@@ -182,7 +183,7 @@ func StorageClient(lc fx.Lifecycle, h host.Host, dataTransfer dtypes.ClientDataT
marketsRetryParams := smnet.RetryParameters(time.Second, 5*time.Minute, 15, 5)
net := smnet.NewFromLibp2pHost(h, marketsRetryParams)

c, err := storageimpl.NewClient(net, dataTransfer, discovery, deals, scn, accessor, storageimpl.DealPollingInterval(time.Second))
c, err := storageimpl.NewClient(net, dataTransfer, discovery, deals, scn, accessor, storageimpl.DealPollingInterval(time.Second), storageimpl.MaxTraversalLinks(config.MaxTraversalLinks))
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only place that graphsync is initiated? It's the only one I could find but do we have other avenues, like go-datatransfer? Otherwise we're relying on unbounded graphsync

@rvagg
Copy link
Member

rvagg commented Oct 2, 2021

#7428 was intended to be in here as well, there's a couple of SelectiveCar uses locally that aren't deferred via gfm.

@codecov
Copy link

codecov bot commented Oct 5, 2021

Codecov Report

Merging #7427 (6d29552) into feat/update-graphsync-0.10.0 (a4e2fce) will increase coverage by 0.55%.
The diff coverage is 89.28%.

❗ Current head 6d29552 differs from pull request most recent head 97c2cdb. Consider uploading reports for the commit 97c2cdb to get more accurate results
Impacted file tree graph

@@                       Coverage Diff                        @@
##           feat/update-graphsync-0.10.0    #7427      +/-   ##
================================================================
+ Coverage                         38.94%   39.50%   +0.55%     
================================================================
  Files                               616      616              
  Lines                             65297    65319      +22     
================================================================
+ Hits                              25432    25806     +374     
+ Misses                            35462    35010     -452     
- Partials                           4403     4503     +100     
Impacted Files Coverage Δ
node/config/def.go 97.38% <33.33%> (-1.29%) ⬇️
node/impl/client/client.go 40.73% <83.33%> (+0.53%) ⬆️
node/modules/client.go 74.03% <100.00%> (ø)
node/modules/graphsync.go 73.07% <100.00%> (+9.91%) ⬆️
node/modules/storageminer.go 62.93% <100.00%> (+0.43%) ⬆️
markets/loggers/loggers.go 89.28% <0.00%> (-10.72%) ⬇️
extern/sector-storage/sched_resources.go 75.00% <0.00%> (-9.38%) ⬇️
chain/stmgr/call.go 67.87% <0.00%> (-3.64%) ⬇️
node/hello/hello.go 63.63% <0.00%> (-3.41%) ⬇️
extern/sector-storage/sched.go 83.05% <0.00%> (-0.83%) ⬇️
... and 30 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 a4e2fce...97c2cdb. Read the comment docs.

@rvagg
Copy link
Member

rvagg commented Oct 5, 2021

Rebased onto current #7405 which has been rebased to master.

@rvagg
Copy link
Member

rvagg commented Oct 5, 2021

Pulled in go-fil-markets@v1.13.0 which pulls in the newly tagged go-data-transfer@v1.11.0 which therefore incorporates #7441 into this branch.

@rvagg rvagg mentioned this pull request Oct 5, 2021
6 tasks
@rvagg
Copy link
Member

rvagg commented Oct 5, 2021

So, the easiest test of this I think is to start the daemon with LOTUS_MAX_TRAVERSAL_LINKS=10 lotus daemon

Importing a file of decent size (in this case just the built lotus binary) using: lotus client generate-car <input> <output> yields:

ERROR: failed to write CAR to output file: traversal budget exceeded: budget for links reached zero while on path "Links/10/Hash" (link: "bafk2bzacecdzw7j4fcqxwcgad7zb4rehej7uqa4a2263dhoglkcgt465p32qy")

There are some other places that are a bit more difficult to run manual tests on without an operating storage network.

@dirkmc
Copy link
Contributor

dirkmc commented Oct 5, 2021

Version Compatibility Testing

Old: lotus v1.11.3
New: This branch

  • New client / Old server
    • Storage ✅
    • Retrieval ✅
  • New client / New server
    • Storage ✅
    • Retrieval ✅
  • Old client / New server
    • Storage ✅
    • Retrieval ✅

@jennijuju jennijuju merged commit f60692c into feat/update-graphsync-0.10.0 Oct 5, 2021
@jennijuju jennijuju deleted the rvagg/traversal-budget branch October 5, 2021 15:21
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.

4 participants