Merge pull request #152 from alekseysidorov/validate_config

Validate anchoring configuration parameters. [ECR-3633]

Cargo.toml

@@ -18,20 +18,22 @@ travis-ci = { repository = "exonum/exonum-btc-anchoring" }
 [dependencies]
 bitcoin = { version = "0.18", features = ["serde"] }
 bitcoin_hashes = { version = "0.3", features = ["serde"] }
+bitcoincore-rpc = "0.7.0"
 btc-transaction-utils = "0.6"
 byteorder = "1.3"
 derive_more = "0.15"
 exonum = "0.12.0"
 exonum-cli = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
+exonum-crypto = { features = ["with-protobuf"], git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
 exonum-derive = "0.12.0"
 exonum-merkledb = "0.12.0"
-exonum-testkit = "0.12.0"
 exonum-proto = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
-exonum-crypto = { features = ["with-protobuf"], git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
+exonum-testkit = "0.12.0"
 failure = "0.1"
 failure_derive = "0.1"
 futures = "0.1"
 hex = "0.4"
+jsonrpc = "0.11"
 log = "0.4"
 protobuf = { version = "2.8", features = ["with-serde"] }
 rand = "0.4"
@@ -42,7 +44,6 @@ serde_derive = "1.0"
 serde_json = "1.0"
 serde_str = "0.1"
 structopt = "0.3"
-bitcoincore-rpc = "0.7.0"
 
 [dev-dependencies]
 proptest = "0.9"
@@ -52,7 +53,7 @@ exonum-build = "0.12.0"
 
 [patch.crates-io]
 exonum = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
-exonum-merkledb = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
+exonum-build = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
 exonum-derive = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
+exonum-merkledb = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
 exonum-testkit = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }
-exonum-build = { git = "https://github.com/exonum/exonum.git", rev = "7404bacc109da33bdbf5c3a5a09f3f56dae965e8" }

exonum-dictionary.txt

@@ -79,6 +79,7 @@ isvalid
 iswatchonly
 JJBZ
 Jjqe
+jsonrpc
 keepalive
 keyhash
 keypair
@@ -93,8 +94,8 @@ libsodium
 libssl
 listunspent
 locktime
-mainnet
 Mainnet
+mainnet
 maintainer's
 maplit
 markdownlint
@@ -154,8 +155,8 @@ pubkeys
 PUSHBYTES
 readonly
 reddit
-Regtest
 regtest
+Regtest
 reimplemented
 repr
 reqwest

launcher/exonum_btc_anchoring_plugin/plugin.py

@@ -41,7 +41,7 @@ def load_spec(self, loader: ProtobufLoader, instance: Instance) -> bytes:
             service_module = import_or_load_module(loader, instance, "service")
             btc_types_module = import_or_load_module(
                 loader, instance, "btc_types")
-            helpers_module = import_or_load_module(loader, instance, "helpers")
+            exonum_types_module = import_or_load_module(loader, instance, "types")
 
             # Create config message
             config = service_module.Config()
@@ -52,7 +52,7 @@ def load_spec(self, loader: ProtobufLoader, instance: Instance) -> bytes:
 
             anchoring_keys = []
             for keypair in instance.config["anchoring_keys"]:
-                service_key = helpers_module.PublicKey(
+                service_key = exonum_types_module.PublicKey(
                     data=bytes.fromhex(keypair["service_key"]))
                 bitcoin_key = btc_types_module.PublicKey(
                     data=bytes.fromhex(keypair["bitcoin_key"]))

src/config.rs

@@ -43,6 +43,13 @@ impl Default for Config {
 impl Config {
     /// Current limit on the number of keys in a redeem script on the Bitcoin network.
     const MAX_NODES_COUNT: usize = 20;
+    /// Minimal fee in satoshis for Bitcoin transaction.
+    const MIN_TOTAL_TX_FEE: u64 = 1000;
+    /// Minimal total transaction size according to
+    /// https://bitcoin.stackexchange.com/questions/1195/how-to-calculate-transaction-size-before-sending-legacy-non-segwit-p2pkh-p2sh     
+    const MIN_TX_LEN: u64 = 10 + 146 + 33 + 81;
+    /// Minimal enough transaction fee per byte.
+    const MIN_TX_FEE: u64 = Self::MIN_TOTAL_TX_FEE / Self::MIN_TX_LEN + 1; // Round up.
 
     /// Creates Bitcoin anchoring config instance with default parameters for the
     /// given Bitcoin network and public keys of participants.
@@ -111,12 +118,24 @@ impl ValidateInput for Config {
     type Error = failure::Error;
 
     fn validate(&self) -> Result<(), Self::Error> {
+        ensure!(
+            !self.anchoring_keys.is_empty(),
+            "The list of anchoring keys must not be empty."
+        );
         ensure!(
             self.anchoring_keys.len() <= Self::MAX_NODES_COUNT,
             "Too many anchoring nodes: amount of anchoring nodes should be less or equal than the {}.",
             Self::MAX_NODES_COUNT
         );
-        // TODO Validate other parameters. [ECR-3633]
+        ensure!(
+            self.anchoring_interval > 0,
+            "Anchoring interval should be greater than zero."
+        );
+        ensure!(
+            self.transaction_fee >= Self::MIN_TX_FEE,
+            "Transaction fee should be greater than {}",
+            Self::MIN_TX_FEE
+        );
 
         // Verify that the redeem script is suitable.
         RedeemScriptBuilder::with_public_keys(self.anchoring_keys.iter().map(|x| x.bitcoin_key.0))
@@ -128,7 +147,10 @@ impl ValidateInput for Config {
 
 #[cfg(test)]
 mod tests {
-    use exonum::{crypto, helpers::Height};
+    use exonum::{
+        crypto,
+        helpers::{Height, ValidateInput},
+    };
 
     use bitcoin::network::constants::Network;
     use btc_transaction_utils::test_data::secp_gen_keypair;
@@ -137,14 +159,18 @@ mod tests {
 
     use super::Config;
 
-    #[test]
-    fn test_config_serde() {
-        let public_keys = (0..4)
+    fn gen_anchoring_keys(network: bitcoin::Network, count: usize) -> Vec<AnchoringKeys> {
+        (0..count)
             .map(|_| AnchoringKeys {
-                bitcoin_key: secp_gen_keypair(Network::Bitcoin).0.into(),
+                bitcoin_key: secp_gen_keypair(network).0.into(),
                 service_key: crypto::gen_keypair().0,
             })
-            .collect::<Vec<_>>();
+            .collect::<Vec<_>>()
+    }
+
+    #[test]
+    fn config_serde() {
+        let public_keys = gen_anchoring_keys(Network::Bitcoin, 4);
 
         let config = Config::with_public_keys(Network::Bitcoin, public_keys).unwrap();
         assert_eq!(config.redeem_script().content().quorum, 3);
@@ -155,13 +181,8 @@ mod tests {
     }
 
     #[test]
-    fn test_config_anchoring_height() {
-        let public_keys = (0..4)
-            .map(|_| AnchoringKeys {
-                bitcoin_key: secp_gen_keypair(Network::Bitcoin).0.into(),
-                service_key: crypto::gen_keypair().0,
-            })
-            .collect::<Vec<_>>();
+    fn config_anchoring_height() {
+        let public_keys = gen_anchoring_keys(Network::Bitcoin, 4);
 
         let mut config = Config::with_public_keys(Network::Bitcoin, public_keys).unwrap();
         config.anchoring_interval = 1000;
@@ -184,4 +205,50 @@ mod tests {
     }
 
     // TODO test validation of the Bitcoin anchoring config
+
+    #[test]
+    fn config_validate_errors() {
+        let test_cases = [
+            (
+                Config::default(),
+                "The list of anchoring keys must not be empty",
+            ),
+            (
+                Config {
+                    anchoring_keys: gen_anchoring_keys(bitcoin::Network::Regtest, 30),
+                    ..Config::default()
+                },
+                "Too many anchoring nodes: amount of anchoring nodes should be less or equal",
+            ),
+            (
+                Config {
+                    anchoring_keys: gen_anchoring_keys(bitcoin::Network::Regtest, 4),
+                    anchoring_interval: 0,
+                    ..Config::default()
+                },
+                "Anchoring interval should be greater than zero",
+            ),
+            (
+                Config {
+                    anchoring_keys: gen_anchoring_keys(bitcoin::Network::Regtest, 4),
+                    transaction_fee: 0,
+                    ..Config::default()
+                },
+                "Transaction fee should be greater than",
+            ),
+            (
+                Config {
+                    anchoring_keys: gen_anchoring_keys(bitcoin::Network::Regtest, 4),
+                    transaction_fee: 3,
+                    ..Config::default()
+                },
+                "Transaction fee should be greater than",
+            ),
+        ];
+
+        for (config, expected_err) in &test_cases {
+            let actual_err = config.validate().unwrap_err().to_string();
+            assert!(actual_err.contains(expected_err), actual_err);
+        }
+    }
 }

src/sync/bitcoin_relay.rs

@@ -15,18 +15,47 @@
 //! Collections of helpers for synchronization with the Bitcoin network.
 
 use bitcoincore_rpc::RpcApi;
+use jsonrpc::Error as JsonRpcError;
 
 use crate::btc;
 
+/// Status of the transaction in the Bitcoin network.
+#[derive(Debug, Copy, Clone, PartialEq)]
+pub enum TransactionStatus {
+    /// Transaction is unknown in the Bitcoin network.
+    Unknown,
+    /// The transaction is not committed, but presented in the Bitcoin node memory pool.
+    Mempool,
+    /// The transaction was completed to the Bitcoin blockchain with the specified number
+    /// of confirmations.
+    Committed(u32),
+}
+
+impl TransactionStatus {
+    /// Checks that this transaction is known by the Bitcoin network.
+    pub fn is_known(self) -> bool {
+        self != TransactionStatus::Unknown
+    }
+
+    /// Returns number of transaction confirmations in Bitcoin blockchain.
+    pub fn confirmations(self) -> Option<u32> {
+        if let TransactionStatus::Committed(confirmations) = self {
+            Some(confirmations)
+        } else {
+            None
+        }
+    }
+}
+
 /// Describes communication with the Bitcoin network node.
 pub trait BitcoinRelay {
     /// Error type for the current Bitcoin relay implementation.
     type Error;
     /// Sends a raw transaction to the Bitcoin network node.
     fn send_transaction(&self, transaction: &btc::Transaction)
         -> Result<btc::Sha256d, Self::Error>;
-    /// Gets the number of transaction confirmations with the specified identifier.
-    fn transaction_confirmations(&self, id: btc::Sha256d) -> Result<Option<u32>, Self::Error>;
+    /// Gets status for the transaction with the specified identifier.
+    fn transaction_status(&self, id: btc::Sha256d) -> Result<TransactionStatus, Self::Error>;
 }
 
 impl BitcoinRelay for bitcoincore_rpc::Client {
@@ -40,14 +69,20 @@ impl BitcoinRelay for bitcoincore_rpc::Client {
             .map(btc::Sha256d)
     }
 
-    fn transaction_confirmations(&self, id: btc::Sha256d) -> Result<Option<u32>, Self::Error> {
-        let result = match self.get_raw_transaction_verbose(id.as_ref(), None) {
-            Ok(result) => result,
+    fn transaction_status(&self, id: btc::Sha256d) -> Result<TransactionStatus, Self::Error> {
+        match self.get_raw_transaction_verbose(id.as_ref(), None) {
+            Ok(info) => {
+                let status = match info.confirmations {
+                    None => TransactionStatus::Mempool,
+                    Some(num) => TransactionStatus::Committed(num),
+                };
+                Ok(status)
+            }
             // TODO Write more graceful error handling. [ECR-3222]
-            Err(bitcoincore_rpc::Error::JsonRpc(_)) => return Ok(None),
-            Err(e) => return Err(e),
-        };
-
-        Ok(result.confirmations)
+            Err(bitcoincore_rpc::Error::JsonRpc(JsonRpcError::Rpc(_))) => {
+                Ok(TransactionStatus::Unknown)
+            }
+            Err(e) => Err(e),
+        }
     }
 }