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/message) duplicate entries #216

Merged
merged 9 commits into from
Oct 28, 2014
Merged

fix(bitswap/message) duplicate entries #216

merged 9 commits into from
Oct 28, 2014

Conversation

btc
Copy link
Contributor

@btc btc commented Oct 27, 2014

@btc btc added the status/in-progress In progress label Oct 27, 2014
@jbenet
Copy link
Member

jbenet commented Oct 27, 2014

@maybebtc lgtm to use a map to avoid dups, but we still need to send an ordered wantlist (unless we change bitswap protocol to communicate priority another way. a normalized priority value may make sense)

(maybe this doesn't change ordering on messages, just not seeing it at a glance)

Brian Tiger Chow added 7 commits October 27, 2014 21:58
@whyrusleeping noticed this a couple days ago

potential long-term fix: prevent duplicate entries in the wantlist by
using a map/set and iterating over this data structure on export
implementation will be patched to ensure bitswap messages cannot contain
duplicate blocks or keys
A nice invariant for bitswap sessions:

        Senders and receivers can trust that messages do not contain
        duplicate blocks or duplicate keys. Backing the message with a
        map enforces this invariant.

        This comes at the cost of O(n) getters.
so there's one less name to think about
comes at the cost of O(n) Blocks() method.
@btc
Copy link
Contributor Author

btc commented Oct 28, 2014

we still need to send an ordered wantlist

ah missed this detail 💡

maybe this doesn't change ordering on messages, just not seeing it at a glance

yup it does 😞 the range map iteration order is random.

a normalized priority value may make sense

Yeah. Let's add it to the pipeline. For now (alpha), let's keep sorted want list.

💡 How about?

  1. keep the map to detect dupes
  2. keep the slice to preserve iteration order

code:
edf99f5

@jbenet
Copy link
Member

jbenet commented Oct 28, 2014

  1. keep the map to detect dupes
  2. keep the slice to preserve iteration order

S+LGTM! just make sure to only send the ordered list :)

@btc btc added ready to merge and removed status/in-progress In progress labels Oct 28, 2014
jbenet added a commit that referenced this pull request Oct 28, 2014
fix(bitswap/message) duplicate entries
@jbenet jbenet merged commit cb3a8bf into master Oct 28, 2014
@jbenet jbenet deleted the fix/bsmsg-duplicates branch October 28, 2014 22:21
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

2 participants