From 18fbc84c077aa93f1b19000e421b458daad163c5 Mon Sep 17 00:00:00 2001 From: Boog900 <54e72d8a-345f-4599-bd90-c6b9bc7d0ec5@aleeas.com> Date: Mon, 2 Dec 2024 20:36:14 +0000 Subject: [PATCH] review changes --- binaries/cuprated/src/p2p/request_handler.rs | 30 +++++++------------- storage/blockchain/Cargo.toml | 2 +- storage/blockchain/src/ops/block.rs | 12 ++++---- storage/blockchain/src/ops/blockchain.rs | 21 +++++++------- storage/blockchain/src/service/read.rs | 27 ++++++++---------- types/src/blockchain.rs | 16 +++++------ types/src/lib.rs | 2 +- types/src/types.rs | 2 +- 8 files changed, 47 insertions(+), 65 deletions(-) diff --git a/binaries/cuprated/src/p2p/request_handler.rs b/binaries/cuprated/src/p2p/request_handler.rs index cb0757c6..620c9df3 100644 --- a/binaries/cuprated/src/p2p/request_handler.rs +++ b/binaries/cuprated/src/p2p/request_handler.rs @@ -38,7 +38,7 @@ use cuprate_p2p_core::{ use cuprate_txpool::service::TxpoolReadHandle; use cuprate_types::{ blockchain::{BlockchainReadRequest, BlockchainResponse}, - BlockCompleteEntry, MissingTxsInBlock, TransactionBlobs, + BlockCompleteEntry, TransactionBlobs, TxsInBlock, }; use cuprate_wire::protocol::{ ChainRequest, ChainResponse, FluffyMissingTransactionsRequest, GetObjectsRequest, @@ -56,9 +56,7 @@ use crate::{ #[derive(Clone)] pub struct P2pProtocolRequestHandlerMaker { pub blockchain_read_handle: BlockchainReadHandle, - pub blockchain_context_service: BlockChainContextService, - pub txpool_read_handle: TxpoolReadHandle, /// The [`IncomingTxHandler`], wrapped in an [`Option`] as there is a cyclic reference between [`P2pProtocolRequestHandlerMaker`] @@ -115,13 +113,9 @@ where #[derive(Clone)] pub struct P2pProtocolRequestHandler { peer_information: PeerInformation, - blockchain_read_handle: BlockchainReadHandle, - blockchain_context_service: BlockChainContextService, - txpool_read_handle: TxpoolReadHandle, - incoming_tx_handler: IncomingTxHandler, } @@ -196,7 +190,7 @@ async fn get_objects( .call(BlockchainReadRequest::BlockCompleteEntries(block_hashes)) .await? else { - panic!("blockchain returned wrong response!"); + unreachable!(); }; Ok(ProtocolResponse::GetObjects(GetObjectsResponse { @@ -233,12 +227,12 @@ async fn get_chain( .call(BlockchainReadRequest::NextChainEntry(block_hashes, 10_000)) .await? else { - panic!("blockchain returned wrong response!"); + unreachable!(); }; - if start_height == 0 { + let Some(start_height) = start_height else { anyhow::bail!("The peers chain has a different genesis block than ours."); - } + }; let (cumulative_difficulty_low64, cumulative_difficulty_top64) = split_u128_into_low_high_bits(cumulative_difficulty); @@ -271,19 +265,19 @@ async fn fluffy_missing_txs( // deallocate the backing `Bytes`. drop(request); - let BlockchainResponse::MissingTxsInBlock(res) = blockchain_read_handle + let BlockchainResponse::TxsInBlock(res) = blockchain_read_handle .ready() .await? - .call(BlockchainReadRequest::MissingTxsInBlock { + .call(BlockchainReadRequest::TxsInBlock { block_hash, tx_indexes, }) .await? else { - panic!("blockchain returned wrong response!"); + unreachable!(); }; - let Some(MissingTxsInBlock { block, txs }) = res else { + let Some(TxsInBlock { block, txs }) = res else { anyhow::bail!("The peer requested txs out of range."); }; @@ -412,11 +406,7 @@ where }; // Drop all the data except the stuff we still need. - let NewTransactions { - txs, - dandelionpp_fluff: _, - padding: _, - } = request; + let NewTransactions { txs, .. } = request; let res = incoming_tx_handler .ready() diff --git a/storage/blockchain/Cargo.toml b/storage/blockchain/Cargo.toml index 2e72143a..782ed1be 100644 --- a/storage/blockchain/Cargo.toml +++ b/storage/blockchain/Cargo.toml @@ -34,7 +34,7 @@ serde = { workspace = true, optional = true } tower = { workspace = true } thread_local = { workspace = true } rayon = { workspace = true } -bytes = "1.7.2" +bytes = { workspace = true } [dev-dependencies] cuprate-constants = { workspace = true } diff --git a/storage/blockchain/src/ops/block.rs b/storage/blockchain/src/ops/block.rs index 94c1fc64..7b9c9976 100644 --- a/storage/blockchain/src/ops/block.rs +++ b/storage/blockchain/src/ops/block.rs @@ -226,6 +226,7 @@ pub fn pop_block( Ok((block_height, block_info.block_hash, block)) } + //---------------------------------------------------------------------------------------------------- `get_block_blob_with_tx_indexes` /// Retrieve a block's raw bytes, the index of the miner transaction and the number of non miner-txs in the block. /// @@ -234,11 +235,8 @@ pub fn get_block_blob_with_tx_indexes( block_height: &BlockHeight, tables: &impl Tables, ) -> Result<(Vec, u64, usize), RuntimeError> { - use monero_serai::io::write_varint; + let miner_tx_idx = tables.block_infos().get(block_height)?.mining_tx_index; - let block_info = tables.block_infos().get(block_height)?; - - let miner_tx_idx = block_info.mining_tx_index; let block_txs = tables.block_txs_hashes().get(block_height)?.0; let numb_txs = block_txs.len(); @@ -250,10 +248,10 @@ pub fn get_block_blob_with_tx_indexes( block.append(&mut miner_tx_blob); // Add the blocks tx hashes. - write_varint(&block_txs.len(), &mut block) + monero_serai::io::write_varint(&block_txs.len(), &mut block) .expect("The number of txs per block will not exceed u64::MAX"); - let block_txs_bytes = bytemuck::cast_slice(&block_txs); + let block_txs_bytes = bytemuck::must_cast_slice(&block_txs); block.extend_from_slice(block_txs_bytes); Ok((block, miner_tx_idx, numb_txs)) @@ -275,7 +273,7 @@ pub fn get_block_complete_entry( let tx_blobs = tables .tx_blobs_iter() - .get_range(first_tx_idx..=usize_to_u64(numb_non_miner_txs))? + .get_range(first_tx_idx..(usize_to_u64(numb_non_miner_txs) + first_tx_idx))? .map(|tx_blob| Ok(Bytes::from(tx_blob?.0))) .collect::>()?; diff --git a/storage/blockchain/src/ops/blockchain.rs b/storage/blockchain/src/ops/blockchain.rs index c6cd4040..f926133f 100644 --- a/storage/blockchain/src/ops/blockchain.rs +++ b/storage/blockchain/src/ops/blockchain.rs @@ -98,17 +98,16 @@ pub fn find_split_point( let mut err = None; // Do a binary search to find the first unknown/known block in the batch. - let idx = - block_ids.partition_point( - |block_id| match block_exists(block_id, table_block_heights) { - Ok(exists) => exists & chronological_order, - Err(e) => { - err.get_or_insert(e); - // if this happens the search is scrapped, just return `false` back. - false - } - }, - ); + let idx = block_ids.partition_point(|block_id| { + match block_exists(block_id, table_block_heights) { + Ok(exists) => exists == chronological_order, + Err(e) => { + err.get_or_insert(e); + // if this happens the search is scrapped, just return `false` back. + false + } + } + }); if let Some(e) = err { return Err(e); diff --git a/storage/blockchain/src/service/read.rs b/storage/blockchain/src/service/read.rs index 01fc92d0..262c8e32 100644 --- a/storage/blockchain/src/service/read.rs +++ b/storage/blockchain/src/service/read.rs @@ -27,7 +27,7 @@ use cuprate_database_service::{init_thread_pool, DatabaseReadService, ReaderThre use cuprate_helper::map::combine_low_high_bits_to_u128; use cuprate_types::{ blockchain::{BlockchainReadRequest, BlockchainResponse}, - Chain, ChainId, ExtendedBlockHeader, MissingTxsInBlock, OutputHistogramInput, OutputOnChain, + Chain, ChainId, ExtendedBlockHeader, OutputHistogramInput, OutputOnChain, TxsInBlock, }; use crate::{ @@ -118,10 +118,10 @@ fn map_request( R::CompactChainHistory => compact_chain_history(env), R::NextChainEntry(block_hashes, amount) => next_chain_entry(env, &block_hashes, amount), R::FindFirstUnknown(block_ids) => find_first_unknown(env, &block_ids), - R::MissingTxsInBlock { + R::TxsInBlock { block_hash, tx_indexes, - } => missing_txs_in_block(env, block_hash, tx_indexes), + } => txs_in_block(env, block_hash, tx_indexes), R::AltBlocksInChain(chain_id) => alt_blocks_in_chain(env, chain_id), R::Block { height } => block(env, height), R::BlockByHash(hash) => block_by_hash(env, hash), @@ -598,7 +598,7 @@ fn next_chain_entry( // This will happen if we have a different genesis block. if idx == block_ids.len() { return Ok(BlockchainResponse::NextChainEntry { - start_height: 0, + start_height: None, chain_height: 0, block_ids: vec![], block_weights: vec![], @@ -632,7 +632,7 @@ fn next_chain_entry( }; Ok(BlockchainResponse::NextChainEntry { - start_height: first_known_height, + start_height: Some(first_known_height), chain_height, block_ids, block_weights, @@ -669,12 +669,8 @@ fn find_first_unknown(env: &ConcreteEnv, block_ids: &[BlockHash]) -> ResponseRes }) } -/// [`BlockchainReadRequest::MissingTxsInBlock`] -fn missing_txs_in_block( - env: &ConcreteEnv, - block_hash: [u8; 32], - missing_txs: Vec, -) -> ResponseResult { +/// [`BlockchainReadRequest::TxsInBlock`] +fn txs_in_block(env: &ConcreteEnv, block_hash: [u8; 32], missing_txs: Vec) -> ResponseResult { // Single-threaded, no `ThreadLocal` required. let env_inner = env.env_inner(); let tx_ro = env_inner.tx_ro()?; @@ -686,7 +682,7 @@ fn missing_txs_in_block( let first_tx_index = miner_tx_index + 1; if numb_txs < missing_txs.len() { - return Ok(BlockchainResponse::MissingTxsInBlock(None)); + return Ok(BlockchainResponse::TxsInBlock(None)); } let txs = missing_txs @@ -694,9 +690,10 @@ fn missing_txs_in_block( .map(|index_offset| Ok(tables.tx_blobs().get(&(first_tx_index + index_offset))?.0)) .collect::>()?; - Ok(BlockchainResponse::MissingTxsInBlock(Some( - MissingTxsInBlock { block, txs }, - ))) + Ok(BlockchainResponse::TxsInBlock(Some(TxsInBlock { + block, + txs, + }))) } /// [`BlockchainReadRequest::AltBlocksInChain`] diff --git a/types/src/blockchain.rs b/types/src/blockchain.rs index 258d526f..58a23b2a 100644 --- a/types/src/blockchain.rs +++ b/types/src/blockchain.rs @@ -11,9 +11,7 @@ use std::{ use monero_serai::block::Block; use crate::{ - types::{ - Chain, ExtendedBlockHeader, MissingTxsInBlock, OutputOnChain, VerifiedBlockInformation, - }, + types::{Chain, ExtendedBlockHeader, OutputOnChain, TxsInBlock, VerifiedBlockInformation}, AltBlockInformation, BlockCompleteEntry, ChainId, ChainInfo, CoinbaseTxSum, OutputHistogramEntry, OutputHistogramInput, }; @@ -122,7 +120,7 @@ pub enum BlockchainReadRequest { FindFirstUnknown(Vec<[u8; 32]>), /// A request for transactions from a specific block. - MissingTxsInBlock { + TxsInBlock { /// The block to get transactions from. block_hash: [u8; 32], /// The indexes of the transactions from the block. @@ -287,10 +285,10 @@ pub enum BlockchainResponse { /// Response to [`BlockchainReadRequest::NextChainEntry`]. /// - /// If all blocks were unknown `start_height` will be `0`, the other fields will be meaningless. + /// If all blocks were unknown `start_height` will be [`None`], the other fields will be meaningless. NextChainEntry { - /// The start height of this entry, `0` if we could not find the split point. - start_height: usize, + /// The start height of this entry, [`None`] if we could not find the split point. + start_height: Option, /// The current chain height. chain_height: usize, /// The next block hashes in the entry. @@ -310,10 +308,10 @@ pub enum BlockchainResponse { /// This will be [`None`] if all blocks were known. FindFirstUnknown(Option<(usize, usize)>), - /// The response for [`BlockchainReadRequest::MissingTxsInBlock`]. + /// The response for [`BlockchainReadRequest::TxsInBlock`]. /// /// Will return [`None`] if the request contained an index out of range. - MissingTxsInBlock(Option), + TxsInBlock(Option), /// The response for [`BlockchainReadRequest::AltBlocksInChain`]. /// diff --git a/types/src/lib.rs b/types/src/lib.rs index 51d37d6c..7aaf0b9e 100644 --- a/types/src/lib.rs +++ b/types/src/lib.rs @@ -26,7 +26,7 @@ pub use transaction_verification_data::{ pub use types::{ AddAuxPow, AltBlockInformation, AuxPow, Chain, ChainId, ChainInfo, CoinbaseTxSum, ExtendedBlockHeader, FeeEstimate, HardForkInfo, MinerData, MinerDataTxBacklogEntry, - MissingTxsInBlock, OutputHistogramEntry, OutputHistogramInput, OutputOnChain, + OutputHistogramEntry, OutputHistogramInput, OutputOnChain, TxsInBlock, VerifiedBlockInformation, VerifiedTransactionInformation, }; diff --git a/types/src/types.rs b/types/src/types.rs index ebb02c56..12e16376 100644 --- a/types/src/types.rs +++ b/types/src/types.rs @@ -261,7 +261,7 @@ pub struct AddAuxPow { /// The inner response for a request for missing txs. #[derive(Clone, Debug, PartialEq, Eq)] -pub struct MissingTxsInBlock { +pub struct TxsInBlock { pub block: Vec, pub txs: Vec>, }