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

[ipfs/go-bitswap] Remove DONT_HAVE when we send a cancel. #124

Open
Stebalien opened this issue Feb 8, 2020 · 7 comments
Open

[ipfs/go-bitswap] Remove DONT_HAVE when we send a cancel. #124

Stebalien opened this issue Feb 8, 2020 · 7 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@Stebalien
Copy link
Member

If we send a cancel to a peer (because, e.g., the request for that block was canceled), we should remove any "don't have" information with respect to that peer. Otherwise, if the peer obtains the block and we suddenly decide that we care about it again, we'll fail to ask them for it.

This can actually happen when, e.g., seeking through a video that multiple peers are downloading at the same time.

@Stebalien Stebalien added the kind/bug A bug in existing code (including security flaws) label Feb 8, 2020
@Stebalien
Copy link
Member Author

This is actually a more general issue. We should only remember "DONT_HAVEs" for a peer when those CIDs are in the peer's wantlist. This issue can also arise when we disconnect and reconnect to a peer.

It would be best if we could store this information inside our copy of the wantlists we send to peers to ensure it stays up-to-date.

@Stebalien
Copy link
Member Author

Actually, do we even need to record "don't have"s? IIRC, we don't remove things from wantlists until we stop wanting the block.

@dirkmc
Copy link
Contributor

dirkmc commented Feb 10, 2020

For each CID that is wanted by any session, we keep a "block presence" table of which peers have sent us a HAVE or DONT_HAVE

  • CID1
    • peer A: DONT_HAVE
    • peer B: HAVE
  • CID2
    • peer A: HAVE
    • peer C: DONT_HAVE

We use this information when deciding which peer to send a want to. Note that this table is shared across all active sessions.
Separately (in the PeerManager) we keep track of each want-have and want-block we've sent to each peer.

When the session shuts down, we remove all CIDs that were wanted by that session (and not wanted by any other session) from the "block presence" table, and send cancels to peers for those CIDs.

When we receive a block we send a cancel to all peers. We don't need the HAVE / DONT_HAVE information any more (although as currently implemented we don't remove it from the "block presence" table).

Is there another scenario in which we send a cancel to a peer?

@Stebalien
Copy link
Member Author

Is there another scenario in which we send a cancel to a peer?

When we cancel a want for a block (without canceling the rest of the session). That is, we can cancel a mySession.GetBlock.

I don't think we usually (or ever?) do this from go-ipfs at the moment but we will if users start using it as a library (and, e.g., start seeking around files).

@dirkmc
Copy link
Contributor

dirkmc commented Feb 10, 2020

When we cancel a want for a block (without canceling the rest of the session). That is, we can cancel a mySession.GetBlock

It looks like in that case we don't actually send out a cancel to the peer (and didn't in the old Bitswap either).

We should send a cancel when the user cancels a call to GetBlock(). It's a little tricky because we need to make sure that no other session wants the same block before we send a cancel, but we can check that with the SessionInterestManager.

As you point out we should also make sure to sync the BlockPresenceManager when there is a disconnect.

@Stebalien
Copy link
Member Author

Ah... could you file a bug for that as well?

@dirkmc
Copy link
Contributor

dirkmc commented Feb 10, 2020

Filed a bug at ipfs/go-bitswap#253

@Jorropo Jorropo changed the title Remove DONT_HAVE when we send a cancel. [ipfs/go-bitswap] Remove DONT_HAVE when we send a cancel. Jan 27, 2023
@Jorropo Jorropo transferred this issue from ipfs/go-bitswap Jan 27, 2023
@guseggert guseggert reopened this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

No branches or pull requests

3 participants