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

fix(bitswap): filter interests from received messages #822

Merged
merged 7 commits into from
Jan 31, 2025

Conversation

guillaumemichel
Copy link
Contributor

@guillaumemichel guillaumemichel commented Jan 29, 2025

Fixresource leak by adding a CID to BlockResponseManager only if there a session interested in it.

Follow up to #752
Based on #821

@Wondertan does that address the leak that you are referring to in 70a6503?

cc: @gammazero

@guillaumemichel guillaumemichel requested a review from a team as a code owner January 29, 2025 17:06
@guillaumemichel guillaumemichel changed the title fix(bitswap): filter interests fix(bitswap): filter interests from received messages Jan 29, 2025
Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

See suggested changes to avoid allocation.

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Additionl change

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.48%. Comparing base (b15619e) to head (7464d4a).
Report is 1 commits behind head on main.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
+ Coverage   60.44%   60.48%   +0.04%     
==========================================
  Files         244      244              
  Lines       31095    31120      +25     
==========================================
+ Hits        18794    18823      +29     
+ Misses      10621    10619       -2     
+ Partials     1680     1678       -2     
Files with missing lines Coverage Δ
...l/sessioninterestmanager/sessioninterestmanager.go 100.00% <100.00%> (ø)
...p/client/internal/sessionmanager/sessionmanager.go 96.00% <100.00%> (+0.21%) ⬆️

... and 7 files with indirect coverage changes

Base automatically changed from fix/sessioninterestmanager to main January 31, 2025 16:20
@guillaumemichel guillaumemichel force-pushed the fix/filter-interests branch 2 times, most recently from ee671ff to bf8ed5a Compare January 31, 2025 16:30
@Wondertan
Copy link
Member

@guillaumemichel, I think it does and should also fix the DOS vector.
In my solution, I combined filtering with InterestedSessions to loop less over map in sim, but that's up to you

P.S. This was really an annoying one to figure out.

@Wondertan
Copy link
Member

After this PR, 7adce5f should be the last leak to fix

@gammazero gammazero merged commit 1ca1b50 into main Jan 31, 2025
16 checks passed
@gammazero gammazero deleted the fix/filter-interests branch January 31, 2025 23:24
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.

3 participants