Inconsistent validate arguments with private entries

guillemcordoba commented 3 years ago

I've found this scenario following a discussion with @Connoropolous about validation related to private entries.

Alice commits a private profile entry.
Alice's validate_create_entry_profile, validate_create_entry and validate get called with the entry present.
Alice passes rules and publishes, Bob receives that publish.
Bob runs only the validate function with the entry hidden.

Is this the expected behavior?

This makes it so that you have to be very careful when writing validation rules, as the parameters themselves and the functions that get called are not the same in the case of the agent publishing things, and the validators receiving those publishes. So achieving determinism is possible, but adds a lot of overhead. Also devs may make assumptions that aren't true, like that the validators nodes are validating the private entry's content, with everything that they are checking in validate_create_entry.

Maybe standardizing what calls are being made and with which arguments is a good first step?

Connoropolous commented 3 years ago

Thanks Guillem. Because my understanding is that the Headers of private entries were definitely always intended to be published, to enable third parties to validate the integrity of their source chain, without knowing its exact contents, it makes sense that there is SOME kind of validation callback on Bob's system running. Which callbacks does seem confusing, and definitely the part about app developers remembering / knowing in the first place that the context/content is different. This is something I could talk about in my post if we pin it down.

Connoropolous commented 3 years ago

By the way, the code that determines which function hooks get called is this... https://github.com/holochain/holochain/blob/2d9401a5e0f74934195a0bf02ca198679b53089d/crates/holochain/src/core/ribosome/guest_callback/validate.rs#L60-L79

You can see that validate_create should/would also get called, if you had defined it in your scenario.

Also, when it comes to how holochain decides which functions to call (all of) based on that FnComponents result, it comes from this logic: https://github.com/holochain/holochain/blob/2d9401a5e0f74934195a0bf02ca198679b53089d/crates/holochain/src/core/ribosome.rs#L183-L190

Connoropolous commented 3 years ago

My sense is that validate validate_create validate_create_entry and validate_create_entry_post even should be called, for a private header of a post type... (since that info is in the header anyways right, entry_def_id / type I mean?

That would require rewriting:

.entry().as_option() { 
         Some(Entry::Agent(_)) => fns.push("agent".into()), 
         Some(Entry::App(_)) => { 
             fns.push("entry".into()); 
             if let Some(EntryDefId::App(entry_def_id)) = self.entry_def_id.clone() { 
                 fns.push(entry_def_id); 
             } 
         } 
         _ => {} 
     }

it should be based on some match in the Header instead.

Also, I think the entry section of the FnComponents should be renamed app_entry

artbrock commented 3 years ago

We don’t send a StoreEntry DhtOp for private entries, so DHT nodes really should never be trying to validate private entries, only elements/headers. In other words, nobody without access to the entry should be running entry validation code.

The confusion this ticket points to is that the validation callback suffixes are correlated to HDK actions instead of DhtOps so confusion emerges.

Maybe we need to revisit the validation suffixing scheme so that everyone knows WHO is running WHAT validation (in terms of DHT roles not HDK actions).

ThetaSinner commented 9 months ago

Did this get looked at? Even if the outcome was to make no changes it would be good to resolve this issue

guillemcordoba commented 9 months ago

@ThetaSinner so this issue has changed in nature since I created it. Now we moved to DhtOps for the validation rules, which means that the StoreEntry DhtOp doesn't get produced for private entries, so no-one actually validates it.

The question now for me is with the StoreRecord, in particular whether it carries the entry inside its record or not. There are two ways of implementing this:

The author of the private entry receives in the validate callback the private entry, because they have access to it, while everyone else receives a RecordEntry::Hidden. Pros is that the author gets a bit more protection from footguns, because they validate their own entries. Cons is that the arguments for the validate callback would be inconsistent, as in, the author of the entry would get different parameters than the other validators, so that's a footgun for happ developers, who must implement some sort of validation rule that says "the entry is valid if it's not present, and if it's present it's valid under conditions X".
The author of the private entry receives the StoreRecord DhtOp with RecordEntry::Hidden, same as everyone else. In this case they could implement validation on the coordinator zome side, before committing the entry.

I think the way it works right now is the former, I'd prefer the latter for consistency reasons but I don't have strong opinions. Either way it would be good to have accessible documentation that explains this to developers, as this can lead to huge network malfunction if hApp devs misunderstand the behavior of validate.