Feat/genesis #119

naterush · 2017-11-25T05:59:49Z

Adds a genesis block. Not sure if we should add a default genesis block to BlockchainProtocol, where all validators who don't get a genesis block sent to them choose this as their genesis.

The benefit of this is that we have to stop worrying about a number of edge cases. For example, I'd like to move the block's estimate to be hash linked. As a result, I want the estimate() function for the blockchain view to return a hash - and it would be much cleaner to not have to worry about None

(addresses #56)

djrtwo · 2017-11-26T15:23:19Z

excited by how small this PR looks!

djrtwo · 2017-11-26T16:06:33Z

I think we need to figure out a solution that does not require if protocol == BlockchainProtocol: scatter about the codebase. We also have not removed network.random_initialization from SimulationRunner.

I think all validators of all protocols should get a genesis block. They can get it from Protocol.genesis_block() to be defined at the protocol level. In blockchain, the method would return the same for everyone, where in binary it would return a random 0/1.

An individual validator's view is pass in messages=Set(v.genesis_block). In BlockchainView, it can initialize the singular message to last_finalized_block and genesis_block. When Network is creating its global_view, it simply iterates through validator_set and collects a Set of their genesis_blocks to pass into global_view as messages. In Blockchain, it would just be a set of length 1 because they will all have the same genesis. In Binary, it will be a set of length len(validator_set).

There might be a detail I missed, but I think the above plan will allow us to have most of the code protocol agnostic and be more modular in allowing us to define the genesis block at a protocol level.

naterush · 2017-11-26T22:10:31Z

@djrtwo there was a version of this PR where the genesis was defined in the Protocol. The issue I had with this was that it later makes it much harder to have a weakly subjective deal where validators start with different genesis blocks - but for it might make sense to not worry about that. I totally agree that having if protocol == everywhere is pretty ugly.

As far as each protocol having a genesis message, I need to think about it for a bit. We would have to have a function called make_genesis_message that accepts a sender; in the case of blockchain, it always returns None (or maybe some random validator we choose as a sender), but for binary, the sender would be set to the caller of the function.

This might be a bit messier than we want, as I'm not sure every protocol deserves a genesis message. For example, in the case of binary, it's not really a genesis message (in the "shared" sense), just a new place to generate each validators initial message. It might make more sense to just define a genesis_message in a view... Need to think about it more

djrtwo · 2017-11-26T22:18:02Z

I'm cool with a function initial_message(validator) defined by the protocol (or maybe somewhere else). [EDIT: might be defined by network because network has the global_view. When you are onboarding, you'd ask the network (friends, family, twitter, EF) for a recent hash.]

For Blockchain, it would be the genesis block initially, but could later be the most recent finalized block when the validator signs on. I guess it would just be the most recent finalized block where most recent finalized starts as the genesis. (initially though, it would just be coded to return genesis. it being dynamic would come in later PR).

For Binary, would be some random initialization.

We should talk it out.

djrtwo · 2017-11-27T16:54:47Z

point merge to feat/hash_linking

naterush · 2017-11-28T01:09:42Z

@djrtwo check it. Still need to fix/remove the random_initialization function (going to remove it and move work to network initialization).

That being said, not totally with happy with moving this message generation to Protocol. Let me know what you think so far.

djrtwo · 2017-11-28T17:09:57Z

changed to base to feat/hash_linking

naterush · 2017-11-29T05:11:42Z

@djrtwo should be good to review. Removed a fair bit of technical debt in the testing as a result of usually having 5 messages generated at the start of some execution.

The one change I made that I'm not totally comfortable with is that the simulation runner will make the validator generate new messages if it does not already have one. That being said, I don't think it's that bad - it just makes the analyzer tests a bit less precise. Looking forward to hearing your thoughts :)

djrtwo

Just some little things. Looks great in general.
I put a note about the extra check for sim runner generating a new message if validator doesn't have one. Don't all validators start with a message now??

djrtwo · 2017-11-29T14:31:22Z

casper/network.py

@@ -34,7 +38,10 @@ def view_initialization(self, view):
        for validator in self.validator_set:
            validator.receive_messages(messages)

-    def random_initialization(self):
-        """Generates starting messages for all validators with None as an estiamte."""
+    def collect_initial_messages(self):


should likely be prefixed with _. Maybe could use a name change. The name is a bit misleading because if the network moves forward in time, this will no longer collect the initial messages.

djrtwo · 2017-11-29T14:33:34Z

casper/abstract_view.py

@@ -18,6 +16,9 @@ def __init__(self, messages=None):

        self.latest_messages = dict()               # validator => message

+        self.add_messages(messages)
+


one line between methods in a class

djrtwo · 2017-11-29T14:41:16Z

tests/simulations/test_analyzer.py

@@ -30,11 +30,11 @@ def test_num_messages(validator_set, mode, messages_generated_per_round):
    analyzer = Analyzer(simulation_runner)

    # due to random_initialization


remove comment

djrtwo · 2017-11-29T14:42:27Z

tests/simulations/test_analyzer.py


    for i in range(10):
        simulation_runner.step()
-        assert analyzer.num_messages() == \
+        assert analyzer.num_messages() <= \


should be == and remove len(validator_set) + from the assertion.

was there from the initial messages

djrtwo · 2017-11-29T14:44:39Z

tests/simulations/test_simulation_runner.py

@@ -107,9 +107,12 @@ def test_simulation_runner_send_messages(
        False
    )

-    assert len(simulation_runner.network.global_view.justified_messages) == len(validator_set)
+    if protocol == BlockchainProtocol:
+        assert len(simulation_runner.network.global_view.justified_messages) == 0


why 0? shouldn't the genesis block be justified?

djrtwo · 2017-11-29T14:45:28Z

tests/simulations/test_simulation_runner.py


    for i in range(10):
        simulation_runner.step()
-        assert len(simulation_runner.network.global_view.justified_messages) == \
-            (i + 1) * messages_generated_per_round + len(validator_set)
+        assert len(simulation_runner.network.global_view.justified_messages) <= \


capture the initial length of the justified_messages, plug it into where len(validator_set) is below, and use ==

djrtwo · 2017-11-29T14:51:21Z

simulations/simulation_runner.py

        for sender, receiver in message_paths:
-            message = sender.my_latest_message()
+            last_message = sender.my_latest_message()


don't all validator's have a latest message when they are initialized now? Do we need this check?

djrtwo · 2017-11-30T14:35:30Z

@naterush I'm going to address my notes and merge

…ssage to messages

[WIP] Feat/genesis message change

Addressed with changes above.

naterush added 3 commits November 24, 2017 23:30

add a genesis block to validator set

a1dc173

fix plotting

d869838

move block creation out of validator set

4daabcd

naterush requested a review from djrtwo November 25, 2017 06:00

lints

c06e13b

move inital message generation to protocol, some tests failing

172af1d

naterush added 2 commits November 28, 2017 00:49

remove random_initlization, some tests failing

0d225e0

sim_runner asks for new message if val does not have one

c2c35ec

djrtwo changed the base branch from chore/cleanup to feat/hash_linking November 28, 2017 17:09

fix broken tests

9606f77

naterush requested review from djrtwo and removed request for djrtwo November 29, 2017 04:47

naterush added 2 commits November 29, 2017 00:01

Merge branch 'feat/hash_linking' into feat/genesis

edb00c7

fix broken test

175c92e

djrtwo previously requested changes Nov 29, 2017

View reviewed changes

djrtwo and others added 4 commits November 30, 2017 14:17

blockchain genesis uses 1st validator as sender; views add intiial me…

20d416c

…ssage to messages

reset blockchain protocol genesis block before each test

1656ee9

ref view.genesis_block in blockcahinplottool

70e91c6

Merge pull request #140 from ethereum/feat/genesis-message-change

c2446aa

[WIP] Feat/genesis message change

naterush merged commit 0f39561 into feat/hash_linking Dec 1, 2017

naterush deleted the feat/genesis branch December 1, 2017 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/genesis #119

Feat/genesis #119

naterush commented Nov 25, 2017 •

edited by djrtwo

Loading

djrtwo commented Nov 26, 2017

djrtwo commented Nov 26, 2017

naterush commented Nov 26, 2017 •

edited

Loading

djrtwo commented Nov 26, 2017 •

edited

Loading

djrtwo commented Nov 27, 2017

naterush commented Nov 28, 2017 •

edited

Loading

djrtwo commented Nov 28, 2017

naterush commented Nov 29, 2017

djrtwo left a comment •

edited

Loading

djrtwo Nov 29, 2017

djrtwo Nov 29, 2017

djrtwo Nov 29, 2017

djrtwo Nov 29, 2017

djrtwo Nov 29, 2017

djrtwo Nov 29, 2017

djrtwo Nov 29, 2017

djrtwo commented Nov 30, 2017

		@@ -18,6 +16,9 @@ def __init__(self, messages=None):

		self.latest_messages = dict() # validator => message

		self.add_messages(messages)

		@@ -30,11 +30,11 @@ def test_num_messages(validator_set, mode, messages_generated_per_round):
		analyzer = Analyzer(simulation_runner)

		# due to random_initialization

Feat/genesis #119

Feat/genesis #119

Conversation

naterush commented Nov 25, 2017 • edited by djrtwo Loading

djrtwo commented Nov 26, 2017

djrtwo commented Nov 26, 2017

naterush commented Nov 26, 2017 • edited Loading

djrtwo commented Nov 26, 2017 • edited Loading

djrtwo commented Nov 27, 2017

naterush commented Nov 28, 2017 • edited Loading

djrtwo commented Nov 28, 2017

naterush commented Nov 29, 2017

djrtwo left a comment • edited Loading

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo Nov 29, 2017

Choose a reason for hiding this comment

djrtwo commented Nov 30, 2017

naterush commented Nov 25, 2017 •

edited by djrtwo

Loading

naterush commented Nov 26, 2017 •

edited

Loading

djrtwo commented Nov 26, 2017 •

edited

Loading

naterush commented Nov 28, 2017 •

edited

Loading

djrtwo left a comment •

edited

Loading