Remove 'NodeHandler::public_key_of' function and some unwraps

[stanislav-tkach]

Closes #28

[stanislav-tkach]

I think if we need mutable ProposeState, then the method should be named propose_mut, but for now it is not needed.

[alekseysidorov]

i see here a lot of boilerplate code. Can you think about simplify it?

[stanislav-tkach]

I don't like this either, but I can not see how to elegantly get rid of it. This code can not be moved in a separate function because it should return Option and in the call site there will be matching with early return, so we are not going to win anything.

Any ideas how to deal with this?

[vldm]

As I understand correctly the problem is in error handling at all.
there so many functions like: fn handle_SOMETHING(...) -> ()
with body:

if some_bad_case {
  error!("ERROR MESSAGE");
  return;
}

we probably could replace it by

// in mod consensus
enum Error {
   Propose(ProposeError),
   Commit(CommitError),
...
}
enum ProposeError {
  WrongLeader{actual: ValidatorId, expected: ValidatorId} ,
  WrongValidator{actual: ValidatorId},
}

and handling errors like this should be trivial:

let validator_id = msg.validator();
let key = self.state.public_key_of(validator_id).ok_or(WrongValidator{actual: validator_id})?

We should consider some way to work with errors.

[stanislav-tkach]

I like the idea, although new error kinds seems like relatively big change, so I propose to perform such changes as a separate pull request. Could you please create an issue?

[dj8yfo]

@alekseysidorov already used crate derive_error https://github.com/rushmorem/derive-error in exonum/exonum-btc-anchoring for chaining error kinds.
Though, https://crates.io/crates/error-chain is a lot more widely spread and involves more capabilities.
@defuz we should choose, what tool to use for chaining errors project wide (in core and services repos).

[alekseysidorov]

@gisochre
Error chain may allocate memory in heap for traces. And it may be too complex for our use cases.

[defuz]

@gisochre @alekseysidorov Personally I prefer derive-error. At least, I understand what it does. Macros from error-chain looks like black implicit magic.

[alekseysidorov]

Here similarly

[alekseysidorov]

And here.

[alekseysidorov]

And here

[defuz]

I am not satisfied with the approach of solving this problem. In all cases, you just trying to handle error which will never happen. All methods which calling public_key_of are previously called by handle_consensus, where we explicitly check that validator_id is correct and public key exist:

match self.state.public_key_of(msg.validator()) {
    // incorrect signature of message
    Some(public_key) => {
        if !msg.verify(public_key) {
            error!("Received message with incorrect signature msg={:?}", msg);
            return;
        }
    }
    // incorrect validator id
    None => {
        error!("Received message from incorrect msg={:?}", msg);
        return;
    }
}

Calling .unwrap() does not necessary means potential panic. Sometimes we may ensure that value exist. Of course, this does not mean that many .unwrap()s are not a problem. But in different cases, the solution must be different. Here I would prefer to pass key: PublicKey argument from handle_consensus into handle_propose, handle_prevote and handle_precommit. Perfectly the same what you do for request_propose_or_txs method.

[stanislav-tkach]

Still I would like to refactor error handling.