From 80d246d66777973bb8c72bbc01c5cd94d0a1d729 Mon Sep 17 00:00:00 2001 From: Pascal Date: Mon, 22 Mar 2021 00:31:47 -0700 Subject: [PATCH 1/6] try readonly and fail if it wrote instead --- src/net/inv.rs | 4 +- src/net/rpc.rs | 32 +++++-- testnet/stacks-node/src/tests/integrations.rs | 88 ++++++++++++++++--- 3 files changed, 103 insertions(+), 21 deletions(-) diff --git a/src/net/inv.rs b/src/net/inv.rs index 823e6cb874..0c09654d55 100644 --- a/src/net/inv.rs +++ b/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 { diff --git a/src/net/rpc.rs b/src/net/rpc.rs index b1ff22c1c6..7d4c65afb8 100644 --- a/src/net/rpc.rs +++ b/src/net/rpc.rs @@ -89,10 +89,12 @@ 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::Unchecked, errors::Error as ClarityRuntimeError, errors::InterpreterError, types::{PrincipalData, QualifiedContractIdentifier, StandardPrincipalData}, @@ -1215,7 +1217,7 @@ impl ConversationHttp { })?; clarity_tx.with_readonly_clarity_env(mainnet, sender.clone(), cost_track, |env| { - env.execute_contract(&contract_identifier, function.as_str(), &args, true) + env.execute_contract(&contract_identifier, function.as_str(), &args, false) }) }); @@ -1228,14 +1230,26 @@ 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 +2881,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 diff --git a/testnet/stacks-node/src/tests/integrations.rs b/testnet/stacks-node/src/tests/integrations.rs index 5949246dda..9eb562984b 100644 --- a/testnet/stacks-node/src/tests/integrations.rs +++ b/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 MAIN_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,19 @@ 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", MAIN_CONTRACT); tenure .mem_pool .submit_raw( @@ -188,7 +213,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 +280,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 +492,7 @@ fn integration_test_get_info() { eprintln!("Test: GET {}", path); let res = client.get(&path).send().unwrap().json::().unwrap(); assert_eq!(u128::from_str_radix(&res.balance[2..], 16).unwrap(), 0); - assert_eq!(res.nonce, 2); + assert_eq!(res.nonce, 3); assert!(res.nonce_proof.is_some()); assert!(res.balance_proof.is_some()); @@ -581,6 +606,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::().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::().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 +704,7 @@ fn integration_test_get_info() { .send() .unwrap().json::().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")); From be29cd89953aa6825a22465924cc5e1e27179e5d Mon Sep 17 00:00:00 2001 From: Pascal Date: Mon, 22 Mar 2021 00:43:12 -0700 Subject: [PATCH 2/6] chore: rustfmt --- src/net/rpc.rs | 14 ++++++++------ testnet/stacks-node/src/tests/integrations.rs | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/net/rpc.rs b/src/net/rpc.rs index 7d4c65afb8..af651e0408 100644 --- a/src/net/rpc.rs +++ b/src/net/rpc.rs @@ -94,8 +94,8 @@ use vm::{ database::{ clarity_store::ContractCommitment, ClarityDatabase, ClaritySerializable, STXBalance, }, - errors::Error::Unchecked, errors::Error as ClarityRuntimeError, + errors::Error::Unchecked, errors::InterpreterError, types::{PrincipalData, QualifiedContractIdentifier, StandardPrincipalData}, ClarityName, ContractName, SymbolicExpression, Value, @@ -1231,8 +1231,10 @@ impl ConversationHttp { }, ), Ok(Some(Err(e))) => match e { - Unchecked(CheckErrors::CostBalanceExceeded(actual_cost, _)) if actual_cost.write_count > 0 => { - HttpResponseType::CallReadOnlyFunction ( + Unchecked(CheckErrors::CostBalanceExceeded(actual_cost, _)) + if actual_cost.write_count > 0 => + { + HttpResponseType::CallReadOnlyFunction( response_metadata, CallReadOnlyResponse { okay: false, @@ -1240,15 +1242,15 @@ impl ConversationHttp { cause: Some("NotReadOnly".to_string()), }, ) - }, - _ => HttpResponseType::CallReadOnlyFunction ( + } + _ => 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()) diff --git a/testnet/stacks-node/src/tests/integrations.rs b/testnet/stacks-node/src/tests/integrations.rs index 9eb562984b..7b48f39a45 100644 --- a/testnet/stacks-node/src/tests/integrations.rs +++ b/testnet/stacks-node/src/tests/integrations.rs @@ -192,12 +192,22 @@ fn integration_test_get_info() { make_contract_publish(&contract_sk, 0, 0, "get-info", GET_INFO_CONTRACT); tenure .mem_pool - .submit_raw(&mut chainstate_copy, &consensus_hash, &header_hash, publish_tx) + .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) + .submit_raw( + &mut chainstate_copy, + &consensus_hash, + &header_hash, + publish_tx, + ) .unwrap(); let publish_tx = make_contract_publish(&contract_sk, 2, 0, "main", MAIN_CONTRACT); tenure From d871d65b02bfc76df11b5c5647043e9d132ccf2a Mon Sep 17 00:00:00 2001 From: Pascal Date: Thu, 1 Apr 2021 15:44:44 -0700 Subject: [PATCH 3/6] make sure write_count and write_length are set to 0 --- src/net/rpc.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/net/rpc.rs b/src/net/rpc.rs index af651e0408..2b6d7b15e2 100644 --- a/src/net/rpc.rs +++ b/src/net/rpc.rs @@ -1202,15 +1202,15 @@ 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) From 215819b2bda27efb42221bc6c5cb252e0a32f78e Mon Sep 17 00:00:00 2001 From: Pascal Date: Thu, 15 Apr 2021 14:55:12 -0700 Subject: [PATCH 4/6] address review feedback --- src/net/rpc.rs | 6 ++++++ testnet/stacks-node/src/tests/integrations.rs | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/net/rpc.rs b/src/net/rpc.rs index 2b6d7b15e2..7a9fd14e2a 100644 --- a/src/net/rpc.rs +++ b/src/net/rpc.rs @@ -1217,6 +1217,12 @@ impl ConversationHttp { })?; clarity_tx.with_readonly_clarity_env(mainnet, sender.clone(), cost_track, |env| { + // 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) }) }); diff --git a/testnet/stacks-node/src/tests/integrations.rs b/testnet/stacks-node/src/tests/integrations.rs index 7b48f39a45..6e26808453 100644 --- a/testnet/stacks-node/src/tests/integrations.rs +++ b/testnet/stacks-node/src/tests/integrations.rs @@ -41,7 +41,7 @@ const OTHER_CONTRACT: &'static str = " (ok (var-set x val))) "; -const MAIN_CONTRACT: &'static str = " +const CALL_READ_CONTRACT: &'static str = " (define-public (public-no-write) (ok (contract-call? .other f1))) (define-public (public-write) @@ -209,7 +209,7 @@ fn integration_test_get_info() { publish_tx, ) .unwrap(); - let publish_tx = make_contract_publish(&contract_sk, 2, 0, "main", MAIN_CONTRACT); + let publish_tx = make_contract_publish(&contract_sk, 2, 0, "main", CALL_READ_CONTRACT); tenure .mem_pool .submit_raw( From 68bc4a6ac231fa7b5253206fa2375a8de4287d8b Mon Sep 17 00:00:00 2001 From: Pascal Date: Thu, 15 Apr 2021 14:59:37 -0700 Subject: [PATCH 5/6] 3 characters too long --- testnet/stacks-node/src/tests/integrations.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/testnet/stacks-node/src/tests/integrations.rs b/testnet/stacks-node/src/tests/integrations.rs index 6e26808453..420558ea3c 100644 --- a/testnet/stacks-node/src/tests/integrations.rs +++ b/testnet/stacks-node/src/tests/integrations.rs @@ -209,7 +209,8 @@ fn integration_test_get_info() { publish_tx, ) .unwrap(); - let publish_tx = make_contract_publish(&contract_sk, 2, 0, "main", CALL_READ_CONTRACT); + let publish_tx = + make_contract_publish(&contract_sk, 2, 0, "main", CALL_READ_CONTRACT); tenure .mem_pool .submit_raw( From 74d3595dbb8f85d8ce7df256cfaf6b32d70c0d1c Mon Sep 17 00:00:00 2001 From: Pascal Date: Mon, 26 Apr 2021 18:07:38 -0700 Subject: [PATCH 6/6] rebase and fix nonces --- testnet/stacks-node/src/tests/integrations.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testnet/stacks-node/src/tests/integrations.rs b/testnet/stacks-node/src/tests/integrations.rs index 420558ea3c..7fee305ea2 100644 --- a/testnet/stacks-node/src/tests/integrations.rs +++ b/testnet/stacks-node/src/tests/integrations.rs @@ -503,7 +503,7 @@ fn integration_test_get_info() { eprintln!("Test: GET {}", path); let res = client.get(&path).send().unwrap().json::().unwrap(); assert_eq!(u128::from_str_radix(&res.balance[2..], 16).unwrap(), 0); - assert_eq!(res.nonce, 3); + assert_eq!(res.nonce, 4); assert!(res.nonce_proof.is_some()); assert!(res.balance_proof.is_some());