Merge pull request #6448 from hstove/fix/cost-error-undefined-var

fix: early exit for top-level undefined errors

Author: hstove

CHANGELOG.md

@@ -25,6 +25,10 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
   expressive (#6337)
 - Removed affirmation maps logic throughout, upgrading chainstate DB schema to 11 and burnchain DB schema to 3 (#6314)
 
+### Changed
+
+- When a contract deploy is analyzed, it will no longer throw a `CostError` when the contract contains an undefined top-level variable. Instead, it will throw a `UndefinedVariable` error.
+
 ## [3.2.0.0.1]
 
 ### Added

clarity/src/vm/analysis/type_checker/v2_05/mod.rs

@@ -711,6 +711,13 @@ impl<'a, 'b> TypeChecker<'a, 'b> {
         } else if let Some(type_result) = context.lookup_trait_reference_type(name) {
             Ok(TypeSignature::TraitReferenceType(type_result.clone()))
         } else {
+            // Special case where a top-level is being looked up, which must
+            // be undefined. This early error prevents a cost function error
+            // due to `context.depth` being 0.
+            if context.depth == 0 {
+                return Err(CheckErrors::UndefinedVariable(name.to_string()).into());
+            }
+
             runtime_cost(
                 ClarityCostFunction::AnalysisLookupVariableDepth,
                 self,

clarity/src/vm/analysis/type_checker/v2_1/mod.rs

@@ -1423,6 +1423,13 @@ impl<'a, 'b> TypeChecker<'a, 'b> {
                 type_result.clone(),
             )))
         } else {
+            // Special case where a top-level is being looked up, which must
+            // be undefined. This early error prevents a cost function error
+            // due to `context.depth` being 0.
+            if context.depth == 0 {
+                return Err(CheckErrors::UndefinedVariable(name.to_string()).into());
+            }
+
             runtime_cost(
                 ClarityCostFunction::AnalysisLookupVariableDepth,
                 self,

clarity/src/vm/analysis/type_checker/v2_1/tests/contracts.rs

@@ -283,7 +283,7 @@ fn test_names_tokens_contracts_interface() {
                     {{ "name": "tn1", "type": "bool" }},
                     {{ "name": "tn2", "type": "int128" }},
                     {{ "name": "tn3", "type": {{ "buffer": {{ "length": 1 }} }}}}
-                ] }} }} 
+                ] }} }}
             }},
             {{ "name": "f11",
                 "access": "private",
@@ -416,7 +416,7 @@ fn test_names_tokens_contracts_interface() {
                                     "name": "n2",
                                     "type": "bool"
                                 }}
-                            ] 
+                            ]
                         }}
                     }}]
                 }}
@@ -3537,6 +3537,19 @@ fn clarity_trait_experiments_cross_epochs(
     };
 }
 
+#[apply(test_clarity_versions)]
+fn clarity_trait_experiments_undefined_top_variable(
+    #[case] version: ClarityVersion,
+    #[case] epoch: StacksEpochId,
+) {
+    let mut marf = MemoryBackingStore::new();
+    let mut db = marf.as_analysis_db();
+    let err = db
+        .execute(|db| load_versioned(db, "undefined-top-variable", version, epoch))
+        .unwrap_err();
+    assert!(err.starts_with("UndefinedVariable(\"foo\")"));
+}
+
 /// Pass various types to `contract-hash?`
 #[apply(test_clarity_versions)]
 fn test_contract_hash(#[case] version: ClarityVersion, #[case] epoch: StacksEpochId) {

clarity/src/vm/analysis/type_checker/v2_1/tests/contracts/undefined-top-variable.clar

@@ -0,0 +1 @@
+foo

stacks-node/src/tests/signer/v0.rs

@@ -20,6 +20,7 @@ use std::sync::{Arc, Mutex};
 use std::time::{Duration, Instant};
 use std::{env, thread};
 
+use clarity::vm::costs::ExecutionCost;
 use clarity::vm::types::PrincipalData;
 use libsigner::v0::messages::{
     BlockAccepted, BlockRejection, BlockResponse, MessageSlotID, MinerSlotID, PeerInfo, RejectCode,
@@ -18385,6 +18386,94 @@ fn multiversioned_signer_protocol_version_calculation() {
     signer_test.shutdown();
 }
 
+/// This is a test for backwards compatibility regarding
+/// how contracts with an undefined top-level variable are handled.
+///
+/// Critically, we want to ensure that the cost of the block, along with
+/// the resulting block hash, are the same.
+#[test]
+#[ignore]
+fn contract_with_undefined_variable_compat() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    let num_signers = 5;
+    let sender_sk = Secp256k1PrivateKey::from_seed("sender_1".as_bytes());
+    let sender_addr = tests::to_addr(&sender_sk);
+    let send_amt = 100;
+    let send_fee = 180;
+    let deploy_fee = 1000000;
+    let call_fee = 1000;
+    let signer_test: SignerTest<SpawnedSigner> =
+        SignerTest::new_with_config_modifications_and_snapshot(
+            num_signers,
+            vec![(
+                sender_addr.clone(),
+                (send_amt + send_fee) * 10 + deploy_fee + call_fee,
+            )],
+            |c| {
+                c.validate_with_replay_tx = true;
+            },
+            |node_config| {
+                node_config.miner.block_commit_delay = Duration::from_secs(1);
+                node_config.miner.replay_transactions = true;
+                node_config.miner.activated_vrf_key_path =
+                    Some(format!("{}/vrf_key", node_config.node.working_dir));
+            },
+            None,
+            None,
+            Some(function_name!()),
+        );
+
+    if signer_test.bootstrap_snapshot() {
+        signer_test.shutdown_and_snapshot();
+        return;
+    }
+
+    info!("------------------------- Beginning test -------------------------");
+
+    signer_test.mine_nakamoto_block(Duration::from_secs(30), true);
+
+    let (txid, deploy_nonce) = signer_test
+        .submit_contract_deploy(&sender_sk, deploy_fee, "foo", "undefined-var")
+        .expect("Failed to submit contract deploy");
+
+    signer_test
+        .wait_for_nonce_increase(&sender_addr, deploy_nonce)
+        .expect("Failed to wait for nonce increase");
+
+    let blocks = test_observer::get_mined_nakamoto_blocks();
+
+    let block = blocks.last().unwrap();
+
+    let tx_event = block
+        .tx_events
+        .iter()
+        .find(|event| event.txid().to_hex() == txid)
+        .expect("Failed to find deploy event");
+
+    info!("Tx event: {:?}", tx_event);
+
+    let TransactionEvent::Success(success_event) = tx_event else {
+        panic!("Failed: Expected success event");
+    };
+
+    let block_cost = block.cost.clone();
+    let expected_cost = ExecutionCost {
+        runtime: 346,
+        write_length: 2,
+        write_count: 1,
+        read_length: 1,
+        read_count: 1,
+    };
+
+    assert_eq!(block_cost, expected_cost.clone());
+    assert_eq!(success_event.execution_cost, expected_cost);
+
+    signer_test.shutdown();
+}
+
 /// Ensure that signers can immediately start participating in signing when starting up
 /// after crashing mid reward cycle.
 ///

stackslib/src/chainstate/stacks/miner.rs

@@ -441,6 +441,18 @@ pub enum TransactionEvent {
     Problematic(TransactionProblematicEvent),
 }
 
+impl TransactionEvent {
+    /// Get the txid of the transaction result
+    pub fn txid(&self) -> &Txid {
+        match self {
+            TransactionEvent::Success(TransactionSuccessEvent { txid, .. }) => txid,
+            TransactionEvent::ProcessingError(TransactionErrorEvent { txid, .. }) => txid,
+            TransactionEvent::Skipped(TransactionSkippedEvent { txid, .. }) => txid,
+            TransactionEvent::Problematic(TransactionProblematicEvent { txid, .. }) => txid,
+        }
+    }
+}
+
 impl TransactionResult {
     /// Logs a queryable message for the case where `txid` has succeeded.
     pub fn log_transaction_success(tx: &StacksTransaction) {

stackslib/src/clarity_vm/tests/analysis_costs.rs

@@ -15,13 +15,15 @@
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 use clarity::vm::ast::ASTRules;
-use clarity::vm::clarity::TransactionConnection;
+use clarity::vm::clarity::{Error as ClarityError, TransactionConnection};
 use clarity::vm::costs::ExecutionCost;
+use clarity::vm::errors::CheckErrors;
 use clarity::vm::functions::NativeFunctions;
 use clarity::vm::test_util::TEST_HEADER_DB;
 use clarity::vm::tests::{test_only_mainnet_to_chain_id, UnitTestBurnStateDB};
 use clarity::vm::types::{PrincipalData, QualifiedContractIdentifier, Value};
 use clarity::vm::{execute as vm_execute, ClarityVersion, ContractName};
+use rstest::rstest;
 use stacks_common::types::chainstate::StacksBlockId;
 use stacks_common::types::StacksEpochId;
 
@@ -304,3 +306,49 @@ fn epoch_205_test_all_mainnet() {
 fn epoch_205_test_all_testnet() {
     epoch_205_test_all(false);
 }
+
+#[rstest]
+#[case(true, StacksEpochId::Epoch21)]
+#[case(true, StacksEpochId::Epoch2_05)]
+#[case(false, StacksEpochId::Epoch21)]
+#[case(false, StacksEpochId::Epoch2_05)]
+fn undefined_top_variable_error(#[case] use_mainnet: bool, #[case] epoch: StacksEpochId) {
+    let mut clarity_instance =
+        setup_tracked_cost_test(use_mainnet, epoch, ClarityVersion::Clarity1);
+    let contract_self = "foo";
+
+    let self_contract_id =
+        QualifiedContractIdentifier::local("undefined-top-variable-error").unwrap();
+
+    let burn_state_db = UnitTestBurnStateDB {
+        epoch_id: epoch,
+        ast_rules: ASTRules::PrecheckSize,
+    };
+
+    {
+        let mut conn = clarity_instance.begin_block(
+            &StacksBlockId([3; 32]),
+            &StacksBlockId([4; 32]),
+            &TEST_HEADER_DB,
+            &burn_state_db,
+        );
+
+        conn.as_transaction(|conn| {
+            let analysis_result = conn.analyze_smart_contract(
+                &self_contract_id,
+                ClarityVersion::Clarity1,
+                &contract_self,
+                ASTRules::PrecheckSize,
+            );
+            let Err(ClarityError::Analysis(check_error)) = analysis_result else {
+                panic!("Bad analysis result: {:?}", &analysis_result);
+            };
+            let CheckErrors::UndefinedVariable(var_name) = check_error.err else {
+                panic!("Bad analysis error: {:?}", &check_error);
+            };
+            assert_eq!(var_name, "foo".to_string());
+        });
+
+        conn.commit_block();
+    }
+}