openethereum / parity-ethereum

The fast, light, and robust client for Ethereum-like networks.
Other
6.82k stars 1.69k forks source link

Panickers: PROVE or REMOVE #1026

Closed gavofyork closed 7 years ago

gavofyork commented 8 years ago

.expect and assert! should all have strings of the form:

"<truth> ( [, <truth>] ... ) ( [; so <derivation>] ... ); qed"

e.g.

fn foo(x: Option<int>) -> int {
    let mut b = false;
    if x.is_some() {
        b = bar();
    }

    if b {
        return x.expect("`b` initially `false`; `b` changed only inside block guarded by `x.is_some()`; `b` is `true`; so `x` must be `Some`; qed") - 1;
    }

    0
}

Any other panickers (e.g. subscripting) must have an additional assert! prepended so they fail with an error message as early as possible.

rphmeier commented 8 years ago

Took a stab at sweeping the panics from the blockchain module: https://github.com/ethcore/parity/tree/blockchain-panics

The issue is that you have to start making everything that used to return Option<T> to return a Result<Option<T>, String> in order to bubble up database errors. There are a couple problems here:

  1. Huge API surface change as well as tons of new try! invocations. The code becomes less readable
  2. We don't actually know how to handle these errors in the first place, as they're all just String. So even once handling could be in place, the only thing we can still do is panic.

So I would suggest (and @NikVolf and @arkpar I would appreciate your input on this as well) that our kvdb wrapper should not return errors at all, but should retry N (= 5?) times upon encountering one internally, warning on each error and panicking if success never occurs.

rphmeier commented 8 years ago

Additionally it seems that the kvdb wrapper is best equipped to do stuff like reopen the underlying rocksdb instance, check that the directory still exists, etc.

rphmeier commented 7 years ago

I've made various PRs to sweep just about all unwraps from the codebase. There might be a few stragglers but it's mostly taken care of.

arkpar commented 7 years ago

I'll make an additional PR that takes care of network and IO crates

gavofyork commented 7 years ago

@arkpar / @rphmeier is this pretty much done, now?

gavofyork commented 7 years ago

assuming so.