Unified structured data improvement

[ustulation]

[dirvine]

This is a very nice addition and simplifies an already simple structure (great) as well as ensuring collision proof stores. This is very good as a Put data will gi via another persona (ClientManager) and then onto store. Cross persona messages should not be allowed really so this is nice that collision errors are gone, means there is no known error for a Put of this data type. :+1 from me (happily)

[inetic]

I think in this case vault can reject the illegal update simply using the fact that A-PublicKey is not listed in owner_keys in the stored version(?).

[ustulation]

That will not be full-proof or exhaustive though. Client-A could have sent:

StructuredData {
    other_fields: Value-22,
    owner_keys: B-PublicKey,
    signatures: Signed by A-PrivateKey,
}

Then even though the PublicKeys in the owner fields would match, Signature still has to be checked and then rejected as illegal update. So signature check is the ultimate indicator of a legal/illegal update.

Further if B were to transfer ownership back to A, B would send:

StructuredData {
    other_fields: Value-22,
    owner_keys: A-PublicKey,
    signatures: Signed by B-PrivateKey,
}

So by your logic, this will be rejected merely on the fact of mismatching keys, whereas it is a perfectly valid update.

To avoid all edge cases, Signature check should be the only goal, not matching of keys IMO.

[inetic]

Then even though the PublicKeys in the owner fields would match, Signature still has to be checked and then rejected as illegal update.

Yeah, I forgot to mention that apart from StructuredData, the real message also contains the PublicKey of the client together with signature of this message, routing will check this signature during parsing of the message so by the time the StructuredData reaches Vault/Client, you can be sure that it came from the given client.

So signature check is the ultimate indicator of a legal/illegal update.

So, yes, but it happens implicitly in routing.

[inetic]

I think there is one problem with this approach when there is multiple owners already of the StructuredData chunk. Say, the chunk currently stored in Vault looks like this:

StructuredData {
    other_fields: Value-1,
    owner_keys: [A-PubKey, B-PubKey],
    // Note, priv keys will not be transferred, this is just a notation to indicate
    // how the signature was created.
    signatures: [sign(A-PrivKey, Value-1), sign(B-PrivKey, Value-1)],
}

Now A-Client want's to change the data, so he sends:

StructuredData {
    other_fields: Value-2,
    owner_keys: [A-PubKey, B-PubKey],
    signatures: [sign(A-PrivKey, Value-2)],
}

Then Vault will replace A's signature with the old one:

StructuredData {
    other_fields: Value-1, // Still on value #1 because majority consensus did not take place yet
    owner_keys: [A-PubKey, B-PubKey],
    signatures: [sign(A-PrivKey, Value-2), sign(B-PrivKey, Value-1)],
}

Now notice that Vault has no proof that A-Client really owns the data with Value-1, because the signature sign(A-PrivKey, Value-2) signs the next data. I think this may be relevant when getting the chunk back, or when churn event happens (and the chunk needs to be redistributed).

[bcndanos]

I can see another problem with this scheme, where a StructuredData has two owners:
 
step 0:
Client-A sends:

StructuredData {
    other_fields: Value-00,
    owner_keys: A-PublicKey B-PublicKey,
    signatures: Signed by A-PrivateKey,
}

step 1:
Vault receives for the first time. So Just Stores it:

StructuredData {
    other_fields: Value-00,
    owner_keys: A-PublicKey B-PublicKey,
    signatures: Signed by A-PrivateKey,
}

step 2:
Client-B wants to add a new owner to this structured data:

StructuredData {
    other_fields: Value-00,
    owner_keys: A-PublicKey B-PublicKey C-PublicKey,
    signatures: Signed by B-PrivateKey,
}

But, at the same time, Client-A makes a change to the other_fields but he doesn't send the full list of owners(He doesn't know the change of Client-B):

StructuredData {
    other_fields: Value-11,
    owner_keys: A-PublicKey B-PublicKey,
    signatures: Signed by A-PrivateKey,
}

step 3:
Vault receives two requests for the same StructuredData. Which one wins the race??

StructuredData {
    other_fields: Value-¿¿00 or 11??,
    owner_keys: A-PublicKey B-PublicKey ¿¿C-PublicKey??,
    signatures: Signed by ¿¿A-PrivateKey or B-PrivateKey??,
}

I suppose that depends on the nonce sent by the clients, which means that one message will be discarded. But if the nonce sent is correct for both requests, it can be a lost of information.

---

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[inetic]

I think this kind of situation where N distinct owners can change the chunk in different ways is resolved by requiring a consensus of majority of current owners, since there can't be two different majorities.

---

Comments from the review on Reviewable.io

[inetic]

Possible alternative (I know we discussed to leave this for later discussion, but I already had the nice ASCI art in place :-P, so didn't want to waste it :) ):

struct StructuredData {
    tag_type   : u64,
    identifier : crypto::hash::sha512::Digest,
    version    : u64,                         // mutable - incrementing (deterministic) version number
    data       : Vec<u8>,                     // mutable - in many cases this is encrypted

             +--> Map ensures only one (Signature, Option<Signature>) pair per one owner.
             |
             |     +--> Owner's identifier.
             |     |   
    owners : Map<PublicKey, (Signature, Option<Signature>)>
                              |                   |
                              |                   +--> Optional signature of the next version
                              |                        of the mutable fields
                              |
                              +--> Signature of the current version of
                                   the mutable data.    
}

[ustulation]

@inetic

Possible alternative

Appears complicated: owners is a field that must be included while calculating the signature (else people can add fake owners). So for multi-sig, any addition to the Map by the next client will invalidate the signature of the previous client.

[inetic]

owners is a field that must be included while calculating the signature

Ah, yes, but it shouldn't be too much of a problem, when calculating the signature, you can just use the keys of the map. Pseudocode:

sign(PrivateKey, concatenate([serialise(version),
                              serialise(data),
                              serialise(owners.get_keys())]));

[maqi]

u64 is better to be retained as it maybe used as a app_tag.
for example MpidMessaging may use structured_data for messages among nodes, and tag = 0x51000 indicates an MpidMessage meanwhile tag = 0x51001 indicates an MpidAlert

[ustulation]

This was with Reference to the now outdated DataRequest as it now also contains NameType field. I'll update this section. But I still have some questions if you could answer. So present day DataRequests are more like DataRequest::StructuredData(id, tag) .. when we route the GETs we already are calculating the name as sha512(id + tag) , so why would you additionally require tags GETs ? In PUT/POST/DELETEs you do have the entire SD with you as a payload and can thus extract the tag information if you need to and use that for some logic. So i don't get what you would miss if u64 was not retained and left out of Get Request, something like this:
DataRequest::StructuredData(Name, SHA512(PublicKeys)) where Name = sha512(id + tag)

[maqi]

there is use case that SD only being used as a wrapping tool for requesting.
For example, the MpidMessaging, the struct of mpid_message and mpid_head is not StructuredData, but the requesting of mpid_message and mpid_head is ustilizing current get/post/delete requests for SD, which in DataRequest(name, tag), tag will be used to differentiate the message or header.

[maqi]

Is this is duplicated with the PR https://github.com/maidsafe/rfcs/pull/21/files ?

[ustulation]

@maqi

Is this is duplicated with the ...

No. That one was to help code the logic for the established RFC on SD. Referring to the Motivation Section there:

The motivation for the current RFC is to establish a collective understanding of the changes needed for routing and sentinel to implementing the parent RFC. This RFC explicitly excludes the intend to change
the design of the actual StructuredData and any such discussions should be posted on the parent RFC.

This RFC modifies SD operations and the way the Network interacts with SD's. So it's a breaking change.

[dirvine]

Where does the Name come from, perhaps needs clarified here?