Merge branch 'develop' of https://github.com/stacks-network/stacks-core into feat/signer-two-phase-commit-impl

Author: jferrant

stacks-node/src/burnchains/bitcoin_regtest_controller.rs

@@ -31,7 +31,7 @@ use stacks::burnchains::bitcoin::indexer::{
     BitcoinIndexer, BitcoinIndexerConfig, BitcoinIndexerRuntime,
 };
 use stacks::burnchains::bitcoin::spv::SpvClient;
-use stacks::burnchains::bitcoin::BitcoinNetworkType;
+use stacks::burnchains::bitcoin::{BitcoinNetworkType, Error as btc_error};
 use stacks::burnchains::db::BurnchainDB;
 use stacks::burnchains::indexer::BurnchainIndexer;
 use stacks::burnchains::{
@@ -78,7 +78,9 @@ use url::Url;
 use super::super::operations::BurnchainOpSigner;
 use super::super::Config;
 use super::{BurnchainController, BurnchainTip, Error as BurnchainControllerError};
-use crate::burnchains::rpc::bitcoin_rpc_client::{BitcoinRpcClient, BitcoinRpcClientError};
+use crate::burnchains::rpc::bitcoin_rpc_client::{
+    BitcoinRpcClient, BitcoinRpcClientError, ImportDescriptorsRequest, Timestamp,
+};
 
 /// The number of bitcoin blocks that can have
 ///  passed since the UTXO cache was last refreshed before
@@ -302,6 +304,20 @@ impl<T> BitcoinRpcClientResultExt<T> for Result<T, BitcoinRpcClientError> {
     }
 }
 
+/// Represents errors that can occur when using [`BitcoinRegtestController`].
+#[derive(Debug, thiserror::Error)]
+pub enum BitcoinRegtestControllerError {
+    /// Error related to Bitcoin RPC failures.
+    #[error("Bitcoin RPC error: {0}")]
+    Rpc(#[from] BitcoinRpcClientError),
+    /// Error related to invalid or malformed [`Secp256k1PublicKey`].
+    #[error("Invalid public key: {0}")]
+    InvalidPublicKey(btc_error),
+}
+
+/// Alias for results returned from [`BitcoinRegtestController`] operations.
+pub type BitcoinRegtestControllerResult<T> = Result<T, BitcoinRegtestControllerError>;
+
 impl BitcoinRegtestController {
     pub fn new(config: Config, coordinator_channel: Option<CoordinatorChannels>) -> Self {
         BitcoinRegtestController::with_burnchain(config, coordinator_channel, None, None)
@@ -664,7 +680,10 @@ impl BitcoinRegtestController {
         };
 
         test_debug!("Import public key '{}'", &pubk.to_hex());
-        let _result = BitcoinRPCRequest::import_public_key(&self.config, &pubk);
+        let result = self.import_public_key(&pubk);
+        if let Err(error) = result {
+            warn!("Import public key '{}' failed: {error:?}", &pubk.to_hex());
+        }
 
         sleep_ms(1000);
 
@@ -740,13 +759,13 @@ impl BitcoinRegtestController {
     }
 
     /// Retrieve all loaded wallets.
-    pub fn list_wallets(&self) -> Result<Vec<String>, BitcoinRpcClientError> {
-        self.rpc_client.list_wallets()
+    pub fn list_wallets(&self) -> BitcoinRegtestControllerResult<Vec<String>> {
+        Ok(self.rpc_client.list_wallets()?)
     }
 
     /// Checks if the config-supplied wallet exists.
     /// If it does not exist, this function creates it.
-    pub fn create_wallet_if_dne(&self) -> Result<(), BitcoinRpcClientError> {
+    pub fn create_wallet_if_dne(&self) -> BitcoinRegtestControllerResult<()> {
         let wallets = self.list_wallets()?;
         let wallet = self.get_wallet_name();
         if !wallets.contains(wallet) {
@@ -807,7 +826,10 @@ impl BitcoinRegtestController {
                     // Assuming that miners are in charge of correctly operating their bitcoind nodes sounds
                     // reasonable to me.
                     // $ bitcoin-cli importaddress mxVFsFW5N4mu1HPkxPttorvocvzeZ7KZyk
-                    let _result = BitcoinRPCRequest::import_public_key(&self.config, &pubk);
+                    let result = self.import_public_key(&pubk);
+                    if let Err(error) = result {
+                        warn!("Import public key '{}' failed: {error:?}", &pubk.to_hex());
+                    }
                     sleep_ms(1000);
                 }
 
@@ -2111,7 +2133,7 @@ impl BitcoinRegtestController {
 
         for pk in pks {
             debug!("Import public key '{}'", &pk.to_hex());
-            if let Err(e) = BitcoinRPCRequest::import_public_key(&self.config, pk) {
+            if let Err(e) = self.import_public_key(pk) {
                 warn!("Error when importing pubkey: {e:?}");
             }
         }
@@ -2174,6 +2196,57 @@ impl BitcoinRegtestController {
     fn get_wallet_name(&self) -> &String {
         &self.config.burnchain.wallet_name
     }
+
+    /// Imports a public key into configured wallet by registering its
+    /// corresponding addresses as descriptors.
+    ///
+    /// This computes both **legacy (P2PKH)** and, if the miner is configured
+    /// with `segwit` enabled, also **SegWit (P2WPKH)** addresses, then imports
+    /// the related descriptors into the wallet.
+    pub fn import_public_key(
+        &self,
+        public_key: &Secp256k1PublicKey,
+    ) -> BitcoinRegtestControllerResult<()> {
+        let pkh = Hash160::from_data(&public_key.to_bytes())
+            .to_bytes()
+            .to_vec();
+        let (_, network_id) = self.config.burnchain.get_bitcoin_network();
+
+        // import both the legacy and segwit variants of this public key
+        let mut addresses = vec![BitcoinAddress::from_bytes_legacy(
+            network_id,
+            LegacyBitcoinAddressType::PublicKeyHash,
+            &pkh,
+        )
+        .map_err(BitcoinRegtestControllerError::InvalidPublicKey)?];
+
+        if self.config.miner.segwit {
+            addresses.push(
+                BitcoinAddress::from_bytes_segwit_p2wpkh(network_id, &pkh)
+                    .map_err(BitcoinRegtestControllerError::InvalidPublicKey)?,
+            );
+        }
+
+        for address in addresses.into_iter() {
+            debug!(
+                "Import address {address} for public key {}",
+                public_key.to_hex()
+            );
+
+            let descriptor = format!("addr({address})");
+            let info = self.rpc_client.get_descriptor_info(&descriptor)?;
+
+            let descr_req = ImportDescriptorsRequest {
+                descriptor: format!("addr({address})#{}", info.checksum),
+                timestamp: Timestamp::Time(0),
+                internal: Some(true),
+            };
+
+            self.rpc_client
+                .import_descriptors(self.get_wallet_name(), &[&descr_req])?;
+        }
+        Ok(())
+    }
 }
 
 impl BurnchainController for BitcoinRegtestController {
@@ -2643,64 +2716,6 @@ impl BitcoinRPCRequest {
         Ok(())
     }
 
-    pub fn import_public_key(config: &Config, public_key: &Secp256k1PublicKey) -> RPCResult<()> {
-        let pkh = Hash160::from_data(&public_key.to_bytes())
-            .to_bytes()
-            .to_vec();
-        let (_, network_id) = config.burnchain.get_bitcoin_network();
-
-        // import both the legacy and segwit variants of this public key
-        let mut addresses = vec![BitcoinAddress::from_bytes_legacy(
-            network_id,
-            LegacyBitcoinAddressType::PublicKeyHash,
-            &pkh,
-        )
-        .expect("Public key incorrect")];
-
-        if config.miner.segwit {
-            addresses.push(
-                BitcoinAddress::from_bytes_segwit_p2wpkh(network_id, &pkh)
-                    .expect("Public key incorrect"),
-            );
-        }
-
-        for address in addresses.into_iter() {
-            debug!(
-                "Import address {address} for public key {}",
-                public_key.to_hex()
-            );
-
-            let payload = BitcoinRPCRequest {
-                method: "getdescriptorinfo".to_string(),
-                params: vec![format!("addr({address})").into()],
-                id: "stacks".to_string(),
-                jsonrpc: "2.0".to_string(),
-            };
-
-            let result = BitcoinRPCRequest::send(config, payload)?;
-            let checksum = result
-                .get("result")
-                .and_then(|res| res.as_object())
-                .and_then(|obj| obj.get("checksum"))
-                .and_then(|checksum_val| checksum_val.as_str())
-                .ok_or(RPCError::Bitcoind(format!(
-                    "Did not receive an object with `checksum` from `getdescriptorinfo \"{address}\"`",
-                )))?;
-
-            let payload = BitcoinRPCRequest {
-                method: "importdescriptors".to_string(),
-                params: vec![
-                    json!([{ "desc": format!("addr({address})#{checksum}"), "timestamp": 0, "internal": true }]),
-                ],
-                id: "stacks".to_string(),
-                jsonrpc: "2.0".to_string(),
-            };
-
-            BitcoinRPCRequest::send(config, payload)?;
-        }
-        Ok(())
-    }
-
     pub fn send(config: &Config, payload: BitcoinRPCRequest) -> RPCResult<serde_json::Value> {
         let request = BitcoinRPCRequest::build_rpc_request(config, &payload);
         let timeout = Duration::from_secs(u64::from(config.burnchain.timeout));
@@ -3430,6 +3445,99 @@ mod tests {
         );
     }
 
+    #[test]
+    #[ignore]
+    fn test_import_public_key_ok() {
+        if env::var("BITCOIND_TEST") != Ok("1".into()) {
+            return;
+        }
+
+        let miner_pubkey = utils::create_miner1_pubkey();
+
+        let config = utils::create_config();
+
+        let mut btcd_controller = BitcoinCoreController::from_stx_config(&config);
+        btcd_controller
+            .start_bitcoind()
+            .expect("bitcoind should be started!");
+
+        let btc_controller = BitcoinRegtestController::new(config.clone(), None);
+        btc_controller
+            .create_wallet_if_dne()
+            .expect("Wallet should be created!");
+
+        let result = btc_controller.import_public_key(&miner_pubkey);
+        assert!(
+            result.is_ok(),
+            "Should be ok, got err instead: {:?}",
+            result.unwrap_err()
+        );
+    }
+
+    #[test]
+    #[ignore]
+    fn test_import_public_key_twice_ok() {
+        if env::var("BITCOIND_TEST") != Ok("1".into()) {
+            return;
+        }
+
+        let miner_pubkey = utils::create_miner1_pubkey();
+
+        let config = utils::create_config();
+
+        let mut btcd_controller = BitcoinCoreController::from_stx_config(&config);
+        btcd_controller
+            .start_bitcoind()
+            .expect("bitcoind should be started!");
+
+        let btc_controller = BitcoinRegtestController::new(config.clone(), None);
+        btc_controller
+            .create_wallet_if_dne()
+            .expect("Wallet should be created!");
+
+        btc_controller
+            .import_public_key(&miner_pubkey)
+            .expect("Import should be ok: first time!");
+
+        //ok, but it is basically a no-op
+        let result = btc_controller.import_public_key(&miner_pubkey);
+        assert!(
+            result.is_ok(),
+            "Should be ok, got err instead: {:?}",
+            result.unwrap_err()
+        );
+    }
+
+    #[test]
+    #[ignore]
+    fn test_import_public_key_segwit_ok() {
+        if env::var("BITCOIND_TEST") != Ok("1".into()) {
+            return;
+        }
+
+        let miner_pubkey = utils::create_miner1_pubkey();
+
+        let mut config = utils::create_config();
+        config.miner.segwit = true;
+
+        let mut btcd_controller = BitcoinCoreController::from_stx_config(&config);
+        btcd_controller
+            .start_bitcoind()
+            .expect("bitcoind should be started!");
+
+        let btc_controller = BitcoinRegtestController::new(config.clone(), None);
+        btc_controller
+            .create_wallet_if_dne()
+            .expect("Wallet should be created!");
+
+        let result = btc_controller.import_public_key(&miner_pubkey);
+        assert!(
+            result.is_ok(),
+            "Should be ok, got err instead: {:?}",
+            result.unwrap_err()
+        );
+    }
+
     /// Tests related to Leader Block Commit operation
     mod leader_commit_op {
         use super::*;

stacks-node/src/burnchains/rpc/bitcoin_rpc_client/mod.rs

@@ -268,7 +268,7 @@ fn convert_sat_to_btc_string(amount: u64) -> String {
 }
 
 /// Represents an error message returned when importing descriptors fails.
-#[derive(Debug, Clone, Deserialize)]
+#[derive(Debug, Clone, Deserialize, PartialEq)]
 pub struct ImportDescriptorsErrorMessage {
     /// Numeric error code identifying the type of error.
     pub code: i64,

stacks-node/src/tests/bitcoin_rpc_integrations.rs

@@ -689,6 +689,50 @@ fn test_import_descriptor_ok() {
         .expect("import descriptor ok!");
     assert_eq!(1, response.len());
     assert!(response[0].success);
+    assert_eq!(0, response[0].warnings.len());
+    assert_eq!(None, response[0].error);
+}
+
+#[ignore]
+#[test]
+fn test_import_descriptor_twice_ok() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    let config = utils::create_stx_config();
+    let wallet = &config.burnchain.wallet_name;
+
+    let mut btcd_controller = BitcoinCoreController::from_stx_config(&config);
+    btcd_controller
+        .start_bitcoind()
+        .expect("bitcoind should be started!");
+
+    let client = BitcoinRpcClient::from_stx_config(&config).expect("Client creation ok!");
+    client
+        .create_wallet(wallet, Some(true))
+        .expect("create wallet ok!");
+
+    let address = "mqqxPdP1dsGk75S7ta2nwyU8ujDnB2Yxvu";
+    let checksum = "spfcmvsn";
+
+    let desc_req = ImportDescriptorsRequest {
+        descriptor: format!("addr({address})#{checksum}"),
+        timestamp: Timestamp::Time(0),
+        internal: Some(true),
+    };
+
+    let _ = client
+        .import_descriptors(wallet, &[&desc_req])
+        .expect("import descriptor ok: first time!");
+
+    let response = client
+        .import_descriptors(wallet, &[&desc_req])
+        .expect("import descriptor ok: second time!");
+    assert_eq!(1, response.len());
+    assert!(response[0].success);
+    assert_eq!(0, response[0].warnings.len());
+    assert_eq!(None, response[0].error);
 }
 
 #[ignore]