[DO NOT MERGE] Testing validator_service.py

[lrettig]

This PR is not meant to be merged, at least not yet. I thought it would make sense to move the discussions we've been having on various other channels inline into Github, which makes it easier to loop others in, reference specific lines of code, etc. It may make sense to submit smaller individual PRs when this code is ready to be merged.

As you can see in this PR, I cleaned up validator_service.py quite a bit along the lines discussed in the Hybrid Casper testnet next steps doc. I also added a test_validator_service.py module.

@karlfloersch you recommended that we use https://github.com/karlfloersch/pyethereum/blob/develop/ethereum/tests/hybrid_casper/testing_lang.py and https://github.com/karlfloersch/pyethereum/blob/develop/ethereum/tools/tester.py to test the validator service. I spent a lot of time working on this for the past couple of days, and got pretty far, but I'm stuck on the fact that transactions coming from the validator service (e.g. https://github.com/lrettig/pyethapp/blob/73eaab6825cdcff73caf2bfa0414982cb31e6847/pyethapp/validator_service.py#L115) never make it into the chain state, causing checks such as https://github.com/lrettig/pyethapp/blob/73eaab6825cdcff73caf2bfa0414982cb31e6847/pyethapp/validator_service.py#L121 to always fail.

I suspect this has to do with the ephemeral_clone operation at https://github.com/karlfloersch/pyethereum/blob/5a4d247bb9c6bbed9c5c6357ea95203d2d9c6805/ethereum/tools/tester.py#L172 but I'm not sure.

Could you explain why you think we need to test the validator service using the tester/testing_lang harness from pyethereum?

[lrettig]

One more question. Do you know why you're using broadcast_transaction instead of add_transaction inside the validator service? E.g. here:

pyethapp/pyethapp/validator_service.py

Line 70 in 2274ab8

self.chainservice.broadcast_transaction(valcode_tx)

[karlfloersch]

@lrettig good idea regarding the format! GitHub is for sure much more readable.

One more question. Do you know why you're using broadcast_transaction instead of add_transaction inside the validator service? E.g. here

This was probably an artifact of me debugging the login transactions. I think we should be able to switch it back to add_transaction()

---

@karlfloersch you recommended that we use https://github.com/karlfloersch/pyethereum/blob/develop/ethereum/tools/testing_lang.py

Not sure if this was on purpose but just to clarify it should be this testing lang instead. The one you linked to isn't actually used and is mostly an artifact of previous work and probably would be best to remove.

My thought regarding transactions getting applied is actually shimming add_transaction(). I would recommend overriding that function and then manually mining a block with it. So for instance something like:

# test_validator_service.py
...
def add_transaction_shim(tx):
    # Instead of following the normal broadcast stuff, just catch the transaction and then manually apply it to the tester Chain object
    test.t.direct_tx(tx)

This way we can test what the validator would do in a bunch of circumstances, while still just using the tester Chain / testing lang. Does that make sense?

[lrettig]

This was probably an artifact of me debugging the login transactions. I think we should be able to switch it back to add_transaction()

Done, will include in my final PR.

Not sure if this was on purpose but just to clarify it should be this testing lang instead.

That was a typo, good catch. Edited the PR message.

My thought regarding transactions getting applied is actually shimming add_transaction()

I did exactly this. It didn't work. As described above:

I'm stuck on the fact that transactions coming from the validator service (e.g. https://github.com/lrettig/pyethapp/blob/73eaab6825cdcff73caf2bfa0414982cb31e6847/pyethapp/validator_service.py#L115) never make it into the chain state, causing checks such as https://github.com/lrettig/pyethapp/blob/73eaab6825cdcff73caf2bfa0414982cb31e6847/pyethapp/validator_service.py#L121 to always fail.
I suspect this has to do with the ephemeral_clone operation at https://github.com/karlfloersch/pyethereum/blob/5a4d247bb9c6bbed9c5c6357ea95203d2d9c6805/ethereum/tools/tester.py#L172 but I'm not sure.

Instead, what's working well is doing the mining manually inside pyethapp, as its other tests do. See here: https://github.com/lrettig/pyethapp/blob/c311055b0dfe59e21d4000b224628a05a4b947fa/pyethapp/tests/test_validator_service.py

[lrettig]

I figured out what's going on. The problem is here: https://github.com/karlfloersch/pyethereum/blob/5a4d247bb9c6bbed9c5c6357ea95203d2d9c6805/ethereum/tools/tester.py#L252. The tester mines a block, and calls self.chain.add_block, and inside this method, the chain calls the callback that updates the validator service, which in turn sends a new transaction (e.g. the valcode tx) to the tester. Because the latter is happening inside the former, the transaction is added to the block that was already added to the chain, and the state is not updated.

V feels that the tester isn't really designed for handling transactions or talking to the chain, so I think we may be better off doing this without the tester.

What do you think?

PS: I saved this code in another branch for posterity: https://github.com/lrettig/pyethapp/tree/dev_env3

[karlfloersch]

@lrettig Ah fun, a circular dependency! That makes some sense. I imagine one option would to have a more complex add_transaction() function which keeps track of transactions and avoids reapplying the same transaction. Do you think that would solve the issue?

Also, if you find that there is another way to easily set up situations like "sign in 3 validators, have one violate a slashing condition, and then see what our validator does" then I'm super open to it. The reason why I suggest using the testinglang is simply because that kind of state is hard to recreate manually with raw transactions & the testing lang is very clear and compact. That said, once again, open to suggestions!

[lrettig]

Do you think that would solve the issue?

Probably, although I fear that other circular issues may pop up. Let me walk a little further down the path I'm on, with the framework I've been using, and see how far it takes me. If it becomes cumbersome we can loop back to this code and give it another shot.

[lrettig]

First test is built and working, would love your feedback:

https://github.com/lrettig/pyethapp/blob/c8f5ef13c03ed70e6fa543f3b2d03c6fa77c0693/pyethapp/tests/test_validator_service.py#L131

[karlfloersch]

This test looks great! My one suggestion is to keep it DRY it might be best to instead use the chain tester for mining.

I took a look at the issue you were having before with txs not applying and it seems I've found a solution. It could be cleaner but the code here seems to work. What do you think about using the testing lang like this?

Generally good stuff with the test though! It shouldn't have to change much at all even if we make this change.

[lrettig]

Thanks for taking a look at this, seeing your code helped. Just to double check my understanding, rather than processing transactions immediately, you're queueing them up and processing them explicitly in batches. It seems that this approach requires explicitly processing each transaction, pretty much one-by-one--which is more work but I suppose in the context of a test it could be good since it adds one more layer of testing (making sure the tx queue always contains exactly what we expect). Am I right?

[karlfloersch]

@lrettig yeah that's right. But you can pretty easily replace my manual tx adding I'm sure with something that applies them automatically. I just did it that way because my plane was about to take off 😄