From ef36e5cc00b0ea50b74a748e54138a339880cf6e Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 6 May 2024 15:25:40 -0400 Subject: [PATCH 1/6] chore: fix #4752 by requiring /v2/fees/transfer to return the median transaction fee for a STX-transfer from the cost estimators --- stackslib/src/net/api/getstxtransfercost.rs | 114 +++++++++++++++--- stackslib/src/net/api/postfeerate.rs | 86 ++++++++----- .../src/net/api/tests/getstxtransfercost.rs | 29 ++++- 3 files changed, 176 insertions(+), 53 deletions(-) diff --git a/stackslib/src/net/api/getstxtransfercost.rs b/stackslib/src/net/api/getstxtransfercost.rs index 5f732c650..bbf7d0bed 100644 --- a/stackslib/src/net/api/getstxtransfercost.rs +++ b/stackslib/src/net/api/getstxtransfercost.rs @@ -16,6 +16,7 @@ use std::io::{Read, Write}; +use clarity::vm::costs::ExecutionCost; use regex::{Captures, Regex}; use stacks_common::types::chainstate::{ BlockHeaderHash, ConsensusHash, StacksBlockId, StacksPublicKey, @@ -23,6 +24,7 @@ use stacks_common::types::chainstate::{ use stacks_common::types::net::PeerHost; use stacks_common::types::StacksPublicKeyBuffer; use stacks_common::util::hash::{Hash160, Sha256Sum}; +use url::form_urlencoded; use crate::burnchains::affirmation::AffirmationMap; use crate::burnchains::Txid; @@ -30,22 +32,30 @@ use crate::chainstate::burn::db::sortdb::SortitionDB; use crate::chainstate::stacks::db::blocks::MINIMUM_TX_FEE_RATE_PER_BYTE; use crate::chainstate::stacks::db::StacksChainState; use crate::core::mempool::MemPoolDB; +use crate::net::api::postfeerate::RPCPostFeeRateRequestHandler; use crate::net::http::{ - parse_json, Error, HttpRequest, HttpRequestContents, HttpRequestPreamble, HttpResponse, - HttpResponseContents, HttpResponsePayload, HttpResponsePreamble, + parse_json, Error, HttpBadRequest, HttpRequest, HttpRequestContents, HttpRequestPreamble, + HttpResponse, HttpResponseContents, HttpResponsePayload, HttpResponsePreamble, }; use crate::net::httpcore::{ HttpPreambleExtensions, RPCRequestHandler, StacksHttpRequest, StacksHttpResponse, }; use crate::net::p2p::PeerNetwork; -use crate::net::{Error as NetError, StacksNodeState}; +use crate::net::{Error as NetError, HttpServerError, StacksNodeState}; use crate::version_string; +pub(crate) const SINGLESIG_TX_TRANSFER_LEN: u64 = 180; + #[derive(Clone)] -pub struct RPCGetStxTransferCostRequestHandler {} +pub struct RPCGetStxTransferCostRequestHandler { + estimated_len: Option, +} + impl RPCGetStxTransferCostRequestHandler { pub fn new() -> Self { - Self {} + Self { + estimated_len: None, + } } } @@ -74,16 +84,33 @@ impl HttpRequest for RPCGetStxTransferCostRequestHandler { ) -> Result { if preamble.get_content_length() != 0 { return Err(Error::DecodeError( - "Invalid Http request: expected 0-length body for GetInfo".to_string(), + "Invalid Http request: expected 0-length body".to_string(), )); } + + let estimated_len = if let Some(qs) = query { + // possibly got a length= parameter + let mut len: Option = None; + for (key, value) in form_urlencoded::parse(qs.as_bytes()) { + if key == "length" { + len = value.parse::().ok(); + } + } + len.unwrap_or(SINGLESIG_TX_TRANSFER_LEN) + } else { + SINGLESIG_TX_TRANSFER_LEN + }; + + self.estimated_len = Some(estimated_len); Ok(HttpRequestContents::new().query_string(query)) } } impl RPCRequestHandler for RPCGetStxTransferCostRequestHandler { /// Reset internal state - fn restart(&mut self) {} + fn restart(&mut self) { + self.estimated_len = None; + } /// Make the response fn try_handle_request( @@ -92,9 +119,56 @@ impl RPCRequestHandler for RPCGetStxTransferCostRequestHandler { _contents: HttpRequestContents, node: &mut StacksNodeState, ) -> Result<(HttpResponsePreamble, HttpResponseContents), NetError> { - // todo -- need to actually estimate the cost / length for token transfers - // right now, it just uses the minimum. - let fee = MINIMUM_TX_FEE_RATE_PER_BYTE; + let estimated_len = self + .estimated_len + .take() + .ok_or(NetError::SendError("`estimated_len` not set".into()))?; + + let fee_resp = node.with_node_state(|_network, sortdb, _chainstate, _mempool, rpc_args| { + let tip = self.get_canonical_burn_chain_tip(&preamble, sortdb)?; + let stacks_epoch = self.get_stacks_epoch(&preamble, sortdb, tip.block_height)?; + + if let Some((_, fee_estimator, metric)) = rpc_args.get_estimators_ref() { + // STX transfer transactions have zero runtime cost + let estimated_cost = ExecutionCost::zero(); + let estimations = + RPCPostFeeRateRequestHandler::estimate_tx_fee_from_cost_and_length( + &preamble, + fee_estimator, + metric, + estimated_cost, + estimated_len, + stacks_epoch, + )? + .estimations; + if estimations.len() != 3 { + // logic bug, but treat as runtime error + return Err(StacksHttpResponse::new_error( + &preamble, + &HttpServerError::new( + "Logic error in fee estimation: did not get three estimates".into(), + ), + )); + } + + // safety -- checked estimations.len() == 3 above + let median_estimation = &estimations[1]; + Ok(median_estimation.fee) + } else { + // unlike `POST /v2/fees/transaction`, this method can't fail due to the + // unavailability of cost estimation, so just assume the minimum fee. + debug!("Fee and cost estimation not configured on this stacks node"); + Ok(MINIMUM_TX_FEE_RATE_PER_BYTE * estimated_len) + } + }); + + let fee = match fee_resp { + Ok(fee) => fee, + Err(response) => { + return response.try_into_contents().map_err(NetError::from); + } + }; + let mut preamble = HttpResponsePreamble::ok_json(&preamble); preamble.set_canonical_stacks_tip_height(Some(node.canonical_stacks_tip_height())); let body = HttpResponseContents::try_from_json(&fee)?; @@ -115,14 +189,20 @@ impl HttpResponse for RPCGetStxTransferCostRequestHandler { } impl StacksHttpRequest { + pub fn new_get_stx_transfer_cost_with_len( + host: PeerHost, + len: Option, + ) -> StacksHttpRequest { + let mut contents = HttpRequestContents::new(); + if let Some(l) = len { + contents = contents.query_arg("length".into(), format!("{}", &l)); + } + StacksHttpRequest::new_for_peer(host, "GET".into(), "/v2/fees/transfer".into(), contents) + .expect("FATAL: failed to construct request from infallible data") + } + pub fn new_get_stx_transfer_cost(host: PeerHost) -> StacksHttpRequest { - StacksHttpRequest::new_for_peer( - host, - "GET".into(), - "/v2/fees/transfer".into(), - HttpRequestContents::new(), - ) - .expect("FATAL: failed to construct request from infallible data") + Self::new_get_stx_transfer_cost_with_len(host, None) } } diff --git a/stackslib/src/net/api/postfeerate.rs b/stackslib/src/net/api/postfeerate.rs index ab9691fde..376d8bf3d 100644 --- a/stackslib/src/net/api/postfeerate.rs +++ b/stackslib/src/net/api/postfeerate.rs @@ -34,7 +34,9 @@ use crate::chainstate::stacks::db::blocks::MINIMUM_TX_FEE_RATE_PER_BYTE; use crate::chainstate::stacks::db::StacksChainState; use crate::chainstate::stacks::TransactionPayload; use crate::core::mempool::MemPoolDB; -use crate::cost_estimates::FeeRateEstimate; +use crate::core::StacksEpoch; +use crate::cost_estimates::metrics::CostMetric; +use crate::cost_estimates::{CostEstimator, FeeEstimator, FeeRateEstimate}; use crate::net::http::{ parse_json, Error, HttpBadRequest, HttpContentType, HttpNotFound, HttpRequest, HttpRequestContents, HttpRequestPreamble, HttpResponse, HttpResponseContents, @@ -92,6 +94,7 @@ pub struct RPCPostFeeRateRequestHandler { pub estimated_len: Option, pub transaction_payload: Option, } + impl RPCPostFeeRateRequestHandler { pub fn new() -> Self { Self { @@ -99,6 +102,48 @@ impl RPCPostFeeRateRequestHandler { transaction_payload: None, } } + + /// Estimate a transaction fee, given its execution cost estimation and length estimation + /// and cost estimators. + /// Returns Ok(fee structure) on success + /// Returns Err(HTTP response) on error + pub fn estimate_tx_fee_from_cost_and_length( + preamble: &HttpRequestPreamble, + fee_estimator: &dyn FeeEstimator, + metric: &dyn CostMetric, + estimated_cost: ExecutionCost, + estimated_len: u64, + stacks_epoch: StacksEpoch, + ) -> Result { + 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 + )), + ) + })?; + + let mut estimations = RPCFeeEstimate::estimate_fees(scalar_cost, fee_rates).to_vec(); + + let minimum_fee = estimated_len * MINIMUM_TX_FEE_RATE_PER_BYTE; + + for estimate in estimations.iter_mut() { + if estimate.fee < minimum_fee { + estimate.fee = minimum_fee; + } + } + + Ok(RPCFeeEstimateResponse { + estimated_cost, + estimations, + estimated_cost_scalar: scalar_cost, + cost_scalar_change_by_byte: metric.change_per_byte(), + }) + } } /// Decode the HTTP request @@ -206,39 +251,14 @@ impl RPCRequestHandler for 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 {}: {:?}", - &tx.name(), - &e - )), - ) - })?; - - let mut estimations = - RPCFeeEstimate::estimate_fees(scalar_cost, fee_rates).to_vec(); - - let minimum_fee = estimated_len * MINIMUM_TX_FEE_RATE_PER_BYTE; - - for estimate in estimations.iter_mut() { - if estimate.fee < minimum_fee { - estimate.fee = minimum_fee; - } - } - - Ok(RPCFeeEstimateResponse { + Self::estimate_tx_fee_from_cost_and_length( + &preamble, + fee_estimator, + metric, estimated_cost, - estimations, - estimated_cost_scalar: scalar_cost, - cost_scalar_change_by_byte: metric.change_per_byte(), - }) + estimated_len, + stacks_epoch, + ) } else { debug!("Fee and cost estimation not configured on this stacks node"); Err(StacksHttpResponse::new_error( diff --git a/stackslib/src/net/api/tests/getstxtransfercost.rs b/stackslib/src/net/api/tests/getstxtransfercost.rs index 6c4cccc36..328afe43d 100644 --- a/stackslib/src/net/api/tests/getstxtransfercost.rs +++ b/stackslib/src/net/api/tests/getstxtransfercost.rs @@ -25,6 +25,7 @@ use stacks_common::types::Address; use super::test_rpc; use crate::chainstate::stacks::db::blocks::MINIMUM_TX_FEE_RATE_PER_BYTE; use crate::core::BLOCK_LIMIT_MAINNET_21; +use crate::net::api::getstxtransfercost::SINGLESIG_TX_TRANSFER_LEN; use crate::net::api::*; use crate::net::connection::ConnectionOptions; use crate::net::httpcore::{ @@ -64,9 +65,12 @@ fn test_try_parse_request() { fn test_try_make_response() { let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 33333); let request = StacksHttpRequest::new_get_stx_transfer_cost(addr.into()); + let request_with_len = + StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(240)); - let mut responses = test_rpc(function_name!(), vec![request]); - assert_eq!(responses.len(), 1); + let mut responses = test_rpc(function_name!(), vec![request, request_with_len]); + assert_eq!(responses.len(), 2); + responses.reverse(); let response = responses.pop().unwrap(); debug!( @@ -80,5 +84,24 @@ fn test_try_make_response() { ); let fee_rate = response.decode_stx_transfer_fee().unwrap(); - assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); + debug!("fee_rate = {:?}", &fee_rate); + assert_eq!( + fee_rate, + MINIMUM_TX_FEE_RATE_PER_BYTE * SINGLESIG_TX_TRANSFER_LEN + ); + + let response = responses.pop().unwrap(); + debug!( + "Response:\n{}\n", + std::str::from_utf8(&response.try_serialize().unwrap()).unwrap() + ); + + assert_eq!( + response.preamble().get_canonical_stacks_tip_height(), + Some(1) + ); + + let fee_rate = response.decode_stx_transfer_fee().unwrap(); + debug!("fee_rate = {:?}", &fee_rate); + assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE * 240); } From dbc91b5b1627f2a8ea740ad3e8c6ef4e5a0da29c Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Mon, 6 May 2024 22:34:40 -0400 Subject: [PATCH 2/6] fix: this endpoint returns the fee _rate_, not the absolute fee --- stackslib/src/net/api/getstxtransfercost.rs | 6 ++++-- stackslib/src/net/api/tests/getstxtransfercost.rs | 9 +++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/stackslib/src/net/api/getstxtransfercost.rs b/stackslib/src/net/api/getstxtransfercost.rs index bbf7d0bed..7b7c2e66c 100644 --- a/stackslib/src/net/api/getstxtransfercost.rs +++ b/stackslib/src/net/api/getstxtransfercost.rs @@ -153,12 +153,14 @@ impl RPCRequestHandler for RPCGetStxTransferCostRequestHandler { // safety -- checked estimations.len() == 3 above let median_estimation = &estimations[1]; - Ok(median_estimation.fee) + + // NOTE: this returns the fee _rate_ + Ok(median_estimation.fee / estimated_len) } else { // unlike `POST /v2/fees/transaction`, this method can't fail due to the // unavailability of cost estimation, so just assume the minimum fee. debug!("Fee and cost estimation not configured on this stacks node"); - Ok(MINIMUM_TX_FEE_RATE_PER_BYTE * estimated_len) + Ok(MINIMUM_TX_FEE_RATE_PER_BYTE) } }); diff --git a/stackslib/src/net/api/tests/getstxtransfercost.rs b/stackslib/src/net/api/tests/getstxtransfercost.rs index 328afe43d..413ec8e35 100644 --- a/stackslib/src/net/api/tests/getstxtransfercost.rs +++ b/stackslib/src/net/api/tests/getstxtransfercost.rs @@ -66,7 +66,7 @@ fn test_try_make_response() { let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 33333); let request = StacksHttpRequest::new_get_stx_transfer_cost(addr.into()); let request_with_len = - StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(240)); + StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(180 * 2)); let mut responses = test_rpc(function_name!(), vec![request, request_with_len]); assert_eq!(responses.len(), 2); @@ -85,10 +85,7 @@ fn test_try_make_response() { let fee_rate = response.decode_stx_transfer_fee().unwrap(); debug!("fee_rate = {:?}", &fee_rate); - assert_eq!( - fee_rate, - MINIMUM_TX_FEE_RATE_PER_BYTE * SINGLESIG_TX_TRANSFER_LEN - ); + assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); let response = responses.pop().unwrap(); debug!( @@ -103,5 +100,5 @@ fn test_try_make_response() { let fee_rate = response.decode_stx_transfer_fee().unwrap(); debug!("fee_rate = {:?}", &fee_rate); - assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE * 240); + assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); } From 590839149721685cc41b6297bf133b08425ee3fb Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 7 May 2024 09:38:41 -0400 Subject: [PATCH 3/6] fix: avoid divide-by-zero with estimated length min --- stackslib/src/net/api/getstxtransfercost.rs | 2 +- .../src/net/api/tests/getstxtransfercost.rs | 24 +++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/stackslib/src/net/api/getstxtransfercost.rs b/stackslib/src/net/api/getstxtransfercost.rs index 7b7c2e66c..7a566eacc 100644 --- a/stackslib/src/net/api/getstxtransfercost.rs +++ b/stackslib/src/net/api/getstxtransfercost.rs @@ -96,7 +96,7 @@ impl HttpRequest for RPCGetStxTransferCostRequestHandler { len = value.parse::().ok(); } } - len.unwrap_or(SINGLESIG_TX_TRANSFER_LEN) + len.unwrap_or(SINGLESIG_TX_TRANSFER_LEN).min(1) } else { SINGLESIG_TX_TRANSFER_LEN }; diff --git a/stackslib/src/net/api/tests/getstxtransfercost.rs b/stackslib/src/net/api/tests/getstxtransfercost.rs index 413ec8e35..eed9937d8 100644 --- a/stackslib/src/net/api/tests/getstxtransfercost.rs +++ b/stackslib/src/net/api/tests/getstxtransfercost.rs @@ -67,9 +67,14 @@ fn test_try_make_response() { let request = StacksHttpRequest::new_get_stx_transfer_cost(addr.into()); let request_with_len = StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(180 * 2)); + let request_with_zero_len = + StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(0)); - let mut responses = test_rpc(function_name!(), vec![request, request_with_len]); - assert_eq!(responses.len(), 2); + let mut responses = test_rpc( + function_name!(), + vec![request, request_with_len, request_with_zero_len], + ); + assert_eq!(responses.len(), 3); responses.reverse(); let response = responses.pop().unwrap(); @@ -101,4 +106,19 @@ fn test_try_make_response() { let fee_rate = response.decode_stx_transfer_fee().unwrap(); debug!("fee_rate = {:?}", &fee_rate); assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); + + let response = responses.pop().unwrap(); + debug!( + "Response:\n{}\n", + std::str::from_utf8(&response.try_serialize().unwrap()).unwrap() + ); + + assert_eq!( + response.preamble().get_canonical_stacks_tip_height(), + Some(1) + ); + + let fee_rate = response.decode_stx_transfer_fee().unwrap(); + debug!("fee_rate = {:?}", &fee_rate); + assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); } From 6e624586ccef2e8a0b467f4e874801edebfdbf4b Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 7 May 2024 14:13:08 -0400 Subject: [PATCH 4/6] chore: remove length= --- stackslib/src/net/api/getstxtransfercost.rs | 46 +++---------------- .../src/net/api/tests/getstxtransfercost.rs | 41 +---------------- 2 files changed, 9 insertions(+), 78 deletions(-) diff --git a/stackslib/src/net/api/getstxtransfercost.rs b/stackslib/src/net/api/getstxtransfercost.rs index 7a566eacc..22e4b4826 100644 --- a/stackslib/src/net/api/getstxtransfercost.rs +++ b/stackslib/src/net/api/getstxtransfercost.rs @@ -47,15 +47,11 @@ use crate::version_string; pub(crate) const SINGLESIG_TX_TRANSFER_LEN: u64 = 180; #[derive(Clone)] -pub struct RPCGetStxTransferCostRequestHandler { - estimated_len: Option, -} +pub struct RPCGetStxTransferCostRequestHandler {} impl RPCGetStxTransferCostRequestHandler { pub fn new() -> Self { - Self { - estimated_len: None, - } + Self {} } } @@ -87,30 +83,13 @@ impl HttpRequest for RPCGetStxTransferCostRequestHandler { "Invalid Http request: expected 0-length body".to_string(), )); } - - let estimated_len = if let Some(qs) = query { - // possibly got a length= parameter - let mut len: Option = None; - for (key, value) in form_urlencoded::parse(qs.as_bytes()) { - if key == "length" { - len = value.parse::().ok(); - } - } - len.unwrap_or(SINGLESIG_TX_TRANSFER_LEN).min(1) - } else { - SINGLESIG_TX_TRANSFER_LEN - }; - - self.estimated_len = Some(estimated_len); Ok(HttpRequestContents::new().query_string(query)) } } impl RPCRequestHandler for RPCGetStxTransferCostRequestHandler { /// Reset internal state - fn restart(&mut self) { - self.estimated_len = None; - } + fn restart(&mut self) {} /// Make the response fn try_handle_request( @@ -119,10 +98,9 @@ impl RPCRequestHandler for RPCGetStxTransferCostRequestHandler { _contents: HttpRequestContents, node: &mut StacksNodeState, ) -> Result<(HttpResponsePreamble, HttpResponseContents), NetError> { - let estimated_len = self - .estimated_len - .take() - .ok_or(NetError::SendError("`estimated_len` not set".into()))?; + // NOTE: The estimated length isn't needed per se because we're returning a fee rate, but + // we do need an absolute length to use the estimator (so supply a common one). + let estimated_len = SINGLESIG_TX_TRANSFER_LEN; let fee_resp = node.with_node_state(|_network, sortdb, _chainstate, _mempool, rpc_args| { let tip = self.get_canonical_burn_chain_tip(&preamble, sortdb)?; @@ -191,21 +169,11 @@ impl HttpResponse for RPCGetStxTransferCostRequestHandler { } impl StacksHttpRequest { - pub fn new_get_stx_transfer_cost_with_len( - host: PeerHost, - len: Option, - ) -> StacksHttpRequest { + pub fn new_get_stx_transfer_cost(host: PeerHost) -> StacksHttpRequest { let mut contents = HttpRequestContents::new(); - if let Some(l) = len { - contents = contents.query_arg("length".into(), format!("{}", &l)); - } StacksHttpRequest::new_for_peer(host, "GET".into(), "/v2/fees/transfer".into(), contents) .expect("FATAL: failed to construct request from infallible data") } - - pub fn new_get_stx_transfer_cost(host: PeerHost) -> StacksHttpRequest { - Self::new_get_stx_transfer_cost_with_len(host, None) - } } impl StacksHttpResponse { diff --git a/stackslib/src/net/api/tests/getstxtransfercost.rs b/stackslib/src/net/api/tests/getstxtransfercost.rs index eed9937d8..66e557f41 100644 --- a/stackslib/src/net/api/tests/getstxtransfercost.rs +++ b/stackslib/src/net/api/tests/getstxtransfercost.rs @@ -65,16 +65,9 @@ fn test_try_parse_request() { fn test_try_make_response() { let addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 33333); let request = StacksHttpRequest::new_get_stx_transfer_cost(addr.into()); - let request_with_len = - StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(180 * 2)); - let request_with_zero_len = - StacksHttpRequest::new_get_stx_transfer_cost_with_len(addr.into(), Some(0)); - let mut responses = test_rpc( - function_name!(), - vec![request, request_with_len, request_with_zero_len], - ); - assert_eq!(responses.len(), 3); + let mut responses = test_rpc(function_name!(), vec![request]); + assert_eq!(responses.len(), 1); responses.reverse(); let response = responses.pop().unwrap(); @@ -91,34 +84,4 @@ fn test_try_make_response() { let fee_rate = response.decode_stx_transfer_fee().unwrap(); debug!("fee_rate = {:?}", &fee_rate); assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); - - let response = responses.pop().unwrap(); - debug!( - "Response:\n{}\n", - std::str::from_utf8(&response.try_serialize().unwrap()).unwrap() - ); - - assert_eq!( - response.preamble().get_canonical_stacks_tip_height(), - Some(1) - ); - - let fee_rate = response.decode_stx_transfer_fee().unwrap(); - debug!("fee_rate = {:?}", &fee_rate); - assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); - - let response = responses.pop().unwrap(); - debug!( - "Response:\n{}\n", - std::str::from_utf8(&response.try_serialize().unwrap()).unwrap() - ); - - assert_eq!( - response.preamble().get_canonical_stacks_tip_height(), - Some(1) - ); - - let fee_rate = response.decode_stx_transfer_fee().unwrap(); - debug!("fee_rate = {:?}", &fee_rate); - assert_eq!(fee_rate, MINIMUM_TX_FEE_RATE_PER_BYTE); } From a5f4151c1be4eb6f2969b61eb50bfc60fa4f42ab Mon Sep 17 00:00:00 2001 From: Jeff Bencin Date: Wed, 8 May 2024 13:07:13 -0400 Subject: [PATCH 5/6] feat: Pass `PeerNetwork` from Neon node to Nakamoto node in order to maintain network connections --- testnet/stacks-node/src/nakamoto_node.rs | 5 ++++- testnet/stacks-node/src/neon_node.rs | 14 +++++++++----- .../stacks-node/src/run_loop/boot_nakamoto.rs | 6 +++--- testnet/stacks-node/src/run_loop/nakamoto.rs | 10 ++++++++-- testnet/stacks-node/src/run_loop/neon.rs | 19 ++++++++++++------- 5 files changed, 36 insertions(+), 18 deletions(-) diff --git a/testnet/stacks-node/src/nakamoto_node.rs b/testnet/stacks-node/src/nakamoto_node.rs index 439486a34..8a1d80de3 100644 --- a/testnet/stacks-node/src/nakamoto_node.rs +++ b/testnet/stacks-node/src/nakamoto_node.rs @@ -25,6 +25,7 @@ use stacks::chainstate::stacks::Error as ChainstateError; use stacks::monitoring; use stacks::monitoring::update_active_miners_count_gauge; use stacks::net::atlas::AtlasConfig; +use stacks::net::p2p::PeerNetwork; use stacks::net::relay::Relayer; use stacks::net::stackerdb::StackerDBs; use stacks_common::types::chainstate::SortitionId; @@ -132,6 +133,7 @@ impl StacksNode { globals: Globals, // relay receiver endpoint for the p2p thread, so the relayer can feed it data to push relay_recv: Receiver, + peer_network: Option, ) -> StacksNode { let config = runloop.config().clone(); let is_miner = runloop.is_miner(); @@ -157,7 +159,8 @@ impl StacksNode { .connect_mempool_db() .expect("FATAL: database failure opening mempool"); - let mut p2p_net = NeonNode::setup_peer_network(&config, &atlas_config, burnchain); + let mut p2p_net = peer_network + .unwrap_or_else(|| NeonNode::setup_peer_network(&config, &atlas_config, burnchain)); let stackerdbs = StackerDBs::connect(&config.get_stacker_db_file_path(), true) .expect("FATAL: failed to connect to stacker DB"); diff --git a/testnet/stacks-node/src/neon_node.rs b/testnet/stacks-node/src/neon_node.rs index 28e655277..3a767b980 100644 --- a/testnet/stacks-node/src/neon_node.rs +++ b/testnet/stacks-node/src/neon_node.rs @@ -298,7 +298,7 @@ pub struct StacksNode { /// True if we're a miner is_miner: bool, /// handle to the p2p thread - pub p2p_thread_handle: JoinHandle<()>, + pub p2p_thread_handle: JoinHandle>, /// handle to the relayer thread pub relayer_thread_handle: JoinHandle<()>, } @@ -4655,7 +4655,10 @@ impl StacksNode { /// Main loop of the p2p thread. /// Runs in a separate thread. /// Continuously receives, until told otherwise. - pub fn p2p_main(mut p2p_thread: PeerThread, event_dispatcher: EventDispatcher) { + pub fn p2p_main( + mut p2p_thread: PeerThread, + event_dispatcher: EventDispatcher, + ) -> Option { let should_keep_running = p2p_thread.globals.should_keep_running.clone(); let (mut dns_resolver, mut dns_client) = DNSResolver::new(10); @@ -4718,6 +4721,7 @@ impl StacksNode { thread::sleep(Duration::from_secs(5)); } info!("P2P thread exit!"); + p2p_thread.net } /// This function sets the global var `GLOBAL_BURNCHAIN_SIGNER`. @@ -4814,7 +4818,7 @@ impl StacksNode { )) .spawn(move || { debug!("p2p thread ID is {:?}", thread::current().id()); - Self::p2p_main(p2p_thread, p2p_event_dispatcher); + Self::p2p_main(p2p_thread, p2p_event_dispatcher) }) .expect("FATAL: failed to start p2p thread"); @@ -5017,8 +5021,8 @@ impl StacksNode { } /// Join all inner threads - pub fn join(self) { + pub fn join(self) -> Option { self.relayer_thread_handle.join().unwrap(); - self.p2p_thread_handle.join().unwrap(); + self.p2p_thread_handle.join().unwrap() } } diff --git a/testnet/stacks-node/src/run_loop/boot_nakamoto.rs b/testnet/stacks-node/src/run_loop/boot_nakamoto.rs index dec1ca757..087e1424e 100644 --- a/testnet/stacks-node/src/run_loop/boot_nakamoto.rs +++ b/testnet/stacks-node/src/run_loop/boot_nakamoto.rs @@ -108,7 +108,7 @@ impl BootRunLoop { let InnerLoops::Epoch3(ref mut naka_loop) = self.active_loop else { panic!("FATAL: unexpectedly invoked start_from_naka when active loop wasn't nakamoto"); }; - naka_loop.start(burnchain_opt, mine_start) + naka_loop.start(burnchain_opt, mine_start, None) } fn start_from_neon(&mut self, burnchain_opt: Option, mine_start: u64) { @@ -120,7 +120,7 @@ impl BootRunLoop { let boot_thread = Self::spawn_stopper(&self.config, neon_loop) .expect("FATAL: failed to spawn epoch-2/3-boot thread"); - neon_loop.start(burnchain_opt.clone(), mine_start); + let peer_network = neon_loop.start(burnchain_opt.clone(), mine_start); let monitoring_thread = neon_loop.take_monitoring_thread(); // did we exit because of the epoch-3.0 transition, or some other reason? @@ -150,7 +150,7 @@ impl BootRunLoop { let InnerLoops::Epoch3(ref mut naka_loop) = self.active_loop else { panic!("FATAL: unexpectedly found epoch2 loop after setting epoch3 active"); }; - naka_loop.start(burnchain_opt, mine_start) + naka_loop.start(burnchain_opt, mine_start, peer_network) } fn spawn_stopper( diff --git a/testnet/stacks-node/src/run_loop/nakamoto.rs b/testnet/stacks-node/src/run_loop/nakamoto.rs index b7cdb8a05..997327287 100644 --- a/testnet/stacks-node/src/run_loop/nakamoto.rs +++ b/testnet/stacks-node/src/run_loop/nakamoto.rs @@ -31,6 +31,7 @@ use stacks::chainstate::stacks::db::{ChainStateBootData, StacksChainState}; use stacks::chainstate::stacks::miner::{signal_mining_blocked, signal_mining_ready, MinerStatus}; use stacks::core::StacksEpochId; use stacks::net::atlas::{AtlasConfig, AtlasDB, Attachment}; +use stacks::net::p2p::PeerNetwork; use stacks_common::types::PublicKey; use stacks_common::util::hash::Hash160; use stx_genesis::GenesisData; @@ -392,7 +393,12 @@ impl RunLoop { /// It will start the burnchain (separate thread), set-up a channel in /// charge of coordinating the new blocks coming from the burnchain and /// the nodes, taking turns on tenures. - pub fn start(&mut self, burnchain_opt: Option, mut mine_start: u64) { + pub fn start( + &mut self, + burnchain_opt: Option, + mut mine_start: u64, + peer_network: Option, + ) { let (coordinator_receivers, coordinator_senders) = self .coordinator_channels .take() @@ -475,7 +481,7 @@ impl RunLoop { // Boot up the p2p network and relayer, and figure out how many sortitions we have so far // (it could be non-zero if the node is resuming from chainstate) - let mut node = StacksNode::spawn(self, globals.clone(), relay_recv); + let mut node = StacksNode::spawn(self, globals.clone(), relay_recv, peer_network); // Wait for all pending sortitions to process let burnchain_db = burnchain_config diff --git a/testnet/stacks-node/src/run_loop/neon.rs b/testnet/stacks-node/src/run_loop/neon.rs index c9925ce19..157fa71cd 100644 --- a/testnet/stacks-node/src/run_loop/neon.rs +++ b/testnet/stacks-node/src/run_loop/neon.rs @@ -21,6 +21,7 @@ use stacks::chainstate::stacks::db::{ChainStateBootData, StacksChainState}; use stacks::chainstate::stacks::miner::{signal_mining_blocked, signal_mining_ready, MinerStatus}; use stacks::core::StacksEpochId; use stacks::net::atlas::{AtlasConfig, AtlasDB, Attachment}; +use stacks::net::p2p::PeerNetwork; use stacks::util_lib::db::Error as db_error; use stacks_common::deps_common::ctrlc as termination; use stacks_common::deps_common::ctrlc::SignalId; @@ -999,7 +1000,11 @@ impl RunLoop { /// It will start the burnchain (separate thread), set-up a channel in /// charge of coordinating the new blocks coming from the burnchain and /// the nodes, taking turns on tenures. - pub fn start(&mut self, burnchain_opt: Option, mut mine_start: u64) { + pub fn start( + &mut self, + burnchain_opt: Option, + mut mine_start: u64, + ) -> Option { let (coordinator_receivers, coordinator_senders) = self .coordinator_channels .take() @@ -1018,12 +1023,12 @@ impl RunLoop { Ok(burnchain_controller) => burnchain_controller, Err(burnchain_error::ShutdownInitiated) => { info!("Exiting stacks-node"); - return; + return None; } Err(e) => { error!("Error initializing burnchain: {}", e); info!("Exiting stacks-node"); - return; + return None; } }; @@ -1142,11 +1147,11 @@ impl RunLoop { globals.coord().stop_chains_coordinator(); coordinator_thread_handle.join().unwrap(); - node.join(); + let peer_network = node.join(); liveness_thread.join().unwrap(); info!("Exiting stacks-node"); - break; + break peer_network; } let remote_chain_height = burnchain.get_headers_height() - 1; @@ -1269,7 +1274,7 @@ impl RunLoop { if !node.relayer_sortition_notify() { // relayer hung up, exit. error!("Runloop: Block relayer and miner hung up, exiting."); - return; + return None; } } @@ -1343,7 +1348,7 @@ impl RunLoop { if !node.relayer_issue_tenure(ibd) { // relayer hung up, exit. error!("Runloop: Block relayer and miner hung up, exiting."); - break; + break None; } } } From c65fa5ce5926f5e575acf03fbce4809d0d8f3a8f Mon Sep 17 00:00:00 2001 From: Jeff Bencin Date: Thu, 9 May 2024 10:44:44 -0400 Subject: [PATCH 6/6] fix: Skip `PeerNetwork::bind()` if re-using object --- stackslib/src/net/p2p.rs | 18 ++++++++++++++++++ testnet/stacks-node/src/nakamoto_node/peer.rs | 9 +++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/stackslib/src/net/p2p.rs b/stackslib/src/net/p2p.rs index 4e358128f..26f85d69e 100644 --- a/stackslib/src/net/p2p.rs +++ b/stackslib/src/net/p2p.rs @@ -646,6 +646,24 @@ impl PeerNetwork { Ok(()) } + /// Call `bind()` only if not already bound + /// Returns: + /// - `Ok(true)` if `bind()` call was successful + /// - `Ok(false)` if `bind()` call was skipped + /// - `Err()` if `bind()`` failed + #[cfg_attr(test, mutants::skip)] + pub fn try_bind( + &mut self, + my_addr: &SocketAddr, + http_addr: &SocketAddr, + ) -> Result { + if self.network.is_some() { + // Already bound + return Ok(false); + } + self.bind(my_addr, http_addr).map(|()| true) + } + /// Get bound neighbor key. This is how this PeerNetwork appears to other nodes. pub fn bound_neighbor_key(&self) -> &NeighborKey { &self.bind_nk diff --git a/testnet/stacks-node/src/nakamoto_node/peer.rs b/testnet/stacks-node/src/nakamoto_node/peer.rs index eeb6789d3..dc060e06b 100644 --- a/testnet/stacks-node/src/nakamoto_node/peer.rs +++ b/testnet/stacks-node/src/nakamoto_node/peer.rs @@ -182,8 +182,13 @@ impl PeerThread { .parse() .unwrap_or_else(|_| panic!("Failed to parse socket: {}", &config.node.rpc_bind)); - net.bind(&p2p_sock, &rpc_sock) - .expect("BUG: PeerNetwork could not bind or is already bound"); + let did_bind = net + .try_bind(&p2p_sock, &rpc_sock) + .expect("BUG: PeerNetwork could not bind"); + + if !did_bind { + info!("`PeerNetwork::bind()` skipped, already bound"); + } let poll_timeout = cmp::min(5000, config.miner.first_attempt_time_ms / 2);