Merge pull request #2527 from psq/feat/rpc-call-read-1641-v2

Implement #1641 rpc call public functions as read-only - take 2

src/net/inv.rs

@@ -1467,7 +1467,7 @@ impl PeerNetwork {
         };
 
         debug!(
-            "{:?}: Send GetPoxInv to {:?} for {} rewad cycles starting at {} ({})",
+            "{:?}: Send GetPoxInv to {:?} for {} reward cycles starting at {} ({})",
             &self.local_peer,
             nk,
             num_reward_cycles,
@@ -1999,7 +1999,7 @@ impl PeerNetwork {
             true,
         );
 
-        debug!("{:?}: {:?} has {} new blocks and {} new microblocks (total {} blocks, {} microblocks, {} sortitions): {:?}", 
+        debug!("{:?}: {:?} has {} new blocks and {} new microblocks (total {} blocks, {} microblocks, {} sortitions): {:?}",
                &self.local_peer, &nk, new_blocks, new_microblocks, stats.inv.num_blocks(), stats.inv.num_microblock_streams(), stats.inv.num_sortitions, &stats.inv);
 
         if new_blocks > 0 || new_microblocks > 0 {

src/net/rpc.rs

@@ -89,11 +89,13 @@ use util::hash::{hex_bytes, to_hex};
 use vm::database::clarity_store::make_contract_hash_key;
 use vm::types::TraitIdentifier;
 use vm::{
+    analysis::errors::CheckErrors,
     costs::{ExecutionCost, LimitedCostTracker},
     database::{
         clarity_store::ContractCommitment, ClarityDatabase, ClaritySerializable, STXBalance,
     },
     errors::Error as ClarityRuntimeError,
+    errors::Error::Unchecked,
     errors::InterpreterError,
     types::{PrincipalData, QualifiedContractIdentifier, StandardPrincipalData},
     ClarityName, ContractName, SymbolicExpression, Value,
@@ -1200,22 +1202,28 @@ impl ConversationHttp {
             .map(|x| SymbolicExpression::atom_value(x.clone()))
             .collect();
         let mainnet = chainstate.mainnet;
+        let mut cost_limit = options.read_only_call_limit.clone();
+        cost_limit.write_length = 0;
+        cost_limit.write_count = 0;
+
         let data_opt_res =
             chainstate.maybe_read_only_clarity_tx(&sortdb.index_conn(), tip, |clarity_tx| {
                 let cost_track = clarity_tx
                     .with_clarity_db_readonly(|clarity_db| {
-                        LimitedCostTracker::new_mid_block(
-                            mainnet,
-                            options.read_only_call_limit.clone(),
-                            clarity_db,
-                        )
+                        LimitedCostTracker::new_mid_block(mainnet, cost_limit, clarity_db)
                     })
                     .map_err(|_| {
                         ClarityRuntimeError::from(InterpreterError::CostContractLoadFailure)
                     })?;
 
                 clarity_tx.with_readonly_clarity_env(mainnet, sender.clone(), cost_track, |env| {
-                    env.execute_contract(&contract_identifier, function.as_str(), &args, true)
+                    // we want to execute any function as long as no actual writes are made as
+                    // opposed to be limited to purely calling `define-read-only` functions,
+                    // so use `read_only = false`.  This broadens the number of functions that
+                    // can be called, and also circumvents limitations on `define-read-only`
+                    // functions that can not use `contrac-call?`, even when calling other
+                    // read-only functions
+                    env.execute_contract(&contract_identifier, function.as_str(), &args, false)
                 })
             });
 
@@ -1228,14 +1236,28 @@ impl ConversationHttp {
                     cause: None,
                 },
             ),
-            Ok(Some(Err(e))) => HttpResponseType::CallReadOnlyFunction(
-                response_metadata,
-                CallReadOnlyResponse {
-                    okay: false,
-                    result: None,
-                    cause: Some(e.to_string()),
-                },
-            ),
+            Ok(Some(Err(e))) => match e {
+                Unchecked(CheckErrors::CostBalanceExceeded(actual_cost, _))
+                    if actual_cost.write_count > 0 =>
+                {
+                    HttpResponseType::CallReadOnlyFunction(
+                        response_metadata,
+                        CallReadOnlyResponse {
+                            okay: false,
+                            result: None,
+                            cause: Some("NotReadOnly".to_string()),
+                        },
+                    )
+                }
+                _ => HttpResponseType::CallReadOnlyFunction(
+                    response_metadata,
+                    CallReadOnlyResponse {
+                        okay: false,
+                        result: None,
+                        cause: Some(e.to_string()),
+                    },
+                ),
+            },
             Ok(None) | Err(_) => {
                 HttpResponseType::NotFound(response_metadata, "Chain tip not found".into())
             }
@@ -2867,7 +2889,7 @@ mod test {
         (define-public (set-bar (x int) (y int))
           (begin (var-set bar (/ x y)) (ok (var-get bar))))
         (define-public (add-unit)
-          (begin 
+          (begin
             (map-set unit-map { account: tx-sender } { units: 1 } )
             (ok 1)))
         (begin

testnet/stacks-node/src/tests/integrations.rs

@@ -20,7 +20,7 @@ use stacks::vm::{
         mem_type_check,
     },
     database::ClaritySerializable,
-    types::{QualifiedContractIdentifier, TupleData},
+    types::{QualifiedContractIdentifier, ResponseData, TupleData},
     Value,
 };
 
@@ -33,6 +33,21 @@ use super::{
     SK_3,
 };
 
+const OTHER_CONTRACT: &'static str = "
+  (define-data-var x uint u0)
+  (define-public (f1)
+    (ok (var-get x)))
+  (define-public (f2 (val uint))
+    (ok (var-set x val)))
+";
+
+const CALL_READ_CONTRACT: &'static str = "
+  (define-public (public-no-write)
+    (ok (contract-call? .other f1)))
+  (define-public (public-write)
+    (ok (contract-call? .other f2 u5)))
+";
+
 const GET_INFO_CONTRACT: &'static str = "
         (define-map block-data
           { height: uint }
@@ -105,14 +120,14 @@ const GET_INFO_CONTRACT: &'static str = "
           (begin
             (unwrap-panic (inner-update-info (- block-height u2)))
             (inner-update-info (- block-height u1))))
-        
+
         (define-trait trait-1 (
             (foo-exec (int) (response int int))))
 
         (define-trait trait-2 (
             (get-1 (uint) (response uint uint))
             (get-2 (uint) (response uint uint))))
-        
+
         (define-trait trait-3 (
             (fn-1 (uint) (response uint uint))
             (fn-2 (uint) (response uint uint))))
@@ -123,7 +138,7 @@ const IMPL_TRAIT_CONTRACT: &'static str = "
         (impl-trait .get-info.trait-1)
         (define-private (test-height) burn-block-height)
         (define-public (foo-exec (a int)) (ok 1))
-    
+
         ;; implicit trait compliance for trait-2
         (define-public (get-1 (x uint)) (ok u1))
         (define-public (get-2 (x uint)) (ok u1))
@@ -172,9 +187,30 @@ fn integration_test_get_info() {
 
             if round == 1 {
                 // block-height = 2
+                eprintln!("Tenure in 1 started!");
                 let publish_tx =
                     make_contract_publish(&contract_sk, 0, 0, "get-info", GET_INFO_CONTRACT);
-                eprintln!("Tenure in 1 started!");
+                tenure
+                    .mem_pool
+                    .submit_raw(
+                        &mut chainstate_copy,
+                        &consensus_hash,
+                        &header_hash,
+                        publish_tx,
+                    )
+                    .unwrap();
+                let publish_tx = make_contract_publish(&contract_sk, 1, 0, "other", OTHER_CONTRACT);
+                tenure
+                    .mem_pool
+                    .submit_raw(
+                        &mut chainstate_copy,
+                        &consensus_hash,
+                        &header_hash,
+                        publish_tx,
+                    )
+                    .unwrap();
+                let publish_tx =
+                    make_contract_publish(&contract_sk, 2, 0, "main", CALL_READ_CONTRACT);
                 tenure
                     .mem_pool
                     .submit_raw(
@@ -188,7 +224,7 @@ fn integration_test_get_info() {
                 // block-height = 3
                 let publish_tx = make_contract_publish(
                     &contract_sk,
-                    1,
+                    3,
                     0,
                     "impl-trait-contract",
                     IMPL_TRAIT_CONTRACT,
@@ -255,8 +291,8 @@ fn integration_test_get_info() {
                 let blocks = StacksChainState::list_blocks(&chain_state.db()).unwrap();
                 assert!(chain_tip.metadata.block_height == 2);
 
-                // Block #1 should have 3 txs
-                assert!(chain_tip.block.txs.len() == 3);
+                // Block #1 should have 5 txs
+                assert!(chain_tip.block.txs.len() == 5);
 
                 let parent = chain_tip.block.header.parent_block;
                 let bhh = &chain_tip.metadata.index_block_hash();
@@ -467,7 +503,7 @@ fn integration_test_get_info() {
                 eprintln!("Test: GET {}", path);
                 let res = client.get(&path).send().unwrap().json::<AccountEntryResponse>().unwrap();
                 assert_eq!(u128::from_str_radix(&res.balance[2..], 16).unwrap(), 0);
-                assert_eq!(res.nonce, 2);
+                assert_eq!(res.nonce, 4);
                 assert!(res.nonce_proof.is_some());
                 assert!(res.balance_proof.is_some());
 
@@ -581,6 +617,49 @@ fn integration_test_get_info() {
                                                                        "(get-exotic-data-info u3)");
                 assert_eq!(result_data, expected_data);
 
+                // how about a non read-only function call which does not modify anything
+                let path = format!("{}/v2/contracts/call-read/{}/{}/{}", &http_origin, &contract_addr, "main", "public-no-write");
+                eprintln!("Test: POST {}", path);
+
+                let body = CallReadOnlyRequestBody {
+                    sender: "'SP139Q3N9RXCJCD1XVA4N5RYWQ5K9XQ0T9PKQ8EE5".into(),
+                    arguments: vec![]
+                };
+
+                let res = client.post(&path)
+                    .json(&body)
+                    .send()
+                    .unwrap().json::<serde_json::Value>().unwrap();
+                assert!(res.get("cause").is_none());
+                assert!(res["okay"].as_bool().unwrap());
+
+                let result_data = Value::try_deserialize_hex_untyped(&res["result"].as_str().unwrap()[2..]).unwrap();
+                let expected_data = Value::Response(ResponseData {
+                    committed: true,
+                    data: Box::new(Value::Response(ResponseData {
+                        committed: true,
+                        data: Box::new(Value::UInt(0))
+                    }))
+                });
+                assert_eq!(result_data, expected_data);
+
+                // how about a non read-only function call which does modify something and should fail
+                let path = format!("{}/v2/contracts/call-read/{}/{}/{}", &http_origin, &contract_addr, "main", "public-write");
+                eprintln!("Test: POST {}", path);
+
+                let body = CallReadOnlyRequestBody {
+                    sender: "'SP139Q3N9RXCJCD1XVA4N5RYWQ5K9XQ0T9PKQ8EE5".into(),
+                    arguments: vec![]
+                };
+
+                let res = client.post(&path)
+                    .json(&body)
+                    .send()
+                    .unwrap().json::<serde_json::Value>().unwrap();
+                assert!(res.get("cause").is_some());
+                assert!(!res["okay"].as_bool().unwrap());
+                assert!(res["cause"].as_str().unwrap().contains("NotReadOnly"));
+
                 // let's try a call with a url-encoded string.
                 let path = format!("{}/v2/contracts/call-read/{}/{}/{}", &http_origin, &contract_addr, "get-info",
                                    "get-exotic-data-info%3F");
@@ -636,7 +715,7 @@ fn integration_test_get_info() {
                     .send()
                     .unwrap().json::<serde_json::Value>().unwrap();
 
-                eprintln!("{}", res["cause"].as_str().unwrap());
+                eprintln!("{:#?}", res["cause"].as_str().unwrap());
                 assert!(res.get("result").is_none());
                 assert!(!res["okay"].as_bool().unwrap());
                 assert!(res["cause"].as_str().unwrap().contains("NotReadOnly"));