Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

move to new protocol structure #179

Merged
merged 15 commits into from
Apr 27, 2018
Merged

move to new protocol structure #179

merged 15 commits into from
Apr 27, 2018

Conversation

naterush
Copy link
Contributor

@naterush naterush commented Apr 8, 2018

Based on spec in this issue: #175. Not finished yet, but opening for initial feedback.

For now, removing code for experiments and analysis. Will reintroduce this in future PR to be compatible with new protocol classes.

TODO:

  • Removed some tests so I could get easier fix others. Will add them back in before final merge.

naterush and others added 5 commits April 7, 2018 22:01
Fixes

  File "/cbc-casper/simulations/network_delay.py", line 31, in gaussian_delay
    return round(random.gauss(MU, SIGMA))
TypeError: 'int' object is not callable
def gaussian_delay(sender, reciever, round):
"""Messages take a random amount of time to deliver
(from a gaussian distribution)"""
return round(random.gauss(MU, SIGMA))
Copy link
Contributor

Choose a reason for hiding this comment

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

See #180

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KentShikama Thanks!

Make math.round in gaussian delay no longer shadowed
@naterush naterush force-pushed the chore/protocol-rework branch from 1892178 to 8de517b Compare April 10, 2018 22:07
@naterush naterush force-pushed the chore/protocol-rework branch from 43a0ed2 to d2e6fab Compare April 13, 2018 06:12
@naterush naterush force-pushed the chore/protocol-rework branch from 74559a2 to d6ac26a Compare April 15, 2018 01:07
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to go over it with a fine toothed comb (esp testing), but I am very happy with the architecture :)

A few comments here and there.

I'll make separate issues if I run into anything else.

Merge!

casper.py Outdated

For more information,
join the gitter: https://gitter.im/cbc-casper/Lobby
read the wiki:
Copy link
Contributor

Choose a reason for hiding this comment

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

add wiki link

casper.py Outdated
save=args.save,
# not all parameters are needed to generate the json for execution
execution_string = generate_json(**{k: v for k, v in vars(args).items() if k not in NOT_NEEDED})
print(type(execution_string))
Copy link
Contributor

Choose a reason for hiding this comment

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

remove print

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to print the execution string - which I think we want (so you can duplicate the execution if you want).

Not sure why I was printing the type :)


self.plot_tool = plot_tool(display, save, self.global_view, self.global_validator_set)

self.unexecuted = execution_string
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the way one string is consumed by the other :)

if message_hash not in validator.view.justified_messages:
current_message_hashes.add(message_hash)

while any(current_message_hashes):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just do

messages_needed = set()
current_message_hashes = set(message.hash)
while any(current_message_hashes):
    etc...

Instead of doing the first for loop which essentially repeats some code from the while?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so! The duplicated code bothered me as well...


return messages_needed

def execute(self, additional_str=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be interested in seeing a step method that allows you to consume one token at a time, and just have the execute use the step until the string is totally consumed. Would allow for very granular inspection into what is going on in a protocol execution if needed.

Might be premature until we have a particular use case.

self.children,
self.latest_messages
)
print(available_outputs)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove print

Copy link
Contributor

Choose a reason for hiding this comment

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

or is this part of the plot? If it is, move to plottool


if fault_tolerance > 0:
self.bet_fault_tolerance[latest_message] = num_node_ft
if self.view:
Copy link
Contributor

Choose a reason for hiding this comment

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

When does not have a view?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This was an issue in a previous version of the architecture, but I fixed it. Removing!

def generate_rrob_execution(num_validators, num_rounds, network_delay_func):
curr_creator = 0
def rrob_creator():
nonlocal curr_creator
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, never seen nonlocal before

@@ -0,0 +1,78 @@
"""The simulution utils module ... """
Copy link
Contributor

Choose a reason for hiding this comment

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

fix comment

CONSTANT
)

def test_meets_interface(delay_function):
Copy link
Contributor

Choose a reason for hiding this comment

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

two new lines between function defs

@naterush
Copy link
Contributor Author

@djrtwo Thanks for the review! Addressed your comments. I'm going to merge this and then do a release (going to skip the sharding release).

@naterush naterush merged commit 5ea49e5 into develop Apr 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants