Merge #467

467: Problem: (CRO-453) Non-live validators are not jailed r=tomtau a=devashishdxt

Solution: After updating liveness of a validator based on `last_commit_info`. Jail if validator is not live.

Co-authored-by: Devashish Dixit <devashish@crypto.com>

chain-abci/src/app/jail_account.rs

@@ -15,6 +15,11 @@ impl<T: EnclaveProxy> ChainNodeApp<T> {
             &self.accounts,
         )?;
 
+        if account.is_jailed() {
+            // Return early if account is already jailed
+            return Ok(());
+        }
+
         let last_state = self
             .last_state
             .as_ref()

chain-abci/src/app/mod.rs

@@ -130,27 +130,27 @@ impl<T: EnclaveProxy> abci::Application for ChainNodeApp<T> {
             ),
         };
 
-        {
-            let last_state = self
+        let last_state = self
             .last_state
             .as_mut()
             .expect("executing begin block, but no app state stored (i.e. no initchain or recovery was executed)");
 
-            last_state.block_time = block_time;
-
-            if block_height > 1 {
-                if let Some(last_commit_info) = req.last_commit_info.as_ref() {
-                    // liveness will always be updated for previous block, i.e., `block_height - 1`
-                    update_validator_liveness(last_state, block_height - 1, last_commit_info);
-                } else {
-                    panic!(
-                        "No last commit info in begin block request for height: {}",
-                        block_height
-                    );
-                }
+        last_state.block_time = block_time;
+
+        if block_height > 1 {
+            if let Some(last_commit_info) = req.last_commit_info.as_ref() {
+                // liveness will always be updated for previous block, i.e., `block_height - 1`
+                update_validator_liveness(last_state, block_height - 1, last_commit_info);
+            } else {
+                panic!(
+                    "No last commit info in begin block request for height: {}",
+                    block_height
+                );
             }
         }
 
+        let mut accounts_to_jail = Vec::new();
+
         for evidence in req.byzantine_validators.iter() {
             if let Some(validator) = evidence.validator.as_ref() {
                 let validator_address =
@@ -164,11 +164,33 @@ impl<T: EnclaveProxy> abci::Application for ChainNodeApp<T> {
                     .get(&validator_address)
                     .expect("Validator not found in liveness tracker")
                     .address();
-                self.jail_account(account_address)
-                    .expect("Unable to jail account in begin block");
+
+                accounts_to_jail.push(account_address);
             }
         }
 
+        let missed_block_threshold = self
+            .last_state
+            .as_ref()
+            .unwrap()
+            .jailing_config
+            .missed_block_threshold;
+
+        accounts_to_jail.extend(
+            self.last_state
+                .as_ref()
+                .unwrap()
+                .validator_liveness
+                .values()
+                .filter(|tracker| !tracker.is_live(missed_block_threshold))
+                .map(LivenessTracker::address),
+        );
+
+        for account_address in accounts_to_jail {
+            self.jail_account(account_address)
+                .expect("Unable to jail account in begin block");
+        }
+
         ResponseBeginBlock::new()
     }
 

chain-abci/src/liveness.rs

@@ -39,7 +39,6 @@ impl LivenessTracker {
 
     /// Checks if validator is live or not
     #[inline]
-    #[allow(dead_code)]
     pub fn is_live(&self, missed_block_threshold: u16) -> bool {
         let zero_count = self.liveness.iter().filter(|x| !x).count();
         zero_count < missed_block_threshold as usize

chain-abci/tests/abci_app.rs

@@ -1247,3 +1247,143 @@ fn begin_block_should_jail_byzantine_validators() {
     let account = get_account(&address, &app);
     assert!(account.is_jailed());
 }
+
+#[test]
+fn begin_block_should_jail_non_live_validators() {
+    use chain_abci::app::into_tendermint_validator_pub_key;
+    use chain_core::state::tendermint::TendermintValidatorAddress;
+    use protobuf::well_known_types::Timestamp;
+
+    let storage = Storage::new_db(create_db());
+    let mut account_storage =
+        AccountStorage::new(Storage::new_db(Arc::new(create(1))), 20).expect("account db");
+
+    let secp = Secp256k1::new();
+    let secret_key = SecretKey::from_slice(&[0xcd; 32]).expect("32 bytes, within curve order");
+    let public_key = PublicKey::from_secret_key(&secp, &secret_key);
+    let address = RedeemAddress::from(&public_key);
+    let staking_account_address = StakedStateAddress::BasicRedeem(address);
+
+    let mut validator_pubkey = PubKey::new();
+    validator_pubkey.field_type = "Ed25519".to_string();
+    validator_pubkey.data = base64::decode("EIosObgfONUsnWCBGRpFlRFq5lSxjGIChRlVrVWVkcE=").unwrap();
+
+    let mut validator_voting_power = BTreeMap::new();
+    validator_voting_power.insert(staking_account_address, TendermintVotePower::zero());
+
+    let mut distribution = BTreeMap::new();
+    distribution.insert(address, (Coin::max(), AccountType::ExternallyOwnedAccount));
+    distribution.insert(
+        RedeemAddress::default(),
+        (Coin::zero(), AccountType::Contract),
+    );
+
+    let init_network_params = InitNetworkParameters {
+        initial_fee_policy: LinearFee::new(Milli::new(0, 0), Milli::new(0, 0)),
+        required_council_node_stake: Coin::max(),
+        unbonding_period: 1,
+        jailing_config: JailingParameters {
+            jail_duration: 60,
+            block_signing_window: 5,
+            missed_block_threshold: 1,
+        },
+    };
+
+    let init_config = InitConfig::new(
+        distribution,
+        RedeemAddress::default(),
+        RedeemAddress::default(),
+        RedeemAddress::default(),
+        init_network_params,
+        vec![InitialValidator {
+            staking_account_address: address,
+            consensus_pubkey_type: ValidatorKeyType::Ed25519,
+            consensus_pubkey_b64: "EIosObgfONUsnWCBGRpFlRFq5lSxjGIChRlVrVWVkcE=".to_string(),
+        }],
+    );
+
+    let timestamp = Timestamp::new();
+
+    let (accounts, rewards_pool_state, _) = init_config
+        .validate_config_get_genesis(timestamp.get_seconds())
+        .expect("Error while validating distribution");
+
+    let mut keys: Vec<StarlingFixedKey> = accounts.iter().map(|account| account.key()).collect();
+    let mut wrapped: Vec<AccountWrapper> = accounts
+        .iter()
+        .map(|account| AccountWrapper(account.clone()))
+        .collect();
+    let new_account_root = account_storage
+        .insert(None, &mut keys, &mut wrapped)
+        .expect("initial insert");
+
+    let transaction_tree = MerkleTree::empty();
+
+    let genesis_app_hash =
+        compute_app_hash(&transaction_tree, &new_account_root, &rewards_pool_state);
+
+    let mut app = ChainNodeApp::new_with_storage(
+        get_enclave_bridge_mock(),
+        &hex::encode_upper(genesis_app_hash),
+        TEST_CHAIN_ID,
+        storage,
+        account_storage,
+    );
+
+    // Init Chain
+
+    let mut request_init_chain = RequestInitChain::default();
+    request_init_chain.set_time(timestamp);
+    request_init_chain.set_app_state_bytes(serde_json::to_vec(&init_config).unwrap());
+    request_init_chain.set_chain_id(String::from(TEST_CHAIN_ID));
+    let response_init_chain = app.init_chain(&request_init_chain);
+
+    let validators = response_init_chain.validators.to_vec();
+
+    assert_eq!(1, validators.len());
+    assert_eq!(
+        100000000000,
+        i64::from(
+            *app.validator_voting_power
+                .get(&staking_account_address)
+                .unwrap()
+        )
+    );
+
+    // Begin Block
+
+    let validator_address: TendermintValidatorAddress = into_tendermint_validator_pub_key(
+        app.validator_pubkeys.get(&staking_account_address).unwrap(),
+    )
+    .into();
+
+    let mut request_begin_block = RequestBeginBlock::default();
+    let mut header = Header::default();
+    header.time = Some(Timestamp::new()).into();
+    header.chain_id = TEST_CHAIN_ID.to_owned();
+    header.height = 2;
+
+    let mut validator = Validator::new();
+    validator.address = <[u8; 20]>::from(&validator_address).to_vec();
+
+    let mut vote_info = VoteInfo::new();
+    vote_info.validator = Some(validator).into();
+    vote_info.signed_last_block = false;
+
+    let mut last_commit_info = LastCommitInfo::new();
+    last_commit_info.votes = vec![vote_info].into();
+
+    request_begin_block.header = Some(header).into();
+    request_begin_block.last_commit_info = Some(last_commit_info).into();
+    app.begin_block(&request_begin_block);
+
+    assert_eq!(
+        TendermintVotePower::zero(),
+        *app.power_changed_in_block
+            .get(&staking_account_address)
+            .unwrap()
+    );
+
+    let account = get_account(&address, &app);
+    assert!(account.is_jailed());
+}