Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Get ValidationPackage over network #727

Merged
merged 50 commits into from
Dec 8, 2018
Merged

Conversation

lucksus
Copy link
Collaborator

@lucksus lucksus commented Dec 5, 2018

Context

Implements this state: https://realtimeboard.com/app/board/o9J_kyiXmFs=/?moveToWidget=3074457346314392714

screenshot 2018-12-07 at 17 36 48

Implementation steps in this PR

  • Protocol for node2node messages DirectMessage with variants RequestValidationPackage and ValidationPackage
  • Action::GetValidationPackage with reducer and action creator that lets the network send a DirectMessage::RequestValidationPackage to the entries source
  • handler for the request that triggers the new workflow:
  • workflows::respond_validation_package_request that checks if we have the entry in the source chain, builds the validation package and then triggers sending of the response via:
  • Action::SendDirectMessage that makes the network send any (initial or responding) node2node message
  • handler for the response of a validation package request DirectMessage::ValidationPackage that updates the state so the GetValidationPackageFuture can resolve

Additional required changes in here

  • Had to make sure that Entry::address() returns the right address also for agent_id entries which override the hashing of the content and instead have their public key be the address (6f494af and b66068a)
  • ProtocolWrapper::SendResult and ProtocolWrapper::HandleSendResult were mixed up (right, @neonphog? Or did I get it wrong?) I've swapped them to have handle always mean that the network requests core to handle something. (76fa842 and e812ae7)

@lucksus lucksus added the review label Dec 5, 2018
@codecov
Copy link

codecov bot commented Dec 7, 2018

Codecov Report

Merging #727 into develop will increase coverage by 0.53%.
The diff coverage is 86.57%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #727      +/-   ##
===========================================
+ Coverage    72.79%   73.31%   +0.53%     
===========================================
  Files          129      138       +9     
  Lines         4541     4690     +149     
===========================================
+ Hits          3305     3438     +133     
- Misses        1236     1252      +16
Impacted Files Coverage Δ
core/src/action.rs 100% <ø> (ø) ⬆️
core/src/network/actions/initialize_network.rs 87.88% <100%> (+0.38%) ⬆️
core_types/src/entry/mod.rs 81.4% <100%> (+1.91%) ⬆️
container_api/src/holochain.rs 72.92% <100%> (ø) ⬆️
core/src/nucleus/ribosome/api/debug.rs 100% <100%> (ø) ⬆️
core/src/network/reducers/handle_get_result.rs 84.62% <100%> (-2.05%) ⬇️
core/src/nucleus/ribosome/run_dna.rs 69.57% <100%> (+0.45%) ⬆️
core/src/network/state.rs 100% <100%> (ø) ⬆️
.../network/reducers/handle_get_validation_package.rs 100% <100%> (ø)
core/src/network/reducers/init.rs 100% <100%> (ø) ⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e97db3c...3b7f21d. Read the comment docs.

Copy link
Member

@zippy zippy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks fabluous. I put in some comments, but all of them should be followups if you agree. Add them to the tree?

core/src/network/direct_message.rs Outdated Show resolved Hide resolved
dna_hash: network_state.dna_hash.clone().unwrap(),
agent_id: network_state.agent_id.clone().unwrap(),
address: entry.address().to_string(),
content: serde_json::from_str(&serde_json::to_string(&entry_with_header).unwrap())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use JsonString here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can and we should. But I didn’t want to change the net crates in this PR and instead just use it. Follow up.

agent_id: network_state.agent_id.clone().unwrap(),
address: link.base().to_string(),
attribute: String::from("link"),
content: serde_json::from_str(&serde_json::to_string(&entry_with_header).unwrap())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto JsonString

dna_hash: network_state.dna_hash.clone().unwrap(),
agent_id: get_dht_data.from_agent_id.clone(),
address: get_dht_data.address.clone(),
content: serde_json::from_str(&serde_json::to_string(&maybe_entry).unwrap()).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto JsonString

core/src/network/reducers/send_direct_message.rs Outdated Show resolved Hide resolved
/// None: process started, but no response yet from the network
/// Some(Err(_)): there was a problem at some point
/// Some(Ok(None)): no problem but also no entry -> it does not exist
/// Some(Ok(Some(entry))): we have it
type GetEntryResult = Option<Result<Option<Entry>, HolochainError>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this enum for state!

) {
let maybe_validation_package = match get_entry(&requested_entry_address, &context) {
Ok(entry) => await!(build_validation_package(&entry, &context)).ok(),
Err(_) => None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really send a None value back across the network if there was an error? I have the feeling that we at least have to tell the requester that we had some kind of error... followup?

core/src/action.rs Outdated Show resolved Hide resolved
core/src/action.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thedavidmeister thedavidmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why so much raw serde handling?

fn address(&self) -> Address {
match &self {
Entry::AgentId(agent_id) => agent_id.address(),
_ => Address::encode_from_str(&String::from(self.content()), Hash::SHA2256),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this seems like it should always defer to the .address() method of the inner value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? Wouldn't that mean that the entry type is not included in the hash? In Montreal we agreed that the "content" that gets hashed needs to include the type.

@lucksus
Copy link
Collaborator Author

lucksus commented Dec 8, 2018

@thedavidmeister raw serde handling because ProtocolWrapper‘s data structs use serde_json::Value. You pointed to that earlier and I agree that should be changed. But in this PR I didn’t want to broaden the scope to include those changes to the net crates and instead just use them.

@thedavidmeister
Copy link
Contributor

@lucksus right... i keep leaving the same feedback for the same thing 😅

carry on then

@lucksus lucksus merged commit 15b1892 into develop Dec 8, 2018
@lucksus lucksus removed the review label Dec 8, 2018
@lucksus lucksus deleted the validation-over-network branch December 14, 2018 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants