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: Enable sync with all peers for not local documents #292

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mykola-vrmchk
Copy link
Contributor

@mykola-vrmchk mykola-vrmchk commented Feb 8, 2024

Added a test with two peers (Alice, Bob)

  • both have a shared policy set to false for all requests
  • it fails to replicate the document even with an explicit request .find

Fixed behavior of method .addDocument on synchronizer to send requests for documents we do not create

@acurrieclark
Copy link
Collaborator

acurrieclark commented Feb 8, 2024

This is a really interesting case, and one which needs consideration.

A "request" for a document is essentially performed by creating an empty document and then attempting to sync that empty doc with connected peers. As a share policy of false instructs repo not to explicitly sync with other peers unless they request it, that "request" won't ever be sent. That should be a relatively simple case to fix.

However, let's say you have 2 unconnected peers which both connect to a central server. This server would typically have a share policy of false. If one of those peers has a local document which it doesn't want to share (share policies can be set depending on documentId and peerId), the sync server won't know about it. If the 2nd peer tries to request that document from the server, the current outcome is that the sync server doesn't have it, so responds with an "unavailable" message. If we were to change it so that you could request documents with a share policy of false, this would effectively pass on the request to all connected peers. Now suddenly the sync server has that document which the initial peer didn't want to share with it.

While I imagine receiving an explicitly requested a document from one peer might be expected behaviour, I would imagine that being able to essentially request any document from a peer anywhere on your network might not be.

@mykola-vrmchk
Copy link
Contributor Author

Added test with MessageChannelNetworkAdapter

@acurrieclark
Copy link
Collaborator

acurrieclark commented Feb 8, 2024

Looks like there is an only in there. Would you mind removing it?

@mykola-vrmchk mykola-vrmchk marked this pull request as draft February 8, 2024 16:17
@mykola-vrmchk
Copy link
Contributor Author

  • Looks like there is an only in there. Would you mind removing it?

I added it intentionally, just not to run all tests on CI for now but I can remove it if you insist

@acurrieclark
Copy link
Collaborator

acurrieclark commented Feb 8, 2024

CI won't run the tests at all if there is an only there, sadly

@mykola-vrmchk mykola-vrmchk marked this pull request as ready for review February 9, 2024 15:41
@mykola-vrmchk mykola-vrmchk changed the title test: Add network test with share policy false fix: Enable replication on request with sharePolicy false Feb 9, 2024
@mykola-vrmchk mykola-vrmchk changed the title fix: Enable replication on request with sharePolicy false fix: Enable sync with all peers for not local documents Feb 9, 2024
@@ -90,6 +91,7 @@ export class Repo extends EventEmitter<RepoEvents> {
// Try to load from disk
const loadedDoc = await storageSubsystem.loadDoc(handle.documentId)
if (loadedDoc) {
isLocal = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is another side effect here. Let's say that we have a peer with a sharePolicy of false. If it boots up for the first time and tries to find a document, will this now be immediately synced with all peers?


await bobDoc.whenReady()

assert.equal(bobDoc.isReady(), true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there should be a test here for a 3rd peer, to show it can't find a document from across the network.

eg. alice --> bob --> charlie

Alice creates a document.
Charlie requests it. Show that it doesn't resolve.

Choose a reason for hiding this comment

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

Added.

@pvh
Copy link
Member

pvh commented Apr 4, 2024

I think this is a relatively significant problem -- we're rather cavalier with how we broadcast documentIds at the moment. If we ask someone who doesn't have a documentId if they've got it... they could make a note and just turn around and request it back.

This is a problem solved by the dat / hypercore project with the idea of a discoveryKey (a hash of the document ID which is used to find peers which claim to have a document before challenging them to prove it.)

We should definitely consider this in the next round of sync & auth{n,z} thinking as we upgrade the network protocol. I don't think there's going to be a great solution in the meantime.

await eventPromise(charlieHandle, "unavailable")
})

it("peers continue replicating existing documents after reload with `sharePolicy` false", async () => {

Choose a reason for hiding this comment

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

This test is currently failing

@pvh pvh force-pushed the main branch 2 times, most recently from e61f8e3 to d3d1a7d Compare July 26, 2024 20:13
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