Merge branch 'develop' of https://github.com/stacks-network/stacks-core…

… into fix/clippy-ci-stacks-lib-needless-borrow

.cargo/config.toml

@@ -1,6 +1,7 @@
 [alias]
 stacks-node = "run --package stacks-node --"
 fmt-stacks = "fmt -- --config group_imports=StdExternalCrate,imports_granularity=Module"
+clippy-stacks = "clippy -p libstackerdb -p stacks-signer -p pox-locking -p clarity -p libsigner -p stacks-common --no-deps --tests --all-features -- -D warnings"
 
 # Uncomment to improve performance slightly, at the cost of portability
 #   * Note that native binaries may not run on CPUs that are different from the build machine

.github/workflows/bitcoin-tests.yml

@@ -167,6 +167,7 @@ jobs:
           - tests::nakamoto_integrations::sip029_coinbase_change
           - tests::nakamoto_integrations::clarity_cost_spend_down
           - tests::nakamoto_integrations::v3_blockbyheight_api_endpoint
+          - tests::nakamoto_integrations::mine_invalid_principal_from_consensus_buff
           - tests::nakamoto_integrations::test_tenure_extend_from_flashblocks
           # TODO: enable these once v1 signer is supported by a new nakamoto epoch
           # - tests::signer::v1::dkg

.github/workflows/clippy.yml

@@ -34,7 +34,4 @@ jobs:
           components: clippy
       - name: Clippy
         id: clippy
-        uses: actions-rs/clippy-check@v1
-        with:
-          token: ${{ secrets.GITHUB_TOKEN }}
-          args: -p libstackerdb -p stacks-signer -p pox-locking -p clarity -p libsigner -p stacks-common --no-deps --tests --all-features -- -D warnings
+        run: cargo clippy-stacks

CHANGELOG.md

@@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to the versioning scheme outlined in the [README.md](README.md).
 
-## [Unreleased]
+## [3.1.0.0.4]
 
 ### Added
 

CONTRIBUTING.md

@@ -387,6 +387,18 @@ You can automatically reformat your commit via:
 cargo fmt-stacks
 ```
 
+## Clippy Warnings
+
+PRs will be checked against `clippy` and will _fail_ if any clippy warnings are generated.
+Unfortunately, not all existing clippy warnings have been addressed throughout stacks-core, so arguments must be passed via the command line. 
+Therefore, we handle `clippy` configurations using a Cargo alias: `cargo clippy-stacks`
+
+You can check what warnings need to be addressed locally via: 
+
+```bash
+cargo clippy-stacks
+```
+
 ## Comments
 
 Comments are very important for the readability and correctness of the codebase. The purpose of comments is:

clarity/src/libclarity.rs

@@ -60,7 +60,8 @@ pub mod boot_util {
     pub fn boot_code_id(name: &str, mainnet: bool) -> QualifiedContractIdentifier {
         let addr = boot_code_addr(mainnet);
         QualifiedContractIdentifier::new(
-            addr.into(),
+            addr.try_into()
+                .expect("FATAL: boot contract addr is not a legal principal"),
             ContractName::try_from(name.to_string())
                 .expect("FATAL: boot contract name is not a legal ContractName"),
         )

clarity/src/vm/contexts.rs

@@ -2134,20 +2134,14 @@ mod test {
         mut tl_env_factory: TopLevelMemoryEnvironmentGenerator,
     ) {
         let mut env = tl_env_factory.get_env(epoch);
-        let u1 = StacksAddress {
-            version: 0,
-            bytes: Hash160([1; 20]),
-        };
-        let u2 = StacksAddress {
-            version: 0,
-            bytes: Hash160([2; 20]),
-        };
+        let u1 = StacksAddress::new(0, Hash160([1; 20])).unwrap();
+        let u2 = StacksAddress::new(0, Hash160([2; 20])).unwrap();
         // insufficient balance must be a non-includable transaction. it must error here,
         //  not simply rollback the tx and squelch the error as includable.
         let e = env
             .stx_transfer(
-                &PrincipalData::from(u1),
-                &PrincipalData::from(u2),
+                &PrincipalData::try_from(u1).unwrap(),
+                &PrincipalData::try_from(u2).unwrap(),
                 1000,
                 &BuffData::empty(),
             )

clarity/src/vm/functions/conversions.rs

@@ -20,6 +20,7 @@ use crate::vm::errors::{
     check_argument_count, CheckErrors, InterpreterError, InterpreterResult as Result,
 };
 use crate::vm::representations::SymbolicExpression;
+use crate::vm::types::serialization::SerializationError;
 use crate::vm::types::SequenceSubtype::BufferType;
 use crate::vm::types::TypeSignature::SequenceType;
 use crate::vm::types::{
@@ -276,6 +277,9 @@ pub fn from_consensus_buff(
         env.epoch().value_sanitizing(),
     ) {
         Ok(value) => value,
+        Err(SerializationError::UnexpectedSerialization) => {
+            return Err(CheckErrors::Expects("UnexpectedSerialization".into()).into())
+        }
         Err(_) => return Ok(Value::none()),
     };
     if !type_arg.admits(env.epoch(), &result)? {

clarity/src/vm/functions/crypto.rs

@@ -27,10 +27,7 @@ use crate::vm::errors::{
     check_argument_count, CheckErrors, InterpreterError, InterpreterResult as Result,
 };
 use crate::vm::representations::SymbolicExpression;
-use crate::vm::types::{
-    BuffData, SequenceData, StacksAddressExtensions, TypeSignature, Value, BUFF_32, BUFF_33,
-    BUFF_65,
-};
+use crate::vm::types::{BuffData, SequenceData, TypeSignature, Value, BUFF_32, BUFF_33, BUFF_65};
 use crate::vm::{eval, ClarityVersion, Environment, LocalContext};
 
 macro_rules! native_hash_func {
@@ -120,7 +117,7 @@ pub fn special_principal_of(
         } else {
             pubkey_to_address_v1(pub_key)?
         };
-        let principal = addr.to_account_principal();
+        let principal = addr.into();
         Ok(Value::okay(Value::Principal(principal))
             .map_err(|_| InterpreterError::Expect("Failed to construct ok".into()))?)
     } else {

clarity/src/vm/functions/principals.rs

@@ -58,15 +58,10 @@ pub fn special_is_standard(
     runtime_cost(ClarityCostFunction::IsStandard, env, 0)?;
     let owner = eval(&args[0], env, context)?;
 
-    let version = match owner {
-        Value::Principal(PrincipalData::Standard(StandardPrincipalData(version, _bytes))) => {
-            version
-        }
-        Value::Principal(PrincipalData::Contract(QualifiedContractIdentifier {
-            issuer,
-            name: _,
-        })) => issuer.0,
-        _ => return Err(CheckErrors::TypeValueError(TypeSignature::PrincipalType, owner).into()),
+    let version = if let Value::Principal(ref p) = owner {
+        p.version()
+    } else {
+        return Err(CheckErrors::TypeValueError(TypeSignature::PrincipalType, owner).into());
     };
 
     Ok(Value::Bool(version_matches_current_network(
@@ -161,10 +156,12 @@ pub fn special_principal_destruct(
     let principal = eval(&args[0], env, context)?;
 
     let (version_byte, hash_bytes, name_opt) = match principal {
-        Value::Principal(PrincipalData::Standard(StandardPrincipalData(version, bytes))) => {
+        Value::Principal(PrincipalData::Standard(p)) => {
+            let (version, bytes) = p.destruct();
             (version, bytes, None)
         }
         Value::Principal(PrincipalData::Contract(QualifiedContractIdentifier { issuer, name })) => {
+            let issuer = issuer.destruct();
             (issuer.0, issuer.1, Some(name))
         }
         _ => {
@@ -254,7 +251,7 @@ pub fn special_principal_construct(
     // Construct the principal.
     let mut transfer_buffer = [0u8; 20];
     transfer_buffer.copy_from_slice(verified_hash_bytes);
-    let principal_data = StandardPrincipalData(version_byte, transfer_buffer);
+    let principal_data = StandardPrincipalData::new(version_byte, transfer_buffer)?;
 
     let principal = if let Some(name) = name_opt {
         // requested a contract principal.  Verify that the `name` is a valid ContractName.

clarity/src/vm/test_util/mod.rs

@@ -108,7 +108,7 @@ impl From<&StacksPrivateKey> for StandardPrincipalData {
             &vec![StacksPublicKey::from_private(o)],
         )
         .unwrap();
-        StandardPrincipalData::from(stacks_addr)
+        StandardPrincipalData::try_from(stacks_addr).unwrap()
     }
 }
 

clarity/src/vm/tests/datamaps.rs

@@ -642,15 +642,15 @@ fn bad_define_maps() {
         "(define-map lists { name: int } contents 5)",
         "(define-map lists { name: int } { contents: (list 5 0 int) })",
     ];
-    let mut expected: Vec<Error> = vec![
+    let expected: Vec<Error> = vec![
         CheckErrors::BadSyntaxExpectedListOfPairs.into(),
         CheckErrors::UnknownTypeName("contents".to_string()).into(),
         CheckErrors::ExpectedName.into(),
         CheckErrors::IncorrectArgumentCount(3, 4).into(),
         CheckErrors::InvalidTypeDescription.into(),
     ];
 
-    for (test, expected_err) in tests.iter().zip(expected.drain(..)) {
+    for (test, expected_err) in tests.iter().zip(expected.into_iter()) {
         let outcome = execute(test).unwrap_err();
         assert_eq!(outcome, expected_err);
     }
@@ -666,7 +666,7 @@ fn bad_tuples() {
         "(get name five (tuple (name 1)))",
         "(get 1234 (tuple (name 1)))",
     ];
-    let mut expected = vec![
+    let expected = vec![
         CheckErrors::NameAlreadyUsed("name".into()),
         CheckErrors::BadSyntaxBinding,
         CheckErrors::BadSyntaxBinding,
@@ -678,7 +678,7 @@ fn bad_tuples() {
         CheckErrors::ExpectedName,
     ];
 
-    for (test, expected_err) in tests.iter().zip(expected.drain(..)) {
+    for (test, expected_err) in tests.iter().zip(expected.into_iter()) {
         let outcome = execute(test).unwrap_err();
         assert_eq!(outcome, expected_err.into());
     }

clarity/src/vm/tests/principals.rs

@@ -668,7 +668,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(22, transfer_buffer)
+                StandardPrincipalData::new(22, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -688,7 +688,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(20, transfer_buffer)
+                StandardPrincipalData::new(20, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -710,7 +710,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(22, transfer_buffer),
+                    StandardPrincipalData::new(22, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -734,7 +734,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(20, transfer_buffer),
+                    StandardPrincipalData::new(20, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -756,7 +756,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(26, transfer_buffer)
+                StandardPrincipalData::new(26, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -776,7 +776,7 @@ fn test_principal_construct_good() {
         Value::Response(ResponseData {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Standard(
-                StandardPrincipalData(21, transfer_buffer)
+                StandardPrincipalData::new(21, transfer_buffer).unwrap()
             )))
         }),
         execute_with_parameters(
@@ -798,7 +798,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(26, transfer_buffer),
+                    StandardPrincipalData::new(26, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -822,7 +822,7 @@ fn test_principal_construct_good() {
             committed: true,
             data: Box::new(Value::Principal(PrincipalData::Contract(
                 QualifiedContractIdentifier::new(
-                    StandardPrincipalData(21, transfer_buffer),
+                    StandardPrincipalData::new(21, transfer_buffer).unwrap(),
                     "hello-world".into()
                 )
             )))
@@ -853,15 +853,14 @@ fn create_principal_from_strings(
     if let Some(name) = name {
         // contract principal requested
         Value::Principal(PrincipalData::Contract(QualifiedContractIdentifier::new(
-            StandardPrincipalData(version_array[0], principal_array),
+            StandardPrincipalData::new(version_array[0], principal_array).unwrap(),
             name.into(),
         )))
     } else {
         // standard principal requested
-        Value::Principal(PrincipalData::Standard(StandardPrincipalData(
-            version_array[0],
-            principal_array,
-        )))
+        Value::Principal(PrincipalData::Standard(
+            StandardPrincipalData::new(version_array[0], principal_array).unwrap(),
+        ))
     }
 }
 

clarity/src/vm/tests/simple_apply_eval.rs

@@ -430,7 +430,7 @@ fn test_secp256k1() {
     )
     .unwrap();
     eprintln!("addr from privk {:?}", &addr);
-    let principal = addr.to_account_principal();
+    let principal = addr.try_into().unwrap();
     if let PrincipalData::Standard(data) = principal {
         eprintln!("test_secp256k1 principal {:?}", data.to_address());
     }
@@ -446,7 +446,7 @@ fn test_secp256k1() {
     )
     .unwrap();
     eprintln!("addr from hex {:?}", addr);
-    let principal = addr.to_account_principal();
+    let principal: PrincipalData = addr.try_into().unwrap();
     if let PrincipalData::Standard(data) = principal.clone() {
         eprintln!("test_secp256k1 principal {:?}", data.to_address());
     }
@@ -491,8 +491,9 @@ fn test_principal_of_fix() {
         .unwrap()],
     )
     .unwrap()
-    .to_account_principal();
-    let testnet_principal = StacksAddress::from_public_keys(
+    .try_into()
+    .unwrap();
+    let testnet_principal: PrincipalData = StacksAddress::from_public_keys(
         C32_ADDRESS_VERSION_TESTNET_SINGLESIG,
         &AddressHashMode::SerializeP2PKH,
         1,
@@ -502,7 +503,8 @@ fn test_principal_of_fix() {
         .unwrap()],
     )
     .unwrap()
-    .to_account_principal();
+    .try_into()
+    .unwrap();
 
     // Clarity2, mainnet, should have a mainnet principal.
     assert_eq!(