AuRa randomness

[mbr]

This is the pull request that implements most of issue #51, AuRa random numbers. The code here is well documented (shoutout to asciiflow) and should be very straightforward, I hope it makes for an entertaining read.

I divided the problem into three subtasks:

Contract interaction

That is, calling actual ethereum contracts from parity performing both, constant calls returning a value immediately, and transactional ones. I did look at the following code for inspiration and did assume that there is a Client instance available to perform these calls:

secret_store/src/listener/service_contract.rs:643
src/engines/validator_set/contract.rs:116
src/engines/validator_set/contract.rs:65

To ease this, I added the BoundContract struct, which ties a client to a specific block and contract, making it possible to call these functions without much of a fuss. All code related to that can be found in ethcore/src/engines/authority_round/util.rs

There might be a better way to accomplish this, but I found only ad-hoc implementations of something similar in other parts of the parity codebase. At the very least, this module neatly separates open questions about internal APIs from the business logic.

Actual business logic

The ethcore/res/authority_round_random.json file contains the ABI description for the relevant contracts, which are included using the use_contract macro. Running cargo doc --all --document-private-items turned out to be a real lifesaver here.

The actual implementation for the randomness generation is inside the ./src/engines/authority_round/randomness.rs, consisting of the finite state machine RandomnessPhase. All its state is loaded from the blockchain itself, using the load() function, which will determine the precise state the process is currently in.

Once the RandomnessPhase has been loaded, the advance() method can be used to create the potentially necessary transactions for the process and apply them. After advance() has been called, the phase should be discarded (it
usually is, advance() takes an owned self precisely for this reason).

The only state that has to be managed externally is the generated secret. On the very first time the function is called, None can be passed as the Option<Secret>, but every consecutive time the function is called, the return value of the last successful previous call should be passed in.

The smart contract ABI used in this PR is from poanetwork/posdao-contracts@8d10a85. If anything changes, the ABI file must be manually updated here.

Still missing from the implementation (as indicated by the two unimplemented!() macros) is the signature generation and packing, I have not had the time to get around to this final puzzle piece (see the TODO for details).

Parity integration

The actual integration is still missing, but should be doable in very few lines of code, provided one knows where to put it. In a function that is executed for every block mined by the validator, the following steps are necessary:

1. Load the Option<Secret> from storage/memory/elsewhere.
2. Create a BoundClient instance with the latest block number and (hardcoded or configured) randomness contract address.
3. Run RandomnessPhase::load.
4. Run RandomnessPhase::advance.
5. Save the returned Option<Secret> back to storage/memory/elsewhere.

The specifics on how/where to store the Option<Secret> and what to do in case of errors depend greatly on where this integration code ends up, for this reason the randomness module is not very opinionated about them.

Compilation & Testing

The branch compiles without any errors, albeit plenty of unused code warnings. Tests fail, because the underlying starting point from the aura-pos branch did not test clean either.

Since the actual integration is not done, the code has little influence on the rest of the system, it just presents two modules (authority_round::util and authority_round::randomness) that compile.

The code is woefully undertested, due to its incompleteness. I would suggest making the underlying aura-pos branch testable again, then rebasing this branch, before adding tests.

Still TODO

• Ensure the system address is used (potentially documenting this fact) when transactions are made using BoundContract.
• Implement message signing and packing (example, hopefully already available inside the parity codebase, so far I have only been able to find rpc/src/v1/helpers/dispatch.rs:246), removing the last two unimplemented!() macro calls. If not, the implementation would probably nicely fit into util.rs.
• Run on a test network to ensure everything functions as it should.

[DemiMarie]

I have implemented signing in 1323ef7. You could use a similar approach. Since the code is so similar, it could probably be factored out into helper functions.

[DemiMarie]

As far as secrets are concerened: one option would be to store the data encrypted using an AEAD cipher with a symmetric key derived (via a hash function of some sort) from the secret signing key and some locally-obtained randomness. The data could then be stored on-blockchain, since it is useless to anyone who does not have this node’s signing key. The precise AEAD cipher is an implementation detail, and different nodes can use different ciphers.

Of course, we could also use the local file system as well. The encryption is still a good idea. A full database is massive overkill ― a flat file is more than adequate, provided that it is protected with a lock of some sort.

[mbr]

I have implemented signing in 1323ef7. You could use a similar approach. Since the code is so similar, it could probably be factored out into helper functions.

That'd make a nice helper function in util.rs. Not sure how much effort we want to spend on refactoring parity though, but this would be limited to our part of the woods at least!

[...] The data could then be stored on-blockchain, since it is useless to anyone who does not have this node’s signing key. [...]

I don't think it's necessary to store this on the blockchain unless the intended punishment for not having the secret available is very draconian.

The best "cryptography" is not telling anyone anything in the first place and any effort and additional risk here does not warrant the small benefit of the above case, in my opinion.

Of course, we could also use the local file system as well. The encryption is still a good idea. A full database is massive overkill ― a flat file is more than adequate, provided that it is protected with a lock of some sort.

I believe we could even get away with just storing it as a field on some data structure for now. I did refrain from use the filesystem, because I felt dirty just randomly writing a file somewhere; I was hoping that parity would already provide capabilities.

[DemiMarie]

FYI, Parity uses different rustfmt settings than most Rust projects.

[c0gent]

This looks pretty good. I haven't quite wrapped my head around the exact details of how and where these contracts will be used but I suspect that the usage of Client will have to be modified slightly. It will either need to be stored (in BoundContract) as a Weak-wrapped reference or passed into the load and advance methods instead.

I think the best thing to do here is to squash and merge these changes now then continue to make needed changes on the aura-pos branch.

[mbr]

FYI, Parity uses different rustfmt settings than most Rust projects.

I noticed, I used rustfmt on these files, but had to comment out the verbose clause in the config file to make it even work.

This looks pretty good. I haven't quite wrapped my head around the exact details of how and where these contracts will be used but I suspect that the usage of Client will have to be modified slightly. It will either need to be stored (in BoundContract) as a Weak-wrapped reference or passed into the load and advance methods instead.

My intention was that BoundContract is a rather "short-term" struct that is just created as needed, possibly passed around and then dropped again. So passing a BoundContract is just a way to pass three parameters in a single one.

I think the best thing to do here is to squash and merge these changes now then continue to make needed changes on the aura-pos branch.

No objections from me here. I assigned this to @afck to he sees it, but if someone wants to preempt this, I guess that's fine, too =).

[afck]

Looks great! Let's just make sure we're using tabs everywhere, then squash and merge.
We can do the integration in a separate commit.

[afck]

I replaced the spaces. From my side, I'm fine with squashing and merging, if @DemiMarie and @vkomenda are okay with it. We can do the integration in a separate PR.

[afck]

I (hope I) fixed broke the "execute as system" part, implemented signing, and am calling the randomness contract in on_close_block now. I'm unsure how to handle the errors, though, and whether the BoundContract still makes sense this way.

Finally, I rebased and fixed a test compilation error. One test is still failing, though, and I don't know whether it has anything to do with this PR, because it doesn't even compile on the aura-pos branch…

[phahulin]

I'll try to deal with error handling

[afck]

OK, I'm pretty sure my code is also wrong: If I understand correctly, on_close_block is also called when verifying a block. The traces aren't sent to other nodes at all, are they?

[afck]

I replaced my last commit and reverted some of the changes.
We need to figure out where we want the randomness logic to happen.

[vkomenda]

@afck, do you still see machine and block belonging in authority_round::BoundedContract? I was trying to fit those elsewhere but your solution looked good.

[vkomenda]

I agree that integration should be a separate task.

[afck]

do you still see machine and block belonging in authority_round::BoundedContract?

I don't think so: For our purposes, the whole execute_as_system thing doesn't work (unless I'm misreading the code again). Validators will have to make regular transactions (albeit hopefully with zero gas cost).

[DemiMarie]

The code looks good, but there should be some more documentation, and the new code should be tested.

[DemiMarie]

This should probably be surrounded with ```text and ```

[DemiMarie]

Signatures should not be necessary here, as all transactions are signed by the private key corresponding to the address that sent them.

[DemiMarie]

Again, no signature should be necessary.

[DemiMarie]

Is there some reason this can only call a constant function? Presumably, it is because on-chain state cannot be updated synchronously, but this should be documented.

[DemiMarie]

Should this function also take an asynchronous callback?

[afck]

I don't think it can: It creates a transaction and puts it in the queue.
(We don't need a callback right now, and adding that functionality would require deep changes to how Parity handles transactions in general.)

[afck]

Thanks! I addressed your comments. Please take another look.

[DemiMarie]

I forgot to rebase before merging, sorry.

[mbr]

👏 Thanks for merging!