Problem (Fix #1142): revised punishment mechanisms not implemented #1292

yihuang · 2020-03-24T03:03:28Z

Solution:

revised staking and punishment implementation

This change is

codecov · 2020-03-24T13:39:52Z

Codecov Report

Merging #1292 into master will decrease coverage by 0.37%.
The diff coverage is 87.55%.

@@            Coverage Diff            @@
##           master   #1292      +/-   ##
=========================================
- Coverage   63.97%   63.6%   -0.38%     
=========================================
  Files         154     152       -2     
  Lines       20876   20680     -196     
=========================================
- Hits        13356   13153     -203     
- Misses       7520    7527       +7

Impacted Files	Coverage Δ
chain-core/src/lib.rs	`91.66% <ø> (-8.34%)`	⬇️
client-cli/src/command.rs	`0% <0%> (ø)`	⬆️
chain-abci/src/app/query.rs	`60.58% <0%> (+3.73%)`	⬆️
chain-abci/tests/abci_app.rs	`94.89% <100%> (-0.14%)`	⬇️
chain-abci/src/app/end_block.rs	`100% <100%> (ø)`	⬆️
test-common/src/chain_env.rs	`94.27% <100%> (+0.12%)`	⬆️
...work/src/network_ops/default_network_ops_client.rs	`75.79% <100%> (-0.01%)`	⬇️
chain-abci/src/app/rewards.rs	`98.29% <100%> (-0.08%)`	⬇️
chain-core/src/init/config.rs	`78.09% <100%> (+3.53%)`	⬆️
chain-abci/tests/punishment.rs	`100% <100%> (ø)`	⬆️
... and 34 more

tomtau

looks good -- still a big PR, but more manageable. For the big PR, I prefer using reviewable, as it's easier for tracking changes across revisions etc., so for the comments you can reply there: https://reviewable.io/reviews/crypto-com/chain/1292#-

Reviewed 34 of 34 files at r1, 13 of 13 files at r2.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @calvinaco, @devashishdxt, @leejw51crypto, @linfeng-crypto, and @yihuang)

chain-abci/src/staking_table.rs, line 8 at r1 (raw file):

use parity_scale_codec::{Decode, Encode};
#[cfg(not(feature = "mesalock_sgx"))]

is this needed? chain-abci doesn't have this feature?

chain-abci/src/staking_table.rs, line 35 at r1 (raw file):

#[derive(Error, Debug)]
pub enum UnjailError {

NIT: perhaps all these data types can be put in some "publictx_error" module?

chain-abci/src/staking_table.rs, line 94 at r1 (raw file):

/// Invarient 2.2:
///   All the secondary indexes should be partial index with condition `validator is not null`
///   So there is no nothing-at-stake risk here, because there is a cost to become validator.

"nothing-at-stake" generally refers to the consensus attack where a validator signs on multiple forks -- whereas here, at least according to that older PR, it's just saying "this shouldn't be prone to DoS as long as minimum required stake is high enough".
it can be rephrased as in that explanatory GitHub comment, otherwise the text is a bit misleading.

and it's not that "there is no risk" -- if my understanding of that comment is correct, there could be risk if it's say Coin::one(). It'd ideally be quantified and also put in documentation (perhaps to the rustdoc near that network parameter)

chain-abci/src/staking_table.rs, line 105 at r1 (raw file):

///   Proof: always update index when related validator fields changed.
#[derive(Clone, Debug, Default, Encode, Decode)]
#[cfg_attr(not(feature = "mesalock_sgx"), derive(Serialize, Deserialize))]

not needed, as this feature isn't in abci? is Deserialize needed?

chain-abci/src/staking_table.rs, line 108 at r1 (raw file):

pub struct StakingTable {
    // Selected validator voting powers of last executed end block
    pub validator_snapshot: BTreeMap<StakedStateAddress, TendermintVotePower>,

does this need to be pub, is it for serde auto-derives? for enforcing the invariants in the comment (if this is in some &mut context), it may be better to hide it (if it's for serde json serialize, there may be some annotation?)?

chain-abci/src/staking_table.rs, line 123 at r1 (raw file):

impl StakingTable {
    /// Init with genesis stakings, panic if:

NIT: for rustdoc https://rust-lang.github.io/api-guidelines/documentation.html#function-docs-include-error-panic-and-safety-considerations-c-failure

chain-abci/src/staking_table.rs, line 143 at r1 (raw file):

    }

    /// After restored from storage, call initialize to populate the indexes

maybe a TODO to move this to be called automatically in Decode?

chain-abci/src/staking_table.rs, line 162 at r1 (raw file):

    }

    /// Handle abci begin_block event

just a comment here whether this is also expected to handle rewards later when the revised implementation is added?

chain-abci/src/staking_table.rs, line 259 at r1 (raw file):

                    block_time,
                    block_height,
                    &mut staking,

instead of having this slashing-related update in two places, it'll be better to unify it. e.g. instead of self.slash(...), there'll be computer_slash_amount (that doesn't take &mut staking) and do everything here or do everything in `.slash(...)

chain-abci/src/staking_table.rs, line 296 at r1 (raw file):

    /// Handle `NodeJoinTx`
    pub fn node_join(

these tx handlers can be perhaps put in separate modules/files where they can be a bit expanded for legibility -- for example, instead of this if-else nesting, it could just have a helper something like check_inactive_validator(...) -> Result<&mut Validator, PublicTxError> (probably not exactly)

chain-abci/src/staking_table.rs, line 392 at r1 (raw file):

            }
        } else {
            Err(UnjailError::NotJailed.into())

is this the right error in this branch (Validator is None?)

chain-abci/src/staking_table.rs, line 396 at r1 (raw file):

    }

    /// Handle reward statistics record

still the old one TODO comment?

chain-abci/src/staking_table.rs, line 451 at r1 (raw file):

            remainder = (remainder - amount).unwrap();
            distributed.push((addr, amount));
            self.add_bonded(amount, &mut staking).unwrap();

again the update add_bonded + inc_nonce can be together

chain-abci/src/staking_table.rs, line 881 at r1 (raw file):

}

/// Return success or not

returns success if less than max bound?

chain-abci/src/app/app_init.rs, line 46 at r1 (raw file):

    /// time in current block's header or genesis time, set in begin block
    pub block_time: Timespec,
    /// time in current block's height or genesis time, set in begin block

nit: comment -- current block height or 0 for genesis?

chain-abci/src/app/app_init.rs, line 49 at r1 (raw file):

    pub block_height: BlockHeight,
    /// Indexings of validator states
    pub staking_table: StakingTable,

should this be dumped by serde? currently this is in response to "info" request serialised in json

chain-abci/src/app/app_init.rs, line 61 at r1 (raw file):

}

impl NodeChecker for &ChainNodeState {

FYI to myself -- check if nodechecker trait was removed

chain-abci/src/app/app_init.rs, line 174 at r1 (raw file):

    mut req_validators: Vec<ValidatorUpdate>,
    distribution: &BTreeMap<RedeemAddress, (StakedStateDestination, Coin)>,
) -> bool {

prefer Result<(),()> if no need to return anything (bool is ambiguous as one doesn't know if true means "yes, it's correct" or "yes, it's invalid")

chain-abci/src/app/mod.rs, line 234 at r1 (raw file):

            }
            Err(msg) => {
                resp.set_code(1);

BTW, I guess it'll be useful to assign 1,2,3,4... to that error enum and set it here for #1293 (and perhaps in checktx)

chain-abci/src/app/validate_tx.rs, line 64 at r1 (raw file):

#[derive(thiserror::Error, Debug)]
pub enum PublicTxError {

nit: this could be in that error module with those other error types

chain-abci/tests/tx_validation.rs, line 174 at r1 (raw file):

}

struct NodeInfoWrap(

is this used for anything?

before this was a mock for isolating out the storage in validation testing -- seems no longer relevant?

chain-tx-validation/src/lib.rs, line 373 at r1 (raw file):

/// Verifies if the account is unjailed
pub fn verify_unjailed(account: &StakedState) -> Result<(), Error> {

is this used given those custom errors / validations in staking_table?

tomtau · 2020-03-24T14:04:59Z

bors try

bors · 2020-03-24T14:20:43Z

try

Build failed

drone-hw/push

yihuang

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @calvinaco, @devashishdxt, @leejw51crypto, @linfeng-crypto, and @tomtau)

chain-abci/src/staking_table.rs, line 8 at r1 (raw file):