From 4de7c20a5198a18a4c2bcae34c2b9454204de760 Mon Sep 17 00:00:00 2001 From: qlrddev Date: Mon, 28 Apr 2025 15:07:51 -0300 Subject: [PATCH 1/3] Fix addnode json-rpc command compliant with bitcoin-core: Partial fix of #453 * added [add|remove|onetry] required argument to addnode command on floresta-cli crate; * added v2transport optional argument to addnode command on floresta-cli crate; * changed the return type of addnode command to null json (compliant with bitcoin-core); * removed the connect method in florestad/server and replaced them to addnode method that deal with the [add|remove|onetry] things; * added PeerAlreadyExists and FailedToAddPeer Errors on floresta-wire crate; * added handlers for connect and disconnect in floresta-wire crate (to comply with the changes with floresta-cli). Co-authored-by: Davidson Souza --- crates/floresta-cli/src/main.rs | 37 ++- crates/floresta-cli/src/rpc.rs | 13 +- crates/floresta-cli/src/rpc_types.rs | 29 ++ crates/floresta-wire/src/p2p_wire/error.rs | 5 + crates/floresta-wire/src/p2p_wire/node.rs | 274 ++++++++++++++++-- .../src/p2p_wire/node_interface.rs | 63 +++- florestad/src/json_rpc/res.rs | 2 + florestad/src/json_rpc/server.rs | 22 +- 8 files changed, 399 insertions(+), 46 deletions(-) diff --git a/crates/floresta-cli/src/main.rs b/crates/floresta-cli/src/main.rs index 9dd339d8..40950950 100644 --- a/crates/floresta-cli/src/main.rs +++ b/crates/floresta-cli/src/main.rs @@ -8,6 +8,7 @@ use clap::Parser; use clap::Subcommand; use floresta_cli::jsonrpc_client::Client; use floresta_cli::rpc::FlorestaRPC; +use floresta_cli::rpc_types::AddNodeCommand; use floresta_cli::rpc_types::GetBlockRes; // Main function that runs the CLI application @@ -85,7 +86,15 @@ fn do_request(cmd: &Cli, client: Client) -> anyhow::Result { } Methods::GetPeerInfo => serde_json::to_string_pretty(&client.get_peer_info()?)?, Methods::Stop => serde_json::to_string_pretty(&client.stop()?)?, - Methods::AddNode { node } => serde_json::to_string_pretty(&client.add_node(node)?)?, + Methods::AddNode { + node, + command, + v2transport, + } => { + let transport = v2transport.unwrap_or(false); + serde_json::to_string_pretty(&client.add_node(node, command, transport)?)? + } + Methods::FindTxOut { txid, vout, @@ -189,10 +198,30 @@ pub enum Methods { /// Stops the node #[command(name = "stop")] Stop, - /// Connects with a peer, given its address and port - /// Usage: addnode + /// Attempts to add or remove a node from the addnode list. + /// Or try a connection to a node once. + /// + /// Arguments: + /// 1. node (string, required): + /// - The address of the peer to connect to; + /// + /// 2. command (string, required): + /// - 'add' to add a node to the list; + /// - 'remove' to remove a node from the list; + /// - 'onetry' to try a connection to the node once. + /// + /// 3. v2transport (boolean, optional, default=false): + /// - Attempt to connect using BIP324 v2 transport protocol (ignored for 'remove' command) + /// + /// Result: json null + /// + /// Usage: addnode [true|false] #[command(name = "addnode")] - AddNode { node: String }, + AddNode { + node: String, + command: AddNodeCommand, + v2transport: Option, + }, #[command(name = "findtxout")] FindTxOut { txid: Txid, diff --git a/crates/floresta-cli/src/rpc.rs b/crates/floresta-cli/src/rpc.rs index 0162c48a..26927b73 100644 --- a/crates/floresta-cli/src/rpc.rs +++ b/crates/floresta-cli/src/rpc.rs @@ -104,7 +104,7 @@ pub trait FlorestaRPC { /// Tells florestad to connect with a peer /// /// You can use this to connect with a given node, providing it's IP address and port. - fn add_node(&self, node: String) -> Result; + fn add_node(&self, node: String, command: AddNodeCommand, v2transport: bool) -> Result; /// Finds an specific utxo in the chain /// /// You can use this to look for a utxo. If it exists, it will return the amount and @@ -170,8 +170,15 @@ impl FlorestaRPC for T { self.call("getrpcinfo", &[]) } - fn add_node(&self, node: String) -> Result { - self.call("addnode", &[Value::String(node)]) + fn add_node(&self, node: String, command: AddNodeCommand, v2transport: bool) -> Result { + self.call( + "addnode", + &[ + Value::String(node), + Value::String(command.to_string()), + Value::Bool(v2transport), + ], + ) } fn stop(&self) -> Result { diff --git a/crates/floresta-cli/src/rpc_types.rs b/crates/floresta-cli/src/rpc_types.rs index a8b113ce..8dbf7730 100644 --- a/crates/floresta-cli/src/rpc_types.rs +++ b/crates/floresta-cli/src/rpc_types.rs @@ -1,5 +1,6 @@ use std::fmt::Display; +use clap::ValueEnum; use serde::Deserialize; use serde::Serialize; @@ -357,4 +358,32 @@ pub struct GetRpcInfoRes { logpath: String, } +#[derive(Debug, Clone, ValueEnum, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +/// Enum to represent the different subcommands for the addnode command +pub enum AddNodeCommand { + /// Add a node to the addnode list (but not connect to it) + Add, + + /// Remove a node from the addnode list (but not necessarily disconnect from it) + Remove, + + /// Connect to a node once, but don't add it to the addnode list + Onetry, +} + +/// A simple implementation to convert the enum to a string. +/// Useful for get the subcommand name of addnode with +/// command.to_string() +impl Display for AddNodeCommand { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let cmd = match self { + AddNodeCommand::Add => "add", + AddNodeCommand::Remove => "remove", + AddNodeCommand::Onetry => "onetry", + }; + write!(f, "{cmd}") + } +} + impl std::error::Error for Error {} diff --git a/crates/floresta-wire/src/p2p_wire/error.rs b/crates/floresta-wire/src/p2p_wire/error.rs index d738403a..8fac5947 100644 --- a/crates/floresta-wire/src/p2p_wire/error.rs +++ b/crates/floresta-wire/src/p2p_wire/error.rs @@ -2,6 +2,7 @@ use std::fmt::Display; use std::fmt::Formatter; use std::fmt::{self}; use std::io; +use std::net::IpAddr; use floresta_chain::BlockchainError; use floresta_common::impl_error_from; @@ -30,6 +31,10 @@ pub enum WireError { PeerMisbehaving, #[error("Failed to init Utreexo peers: anchors.json does not exist yet")] AnchorFileNotFound, + #[error("Peer {0}:{1} already exists")] + PeerAlreadyExists(IpAddr, u16), + #[error("Peer {0}:{1} not found")] + PeerNotFoundAtAddress(IpAddr, u16), #[error("Generic io error: {0}")] Io(std::io::Error), #[error("{0}")] diff --git a/crates/floresta-wire/src/p2p_wire/node.rs b/crates/floresta-wire/src/p2p_wire/node.rs index d159ae41..3285820e 100644 --- a/crates/floresta-wire/src/p2p_wire/node.rs +++ b/crates/floresta-wire/src/p2p_wire/node.rs @@ -75,7 +75,7 @@ const DNS_SEED_RETRY_PERIOD: Duration = Duration::from_secs(5 * 60); pub enum NodeNotification { DnsSeedAddresses(Vec), FromPeer(u32, PeerMessages), - FromUser(UserRequest, tokio::sync::oneshot::Sender), + FromUser(UserRequest, oneshot::Sender), } #[derive(Debug, Clone, PartialEq, Hash)] @@ -176,6 +176,7 @@ pub struct NodeCommon { pub(crate) peer_by_service: HashMap>, pub(crate) max_banscore: u32, pub(crate) address_man: AddressMan, + pub(crate) added_peers: Vec, // 3. Internal Communication pub(crate) node_rx: UnboundedReceiver, @@ -237,6 +238,19 @@ impl DerefMut for UtreexoNode { } } +#[derive(Debug, Clone)] +/// A simple struct of added peers, used to track the ones we added manually by `addnode add` command. +pub struct AddedPeerInfo { + /// The address of the peer + pub(crate) address: AddrV2, + + /// The port of the peer + pub(crate) port: u16, + + /// The transport protocol used to connect to the peer (either [`TransportProtocol::V1`] or [`TransportProtocol::V2`]) + pub(crate) transport_protocol: TransportProtocol, +} + #[derive(Debug, PartialEq, Clone, Copy, Deserialize, Serialize)] pub enum PeerStatus { Awaiting, @@ -301,6 +315,7 @@ where fixed_peer, config, kill_signal, + added_peers: Vec::new(), }, context: T::default(), }) @@ -362,7 +377,7 @@ where /// Handles getpeerinfo requests, returning a list of all connected peers and some useful /// information about it. - fn handle_get_peer_info(&self, responder: tokio::sync::oneshot::Sender) { + fn handle_get_peer_info(&self, responder: oneshot::Sender) { let mut peers = Vec::new(); for peer in self.peer_ids.iter() { peers.push(self.get_peer_info(peer)); @@ -372,6 +387,110 @@ where responder.send(NodeResponse::GetPeerInfo(peers)).unwrap(); } + // Helper function to resolve an IpAddr to AddrV2 + // This is a little bit of a hack while rust-bitcoin + // do not have an `from` or `into` that do IpAddr <> AddrV2 + fn to_addr_v2(&self, addr: IpAddr) -> AddrV2 { + match addr { + IpAddr::V4(addr) => AddrV2::Ipv4(addr), + IpAddr::V6(addr) => AddrV2::Ipv6(addr), + } + } + + /// Handles addnode add requests, adding a new peer to the node and add a node to the added_node list. + /// This means the node is marked as a "manually added node" for future connection attempts does not directly establish a connection to the node. + /// Instead, the connection management process will attempt to connect to the manually added nodes in the background + pub fn handle_addnode_add_peer( + &mut self, + addr: IpAddr, + port: u16, + transport_protocol: TransportProtocol, + ) -> Result<(), WireError> { + // See https://github.com/bitcoin/bitcoin/blob/8309a9747a8df96517970841b3648937d05939a3/src/net.cpp#L3558 + debug!("Trying to add peer {addr}:{port} with transport_protocol={transport_protocol:?}"); + let address = self.to_addr_v2(addr); + + // Check if the peer already exists + if self + .added_peers + .iter() + .any(|info| address == info.address && port == info.port) + { + return Err(WireError::PeerAlreadyExists(addr, port)); + } + + // Add a simple reference to the peer + self.added_peers.push(AddedPeerInfo { + address, + port, + transport_protocol, + }); + Ok(()) + } + + /// Handles remove node requests, removing a peer from the node. + /// + /// Removes a node from the [`added_peers`] list but does not + /// disconnect the node if it was already connected. It only ensures + /// that the node is no longer treated as a manually added node + /// (i.e., it won't be reconnected if disconnected). + /// + /// If someone wants to remove a peer, it should be done using the + /// `disconnectnode`. + pub fn handle_addnode_remove_peer(&mut self, addr: IpAddr, port: u16) -> Result<(), WireError> { + // + // (TODO) Make `disconnectnode`` command. + debug!("Trying to remove peer {addr}:{port}"); + + let address = self.to_addr_v2(addr); + let index = self + .added_peers + .iter() + .position(|info| address == info.address && port == info.port); + + match index { + Some(peer_id) => self.added_peers.remove(peer_id), + None => return Err(WireError::PeerNotFoundAtAddress(addr, port)), + }; + + Ok(()) + } + + /// Handles addnode onetry requests, connecting to the node and this will try to connect to the given address and port. + /// If it's successful, it will add the node to the peers list, but not to the added_peers list (e.g., it won't be reconnected if disconnected). + pub async fn handle_addnode_onetry_peer( + &mut self, + addr: IpAddr, + port: u16, + transport_protocol: TransportProtocol, + ) -> Result<(), WireError> { + debug!("Trying to connect to peer {addr}:{port} with transport_protocol={transport_protocol:?}"); + + // Check if the peer already exists + if self + .peers + .iter() + .any(|(_, peer)| addr == peer.address && port == peer.port) + { + return Err(WireError::PeerAlreadyExists(addr, port)); + } + + let kind = ConnectionKind::Regular(ServiceFlags::NONE); + let peer_id = self.peer_id_count; + let address = LocalAddress::new( + self.to_addr_v2(addr), + 0, + AddressState::NeverTried, + ServiceFlags::NONE, + port, + peer_id as usize, + ); + + // Return true if exists or false if anything fails during connection + self.open_connection(kind, peer_id as usize, address, transport_protocol) + .await + } + /// Actually perform the user request /// /// These are requests made by some consumer of `floresta-wire` using the [`NodeInterface`], and may @@ -379,7 +498,7 @@ where pub(crate) async fn perform_user_request( &mut self, user_req: UserRequest, - responder: tokio::sync::oneshot::Sender, + responder: oneshot::Sender, ) { if self.inflight.len() >= RunningNode::MAX_INFLIGHT_REQUESTS { return; @@ -395,24 +514,65 @@ where self.handle_get_peer_info(responder); return; } - UserRequest::Connect((addr, port)) => { - let addr_v2 = match addr { - IpAddr::V4(addr) => AddrV2::Ipv4(addr), - IpAddr::V6(addr) => AddrV2::Ipv6(addr), + UserRequest::Add((addr, port, v2transport)) => { + let transport_protocol = if v2transport { + TransportProtocol::V2 + } else { + TransportProtocol::V1 + }; + + let node_response = + match self.handle_addnode_add_peer(addr, port, transport_protocol) { + Ok(_) => { + info!("Added peer {addr}:{port}"); + NodeResponse::Add(true) + } + Err(err) => { + warn!("{err:?}"); + NodeResponse::Add(false) + } + }; + + let _ = responder.send(node_response); + return; + } + UserRequest::Remove((addr, port)) => { + let node_response = match self.handle_addnode_remove_peer(addr, port) { + Ok(_) => { + info!("Removed peer {addr}:{port}"); + NodeResponse::Remove(true) + } + Err(err) => { + warn!("{err:?}"); + NodeResponse::Remove(false) + } }; - let local_addr = LocalAddress::new( - addr_v2, - 0, - AddressState::NeverTried, - 0.into(), - port, - self.peer_id_count as usize, - ); - self.open_connection(ConnectionKind::Regular(ServiceFlags::NONE), 0, local_addr) - .await; - self.peer_id_count += 1; - let _ = responder.send(NodeResponse::Connect(true)); + let _ = responder.send(node_response); + return; + } + UserRequest::Onetry((addr, port, v2transport)) => { + let transport_protocol = if v2transport { + TransportProtocol::V2 + } else { + TransportProtocol::V1 + }; + + let node_response = match self + .handle_addnode_onetry_peer(addr, port, transport_protocol) + .await + { + Ok(_) => { + info!("Connected to peer {addr}:{port}"); + NodeResponse::Onetry(true) + } + Err(err) => { + warn!("{err:?}"); + NodeResponse::Onetry(false) + } + }; + + let _ = responder.send(node_response); return; } }; @@ -1025,8 +1185,13 @@ where } for address in anchors { - self.open_connection(ConnectionKind::Regular(UTREEXO.into()), address.id, address) - .await; + self.open_connection( + ConnectionKind::Regular(UTREEXO.into()), + address.id, + address, + TransportProtocol::V1, // Default to V1, will be updated when peer is ready, + ) + .await?; } Ok(()) @@ -1181,6 +1346,46 @@ where self.address_man.add_fixed_addresses(net); } + pub(crate) async fn maybe_open_connection_with_added_peers(&mut self) -> Result<(), WireError> { + if self.added_peers.is_empty() { + return Ok(()); + } + + let peers_count = self.peer_id_count; + for added_peer in self.added_peers.clone() { + let matching_peer = self.peers.values().find(|peer| { + self.to_addr_v2(peer.address) == added_peer.address && peer.port == added_peer.port + }); + + if matching_peer.is_none() { + let address = LocalAddress::new( + added_peer.address.clone(), + 0, + AddressState::Tried( + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs(), + ), + ServiceFlags::NONE, + added_peer.port, + peers_count as usize, + ); + + // Finally, open the connection with the node + self.open_connection( + ConnectionKind::Regular(ServiceFlags::NONE), + peers_count as usize, + address, + added_peer.transport_protocol, + ) + .await? + } + } + + Ok(()) + } + pub(crate) async fn maybe_open_connection( &mut self, required_service: ServiceFlags, @@ -1196,6 +1401,9 @@ where self.maybe_ask_for_dns_peers(); self.maybe_use_hadcoded_addresses(); + // try to connect with mannually added peers + self.maybe_open_connection_with_added_peers().await?; + let connection_kind = ConnectionKind::Regular(required_service); if self.peers.len() < T::MAX_OUTGOING_PEERS { self.create_connection(connection_kind).await; @@ -1273,7 +1481,11 @@ where { return None; } - self.open_connection(kind, peer_id, address).await; + + // Default to V1, will be updated when peer is ready.) + self.open_connection(kind, peer_id, address, TransportProtocol::V1) + .await + .ok()?; Some(()) } @@ -1368,15 +1580,18 @@ where Ok(()) } - /// Creates a new outgoing connection with `address`. Connection may or may not be feeler, - /// a special connection type that is used to learn about good peers, but are not kept after - /// handshake. + /// Creates a new outgoing connection with `address`. The [`kind`] may or may not be a + /// a [`ConnectionKind::Feeler`], a special connection type that is used to learn about + /// good peers, but are not kept after handshake (others are [`ConnectionKind::Regular`] and + /// [`ConnectionKind::Extra`]). The `transport_protocol` identify the version of the + /// transport protocol used, either [`TransportProtocol::V1`] or [`TransportProtocol::V2`]. pub(crate) async fn open_connection( &mut self, kind: ConnectionKind, peer_id: usize, address: LocalAddress, - ) { + transport_protocol: TransportProtocol, + ) -> Result<(), WireError> { let (requests_tx, requests_rx) = unbounded_channel(); if let Some(ref proxy) = self.socks5 { spawn(timeout( @@ -1434,11 +1649,16 @@ where address_id: peer_id as u32, height: 0, banscore: 0, - transport_protocol: TransportProtocol::V1, // Default to V1, will be updated when peer is ready. + transport_protocol, }, ); + // Increment peer_id count and the list of peer ids + // so we can get information about connected or + // added peers when requesting with getpeerinfo command self.peer_id_count += 1; + + Ok(()) } } diff --git a/crates/floresta-wire/src/p2p_wire/node_interface.rs b/crates/floresta-wire/src/p2p_wire/node_interface.rs index 14855973..61cf6bc7 100644 --- a/crates/floresta-wire/src/p2p_wire/node_interface.rs +++ b/crates/floresta-wire/src/p2p_wire/node_interface.rs @@ -18,6 +18,24 @@ use super::node::NodeNotification; use super::node::PeerStatus; use super::transport::TransportProtocol; +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +/// A request to addnode that can be made to the node. +/// +/// This enum represents all the possible requests that can be made to the node to add, remove +/// or just try to connect to a peer, following the same pattern as the `addnode` command in [Bitcoin Core]. +/// +/// [Bitcoin Core]: (https://bitcoincore.org/en/doc/29.0.0/rpc/network/addnode/) +pub enum AddNode { + /// The `Add` variant is used to add a peer to the node's peer list + Add((IpAddr, u16)), + + /// The `Remove` variant is used to remove a peer from the node's peer list + Remove((IpAddr, u16)), + + /// The `Onetry` variant is used to try a connection to the peer once, but not add it to the peer list. + Onetry((IpAddr, u16)), +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] /// A request that can be made to the node. /// @@ -30,7 +48,9 @@ pub enum UserRequest { UtreexoBlock(BlockHash), MempoolTransaction(Txid), GetPeerInfo, - Connect((IpAddr, u16)), + Add((IpAddr, u16, bool)), + Remove((IpAddr, u16)), + Onetry((IpAddr, u16, bool)), } #[derive(Debug, Clone, Serialize)] @@ -59,7 +79,9 @@ pub enum NodeResponse { UtreexoBlock(Option), MempoolTransaction(Option), GetPeerInfo(Vec), - Connect(bool), + Add(bool), + Remove(bool), + Onetry(bool), } #[derive(Debug, Clone)] @@ -101,19 +123,48 @@ impl NodeInterface { } /// Connects to a specified address and port. - /// /// This function will return a boolean indicating whether the connection was successful. It /// may be called multiple times, and may use hostnames or IP addresses. - pub async fn connect( + pub async fn add_peer( &self, addr: IpAddr, port: u16, + v2transport: bool, ) -> Result { let val = self - .send_request(UserRequest::Connect((addr, port))) + .send_request(UserRequest::Add((addr, port, v2transport))) .await?; - extract_variant!(Connect, val); + extract_variant!(Add, val); + } + + /// Removes a peer from the node's peer list. + /// This function will return a boolean indicating whether the peer was successfully removed. + /// It may be called multiple times, and may use hostnames or IP addresses. + pub async fn remove_peer( + &self, + addr: IpAddr, + port: u16, + ) -> Result { + let val = self.send_request(UserRequest::Remove((addr, port))).await?; + extract_variant!(Remove, val); + } + + /// Attempts to connect to a peer once. + /// + /// This function will try to connect to the peer once, but will not add it to the node's + /// peer list. It will return a boolean indicating whether the connection was successful. + /// It may be called multiple times, and may use hostnames or IP addresses. + pub async fn onetry_peer( + &self, + addr: IpAddr, + port: u16, + v2transport: bool, + ) -> Result { + let val = self + .send_request(UserRequest::Onetry((addr, port, v2transport))) + .await?; + extract_variant!(Onetry, val); } /// Gets a block by its hash. diff --git a/florestad/src/json_rpc/res.rs b/florestad/src/json_rpc/res.rs index 358caf37..40f50dea 100644 --- a/florestad/src/json_rpc/res.rs +++ b/florestad/src/json_rpc/res.rs @@ -204,6 +204,7 @@ pub enum Error { InvalidMemInfoMode, Wallet(String), Filters(String), + InvalidAddnodeCommand, } impl Display for Error { @@ -235,6 +236,7 @@ impl Display for Error { Error::InvalidMemInfoMode => write!(f, "Invalid meminfo mode, should be stats or mallocinfo"), Error::Wallet(e) => write!(f, "Wallet error: {e}"), Error::Filters(e) => write!(f, "Error with filters: {e}"), + Error::InvalidAddnodeCommand => write!(f, "Invalid addnode command"), } } } diff --git a/florestad/src/json_rpc/server.rs b/florestad/src/json_rpc/server.rs index 03604856..758385e7 100644 --- a/florestad/src/json_rpc/server.rs +++ b/florestad/src/json_rpc/server.rs @@ -76,7 +76,7 @@ pub struct RpcImpl { type Result = std::result::Result; impl RpcImpl { - async fn add_node(&self, node: String) -> Result { + async fn add_node(&self, node: String, command: String, v2transport: bool) -> Result { let node = node.split(':').collect::>(); let (ip, port) = if node.len() == 2 { (node[0], node[1].parse().map_err(|_| Error::InvalidPort)?) @@ -91,10 +91,15 @@ impl RpcImpl { }; let peer = ip.parse().map_err(|_| Error::InvalidAddress)?; - self.node - .connect(peer, port) - .await - .map_err(|_| Error::Node("Failed to connect".to_string())) + + let _ = match command.as_str() { + "add" => self.node.add_peer(peer, port, v2transport).await, + "remove" => self.node.remove_peer(peer, port).await, + "onetry" => self.node.onetry_peer(peer, port, v2transport).await, + _ => return Err(Error::InvalidAddnodeCommand), + }; + + Ok(json!(null)) } fn get_transaction(&self, tx_id: Txid, verbosity: Option) -> Result { @@ -357,8 +362,11 @@ async fn handle_json_rpc_request( "addnode" => { let node = params[0].as_str().ok_or(Error::InvalidAddress)?; + let command = params[1].as_str().ok_or(Error::InvalidAddnodeCommand)?; + let v2transport = params[2].as_bool().unwrap_or(false); + state - .add_node(node.to_string()) + .add_node(node.to_string(), command.to_string(), v2transport) .await .map(|v| ::serde_json::to_value(v).unwrap()) } @@ -416,6 +424,7 @@ fn get_http_error_code(err: &Error) -> u16 { | Error::MissingReq | Error::NoBlockFilters | Error::InvalidMemInfoMode + | Error::InvalidAddnodeCommand | Error::Wallet(_) => 400, // idunnolol @@ -452,6 +461,7 @@ fn get_json_rpc_error_code(err: &Error) -> i32 { | Error::TxNotFound | Error::BlockNotFound | Error::InvalidMemInfoMode + | Error::InvalidAddnodeCommand | Error::Wallet(_) => -32600, // server error From 7eb9bd68c5e5a041b7c77034d92edafe36ef7b3f Mon Sep 17 00:00:00 2001 From: qlrddev Date: Tue, 13 May 2025 09:50:38 -0300 Subject: [PATCH 2/3] Fix test_framework * removed --daemon from test_framework/__init__.py to better logging the tests; * added a assertFalse assertion method to test_framework/__init__.py; * added a static method for test_framework/rpc/base.py to human readable logging in rpc calls; * fixed addnode method in test_framework/rpc/floresta.py to accept bitcoin-core's compliant JSONRPC call; * added bitcoin-core's compliant addnode and getpeerinfo into test_framework/rpc/utreexo.py. --- tests/test_framework/__init__.py | 21 +++++++++++-- tests/test_framework/daemon/base.py | 16 ++++++++-- tests/test_framework/rpc/base.py | 46 +++++++++++++++++++++++++--- tests/test_framework/rpc/floresta.py | 8 +++-- tests/test_framework/rpc/utreexo.py | 26 ++++++++++++++++ 5 files changed, 106 insertions(+), 11 deletions(-) diff --git a/tests/test_framework/__init__.py b/tests/test_framework/__init__.py index 2dad7d93..cd7678be 100644 --- a/tests/test_framework/__init__.py +++ b/tests/test_framework/__init__.py @@ -12,7 +12,7 @@ import os import re -from datetime import datetime +from datetime import datetime, timezone from typing import Any, Dict, List, Pattern from test_framework.crypto.pkcs8 import ( @@ -180,9 +180,16 @@ def __init__(self): """ self._nodes = [] + # pylint: disable=R0801 def log(self, msg: str): """Log a message with the class caller""" - print(f"[{self.__class__.__name__} {datetime.utcnow()}] {msg}") + + now = ( + datetime.now(timezone.utc) + .replace(microsecond=0) + .strftime("%Y-%m-%d %H:%M:%S") + ) + print(f"[{self.__class__.__name__} {now}] {msg}") def main(self): """ @@ -394,6 +401,16 @@ def assertTrue(self, condition: bool): self.stop() raise AssertionError(f"Actual: {condition}\nExpected: True") + def assertFalse(self, condition: bool): + """ + Assert if the condition is False, otherwise + all nodes will be stopped and an AssertionError will + be raised. + """ + if condition: + self.stop() + raise AssertionError(f"Actual: {condition}\nExpected: False") + # pylint: disable=invalid-name def assertIsNone(self, thing: Any): """ diff --git a/tests/test_framework/daemon/base.py b/tests/test_framework/daemon/base.py index d084bf63..ff360c69 100644 --- a/tests/test_framework/daemon/base.py +++ b/tests/test_framework/daemon/base.py @@ -111,9 +111,16 @@ def __init__(self): self._process = None self._settings = [] + # pylint: disable=R0801 def log(self, message: str): """Log a message to the console""" - print(f"[{str(self._name).upper()} {datetime.now(timezone.utc)}] {message}") + now = ( + datetime.now(timezone.utc) + .replace(microsecond=0) + .strftime("%Y-%m-%d %H:%M:%S") + ) + + print(f"[{self.__class__.__name__.upper()} {now}] {message}") @property def target(self) -> str: @@ -189,7 +196,12 @@ def start(self): ) elif self.name == "florestad": - cmd.extend(["--network=regtest"]) + cmd.extend( + [ + "--network=regtest", + "--debug", + ] + ) elif self.name == "bitcoind": cmd.extend( diff --git a/tests/test_framework/rpc/base.py b/tests/test_framework/rpc/base.py index 07b1760c..674359f7 100644 --- a/tests/test_framework/rpc/base.py +++ b/tests/test_framework/rpc/base.py @@ -10,7 +10,8 @@ import time from datetime import datetime, timezone from subprocess import Popen -from typing import Any, Dict, List +from typing import Any, Dict, List, Optional +from urllib.parse import quote from requests import post from requests.exceptions import HTTPError @@ -167,12 +168,44 @@ def rpcserver(self, value: RPCServerConfig): """Setter for `rpcserver` property""" self._rpcserver = value + # pylint: disable=R0801 def log(self, message: str): """Log a message to the console""" - print( - f"[{self.__class__.__name__.upper()} {datetime.now(timezone.utc)}] {message}" + now = ( + datetime.now(timezone.utc) + .replace(microsecond=0) + .strftime("%Y-%m-%d %H:%M:%S") ) + print(f"[{self.__class__.__name__.upper()} {now}] {message}") + + @staticmethod + def build_log_message( + url: str, + method: str, + params: List[Any], + user: Optional[str] = None, + password: Optional[str] = None, + ) -> str: + """ + Construct a log string for an RPC call like: + POST @http://host:port/method?args[0]=val0&args[1]=val1... + """ + logmsg = "POST " + + if user or password: + logmsg += f"<{user or ''}:{password or ''}>@" + + logmsg += f"{url}{method}" + + if params: + query_string = "&".join( + f"args[{i}]={quote(str(val))}" for i, val in enumerate(params) + ) + logmsg += f"?{query_string}" + + return logmsg + # pylint: disable=unused-argument,dangerous-default-value def perform_request( self, @@ -223,7 +256,11 @@ def perform_request( kwargs["auth"] = HTTPBasicAuth(user, password) # Now make the POST request to the RPC server - logmsg += f"{kwargs['url']}{method}?params={params}" + logmsg = BaseRPC.build_log_message( + kwargs["url"], method, params, user, password + ) + + self.log(logmsg) response = post(**kwargs) # If response isnt 200, raise an HTTPError @@ -243,7 +280,6 @@ def perform_request( message=result["error"]["message"], ) - self.log(logmsg) self.log(result["result"]) return result["result"] diff --git a/tests/test_framework/rpc/floresta.py b/tests/test_framework/rpc/floresta.py index f3c4fefd..af7e9ae1 100644 --- a/tests/test_framework/rpc/floresta.py +++ b/tests/test_framework/rpc/floresta.py @@ -87,7 +87,7 @@ def get_peerinfo(self): """ return self.perform_request("getpeerinfo") - def addnode(self, node: str): + def addnode(self, node: str, command: str, v2transport: bool = False): """ Adds a new node to our list of peers performing `perform_request('addnode', params=[str])` @@ -112,7 +112,11 @@ def addnode(self, node: str): if not pattern.match(node): raise ValueError("Invalid ip[:port] format") - return self.perform_request("addnode", params=[node]) + + if command not in ("add", "remove", "onetry"): + raise ValueError(f"Invalid command '{command}'") + + return self.perform_request("addnode", params=[node, command, v2transport]) def get_roots(self): """ diff --git a/tests/test_framework/rpc/utreexo.py b/tests/test_framework/rpc/utreexo.py index 8eb2a6e9..f7b2ad12 100644 --- a/tests/test_framework/rpc/utreexo.py +++ b/tests/test_framework/rpc/utreexo.py @@ -61,3 +61,29 @@ def get_balance(self): Perform the `getbalance` RPC command to utreexod """ return self.perform_request("getbalance", []) + + def get_peerinfo(self): + """ + Perform the `getpeerinfo` RPC command to utreexod + """ + return self.perform_request("getpeerinfo", []) + + def addnode( + self, node: str, command: str, v2transport: bool = False, rpcquirk: bool = False + ): + """ + Adds a new node to our list of peers performing + `perform_request('addnode', params=[str])` + + This will make our node try to connect to this peer. + + Args + node: A network address with the format ip[:port] + + Returns + success: Whether we successfully added this node to our list of peers + """ + if rpcquirk: + return self.perform_request("addnode", params=[node, command, v2transport]) + + return self.perform_request("addnode", params=[node, command]) From 47864c8ab3f637ab2a8eece57d3b6efca777c227 Mon Sep 17 00:00:00 2001 From: qlrddev Date: Tue, 29 Apr 2025 10:21:22 -0300 Subject: [PATCH 3/3] Fix functional/integration tests for improved addnode JSON-RPC command * split functional/integration tests for addnode into two tests in context of v1 and v2 transports; * addnode tests between floresta and utreexod nodes in a regtest network with only genesis block. --- tests/floresta-cli/addnode-test.py | 426 ++++++++++++++++++++++++----- 1 file changed, 358 insertions(+), 68 deletions(-) diff --git a/tests/floresta-cli/addnode-test.py b/tests/floresta-cli/addnode-test.py index 4b01efbb..6d4efe0f 100644 --- a/tests/floresta-cli/addnode-test.py +++ b/tests/floresta-cli/addnode-test.py @@ -1,102 +1,392 @@ """ -floresta_cli_addnode.py +addnode-test.py -This functional test cli utility to interact with a Floresta node with `addnode` +This functional test cli utility to interact with a Floresta +node with `addnode` that should be both compliant with the Bitcoin-core +in context of the v1/v2 transport protocol. + +(see more at https://bitcoincore.org/en/doc/29.0.0/rpc/network/addnode/) """ import os -import tempfile +import re +import time from test_framework import FlorestaTestFramework -from test_framework.rpc.floresta import REGTEST_RPC_SERVER +from test_framework.rpc.floresta import REGTEST_RPC_SERVER as floresta_config +from test_framework.rpc.utreexo import REGTEST_RPC_SERVER as utreexod_config +DATA_DIR = FlorestaTestFramework.get_integration_test_dir() +TIMEOUT = 25 -class GetAddnodeIDBErrorTest(FlorestaTestFramework): - """ - Test `addnode` rpc call, by creating two nodes (in its IDB state), where - the first one should connect with the second one by calling `addnode ip[:port]`. - Maybe its worth to add custom electrum servers and custom data-dirs for each florestad process +def create_data_dirs( + base_name: str, nodes: int, v2transport: bool = False +) -> list[str]: + """ + Create the data directories for the two nodes + to be used in the test. """ + transport = "v2" if v2transport else "v1" + dir_name = f"{base_name}-{transport}-transport" - nodes = [-1, -1] - data_dirs = [ - os.path.normpath( - os.path.join( - FlorestaTestFramework.get_integration_test_dir(), - "data", - "florestacli-addnode-test", - "node-0", - ) - ), - os.path.normpath( - os.path.join( - FlorestaTestFramework.get_integration_test_dir(), - "data", - "floresta-cli-addnode-test", - "node-1", + paths = [] + for i in range(nodes): + p = os.path.join(str(DATA_DIR), "data", dir_name, f"node-{i}") + os.makedirs(p, exist_ok=True) + paths.append(p) + + return paths + + +def run_test(name: str, v2transport: bool = False): + + class _AddnodeTest(FlorestaTestFramework): + + def set_test_params(self): + self.log(f"**************** Running {name} test") + self.nodes = [-1, -1] + self.data_dirs = create_data_dirs( + self.__class__.__name__, 2, v2transport=v2transport ) - ), - ] + self.v2transport = v2transport + AddnodeTestWrapper.set_test_params(self) + + def run_test(self): + nodes = AddnodeTestWrapper.start_both_nodes(self) + AddnodeTestWrapper.run_test(self, nodes) + for node in nodes: + node.rpc.wait_for_connections(opened=False) - # The port 50002 do not have any TLS meaning here, - # it's just another port for another node - electrum_addrs = ["0.0.0.0:50001", "0.0.0.0:50002"] - rpc_addrs = ["0.0.0.0:18442", "0.0.0.0:18443"] - node_ibd_error = "Node is in initial block download, wait until it's finished" + self.log(f"**************** Test {name} done!") - def set_test_params(self): + _AddnodeTest().main() + + +class AddnodeTestWrapper: + + @staticmethod + def set_test_params(test: FlorestaTestFramework): """ - Setup the two node florestad process with different data-dirs, electrum-addresses - and rpc-addresses in the same regtest network + Setup the two nodes (florestad and utreexod) + in the same regtest network. """ - GetAddnodeIDBErrorTest.nodes[0] = self.add_node( - extra_args=[ - f"--data-dir={GetAddnodeIDBErrorTest.data_dirs[0]}", - f"--electrum-address={GetAddnodeIDBErrorTest.electrum_addrs[0]}", - f"--rpc-address={GetAddnodeIDBErrorTest.rpc_addrs[0]}", - ], - rpcserver=REGTEST_RPC_SERVER, + test.nodes[0] = test.add_node( + variant="florestad", + extra_args=[f"--data-dir={test.data_dirs[0]}"], + rpcserver=floresta_config, ssl=False, ) - GetAddnodeIDBErrorTest.nodes[1] = self.add_node( + # --rpcquirks is used to make the utreexod node + # to be compliant with the bitcoin-core + test.nodes[1] = test.add_node( + variant="utreexod", extra_args=[ - f"--data-dir={GetAddnodeIDBErrorTest.data_dirs[1]}", - f"--electrum-address={GetAddnodeIDBErrorTest.electrum_addrs[1]}", - f"--rpc-address={GetAddnodeIDBErrorTest.rpc_addrs[1]}", + "--rpcquirks", + f"--datadir={test.data_dirs[1]}", ], - rpcserver={ - "host": "127.0.0.1", - "ports": {"rpc": 18443, "server": 50002}, - "jsonrpc": "2.0", - "timeout": 10000, - }, + rpcserver=utreexod_config, ssl=False, ) - def run_test(self): + @staticmethod + def start_both_nodes(test): + """ + Start both nodes, by calling `run_node` for each node + and returning the nodes with `get_node` method. + """ + for i in test.nodes: + test.log(f"=========== Starting node {i}...") + test.run_node(test.nodes[i]) + + return [test.get_node(i) for i in test.nodes] + + @staticmethod + def test_should_floresta_add_utreexod(test, nodes, v2transport=False): + """ + The test follows: + + - Call `addnode add false`; + - the result should be a null json compliant to bitcoin-core; + - call `getpeerinfo` on floresta. That should return a list + with the utreexod peer in Ready state. + """ + test.log("=========== Testing should_floresta_add_utreexod...") + # Floresta adds the utreexod node + result = nodes[0].rpc.addnode( + node="127.0.0.1:18444", command="add", v2transport=v2transport + ) + + # `addnode` bitcoin-core compliant command + # should return a null json object + test.assertIsNone(result) + + # For now the node will be in awaiting state + # and will not be available in `get_peerinfo` + # (an empty list). The `addnode` just add the + # node to the added_peers list but the connection + # and handshake is not established yet (it's managed + # in `UtreexoNode::maybe_open_connections` method) that + # will be called in subsequent iterations on + # `Peer::run_inner_loop` method + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 0) + + # add some time to guarantee + # the handshake establishment + time.sleep(TIMEOUT) + + # now we expect the node to be in Ready state + # with some expressive information. The node + # should be in the `getpeerinfo` list. + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 1) + test.assertEqual(peer_info[0]["address"], "127.0.0.1:18444") + test.assertEqual(peer_info[0]["initial_height"], 0) + test.assertEqual(peer_info[0]["kind"], "regular") + test.assertEqual( + peer_info[0]["services"], "ServiceFlags(BLOOM|WITNESS|0x1000000)" + ) + test.assertEqual(peer_info[0]["state"], "Ready") + test.assertEqual(peer_info[0]["transport_protocol"], "V1") + test.assertMatch( + peer_info[0]["user_agent"], + re.compile(r"\/btcwire:\d.\d.\d\/utreexod:\d.\d.\d\/"), + ) + + @staticmethod + def test_should_utreexod_see_floresta(test, nodes): + """ + The test follows: + + - Call `getpeerinfo` on utreexod. That should return a list + with the floresta peer. + """ + # now see how utreexod see floresta + test.log("=========== Testing should utreexod_see_floresta...") + peer_info = nodes[1].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 1) + test.assertEqual(peer_info[0]["addrlocal"], "127.0.0.1:18444") + test.assertEqual(peer_info[0]["startingheight"], 0) + test.assertEqual(peer_info[0]["services"], "16777225") + test.assertMatch( + peer_info[0]["subver"], + re.compile(r"\/Floresta:.*\/"), + ) + test.assertEqual(peer_info[0]["inbound"], True) + + @staticmethod + def test_should_utreexod_disconnect(test, nodes): + """ + The test follows: + + - call `stop` on utreexod; + - call `getpeerinfo` on floresta. That should return a list + with the utreexod peer in Awaiting state; + """ + test.log( + "=========== Testing should utreexod disconnect and floresta not see anymore..." + ) + # lets try to disconnect the node + # and wait for disconnection to proceed + # with the test + test.stop_node(1) + time.sleep(TIMEOUT) + + # now we expect the node to be in the + # awaiting state. It will be in that state + # until the node reconnects again + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 0) + + @staticmethod + def test_should_utreexod_reconnect(test, nodes): + """ + The test follows: + - call `run_node` on utreexod; + - call `getpeerinfo` on floresta. That should return a list + with the utreexod peer in Ready state; + """ + test.log( + "=========== Testing should utreexod reconnect and floresta await for it be ready..." + ) + # reconnect the utreexod node + test.run_node(1) + nodes[1].rpc.wait_for_connections(opened=True) + time.sleep(TIMEOUT) + + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 1) + test.assertEqual(peer_info[0]["state"], "Ready") + + @staticmethod + def test_should_floresta_not_add_utreexod_again(test, nodes, v2transport=False): + """ + The test follows: + + - Call `addnode false`; + - the result should be a null json compliant to bitcoin-core; + - call `getpeerinfo` on floresta. That should the same + list as before, meaning that the utreexod peer was not added again. + """ + test.log("=========== Testing should floresta not add utreexod again...") + result = nodes[0].rpc.addnode( + node="127.0.0.1:18444", command="add", v2transport=v2transport + ) + + # `addnode` bitcoin-core compliant command + # should return a null json object + test.assertIsNone(result) + + # Check if the list of peers is the same from + # the previous test, meaning that the + # `addnode` command was not able to add the node + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 1) + + @staticmethod + def test_should_floresta_remove_utreexod(test, nodes): + """ + The test follows: + + - Call `addnode remove false`; + - the result should be a null json compliant to bitcoin-core; + - call `getpeerinfo` on floresta. That should return a list + with zero peers. + """ + test.log("=========== Testing should floresta remove utreexod...") + result = nodes[0].rpc.addnode( + node="127.0.0.1:18444", + command="remove", + ) + + # `addnode` bitcoin-core compliant command + # should return a null json object + test.assertIsNone(result) + + # For now the node will be in ready state + # and will be available in `get_peerinfo` + # The `addnode remove` just remove the + # node from the added_peers list but it will + # still be in the peers list. + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 1) + test.assertEqual(peer_info[0]["state"], "Ready") + + # to check if removed, let's stop the utreexod + # restart it and check the `getpeerinfo` again + test.stop_node(1) + test.run_node(1) + + # wait some time to guarantee + # that it will not be in the peers list again + time.sleep(TIMEOUT) + + # now we expect the node to be in the + # awaiting state. It will be in that state + # until the node reconnects again + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 0) + + @staticmethod + def test_should_utreexod_not_see_floresta(test, nodes): + """ + The test follows: + - Call `getpeerinfo` on utreexod. That should return a list + with zero peers. + """ + test.log("=========== Testing should utreexod not see floresta...") + peer_info = nodes[1].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 0) + + @staticmethod + def test_should_floresta_onetry_connection_with_utreexod( + test, nodes, v2transport=False + ): """ - Run JSONRPC server on first, wait to connect, then call `addnode ip[:port]` + The test follows: + + - Call `addnode onetry false` in the floresta node; + - the result should be a null json compliant to bitcoin-core; + - call `getpeerinfo` on floresta. That should return a list + with the utreexod peer in Ready state; """ - # Start node - self.run_node(GetAddnodeIDBErrorTest.nodes[0]) - node_0 = self.get_node(GetAddnodeIDBErrorTest.nodes[0]) + test.log( + "=========== Testing should floresta onetry connection with utreexod..." + ) + result = nodes[0].rpc.addnode( + node="127.0.0.1:18444", command="onetry", v2transport=v2transport + ) + + # `addnode` bitcoin-core compliant command + # should return a null json object + test.assertIsNone(result) - # start a second node - self.run_node(GetAddnodeIDBErrorTest.nodes[1]) - node_1 = self.get_node(GetAddnodeIDBErrorTest.nodes[1]) + # add some time to establish the handshake + time.sleep(5) - # Test assertions - result_0 = node_0.rpc.addnode(node="0.0.0.0:18443") - self.assertTrue(result_0) + # Check if the added node was added + # to the peers list with the `getpeerinfo` command + # but should be in the "Awaiting" state + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 1) + test.assertEqual(peer_info[0]["address"], "127.0.0.1:18444") + test.assertEqual(peer_info[0]["initial_height"], 0) + test.assertEqual(peer_info[0]["kind"], "regular") + test.assertEqual( + peer_info[0]["services"], "ServiceFlags(BLOOM|WITNESS|0x1000000)" + ) + test.assertEqual(peer_info[0]["state"], "Ready") + test.assertEqual(peer_info[0]["transport_protocol"], "V1") + test.assertMatch( + peer_info[0]["user_agent"], + re.compile(r"\/btcwire:\d.\d.\d\/utreexod:\d.\d.\d\/"), + ) + + # now we need to force a disconnection by shutdown utreexod + # and see if, when utreexod restart, it will not be reconnected + test.stop_node(1) + test.run_node(1) + + # wait some time to guarantee + # that it will not be in the peers list again + time.sleep(TIMEOUT) + + peer_info = nodes[0].rpc.get_peerinfo() + test.assertEqual(len(peer_info), 0) - result_1 = node_1.rpc.addnode(node="0.0.0.0:18442") - self.assertTrue(result_1) + @staticmethod + def run_test(test, nodes): + """ + First initialize both nodes. Then run above tests + in the following order: - # stop both nodes - self.stop() + - should floresta add utreexod; + - should utreexod see floresta; + - should floresta not add utreexod again; + - should floresta remove utreexod; + - should utreexod not see floresta; + - should floresta onetry connection with utreexod; + - should floresta remove onetry connection with utreexod; + """ + AddnodeTestWrapper.test_should_floresta_add_utreexod( + test, nodes, test.v2transport + ) + AddnodeTestWrapper.test_should_utreexod_see_floresta(test, nodes) + AddnodeTestWrapper.test_should_utreexod_disconnect(test, nodes) + AddnodeTestWrapper.test_should_utreexod_reconnect(test, nodes) + AddnodeTestWrapper.test_should_floresta_not_add_utreexod_again( + test, nodes, test.v2transport + ) + AddnodeTestWrapper.test_should_floresta_remove_utreexod(test, nodes) + AddnodeTestWrapper.test_should_utreexod_not_see_floresta(test, nodes) + AddnodeTestWrapper.test_should_floresta_onetry_connection_with_utreexod( + test, nodes, test.v2transport + ) + test.stop() if __name__ == "__main__": - GetAddnodeIDBErrorTest().main() + run_test("addnode_v1_transport", v2transport=False) + run_test("addnode_v2_transport", v2transport=True)