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

feat: Add ability for P2P to wait for pushlog by peer #1098

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

fredcarle
Copy link
Collaborator

Relevant issue(s)

Resolves #1097

Description

This PR splits the WaitForPushLogEvent function into two distinct versions. We can now wait for a log create by a specific peer or sent by an intermediary peer.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

integration test

(replace) Describe the tests performed to verify the changes. Provide instructions to reproduce them.

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added feature New feature or request area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. labels Feb 14, 2023
@fredcarle fredcarle added this to the DefraDB v0.5 milestone Feb 14, 2023
@fredcarle fredcarle requested a review from a team February 14, 2023 17:20
@fredcarle fredcarle self-assigned this Feb 14, 2023
@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #1098 (61ad751) into develop (0b18706) will decrease coverage by 0.04%.
The diff coverage is 40.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1098      +/-   ##
===========================================
- Coverage    67.43%   67.39%   -0.04%     
===========================================
  Files          178      178              
  Lines        16618    16635      +17     
===========================================
+ Hits         11206    11211       +5     
- Misses        4465     4476      +11     
- Partials       947      948       +1     
Impacted Files Coverage Δ
node/node.go 63.96% <18.18%> (-2.42%) ⬇️
net/server.go 59.87% <50.00%> (-0.33%) ⬇️
net/client.go 78.57% <100.00%> (+0.52%) ⬆️
net/peer.go 46.04% <100.00%> (+0.21%) ⬆️

Copy link
Contributor

@orpheuslummis orpheuslummis left a comment

Choose a reason for hiding this comment

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

just my preliminary read, flagging some typos :P

net/pb/net.proto Outdated Show resolved Hide resolved
net/server.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Looks good, and good job figuring this one out :)

@@ -270,6 +270,7 @@ func (p *Peer) RegisterNewDocument(
DocKey: &pb.ProtoDocKey{DocKey: dockey},
Cid: &pb.ProtoCid{Cid: c},
SchemaID: []byte(schemaID),
Creator: p.host.ID().String(),
Copy link
Contributor

@AndrewSisley AndrewSisley Feb 14, 2023

Choose a reason for hiding this comment

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

thought: I do worry a bit that we'll end up running into some kind of privacy/security issue by either broadcasting or relying-on fixed host ids like this, a problem for another day (if ever) though I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ID was already used in the P2P exchange so I'm not sure if adding it here make a huge difference? You think it does?

John was also saying we'll need the creator ID at some point in the future anyways for state-proofs if I recall correctly.

Copy link
Contributor

@AndrewSisley AndrewSisley Feb 14, 2023

Choose a reason for hiding this comment

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

The ID was already used in the P2P exchange so I'm not sure if adding it here make a huge difference? You think it does?

I really don't know. Just feels odd to see a trackable, fixed, actor identifier in a (privacy focused?) decentralised system. I also no idea how avoidable such a thing is. But it might be good to avoid spreading its use where we can do so easily (not saying it is easy here, I have no idea).

@fredcarle fredcarle merged commit e2160eb into develop Feb 14, 2023
@fredcarle fredcarle deleted the fredcarle/feat/I1097-wait-by-peer branch February 14, 2023 21:58
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
Relevant issue(s)
Resolves #1097

Description
This PR splits the WaitForPushLogEvent function into two distinct versions. We can now wait for a log create by a specific peer or sent by an intermediary peer.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…1098)

Relevant issue(s)
Resolves sourcenetwork#1097

Description
This PR splits the WaitForPushLogEvent function into two distinct versions. We can now wait for a log create by a specific peer or sent by an intermediary peer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/p2p Related to the p2p networking system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ByPeer version of the WaitForPushLogEvent function
4 participants