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

Live edges #2979

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Live edges #2979

merged 1 commit into from
Jun 27, 2017

Conversation

qstokkink
Copy link
Contributor

@qstokkink qstokkink commented Jun 9, 2017

This PR implements live edges for the MultiChain community as discussed in #2894 and #2458 .

Extra functionality:

  • Refactored MultiChainCommunity.cleanup_pending() to be a TaskManager task, so that it gets destroyed when the community is unloaded.
  • Added negative chain sequence numbers. This behaves like Python negative array indices: -1 gives the last element, -2 the last two elements and so on.

@qstokkink qstokkink force-pushed the live_edges branch 14 times, most recently from 89c82b0 to f288303 Compare June 10, 2017 18:39
@qstokkink qstokkink changed the title WIP: Live edges READY: Live edges Jun 12, 2017
@qstokkink qstokkink changed the title READY: Live edges WIP: Live edges Jun 12, 2017
@qstokkink qstokkink force-pushed the live_edges branch 5 times, most recently from bf68a66 to acd9db5 Compare June 19, 2017 14:34
@qstokkink qstokkink changed the title WIP: Live edges READY: Live edges Jun 19, 2017
@qstokkink
Copy link
Contributor Author

@devos50 Do we want an on/off switch for this?

@devos50
Copy link
Contributor

devos50 commented Jun 21, 2017

@qstokkink this would be nice as a config option indeed 👍

@devos50
Copy link
Contributor

devos50 commented Jun 21, 2017

@qstokkink also, want me to review this?

@qstokkink qstokkink changed the title READY: Live edges WIP: Live edges Jun 22, 2017
@qstokkink
Copy link
Contributor Author

@devos50 I have yet to add the config option, but if you have some spare time: yes, please 👍

@qstokkink qstokkink force-pushed the live_edges branch 2 times, most recently from 49112bc to c09af3d Compare June 26, 2017 11:19
@qstokkink qstokkink changed the title WIP: Live edges READY: Live edges Jun 26, 2017
TestTriblerChainCommunity.create_block(other, node, self._create_target(other, node), transaction)

# Get statistics
node_trust = blockingCallFromThread(reactor, node.community.get_trust, other.community.my_member)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply decorate this whole method with an @blockingcallonreactorthread (and use yield self.create_nodes(2))?

if block:
return block.sequence_number
else:
return 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment that this is here so there is at least some chance a node will be visited.

candidate.walk_response(time.time())

# Assert
intro = blockingCallFromThread(reactor, node.community.dispersy_get_introduce_candidate,
Copy link
Contributor

Choose a reason for hiding this comment

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

See my previous comment about @blockingcallonreactorthread

# Arrange
node, = self.create_nodes(1)

called = [False]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can set attributes on callables so you could simply do something like:

def check_live_edge(...):
   ...
   check_live_edge.called = True
check_live_edge.called = False

node.community.take_step()

# Assert
self.assertTrue(called)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not valid I think, called will always be true, regardless whether called[0] is true or false.


# Create a cache, so our introduction response is expected by the node
cache = object.__new__(IntroductionRequestCache)
blockingCallFromThread(reactor, IntroductionRequestCache.__init__, cache,
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, see my comment about the @blockingcallfromreactorthread.

blockingCallFromThread(reactor, node.community.on_introduction_response, [response])

# Assert
self.assertTrue(called[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

See my previous comment about setting callables on attributes.

def get_trust(self, member):
"""
Get the trust for another member.
Currently this is just the amount of MBs downloaded from them.
Copy link
Contributor

Choose a reason for hiding this comment

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

No only the MBs downloaded from them, also the MBs uploaded to them right?


total_trust = sum([self.get_trust(candidate.get_member()) for candidate in eligible])

random_trust_i = randint(0, total_trust - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some comment above this piece of code to explain how the candidate selection works.

def on_introduction_response(self, messages):
super(TrustChainCommunity, self).on_introduction_response(messages)

if self._live_edges_enabled:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just return when self._live_edges_enabled is false.

@devos50 devos50 changed the title READY: Live edges Live edges Jun 27, 2017
@devos50 devos50 merged commit 9985e2d into Tribler:devel Jun 27, 2017
@qstokkink qstokkink deleted the live_edges branch February 20, 2018 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants