From 1174fbfa864b36aa68cd3699e64fe958dc2f762c Mon Sep 17 00:00:00 2001 From: Roland Sherwin Date: Mon, 23 Oct 2023 23:01:26 +0530 Subject: [PATCH] feat(protocol)!: implement `PrettyPrintRecordKey` as a `Cow` type - this allows us to hold a reference to RecordKey when we just want to log things. --- sn_client/src/api.rs | 16 +++--- sn_client/src/chunks/error.rs | 2 +- sn_client/src/file_apis.rs | 6 +-- sn_networking/src/cmd.rs | 4 +- sn_networking/src/error.rs | 7 ++- sn_networking/src/event.rs | 18 +++---- sn_networking/src/lib.rs | 34 +++++------- sn_networking/src/record_store.rs | 12 ++--- sn_networking/src/replication_fetcher.rs | 4 +- sn_networking/src/transfers.rs | 2 +- sn_node/src/log_markers.rs | 8 +-- sn_node/src/node.rs | 8 +-- sn_node/src/put_validation.rs | 57 ++++++++++---------- sn_node/src/replication.rs | 2 +- sn_node/tests/verify_data_location.rs | 10 ++-- sn_protocol/src/error.rs | 10 ++-- sn_protocol/src/lib.rs | 67 +++++++++++++++++------- sn_protocol/src/storage/header.rs | 2 +- 18 files changed, 142 insertions(+), 127 deletions(-) diff --git a/sn_client/src/api.rs b/sn_client/src/api.rs index b37cd87ce0..7f3d1f4201 100644 --- a/sn_client/src/api.rs +++ b/sn_client/src/api.rs @@ -284,7 +284,7 @@ impl Client { debug!( "Got record from the network, {:?}", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); let register = get_register_from_record(record) @@ -497,7 +497,7 @@ impl Client { trace!( "Getting spend {unique_pubkey:?} with record_key {:?}", - PrettyPrintRecordKey::from(key.clone()) + PrettyPrintRecordKey::from(&key) ); let record = self .network @@ -510,7 +510,7 @@ impl Client { })?; debug!( "For spend {unique_pubkey:?} got record from the network, {:?}", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); let header = RecordHeader::from_record(&record).map_err(|err| { @@ -564,7 +564,7 @@ impl Client { error!("Found double spend for {address:?}"); Err(Error::CouldNotVerifyTransfer(format!( "Found double spend for the unique_pubkey {unique_pubkey:?} - {:?}: spend_one {:?} and spend_two {:?}", - PrettyPrintRecordKey::from(key), one.derived_key_sig, two.derived_key_sig + PrettyPrintRecordKey::from(&key), one.derived_key_sig, two.derived_key_sig ))) } } @@ -615,15 +615,15 @@ fn merge_split_register_records( address: RegisterAddress, map: &HashMap)>, ) -> Result { - let key = - PrettyPrintRecordKey::from(NetworkAddress::from_register_address(address).to_record_key()); - debug!("Got multiple records from the network for key: {key:?}"); + let key = NetworkAddress::from_register_address(address).to_record_key(); + let pretty_key = PrettyPrintRecordKey::from(&key); + debug!("Got multiple records from the network for key: {pretty_key:?}"); let mut all_registers = vec![]; for (record, peers) in map.values() { match get_register_from_record(record.clone()) { Ok(r) => all_registers.push(r), Err(e) => { - warn!("Ignoring invalid register record found for {key:?} received from {peers:?}: {:?}", e); + warn!("Ignoring invalid register record found for {pretty_key:?} received from {peers:?}: {:?}", e); continue; } } diff --git a/sn_client/src/chunks/error.rs b/sn_client/src/chunks/error.rs index aa624ebad4..fa4bd18477 100644 --- a/sn_client/src/chunks/error.rs +++ b/sn_client/src/chunks/error.rs @@ -18,7 +18,7 @@ pub(crate) type Result = std::result::Result; #[allow(missing_docs)] pub enum Error { #[error("Failed to get find payment for record: {0:?}")] - NoPaymentForRecord(PrettyPrintRecordKey), + NoPaymentForRecord(PrettyPrintRecordKey<'static>), #[error("Failed to get chunk permit")] CouldNotGetChunkPermit, diff --git a/sn_client/src/file_apis.rs b/sn_client/src/file_apis.rs index eeb02d64de..d49e253b7f 100644 --- a/sn_client/src/file_apis.rs +++ b/sn_client/src/file_apis.rs @@ -169,9 +169,9 @@ impl Files { if payment.is_empty() { warn!("Failed to get payment proof for chunk: {chunk_addr:?} it was not found in the local wallet"); - return Err(ChunksError::NoPaymentForRecord(PrettyPrintRecordKey::from( - chunk_addr.to_record_key(), - )))?; + return Err(ChunksError::NoPaymentForRecord( + PrettyPrintRecordKey::from(&chunk_addr.to_record_key()).into_owned(), + ))?; } trace!( diff --git a/sn_networking/src/cmd.rs b/sn_networking/src/cmd.rs index 3b72322dda..00387a1f57 100644 --- a/sn_networking/src/cmd.rs +++ b/sn_networking/src/cmd.rs @@ -186,7 +186,7 @@ impl SwarmDriver { let query_id = self.swarm.behaviour_mut().kademlia.get_record(key.clone()); if !expected_holders.is_empty() { - debug!("Record {:?} with task {query_id:?} expected to be held by {expected_holders:?}", PrettyPrintRecordKey::from(key)); + debug!("Record {:?} with task {query_id:?} expected to be held by {expected_holders:?}", PrettyPrintRecordKey::from(&key)); } if self @@ -225,7 +225,7 @@ impl SwarmDriver { let _ = sender.send(record); } SwarmCmd::PutRecord { record, sender } => { - let record_key = PrettyPrintRecordKey::from(record.key.clone()); + let record_key = PrettyPrintRecordKey::from(&record.key).into_owned(); trace!( "Putting record sized: {:?} to network {:?}", record.value.len(), diff --git a/sn_networking/src/error.rs b/sn_networking/src/error.rs index 0252ed4876..449396332b 100644 --- a/sn_networking/src/error.rs +++ b/sn_networking/src/error.rs @@ -46,10 +46,10 @@ pub enum Error { /// No put_record attempts were successfully verified. #[error("Could not retrieve the record after storing it: {0:}")] - FailedToVerifyRecordWasStored(PrettyPrintRecordKey), + FailedToVerifyRecordWasStored(PrettyPrintRecordKey<'static>), #[error("Record retrieved from the network does not match the one we attempted to store {0:}")] - ReturnedRecordDoesNotMatch(PrettyPrintRecordKey), + ReturnedRecordDoesNotMatch(PrettyPrintRecordKey<'static>), #[error("Could not create storage dir: {path:?}, error: {source}")] FailedToCreateRecordStoreDir { @@ -130,8 +130,7 @@ mod tests { let address = ChunkAddress::new(xor_name); let network_address = NetworkAddress::from_chunk_address(address); let record_key = network_address.to_record_key(); - let pretty_record: PrettyPrintRecordKey = record_key.into(); - let record_str = format!("{}", pretty_record); + let record_str = format!("{}", PrettyPrintRecordKey::from(&record_key)); let xor_name_str = format!( "{:64x}({:?})", xor_name, diff --git a/sn_networking/src/event.rs b/sn_networking/src/event.rs index 5f0241f1a4..3ed6184010 100644 --- a/sn_networking/src/event.rs +++ b/sn_networking/src/event.rs @@ -172,7 +172,7 @@ impl Debug for NetworkEvent { NetworkEvent::KeysForReplication(list) => { let pretty_list: Vec<_> = list .iter() - .map(|(holder, key)| (*holder, PrettyPrintRecordKey::from(key.clone()))) + .map(|(holder, key)| (*holder, PrettyPrintRecordKey::from(key))) .collect(); write!(f, "NetworkEvent::KeysForReplication({pretty_list:?})") } @@ -183,11 +183,11 @@ impl Debug for NetworkEvent { write!(f, "NetworkEvent::NatStatusChanged({nat_status:?})") } NetworkEvent::UnverifiedRecord(record) => { - let pretty_key = PrettyPrintRecordKey::from(record.key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&record.key); write!(f, "NetworkEvent::UnverifiedRecord({pretty_key:?})") } NetworkEvent::FailedToWrite(record_key) => { - let pretty_key = PrettyPrintRecordKey::from(record_key.clone()); + let pretty_key = PrettyPrintRecordKey::from(record_key); write!(f, "NetworkEvent::FailedToWrite({pretty_key:?})") } NetworkEvent::GossipsubMsgReceived { topic, .. } => { @@ -607,7 +607,7 @@ impl SwarmDriver { let content_hash = XorName::from_content(&peer_record.record.value); trace!( "Query task {id:?} returned with record {:?}(content {content_hash:?}) from peer {:?}, {stats:?} - {step:?}", - PrettyPrintRecordKey::from(peer_record.record.key.clone()), + PrettyPrintRecordKey::from(&peer_record.record.key), peer_record.peer ); self.accumulate_get_record_ok(id, peer_record, step.count); @@ -634,7 +634,7 @@ impl SwarmDriver { .map_err(|_| Error::InternalMsgChannelDropped)?; format!( "Getting record {:?} completed with only {:?} copies received", - PrettyPrintRecordKey::from(record.key.clone()), + PrettyPrintRecordKey::from(&record.key), usize::from(step.count) - 1 ) } else { @@ -664,14 +664,14 @@ impl SwarmDriver { match err.clone() { GetRecordError::NotFound { key, closest_peers } => { info!("Query task {id:?} NotFound record {:?} among peers {closest_peers:?}, {stats:?} - {step:?}", - PrettyPrintRecordKey::from(key.clone())); + PrettyPrintRecordKey::from(&key)); } GetRecordError::QuorumFailed { key, records, quorum, } => { - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key); let peers = records .iter() .map(|peer_record| peer_record.peer) @@ -679,7 +679,7 @@ impl SwarmDriver { info!("Query task {id:?} QuorumFailed record {pretty_key:?} among peers {peers:?} with quorum {quorum:?}, {stats:?} - {step:?}"); } GetRecordError::Timeout { key } => { - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key); debug!( "Query task {id:?} timed out when looking for record {pretty_key:?}" @@ -909,7 +909,7 @@ impl SwarmDriver { if let Some((sender, mut result_map, quorum, mut expected_holders)) = self.pending_get_record.remove(&query_id) { - let pretty_key = PrettyPrintRecordKey::from(peer_record.record.key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&peer_record.record.key).into_owned(); if !expected_holders.is_empty() { if expected_holders.remove(&peer_id) { diff --git a/sn_networking/src/lib.rs b/sn_networking/src/lib.rs index 1bba715025..a2f65bb211 100644 --- a/sn_networking/src/lib.rs +++ b/sn_networking/src/lib.rs @@ -295,12 +295,11 @@ impl Network { let total_attempts = if re_attempt { VERIFICATION_ATTEMPTS } else { 1 }; let mut verification_attempts = 0; - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key); while verification_attempts < total_attempts { verification_attempts += 1; info!( - "Getting record of {:?} attempts {verification_attempts:?}/{total_attempts:?}", - PrettyPrintRecordKey::from(key.clone()), + "Getting record of {pretty_key:?} attempts {verification_attempts:?}/{total_attempts:?}", ); let (sender, receiver) = oneshot::channel(); @@ -318,10 +317,7 @@ impl Network { Ok(returned_record) => { let header = RecordHeader::from_record(&returned_record)?; let is_chunk = matches!(header.kind, RecordKind::Chunk); - info!( - "Record returned: {:?}", - PrettyPrintRecordKey::from(key.clone()) - ); + info!("Record returned: {pretty_key:?}",); // Returning OK whenever fulfill one of the followings: // 1, No targeting record @@ -339,7 +335,7 @@ impl Network { } else if verification_attempts >= total_attempts { info!("Error: Returned record does not match target"); return Err(Error::ReturnedRecordDoesNotMatch( - returned_record.key.into(), + PrettyPrintRecordKey::from(&returned_record.key).into_owned(), )); } } @@ -354,7 +350,7 @@ impl Network { return Ok(returned_record); } else { return Err(Error::ReturnedRecordDoesNotMatch( - returned_record.key.into(), + PrettyPrintRecordKey::from(&returned_record.key).into_owned(), )); } } @@ -367,20 +363,14 @@ impl Network { break; } - warn!( - "No holder of record '{:?}' found. Retrying the fetch ...", - PrettyPrintRecordKey::from(key.clone()), - ); + warn!("No holder of record '{pretty_key:?}' found. Retrying the fetch ...",); } Err(error) => { error!("{error:?}"); if verification_attempts >= total_attempts { break; } - warn!( - "Did not retrieve Record '{:?}' from network!. Retrying...", - PrettyPrintRecordKey::from(key.clone()), - ); + warn!("Did not retrieve Record '{pretty_key:?}' from network!. Retrying...",); } } @@ -431,7 +421,7 @@ impl Network { while retries < PUT_RECORD_RETRIES { info!( "Attempting to PUT record of {:?} to network", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); let res = self @@ -451,7 +441,9 @@ impl Network { retries += 1; } - Err(Error::FailedToVerifyRecordWasStored(record.key.into())) + Err(Error::FailedToVerifyRecordWasStored( + PrettyPrintRecordKey::from(&record.key).into_owned(), + )) } async fn put_record_once( @@ -461,7 +453,7 @@ impl Network { expected_holders: ExpectedHoldersList, ) -> Result<()> { let record_key = record.key.clone(); - let pretty_key = PrettyPrintRecordKey::from(record_key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&record_key); info!( "Putting record of {} - length {:?} to network", pretty_key, @@ -504,7 +496,7 @@ impl Network { pub fn put_local_record(&self, record: Record) -> Result<()> { trace!( "Writing Record locally, for {:?} - length {:?}", - PrettyPrintRecordKey::from(record.key.clone()), + PrettyPrintRecordKey::from(&record.key), record.value.len() ); self.send_swarm_cmd(SwarmCmd::PutLocalRecord { record }) diff --git a/sn_networking/src/record_store.rs b/sn_networking/src/record_store.rs index 6b71c0f387..6dde97a38b 100644 --- a/sn_networking/src/record_store.rs +++ b/sn_networking/src/record_store.rs @@ -160,8 +160,8 @@ impl NodeRecordStore { { trace!( "{:?} will be pruned to make space for new record: {:?}", - PrettyPrintRecordKey::from(furthest_record.clone()), - PrettyPrintRecordKey::from(r.clone()) + PrettyPrintRecordKey::from(&furthest_record), + PrettyPrintRecordKey::from(r) ); // we should prune and make space self.remove(&furthest_record); @@ -207,7 +207,7 @@ impl NodeRecordStore { /// Warning: PUTs a `Record` to the store without validation /// Should be used in context where the `Record` is trusted pub(crate) fn put_verified(&mut self, r: Record) -> Result<()> { - let record_key = PrettyPrintRecordKey::from(r.key.clone()); + let record_key = PrettyPrintRecordKey::from(&r.key).into_owned(); trace!("PUT a verified Record: {record_key:?}"); self.prune_storage_if_needed_for_record(&r.key)?; @@ -301,7 +301,7 @@ impl RecordStore for NodeRecordStore { // When a client calls GET, the request is forwarded to the nodes until one node returns // with the record. Thus a node can be bombarded with GET reqs for random keys. These can be safely // ignored if we don't have the record locally. - let key = PrettyPrintRecordKey::from(k.clone()); + let key = PrettyPrintRecordKey::from(k); if !self.records.contains(k) { trace!("Record not found locally: {key}"); return None; @@ -324,7 +324,7 @@ impl RecordStore for NodeRecordStore { if self.records.contains(&record.key) { trace!( "Unverified Record {:?} already exists.", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); // Blindly sent to validation to allow double spend can be detected. @@ -332,7 +332,7 @@ impl RecordStore for NodeRecordStore { } trace!( "Unverified Record {:?} try to validate and store", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); if let Some(event_sender) = self.event_sender.clone() { // push the event off thread so as to be non-blocking diff --git a/sn_networking/src/replication_fetcher.rs b/sn_networking/src/replication_fetcher.rs index 2313c40c18..b84ab1f921 100644 --- a/sn_networking/src/replication_fetcher.rs +++ b/sn_networking/src/replication_fetcher.rs @@ -100,7 +100,7 @@ impl ReplicationFetcher { let pretty_keys: Vec<_> = data_to_fetch .iter() - .map(|(holder, key)| (*holder, PrettyPrintRecordKey::from(key.clone()))) + .map(|(holder, key)| (*holder, PrettyPrintRecordKey::from(key))) .collect(); if !data_to_fetch.is_empty() { @@ -123,7 +123,7 @@ impl ReplicationFetcher { self.on_going_fetches.retain(|data, node| data != key || node != holder); debug!( "Prune record {:?} at {holder:?} from the replication_fetcher due to timeout.", - PrettyPrintRecordKey::from(key.clone()) + PrettyPrintRecordKey::from(key) ); true } else { diff --git a/sn_networking/src/transfers.rs b/sn_networking/src/transfers.rs index 76a73ba962..6657b57a14 100644 --- a/sn_networking/src/transfers.rs +++ b/sn_networking/src/transfers.rs @@ -29,7 +29,7 @@ impl Network { .map_err(|_| Error::SpendNotFound(address))?; debug!( "Got record from the network, {:?}", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); let header = RecordHeader::from_record(&record).map_err(|_| Error::SpendNotFound(address))?; diff --git a/sn_node/src/log_markers.rs b/sn_node/src/log_markers.rs index a20557a1b8..40826f1bee 100644 --- a/sn_node/src/log_markers.rs +++ b/sn_node/src/log_markers.rs @@ -51,14 +51,14 @@ pub enum Marker<'a> { }, /// Valid non-existing Chunk record PUT from the network received and stored - ValidChunkRecordPutFromNetwork(&'a PrettyPrintRecordKey), + ValidChunkRecordPutFromNetwork(&'a PrettyPrintRecordKey<'a>), /// Valid non-existing Register record PUT from the network received and stored - ValidRegisterRecordPutFromNetwork(&'a PrettyPrintRecordKey), + ValidRegisterRecordPutFromNetwork(&'a PrettyPrintRecordKey<'a>), /// Valid non-existing Spend record PUT from the network received and stored - ValidSpendRecordPutFromNetwork(&'a PrettyPrintRecordKey), + ValidSpendRecordPutFromNetwork(&'a PrettyPrintRecordKey<'a>), /// Record rejected - RecordRejected(&'a PrettyPrintRecordKey), + RecordRejected(&'a PrettyPrintRecordKey<'a>), } impl<'a> Marker<'a> { diff --git a/sn_node/src/node.rs b/sn_node/src/node.rs index aee6009b34..1de6bdfe13 100644 --- a/sn_node/src/node.rs +++ b/sn_node/src/node.rs @@ -340,7 +340,7 @@ impl Node { } } NetworkEvent::UnverifiedRecord(record) => { - let key = PrettyPrintRecordKey::from(record.key.clone()); + let key = PrettyPrintRecordKey::from(&record.key).into_owned(); match self.validate_and_store_record(record).await { Ok(cmdok) => trace!("UnverifiedRecord {key} stored with {cmdok:?}."), Err(err) => { @@ -420,9 +420,9 @@ impl Node { if record_exists { QueryResponse::GetStoreCost { - store_cost: Err(ProtocolError::RecordExists(PrettyPrintRecordKey::from( - address.to_record_key(), - ))), + store_cost: Err(ProtocolError::RecordExists( + PrettyPrintRecordKey::from(&address.to_record_key()).into_owned(), + )), payment_address, } } else { diff --git a/sn_node/src/put_validation.rs b/sn_node/src/put_validation.rs index 3e094607b6..c6c3238e9b 100644 --- a/sn_node/src/put_validation.rs +++ b/sn_node/src/put_validation.rs @@ -62,14 +62,14 @@ impl Node { RecordKind::Chunk => { error!("Chunk should not be validated at this point"); Err(ProtocolError::InvalidPutWithoutPayment( - PrettyPrintRecordKey::from(record.key), + PrettyPrintRecordKey::from(&record.key).into_owned(), )) } RecordKind::Spend => self.validate_spend_record(record).await, RecordKind::Register => { error!("Register should not be validated at this point"); Err(ProtocolError::InvalidPutWithoutPayment( - PrettyPrintRecordKey::from(record.key), + PrettyPrintRecordKey::from(&record.key).into_owned(), )) } RecordKind::RegisterWithPayment => { @@ -125,9 +125,11 @@ impl Node { ) -> Result { let record_header = RecordHeader::from_record(&record)?; match record_header.kind { - RecordKind::ChunkWithPayment | RecordKind::RegisterWithPayment => Err( - ProtocolError::UnexpectedRecordWithPayment(PrettyPrintRecordKey::from(record.key)), - ), + RecordKind::ChunkWithPayment | RecordKind::RegisterWithPayment => { + Err(ProtocolError::UnexpectedRecordWithPayment( + PrettyPrintRecordKey::from(&record.key).into_owned(), + )) + } RecordKind::Chunk => { let chunk = try_deserialize_record::(&record)?; @@ -168,12 +170,12 @@ impl Node { expected_record_key: &RecordKey, ) -> Result { let data_key = address.to_record_key(); - let pretty_key = PrettyPrintRecordKey::from(data_key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&data_key); if expected_record_key != &data_key { warn!( "record key: {:?}, key: {:?}", - PrettyPrintRecordKey::from(expected_record_key.clone()), + PrettyPrintRecordKey::from(expected_record_key), pretty_key ); warn!("Record's key does not match with the value's address, ignoring PUT."); @@ -187,7 +189,7 @@ impl Node { .map_err(|err| { let msg = format!("Error while checking if Chunk's key is present locally {err}"); warn!("{msg}"); - ProtocolError::RecordNotStored(pretty_key, msg) + ProtocolError::RecordNotStored(pretty_key.into_owned(), msg) })?; if present_locally { @@ -208,7 +210,7 @@ impl Node { let chunk_addr = *chunk.address(); let key = NetworkAddress::from_chunk_address(*chunk.address()).to_record_key(); - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key).into_owned(); let record = Record { key, @@ -252,7 +254,7 @@ impl Node { ProtocolError::RegisterNotStored(Box::new(*reg_addr)) })?; - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key); // check register and merge if needed let updated_register = match self.register_validation(®ister, present_locally).await? { @@ -264,7 +266,7 @@ impl Node { // store in kad let record = Record { - key, + key: key.clone(), value: try_serialize_record(&updated_register, RecordKind::Register)?, publisher: None, expires: None, @@ -310,11 +312,10 @@ impl Node { let cash_note_addr = SpendAddress::from_unique_pubkey(&unique_pubkey); let key = NetworkAddress::from_cash_note_address(cash_note_addr).to_record_key(); - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key); debug!( - "validating and storing spends {:?} - {:?}", + "validating and storing spends {:?} - {pretty_key:?}", cash_note_addr.xorname(), - pretty_key ); let present_locally = self @@ -337,10 +338,7 @@ impl Node { Some(spends) => spends, None => { // we trust the replicated data - debug!( - "Trust replicated spend for {:?}", - PrettyPrintRecordKey::from(key.clone()) - ); + debug!("Trust replicated spend for {pretty_key:?}",); // TODO: may need to tweak the `signed_spend_validation` function, // instead of trusting replicated spend directly signed_spends @@ -348,14 +346,13 @@ impl Node { }; debug!( - "Got {} validated spends for {:?}", + "Got {} validated spends for {pretty_key:?}", validated_spends.len(), - PrettyPrintRecordKey::from(key.clone()) ); // store the record into the local storage let record = Record { - key, + key: key.clone(), value: try_serialize_record(&validated_spends, RecordKind::Spend)?, publisher: None, expires: None, @@ -391,7 +388,7 @@ impl Node { &self, payment: &Vec, wallet: &LocalWallet, - pretty_key: PrettyPrintRecordKey, + pretty_key: PrettyPrintRecordKey<'static>, ) -> Result<(Vec, Transfer), ProtocolError> { for transfer in payment { match self @@ -417,7 +414,8 @@ impl Node { address: &NetworkAddress, payment: Vec, ) -> Result<(), ProtocolError> { - let pretty_key = PrettyPrintRecordKey::from(address.to_record_key()); + let key = address.to_record_key(); + let pretty_key = PrettyPrintRecordKey::from(&key); trace!("Validating record payment for {pretty_key}"); // load wallet @@ -427,7 +425,7 @@ impl Node { // unpack transfer trace!("Unpacking incoming Transfers for record {pretty_key}"); let (cash_notes, transfer_for_us) = self - .cash_notes_from_payment(&payment, &wallet, pretty_key.clone()) + .cash_notes_from_payment(&payment, &wallet, pretty_key.clone().into_owned()) .await?; info!("{:?} cash notes are for us", cash_notes.len()); @@ -436,7 +434,7 @@ impl Node { for cash_note in cash_notes.iter() { let amount = cash_note.value().map_err(|_| { ProtocolError::RecordNotStored( - pretty_key.clone(), + pretty_key.clone().into_owned(), "Failed to get CashNote value".to_string(), ) })?; @@ -444,7 +442,7 @@ impl Node { received_fee .checked_add(amount) .ok_or(ProtocolError::RecordNotStored( - pretty_key.clone(), + pretty_key.clone().into_owned(), "CashNote value overflow".to_string(), ))?; @@ -479,10 +477,9 @@ impl Node { .set(wallet.balance().as_nano() as i64); // check payment is sufficient - let current_store_cost = - self.network.get_local_storecost().await.map_err(|e| { - ProtocolError::RecordNotStored(pretty_key.clone(), format!("{e:?}")) - })?; + let current_store_cost = self.network.get_local_storecost().await.map_err(|e| { + ProtocolError::RecordNotStored(pretty_key.clone().into_owned(), format!("{e:?}")) + })?; // finally, (after we accept any payments to us as they are ours now anyway) // lets check they actually paid enough if received_fee < current_store_cost { diff --git a/sn_node/src/replication.rs b/sn_node/src/replication.rs index 11a6989de7..7b3340d817 100644 --- a/sn_node/src/replication.rs +++ b/sn_node/src/replication.rs @@ -148,7 +148,7 @@ impl Node { let node = self.clone(); let requester = NetworkAddress::from_peer(self.network.peer_id); let _handle: JoinHandle> = tokio::spawn(async move { - let pretty_key = PrettyPrintRecordKey::from(key.clone()); + let pretty_key = PrettyPrintRecordKey::from(&key).into_owned(); trace!("Fetching record {pretty_key:?} from node {holder:?}"); let req = Request::Query(Query::GetReplicatedRecord { requester, diff --git a/sn_node/tests/verify_data_location.rs b/sn_node/tests/verify_data_location.rs index a4e349c651..242922b642 100644 --- a/sn_node/tests/verify_data_location.rs +++ b/sn_node/tests/verify_data_location.rs @@ -184,7 +184,7 @@ async fn verify_location(record_holders: &RecordHolders, all_peers: &[PeerId]) - while verification_attempts < VERIFICATION_ATTEMPTS { failed.clear(); for (key, actual_holders_idx) in record_holders.iter() { - println!("Verifying {:?}", PrettyPrintRecordKey::from(key.clone())); + println!("Verifying {:?}", PrettyPrintRecordKey::from(key)); let record_key = KBucketKey::from(key.to_vec()); let expected_holders = sort_peers_by_key(all_peers, &record_key, CLOSE_GROUP_SIZE)? .into_iter() @@ -215,11 +215,11 @@ async fn verify_location(record_holders: &RecordHolders, all_peers: &[PeerId]) - error!( "Record {:?} is not stored by {missing_peers:?}", - PrettyPrintRecordKey::from(key.clone()), + PrettyPrintRecordKey::from(key), ); println!( "Record {:?} is not stored by {missing_peers:?}", - PrettyPrintRecordKey::from(key.clone()), + PrettyPrintRecordKey::from(key), ); } @@ -230,7 +230,7 @@ async fn verify_location(record_holders: &RecordHolders, all_peers: &[PeerId]) - .for_each(|expected| failed_peers.push(*expected)); if !failed_peers.is_empty() { - failed.insert(PrettyPrintRecordKey::from(key.clone()), failed_peers); + failed.insert(PrettyPrintRecordKey::from(key), failed_peers); } } @@ -325,7 +325,7 @@ async fn store_chunks(client: Client, chunk_count: usize, wallet_dir: PathBuf) - random_bytes.len() ); - let key = PrettyPrintRecordKey::from(RecordKey::new(&file_addr)); + let key = PrettyPrintRecordKey::from(&RecordKey::new(&file_addr)).into_owned(); file_api .pay_and_upload_bytes_test(file_addr, chunks) .await?; diff --git a/sn_protocol/src/error.rs b/sn_protocol/src/error.rs index 870939d589..39d5214476 100644 --- a/sn_protocol/src/error.rs +++ b/sn_protocol/src/error.rs @@ -23,11 +23,11 @@ pub type Result = std::result::Result; pub enum Error { // ---------- record layer + payment errors #[error("Record was not stored as no payment supplied: {0:?}")] - InvalidPutWithoutPayment(PrettyPrintRecordKey), + InvalidPutWithoutPayment(PrettyPrintRecordKey<'static>), /// At this point in replication flows, payment is unimportant and should not be supplied #[error("Record should not be a `WithPayment` type: {0:?}")] - UnexpectedRecordWithPayment(PrettyPrintRecordKey), + UnexpectedRecordWithPayment(PrettyPrintRecordKey<'static>), // ---------- register errors #[error("Register was not stored: {0}")] @@ -69,7 +69,7 @@ pub enum Error { #[error( "Payment proof received with record:{0:?}. No payment for our node in its transaction" )] - NoPaymentToOurNode(PrettyPrintRecordKey), + NoPaymentToOurNode(PrettyPrintRecordKey<'static>), /// Payments received could not be stored on node's local wallet #[error("Payments received could not be stored on node's local wallet: {0}")] FailedToStorePaymentIntoNodeWallet(String), @@ -94,7 +94,7 @@ pub enum Error { // ---------- record errors #[error("Record was not stored: {0:?}: {1:?}")] - RecordNotStored(PrettyPrintRecordKey, String), + RecordNotStored(PrettyPrintRecordKey<'static>, String), // Could not Serialize/Deserialize RecordHeader from Record #[error("Could not Serialize/Deserialize RecordHeader to/from Record")] RecordHeaderParsingFailed, @@ -109,5 +109,5 @@ pub enum Error { RecordKindMismatch(RecordKind), // The record already exists at this node #[error("The record already exists, so do not charge for it: {0:?}")] - RecordExists(PrettyPrintRecordKey), + RecordExists(PrettyPrintRecordKey<'static>), } diff --git a/sn_protocol/src/lib.rs b/sn_protocol/src/lib.rs index d799006823..88666e6ef1 100644 --- a/sn_protocol/src/lib.rs +++ b/sn_protocol/src/lib.rs @@ -24,7 +24,10 @@ use libp2p::{ }; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use sha2::{Digest, Sha256}; -use std::fmt::{self, Debug, Display, Formatter}; +use std::{ + borrow::Cow, + fmt::{self, Debug, Display, Formatter}, +}; use xor_name::XorName; /// This is the address in the network by which proximity/distance @@ -187,7 +190,7 @@ impl Debug for NetworkAddress { ), NetworkAddress::RecordKey(bytes) => format!( "NetworkAddress::RecordKey({:?} - ", - PrettyPrintRecordKey::from(RecordKey::new(bytes)) + PrettyPrintRecordKey::from(&RecordKey::new(bytes)) ), }; write!( @@ -240,27 +243,30 @@ impl std::fmt::Debug for PrettyPrintKBucketKey { } } -/// Pretty print a `kad::RecordKey` as a hex string. -/// So clients can use the hex string for xorname and record keys interchangeably. -/// This makes errors actionable for clients. -/// The only cost is converting kad::RecordKey into it before sending it in errors: `record_key.into()` +/// Provides a hex representation of a `kad::RecordKey`. +/// +/// This internally stores the RecordKey as a `Cow` type. Use `PrettyPrintRecordKey::from(&RecordKey)` to create a +/// borrowed version for printing/logging. +/// To use in error messages, to pass to other functions, call `PrettyPrintRecordKey::from(&RecordKey).into_owned()` to +/// obtain a cloned, non-referenced `RecordKey`. #[derive(Clone, Hash, Eq, PartialEq)] -pub struct PrettyPrintRecordKey(RecordKey); +pub struct PrettyPrintRecordKey<'a> { + key: Cow<'a, RecordKey>, +} -// Implementing Serialize for PrettyPrintRecordKey -impl Serialize for PrettyPrintRecordKey { +impl<'a> Serialize for PrettyPrintRecordKey<'a> { fn serialize(&self, serializer: S) -> Result where S: Serializer, { // Use the `to_vec` function of the inner RecordKey to get the bytes // and then serialize those bytes - self.0.to_vec().serialize(serializer) + self.key.to_vec().serialize(serializer) } } // Implementing Deserialize for PrettyPrintRecordKey -impl<'de> Deserialize<'de> for PrettyPrintRecordKey { +impl<'de> Deserialize<'de> for PrettyPrintRecordKey<'static> { fn deserialize(deserializer: D) -> Result where D: Deserializer<'de>, @@ -268,30 +274,51 @@ impl<'de> Deserialize<'de> for PrettyPrintRecordKey { // Deserialize to bytes first let bytes = Vec::::deserialize(deserializer)?; // Then use the bytes to create a RecordKey and wrap it in PrettyPrintRecordKey - Ok(PrettyPrintRecordKey(RecordKey::new(&bytes))) + Ok(PrettyPrintRecordKey { + key: Cow::Owned(RecordKey::new(&bytes)), + }) + } +} +/// This is the only interface to create a PrettyPrintRecordKey. +/// `.into_owned()` must be called explicitly if you want a Owned version to be used for errors/args. +impl<'a> From<&'a RecordKey> for PrettyPrintRecordKey<'a> { + fn from(key: &'a RecordKey) -> Self { + PrettyPrintRecordKey { + key: Cow::Borrowed(key), + } } } -// seamless conversion from `kad::RecordKey` to `PrettyPrintRecordKey` -impl From for PrettyPrintRecordKey { - fn from(key: RecordKey) -> Self { - PrettyPrintRecordKey(key) + +impl<'a> PrettyPrintRecordKey<'a> { + /// Creates a owned version that can be then used to pass as error values. + /// Do not call this if you just want to print/log `PrettyPrintRecordKey` + pub fn into_owned(self) -> PrettyPrintRecordKey<'static> { + let cloned_key = match self.key { + Cow::Borrowed(key) => Cow::Owned(key.clone()), + Cow::Owned(key) => Cow::Owned(key), + }; + + PrettyPrintRecordKey { key: cloned_key } } } -impl std::fmt::Display for PrettyPrintRecordKey { +impl<'a> std::fmt::Display for PrettyPrintRecordKey<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let b: Vec = self.0.as_ref().to_vec(); + let b: Vec = self.key.as_ref().to_vec(); let record_key_b = Bytes::from(b); + // to get the inner RecordKey + let key = self.key.clone().into_owned(); + write!( f, "{:64x}({:?})", record_key_b, - PrettyPrintKBucketKey(NetworkAddress::from_record_key(self.0.clone()).as_kbucket_key()) + PrettyPrintKBucketKey(NetworkAddress::from_record_key(key).as_kbucket_key()) ) } } -impl std::fmt::Debug for PrettyPrintRecordKey { +impl<'a> std::fmt::Debug for PrettyPrintRecordKey<'a> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", self) } diff --git a/sn_protocol/src/storage/header.rs b/sn_protocol/src/storage/header.rs index ee322bfa08..689c8ce516 100644 --- a/sn_protocol/src/storage/header.rs +++ b/sn_protocol/src/storage/header.rs @@ -106,7 +106,7 @@ pub fn try_deserialize_record(record: &Record) - rmp_serde::from_slice(bytes).map_err(|err| { error!( "Failed to deserialized record {} with error: {err:?}", - PrettyPrintRecordKey::from(record.key.clone()) + PrettyPrintRecordKey::from(&record.key) ); Error::RecordParsingFailed })