MultiChain community

[snorberhuis]

This is a pull request to merge the MultiChain community into devel.

[synctext]

Starting to look good. Please collapse into 1 commit and fix the tests:

    Test Result (20 failures / +20)
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_add_block
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_add_block
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_add_two_blocks
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_add_two_blocks
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_contains_block_id_negative
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_contains_block_id_negative
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_contains_block_id_positive
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_contains_block_id_positive
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_contains_signature_pk_false
Tribler.Test.test_doubleentry_persistence.TestPersistence.test_contains_signature_pk_false
Show all failed tests >>>

[NielsZeilemaker]

You're attempting to store, and update and message which is destined for candidate.
I would switch to _forward(

[snorberhuis]

The _forward method is private and gives a pylint error. There are two solutions:

• Use store_update_forward([message], false, false, true). But in the doc of the method it is said that false on store and update should be done mostly for debugging purposes.
• Change the method to public. It is accessed publicly in a lot of other places aswell, so this might be a good change overall.

What do you prefer?

[NielsZeilemaker]

Do the first one, but create an issue in the dispersy repo stating that these methods should be public.

[synctext]

Many thanks for those detailed comments Niels!

[NielsZeilemaker]

This method is just plain lazy. If you're inserting binary into the database wrap it into a buffer object, if not don't.
You should be able to know the datatypes beforehand.

[snorberhuis]

Do you object to the use of mapping this function to the list of data to be inserted into the database? I could transform it manually at the point where I have that list.

Or do you object to the laziness of the inner workings of the method?
I can change it so that it checks the type and handles more accordingly for binary.

[NielsZeilemaker]

You're encoding a binary string into base64 and then storing it as a unicode string to get around the asserts of dispersy regarding not accepting strings. Just use buffer object in the appropriate places.

And explicitly convert some columns into buffer and not use a method like this.

[NielsZeilemaker]

Btw, this all seems familiar to DoubleMemberAuthentication, which is implemented in Dispersy.
Have a look at https://github.com/Tribler/dispersy/blob/devel/tests/debugcommunity/community.py#L63
The allow_signature_func is a callback when the first message is received, you can then accept this message or modify it. If you accept a message you will sign it for the second time, and this message then has two signatures.

[NielsZeilemaker]

Here's the documentation to create a signature request
https://github.com/Tribler/dispersy/blob/devel/community.py#L2245

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester-turbo_devel/112/
Test PASSed.

[synctext]

@NielsZeilemaker : studied the DoubleMemberAuthentication code a bit.. Do you think the overly complex should be repaired or removed from the core? Clearly the "technical debt" theory applies :-)

DoubleMemberAuthentication is not a simple one-shot just-sign the message thingie. It has negotiation and various exceptions. "Assuming that he does the new message M2, which now includes Bob's signature while Alice's is still \0, is wrapped in a dispersy-signature-response message (E) and sent back to Alice. " https://github.com/Tribler/dispersy/blob/devel/authentication.py#L203

Shall we ask @snorberhuis to get his work operational in community.py and then move his merged code into Dispersy-core, thus replacing the un-used and complex DoubleMemberAuthentication?

btw very scary comment: # todo: verify the signature
https://github.com/Tribler/dispersy/blob/devel/authentication.py#L308 Trust incoming data without a verify or a comment about your own signature?

[NielsZeilemaker]

The DoubleMemberAuth stuff is perfectly working, and I actually like it better than the custom stuff @snorberhuis is creating. You just have to implement two callbacks and you're done.

The negotiation is handled in the callbacks, so if you're not interested in negotiating then is should be pretty simple.

Moreover, the scary comment isn't that scary actually, all the verification is done in the conversion layer.

The basic protocol is as follows:
Alice creates a message, wraps it in a signature request message. This triggers a callback at Bob, which can modify of accept the message if he agrees. Alice then get's a callback triggered with a modified or the same message, which she then can accept or not. If she does, she adds her signature and you'll have your double signed message.

Again, I like it better than the implementation of @snorberhuis and this code is actually tested etc. Moreover, it was simplified a bit by me and @whirm during the Dispersy v2.0 refactor.

[synctext]

Alice then get's a callback triggered with a modified or the same message, which she then can accept or not. If she does, she adds her signature and you'll have your double signed message.
OK, that is almost exactly what we need. Can Alice also copy the signed message from Bob, append it with additional info, and DoubleSign this whole extended message? (still a one-shot primitive)

Then the custom code from @snorberhuis can be cleaned up considerably.

[NielsZeilemaker]

Bob can return any message, so if Bob creates a new message containing the values of Alice and him, then yes if Alice accepts it, a double signed message is created.

[synctext]

Alice creates a message, wraps it in a signature request message.
Is the first message already signed by Alice? Or was that vital efficiency boost only added in Dispersy 2.0 refactor? (Vaguely remember something was not ideal).

[NielsZeilemaker]

Honestly, I'm not sure. I think it's optional, but that doesn't make any sense.
We can refactor it such that it's always signed by Alice, and if Bob modifies the message she signs it again.

[synctext]

Thnx Niels! @snorberhuis please dive into the code handling the Dispersy DoubleMemberAuth and try to understand if it delivers what we need + operates as expected.

[synctext]

Tribler/dispersy#432 vital changes merged.
Plus: Tribler/dispersy#433

[NielsZeilemaker]

I've created a pull request which contains a refactor of the community using the DoubleMemberAuth. It's not tested, and hence won't work but it should be close.
snorberhuis#35

It also contains the new dispersy pointer.

[snorberhuis]

I used your example in snorberhuis#35 to implement the community using the create_signature_request. It was working and messages were passed along (see my branch devel_core in my repository), but then I found a problem with the create_signature_request.

The create_signature_request implementation does not follow an append only methodology to the signatures. Bob always needs to add information he only haves. Only he knows his current previous_hash. Alice will therefor always need to sent a third message back to Bob with a new valid signature.

I am also doubtful how well the current implementation of create_signature_request is tested, because it did not even work for me at first, because method calls were done on the wrong objects, see Tribler/dispersy#445.

Because of this, it was decided with @synctext to continue forward with the current way for now.

[NielsZeilemaker]

#445 doesn't seem like a big bug, it's a wrong default value for the timeout. it was tested with a passed timeout which was a float.
Btw, what's the problem with alice needing to send a third message to bob? And does bob even need to receive this message?

[synctext]

@NielsZeilemaker So you think repairing the create_signature_request in the Dispersy core is not a difficult undertaking? We should try to avoid useless work so this PR essentially duplicates existing half-broken functionality. 3 messages is not a problem, but needs to be 2 messages eventually.

[NielsZeilemaker]

It's fixed already, Tribler/dispersy#445 was a bug in a default value for the timeout.

[synctext]

So you're @NielsZeilemaker arguing to get-it-right the first time by using the Dispersy-Core, instead of merging this PR with duplicated functionality?
So create_signature_request does not contain bugs, it just a 3-message approach.

@snorberhuis just checked the code (https://github.com/Tribler/dispersy/blob/devel/community.py#L1673) it's not clear to me that these signatures are automatically spread in the community as you incicated. We don't want auto-full-sync-spreading. The directdistribution seems also the default. No showstoppers.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5086/
Test FAILed.

[NielsZeilemaker]

No, the candidate is not shared.
@snorberhuis I was talking about the member instance. The pub member actually has a database id in there which is the id of this member in the database of other. Using that object in node (which has a different database) will cause strange and unexpected stuff to happen.

Why don't you want to use give_packet/give_message?

If a peer would disagree, he should prove that using the signed message. If he sends such a signed message, you can then subtract those values and correct them. I think the goal of this community is to ~ estimate the data flowing between two peers. If that's the case, the simply resetting these values does not make a lot of sense to me.

Aren't you branching regardless if you store the message or not. If you store this half message then a peer could eventually reveal the double signed message to create a branch. If the message was really lost in translation, then it doesn't matter if you store the half message or not.

Maybe the only solution to this problem is to switch from a push based protocol to a pull based one.
E.g. instead of sending a half signed message, and letting the other peer add the second signature. You could request a half signed message, which you could the complete by setting the second signature.

And really, I'm only here to help. This community should be the start of more things to come, so if these details aren't implemented correct, we can start all over in 6 months time.

[snorberhuis]

Do you mean with using give_packet/give_message to change:

node.call(node.community.publish_signature_request_message, target_other_from_node, 5, 
 _, signature_request = other.receive_message(names=[u"dispersy-signature-request"]).next()
other.give_message(signature_request, node)

to something like:

message = node.call(node.community.create_signature_request_message, candidate, up, down) 
other.give_message(message, node)

(create_signature_request_message is a MultiChain method)
I don't want to do this, because then the tests would not test the complete flow of the community.

[snorberhuis]

Aren't you branching regardless if you store the message or not.

The idea is that parts of the block correspond and this shows your willingness to not cheat. And as such does not constitute a real branch. Your part of the block is still part of your chain and only the total_up, total_down, and signature of the other is not present . Because the signature is not present you do not up your total amounts in the next block.

[snorberhuis]

If a peer would disagree, he should prove that using the signed message
This could be a good improvement in the future. But it does sound very complicated. It might just be much easier to be less altruistic and validate the amounts more often and still estimate the data flow accurately enough.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5087/
Test FAILed.

[snorberhuis]

retest this please

[whirm]

retest this please

[NielsZeilemaker]

@snorberhuis Sending back a message if a peer disagrees is something which is quite common in Dispersy.
Basically, in the check method of a message you do something like:

# the sender apparently does not have the lower dispersy-undo message, lets give it back
self._dispersy._send_packets([message.candidate], [db_msg.packet], self, db_msg.name)

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5094/
Test PASSed.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5095/
Test PASSed.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5096/
Test FAILed.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5097/
Test FAILed.

[snorberhuis]

The feature creep of this PR has already been huge. The PR has been open since may 22th. I would propose you create a separate issue where we can discuss the proposal about the disagree protocol.

[snorberhuis]

retest this please

[synctext]

retest this please

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5129/
Test PASSed.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5134/
Test FAILed.

[tribler-ci]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.tribler.org//job/GH_Tribler_pull-request-tester_devel/5136/
Test FAILed.