From c94f473eef03e2cef8b02b888cc91a3c81696d81 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 3 Feb 2025 22:05:30 -0500 Subject: [PATCH 1/8] chore: make test constant estimator public --- stackslib/src/cost_estimates/tests/fee_rate_fuzzer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stackslib/src/cost_estimates/tests/fee_rate_fuzzer.rs b/stackslib/src/cost_estimates/tests/fee_rate_fuzzer.rs index 8fe0622dc8..2682894e51 100644 --- a/stackslib/src/cost_estimates/tests/fee_rate_fuzzer.rs +++ b/stackslib/src/cost_estimates/tests/fee_rate_fuzzer.rs @@ -15,7 +15,7 @@ use crate::cost_estimates::fee_rate_fuzzer::FeeRateFuzzer; use crate::cost_estimates::tests::common::make_block_receipt; use crate::cost_estimates::{EstimatorError, FeeEstimator, FeeRateEstimate}; -struct ConstantFeeEstimator {} +pub struct ConstantFeeEstimator {} /// Returns a constant fee rate estimate. impl FeeEstimator for ConstantFeeEstimator { From 41a1026ebb71a509721d723a3876e4658010a693 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 3 Feb 2025 22:05:44 -0500 Subject: [PATCH 2/8] chore: fix 4145 by returning the JSON representation of a fee estimator error --- stackslib/src/net/api/postfeerate.rs | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/stackslib/src/net/api/postfeerate.rs b/stackslib/src/net/api/postfeerate.rs index cb012bbc6c..7077649791 100644 --- a/stackslib/src/net/api/postfeerate.rs +++ b/stackslib/src/net/api/postfeerate.rs @@ -18,6 +18,7 @@ use std::io::{Read, Write}; use clarity::vm::costs::ExecutionCost; use regex::{Captures, Regex}; +use serde_json::json; use stacks_common::codec::{StacksMessageCodec, MAX_PAYLOAD_LEN}; use stacks_common::types::chainstate::{ BlockHeaderHash, ConsensusHash, StacksBlockId, StacksPublicKey, @@ -118,13 +119,7 @@ impl RPCPostFeeRateRequestHandler { let scalar_cost = metric.from_cost_and_len(&estimated_cost, &stacks_epoch.block_limit, estimated_len); let fee_rates = fee_estimator.get_rate_estimates().map_err(|e| { - StacksHttpResponse::new_error( - preamble, - &HttpBadRequest::new(format!( - "Estimator RPC endpoint failed to estimate fees for tx: {:?}", - &e - )), - ) + StacksHttpResponse::new_error(preamble, &HttpBadRequest::new_json(e.into_json())) })?; let mut estimations = RPCFeeEstimate::estimate_fees(scalar_cost, fee_rates).to_vec(); @@ -243,11 +238,7 @@ impl RPCRequestHandler for RPCPostFeeRateRequestHandler { .map_err(|e| { StacksHttpResponse::new_error( &preamble, - &HttpBadRequest::new(format!( - "Estimator RPC endpoint failed to estimate tx {}: {:?}", - &tx.name(), - &e - )), + &HttpBadRequest::new_json(e.into_json()), ) })?; @@ -263,9 +254,9 @@ impl RPCRequestHandler for RPCPostFeeRateRequestHandler { debug!("Fee and cost estimation not configured on this stacks node"); Err(StacksHttpResponse::new_error( &preamble, - &HttpBadRequest::new( - "Fee estimation not supported on this node".to_string(), - ), + &HttpBadRequest::new_json(json!( + "Fee estimation not supported on this node" + )), )) } }); From 22297f7ce3d900fa9b4951b708ddad7b217732de Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 3 Feb 2025 22:06:04 -0500 Subject: [PATCH 3/8] chore: instantiate different kinds of RPCHandlerArgs to test fee estimation errors --- stackslib/src/net/api/tests/mod.rs | 46 ++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/stackslib/src/net/api/tests/mod.rs b/stackslib/src/net/api/tests/mod.rs index 14034e3eaf..e85ed7a290 100644 --- a/stackslib/src/net/api/tests/mod.rs +++ b/stackslib/src/net/api/tests/mod.rs @@ -15,6 +15,7 @@ // along with this program. If not, see . use std::net::SocketAddr; +use std::sync::{Arc, Mutex}; use clarity::vm::costs::ExecutionCost; use clarity::vm::types::{QualifiedContractIdentifier, StacksAddressExtensions}; @@ -46,7 +47,7 @@ use crate::net::db::PeerDB; use crate::net::httpcore::{StacksHttpRequest, StacksHttpResponse}; use crate::net::relay::Relayer; use crate::net::rpc::ConversationHttp; -use crate::net::test::{TestEventObserver, TestPeer, TestPeerConfig}; +use crate::net::test::{RPCHandlerArgsType, TestEventObserver, TestPeer, TestPeerConfig}; use crate::net::tests::inv::nakamoto::make_nakamoto_peers_from_invs_ext; use crate::net::{ Attachment, AttachmentInstance, MemPoolEventDispatcher, RPCHandlerArgs, StackerDBConfig, @@ -226,10 +227,28 @@ pub struct TestRPC<'a> { impl<'a> TestRPC<'a> { pub fn setup(test_name: &str) -> TestRPC<'a> { - Self::setup_ex(test_name, true) + Self::setup_ex(test_name, true, None, None) } - pub fn setup_ex(test_name: &str, process_microblock: bool) -> TestRPC<'a> { + pub fn setup_with_rpc_args( + test_name: &str, + rpc_handler_args_opt_1: Option, + rpc_handler_args_opt_2: Option, + ) -> TestRPC<'a> { + Self::setup_ex( + test_name, + true, + rpc_handler_args_opt_1, + rpc_handler_args_opt_2, + ) + } + + pub fn setup_ex( + test_name: &str, + process_microblock: bool, + rpc_handler_args_opt_1: Option, + rpc_handler_args_opt_2: Option, + ) -> TestRPC<'a> { // ST2DS4MSWSGJ3W9FBC6BVT0Y92S345HY8N3T6AV7R let privk1 = StacksPrivateKey::from_hex( "9f1f85a512a96a244e4c0d762788500687feb97481639572e3bffbd6860e6ab001", @@ -317,6 +336,9 @@ impl<'a> TestRPC<'a> { let mut peer_1 = TestPeer::new(peer_1_config); let mut peer_2 = TestPeer::new(peer_2_config); + peer_1.rpc_handler_args = rpc_handler_args_opt_1; + peer_2.rpc_handler_args = rpc_handler_args_opt_2; + // mine one block with a contract in it // first the coinbase // make a coinbase for this miner @@ -976,7 +998,11 @@ impl<'a> TestRPC<'a> { } { - let mut rpc_args = RPCHandlerArgs::default(); + let mut rpc_args = peer_1 + .rpc_handler_args + .as_ref() + .map(|args_type| args_type.instantiate()) + .unwrap_or(RPCHandlerArgsType::make_default()); rpc_args.event_observer = event_observer; let mut node_state = StacksNodeState::new( &mut peer_1.network, @@ -1020,7 +1046,11 @@ impl<'a> TestRPC<'a> { } { - let mut rpc_args = RPCHandlerArgs::default(); + let mut rpc_args = peer_2 + .rpc_handler_args + .as_ref() + .map(|args_type| args_type.instantiate()) + .unwrap_or(RPCHandlerArgsType::make_default()); rpc_args.event_observer = event_observer; let mut node_state = StacksNodeState::new( &mut peer_2.network, @@ -1076,7 +1106,11 @@ impl<'a> TestRPC<'a> { let mut peer_1_stacks_node = peer_1.stacks_node.take().unwrap(); let mut peer_1_mempool = peer_1.mempool.take().unwrap(); - let rpc_args = RPCHandlerArgs::default(); + let rpc_args = peer_1 + .rpc_handler_args + .as_ref() + .map(|args_type| args_type.instantiate()) + .unwrap_or(RPCHandlerArgsType::make_default()); let mut node_state = StacksNodeState::new( &mut peer_1.network, &peer_1_sortdb, From f85611ca003aaaed5631402410689b6f9d80c704 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 3 Feb 2025 22:06:31 -0500 Subject: [PATCH 4/8] chore: test 4145 fix by supplying different fee estimators --- stackslib/src/net/api/tests/postfeerate.rs | 88 +++++++++++++++++++++- 1 file changed, 84 insertions(+), 4 deletions(-) diff --git a/stackslib/src/net/api/tests/postfeerate.rs b/stackslib/src/net/api/tests/postfeerate.rs index b762264731..753684d483 100644 --- a/stackslib/src/net/api/tests/postfeerate.rs +++ b/stackslib/src/net/api/tests/postfeerate.rs @@ -15,23 +15,29 @@ // along with this program. If not, see . use std::net::{IpAddr, Ipv4Addr, SocketAddr}; +use std::sync::Arc; use clarity::vm::types::{PrincipalData, QualifiedContractIdentifier, StacksAddressExtensions}; use clarity::vm::{ClarityName, ContractName, Value}; use stacks_common::types::chainstate::StacksAddress; use stacks_common::types::net::PeerHost; use stacks_common::types::Address; -use stacks_common::util::hash::to_hex; +use stacks_common::util::hash::{to_hex, Sha256Sum}; use super::test_rpc; use crate::chainstate::stacks::TransactionPayload; use crate::core::BLOCK_LIMIT_MAINNET_21; +use crate::cost_estimates::metrics::UnitMetric; +use crate::cost_estimates::tests::fee_rate_fuzzer::ConstantFeeEstimator; +use crate::cost_estimates::UnitEstimator; +use crate::net::api::tests::TestRPC; use crate::net::api::*; use crate::net::connection::ConnectionOptions; use crate::net::httpcore::{ HttpRequestContentsExtensions, RPCRequestHandler, StacksHttp, StacksHttpRequest, }; -use crate::net::{ProtocolFamily, TipRequest}; +use crate::net::test::RPCHandlerArgsType; +use crate::net::{ProtocolFamily, RPCHandlerArgs, TipRequest}; #[test] fn test_try_parse_request() { @@ -89,9 +95,37 @@ fn test_try_make_response() { TransactionPayload::new_contract_call(sender_addr, "hello-world", "add-unit", vec![]) .unwrap(); + // case 1: no fee estimates let mut requests = vec![]; let request = StacksHttpRequest::new_post_fee_rate( - addr.into(), + addr.clone().into(), + postfeerate::FeeRateEstimateRequestBody { + estimated_len: Some(123), + transaction_payload: to_hex(&tx_payload.serialize_to_vec()), + }, + ); + requests.push(request); + + let test_rpc = TestRPC::setup(function_name!()); + let mut responses = test_rpc.run(requests); + + let response = responses.remove(0); + debug!( + "Response:\n{}\n", + std::str::from_utf8(&response.try_serialize().unwrap()).unwrap() + ); + + let (preamble, body) = response.destruct(); + let body_json: serde_json::Value = body.try_into().unwrap(); + + // get back a JSON string and a 400 + assert_eq!(preamble.status_code, 400); + debug!("Response JSON no estimator: {}", &body_json); + + // case 2: no estimate avaialable + let mut requests = vec![]; + let request = StacksHttpRequest::new_post_fee_rate( + addr.clone().into(), postfeerate::FeeRateEstimateRequestBody { estimated_len: Some(123), transaction_payload: to_hex(&tx_payload.serialize_to_vec()), @@ -99,7 +133,12 @@ fn test_try_make_response() { ); requests.push(request); - let mut responses = test_rpc(function_name!(), requests); + let test_rpc = TestRPC::setup_with_rpc_args( + function_name!(), + Some(RPCHandlerArgsType::Null), + Some(RPCHandlerArgsType::Null), + ); + let mut responses = test_rpc.run(requests); let response = responses.remove(0); debug!( @@ -108,5 +147,46 @@ fn test_try_make_response() { ); let (preamble, body) = response.destruct(); + let body_json: serde_json::Value = body.try_into().unwrap(); + + // get back a JSON object and a 400 assert_eq!(preamble.status_code, 400); + debug!("Response JSON no estimate fee: {}", &body_json); + assert_eq!( + body_json.get("reason").unwrap().as_str().unwrap(), + "NoEstimateAvailable" + ); + assert!(body_json.get("error").is_some()); + assert!(body_json.get("reason_data").is_some()); + + // case 3: get an estimate + let mut requests = vec![]; + let request = StacksHttpRequest::new_post_fee_rate( + addr.clone().into(), + postfeerate::FeeRateEstimateRequestBody { + estimated_len: Some(123), + transaction_payload: to_hex(&tx_payload.serialize_to_vec()), + }, + ); + requests.push(request); + + let test_rpc = TestRPC::setup_with_rpc_args( + function_name!(), + Some(RPCHandlerArgsType::Unit), + Some(RPCHandlerArgsType::Unit), + ); + let mut responses = test_rpc.run(requests); + + let response = responses.remove(0); + debug!( + "Response:\n{}\n", + std::str::from_utf8(&response.try_serialize().unwrap()).unwrap() + ); + + let (preamble, body) = response.destruct(); + let body_json: serde_json::Value = body.try_into().unwrap(); + + // get back a JSON object and a 200 + assert_eq!(preamble.status_code, 200); + debug!("Response JSON success: {}", &body_json); } From da2ab6b6dc84c51e1376baf9bb00f18b5ea1c151 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 3 Feb 2025 22:06:55 -0500 Subject: [PATCH 5/8] chore: API sync --- stackslib/src/net/api/tests/postmicroblock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stackslib/src/net/api/tests/postmicroblock.rs b/stackslib/src/net/api/tests/postmicroblock.rs index 92504a5560..90802adfd8 100644 --- a/stackslib/src/net/api/tests/postmicroblock.rs +++ b/stackslib/src/net/api/tests/postmicroblock.rs @@ -102,7 +102,7 @@ fn test_try_parse_request() { fn test_try_make_response() { let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 33333); - let test_rpc = TestRPC::setup_ex(function_name!(), false); + let test_rpc = TestRPC::setup_ex(function_name!(), false, None, None); let mblock = test_rpc.next_microblock.clone().unwrap(); let mut requests = vec![]; From 64bf350672716ed29b65c584528c4300cab0d682 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 3 Feb 2025 22:07:02 -0500 Subject: [PATCH 6/8] chore: add support for instantiating owned RPCHandlerArgs in TestPeer that don't need to cross thread boundaries --- stackslib/src/net/mod.rs | 97 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 90 insertions(+), 7 deletions(-) diff --git a/stackslib/src/net/mod.rs b/stackslib/src/net/mod.rs index 6e6870cbfb..695574769c 100644 --- a/stackslib/src/net/mod.rs +++ b/stackslib/src/net/mod.rs @@ -619,7 +619,7 @@ impl PeerHostExtensions for PeerHost { } /// Runtime arguments to an RPC handler -#[derive(Default)] +#[derive(Default, Clone)] pub struct RPCHandlerArgs<'a> { /// What height at which this node will terminate (testnet only) pub exit_at_block_height: Option, @@ -2222,7 +2222,7 @@ pub mod test { use std::net::*; use std::ops::{Deref, DerefMut}; use std::sync::mpsc::sync_channel; - use std::sync::Mutex; + use std::sync::{Arc, Mutex}; use std::{fs, io, thread}; use clarity::boot_util::boot_code_id; @@ -2277,6 +2277,9 @@ pub mod test { use crate::chainstate::*; use crate::clarity::vm::clarity::TransactionConnection; use crate::core::{EpochList, StacksEpoch, StacksEpochExtension, NETWORK_P2P_PORT}; + use crate::cost_estimates::metrics::UnitMetric; + use crate::cost_estimates::tests::fee_rate_fuzzer::ConstantFeeEstimator; + use crate::cost_estimates::UnitEstimator; use crate::net::asn::*; use crate::net::atlas::*; use crate::net::chat::*; @@ -2287,7 +2290,7 @@ pub mod test { use crate::net::p2p::*; use crate::net::poll::*; use crate::net::relay::*; - use crate::net::Error as net_error; + use crate::net::{Error as net_error, ProtocolFamily, TipRequest}; use crate::util_lib::boot::boot_code_test_addr; use crate::util_lib::strings::*; @@ -2552,6 +2555,75 @@ pub mod test { } } + const DEFAULT_RPC_HANDLER_ARGS: RPCHandlerArgs<'static> = RPCHandlerArgs { + exit_at_block_height: None, + genesis_chainstate_hash: Sha256Sum([0x00; 32]), + event_observer: None, + cost_estimator: None, + fee_estimator: None, + cost_metric: None, + coord_comms: None, + }; + + const NULL_COST_ESTIMATOR: () = (); + const NULL_FEE_ESTIMATOR: () = (); + const NULL_COST_METRIC: UnitMetric = UnitMetric {}; + const NULL_RPC_HANDLER_ARGS: RPCHandlerArgs<'static> = RPCHandlerArgs { + exit_at_block_height: None, + genesis_chainstate_hash: Sha256Sum([0x00; 32]), + event_observer: None, + cost_estimator: Some(&NULL_COST_ESTIMATOR), + fee_estimator: Some(&NULL_FEE_ESTIMATOR), + cost_metric: Some(&NULL_COST_METRIC), + coord_comms: None, + }; + + const UNIT_COST_ESTIMATOR: UnitEstimator = UnitEstimator {}; + const CONSTANT_FEE_ESTIMATOR: ConstantFeeEstimator = ConstantFeeEstimator {}; + const UNIT_COST_METRIC: UnitMetric = UnitMetric {}; + const UNIT_RPC_HANDLER_ARGS: RPCHandlerArgs<'static> = RPCHandlerArgs { + exit_at_block_height: None, + genesis_chainstate_hash: Sha256Sum([0x00; 32]), + event_observer: None, + cost_estimator: Some(&UNIT_COST_ESTIMATOR), + fee_estimator: Some(&CONSTANT_FEE_ESTIMATOR), + cost_metric: Some(&UNIT_COST_METRIC), + coord_comms: None, + }; + + /// Templates for RPC Handler Args (which must be owned by the TestPeer, and cannot be a bare + /// RPCHandlerArgs since references to the inner members cannot be made thread-safe). + #[derive(Clone, Debug, PartialEq)] + pub enum RPCHandlerArgsType { + Default, + Null, + Unit, + } + + impl RPCHandlerArgsType { + pub fn instantiate(&self) -> RPCHandlerArgs<'static> { + match self { + Self::Default => { + debug!("Default RPC Handler Args"); + DEFAULT_RPC_HANDLER_ARGS.clone() + } + Self::Null => { + debug!("Null RPC Handler Args"); + NULL_RPC_HANDLER_ARGS.clone() + } + Self::Unit => { + debug!("Unit RPC Handler Args"); + UNIT_RPC_HANDLER_ARGS.clone() + } + } + } + + pub fn make_default() -> RPCHandlerArgs<'static> { + debug!("Default RPC Handler Args"); + DEFAULT_RPC_HANDLER_ARGS.clone() + } + } + // describes a peer's initial configuration #[derive(Debug, Clone)] pub struct TestPeerConfig { @@ -2771,6 +2843,8 @@ pub mod test { /// tenure-start block of tenure to mine on. /// gets consumed on the call to begin_nakamoto_tenure pub nakamoto_parent_tenure_opt: Option>, + /// RPC handler args to use + pub rpc_handler_args: Option, } impl<'a> TestPeer<'a> { @@ -3190,6 +3264,7 @@ pub mod test { malleablized_blocks: vec![], mine_malleablized_blocks: true, nakamoto_parent_tenure_opt: None, + rpc_handler_args: None, } } @@ -3301,6 +3376,11 @@ pub mod test { let mut stacks_node = self.stacks_node.take().unwrap(); let mut mempool = self.mempool.take().unwrap(); let indexer = self.indexer.take().unwrap(); + let rpc_handler_args = self + .rpc_handler_args + .as_ref() + .map(|args_type| args_type.instantiate()) + .unwrap_or(RPCHandlerArgsType::make_default()); let old_tip = self.network.stacks_tip.clone(); @@ -3313,14 +3393,13 @@ pub mod test { false, ibd, 100, - &RPCHandlerArgs::default(), + &rpc_handler_args, ); self.sortdb = Some(sortdb); self.stacks_node = Some(stacks_node); self.mempool = Some(mempool); self.indexer = Some(indexer); - ret } @@ -3381,7 +3460,11 @@ pub mod test { burn_tip_height, ); let indexer = BitcoinIndexer::new_unit_test(&self.config.burnchain.working_dir); - + let rpc_handler_args = self + .rpc_handler_args + .as_ref() + .map(|args_type| args_type.instantiate()) + .unwrap_or(RPCHandlerArgsType::make_default()); let old_tip = self.network.stacks_tip.clone(); let ret = self.network.run( @@ -3393,7 +3476,7 @@ pub mod test { false, ibd, 100, - &RPCHandlerArgs::default(), + &rpc_handler_args, ); self.sortdb = Some(sortdb); From b4af50df9fac23336c90c5f8dcc772a4363d1a25 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 4 Feb 2025 13:49:14 -0500 Subject: [PATCH 7/8] chore: address PR feedback and add changelog entry --- CHANGELOG.md | 1 + stackslib/src/net/api/tests/postfeerate.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1644446dd7..a5bf7d7137 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE - Miners who restart their nodes immediately before a winning tenure now correctly detect that they won the tenure after their nodes restart ([#5750](https://github.com/stacks-network/stacks-core/issues/5750)). +- Error responses to /v2/transactions/fees are once again expressed as JSON ([#4145](https://github.com/stacks-network/stacks-core/issues/4145)). ## [3.1.0.0.4] diff --git a/stackslib/src/net/api/tests/postfeerate.rs b/stackslib/src/net/api/tests/postfeerate.rs index 753684d483..85ba4feb6f 100644 --- a/stackslib/src/net/api/tests/postfeerate.rs +++ b/stackslib/src/net/api/tests/postfeerate.rs @@ -122,7 +122,7 @@ fn test_try_make_response() { assert_eq!(preamble.status_code, 400); debug!("Response JSON no estimator: {}", &body_json); - // case 2: no estimate avaialable + // case 2: no estimate available let mut requests = vec![]; let request = StacksHttpRequest::new_post_fee_rate( addr.clone().into(), From 8ad0a2bce397ee53beaa8e7a5c087cc29cf31017 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 4 Feb 2025 20:47:15 -0500 Subject: [PATCH 8/8] chore: address merge artifact in CHANGELOG --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e08b1b5a4..9fb0772933 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE ### Fixed +- Error responses to /v2/transactions/fees are once again expressed as JSON ([#4145](https://github.com/stacks-network/stacks-core/issues/4145)). + ## [3.1.0.0.5] ### Added @@ -29,7 +31,6 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE - Miners who restart their nodes immediately before a winning tenure now correctly detect that they won the tenure after their nodes restart ([#5750](https://github.com/stacks-network/stacks-core/issues/5750)). -- Error responses to /v2/transactions/fees are once again expressed as JSON ([#4145](https://github.com/stacks-network/stacks-core/issues/4145)). ## [3.1.0.0.4]