Sequence data observations and errors

Core Libraries
1

A few observations about sequences (docs for future reference). Not sure where this belongs (probably github but not sure which repo, probably sn_data_types or maybe sn_node)

Test setup process

Basically, create a sequence then append another value to it.

# create a new seq with value 'test'
$ safe seq store test
$ safe cat safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy
# output is
Public Sequence (version 0) at "safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy":
test

# get the xorname for later checking chunk files of nodes
$ safe dog safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy
# xorname is 0xd1f2ce6cc29e3364bbe8a8964d598a3e47fcaec3351ea01181ae152a9ce1f06f

# append 'test' again to the same seq
# in hindsight this is not a great approach but it's accurate for
# what I did so I'll keep it here in this way. But a different
# value for the second entry is probably better.
$ safe seq append test safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy
# output is now at version 1
$ safe cat safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy
Public Sequence (version 1) at "safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy":
test

1. Versions not being fetched

# fetching throws an error when ?v=1 is specified
$ safe cat safe://hyryyyy4nss6z1gs6cyf9xgi88pteczaktsgbtti3fc7ztaxffx3syecrconmy?v=1
[2021-01-01T03:54:20Z ERROR safe] sn_cli error: [Error] VersionNotFound - Version '1' is invalid for the Sequence found at "safe://hyryyyy6t6m8g3ow6gp1mz4fe13giunt6e96k7o3id4obdypqnwij3axophnmy?v=1"

2. Storecost size does not match chunkstore size

There doesn’t seem to be a correlation between the storecost size (amount of data being paid for) and the chunkstore size (amount of data being stored).

$ grep "Computing\|consumed" ~/.safe/node/baby-fleming-nodes/sn-node-2/sn_node.log 
[sn_node] INFO 2021-01-01T14:51:55.564172109+11:00 [src/node/elder_duties/key_section/transfers/mod.rs:325] Computing StoreCost for 196 bytes
[sn_node] INFO 2021-01-01T14:51:55.846738331+11:00 [src/chunk_store/mod.rs:113] consumed space: 188
[sn_node] INFO 2021-01-01T14:52:02.762193218+11:00 [src/node/elder_duties/key_section/transfers/mod.rs:325] Computing StoreCost for 346 bytes
[sn_node] INFO 2021-01-01T14:52:03.013687487+11:00 [src/chunk_store/mod.rs:113] consumed space: 466
[sn_node] INFO 2021-01-01T14:52:13.268470098+11:00 [src/node/elder_duties/key_section/transfers/mod.rs:325] Computing StoreCost for 718 bytes
[sn_node] INFO 2021-01-01T14:52:13.504305907+11:00 [src/chunk_store/mod.rs:113] consumed space: 710
[sn_node] INFO 2021-01-01T14:53:25.006017471+11:00 [src/node/elder_duties/key_section/transfers/mod.rs:325] Computing StoreCost for 331 bytes
[sn_node] INFO 2021-01-01T14:53:25.238370396+11:00 [src/chunk_store/mod.rs:113] consumed space: 819

3. Seralization

Looking at the file in the node storing the sequence, there’s some data that seems like it doesn’t belong (ascii chars, partial hex keys, etc).

$ cat ~/.safe/node/baby-fleming-nodes/sn-node-2/chunks/sequence/00000000d1f2ce6cc29e3364bbe8a8964d598a3e47fcaec3351ea01181ae152a9ce1f06fb004000000000000

# sometimes the output is short like this
152a9ce1f06fb004000000000000
 g�&��1Q�G?�]���;{5U�F������V�PublicKey::Ed25519(67fd26..)���

# but other times when running the same test steps from scratch again it looks like this
25a18f3a782b004000000000000
 �q�^~�+w}��LL��0��|���Xo�MD�7�5PublicKey::Ed25519(0f9c71..)�ek,[���C\���c�
�
�7D	�RZ󧂰PublicKey::Ed25519(0f9c71..)�PublicKey::Ed25519(0f9c71..)PublicKey::Ed25519(0f9c71..)test�PublicKey::Ed25519(0f9c71..)�PublicKey::Ed25519(0f9c71..)PublicKey::Ed25519(0f9c71..)test
PublicKey::Ed25519(0f9c71..)PublicKey::Ed25519(0f9c71..)PublicKey::Ed25519(0f9c71..)PublicKey::Ed25519(0f9c71..) �q�^~�+w}��LL��0��|���Xo�MD�7�5 �q�^~�+w}��LL��0��|���Xo�MD�7�5
PublicKey::Ed25519(0f9c71..)PublicKey::Ed25519(0f9c71..)

I would not expect to see PublicKey::Ed25519(0f9c71..) in this file.

Do these things look like issues or are they mistakes in my understanding?

3 Likes
2

This looks like a few bugs all together. We need to dig into this, right now we are looking at msg formats and serialisation so hopefully this will clear up soon. I am on the verge of recommending just json for now, but we will see.

2 Likes
3

I’ve been looking into why this string appears in the file.

Why do I feel it’s suspicious to have it there? This data is the output of a call to bincode::serialize, and when it’s deserialized again there’s no way that string is going to be parsed into a valid publickey value.

Somewhere it seems that .to_string() is being called on the public key, maybe not by maidsafe code, but somewhere. If this value were a ‘real’ public key and not a ‘string’ public key, it would be serialized as bytes, not as a string, since bincode would go for the most direct representation.

I tried serialize the sequence chunk into json (between these two lines), but there’s no way to convert some of the map keys to strings so it fails (in json all keys must be strings, but in a sequence chunk some keys are objects).

msgpack can use non-strings as keys, and yes msgpack does work for serializing a sequence chunk. So we can begin to investigate this data by converting the raw msgpack bytes to json.

This is the closest I could get (so far) to a human-readable format for the sequence chunk that is stored in the chunkstore:

{
  "operations_pk": {
    "Ed25519": "TDIfnZXtbwIhQqzTfqTokNMSAIMKF16gB+hRUAGNpRw="
  },
  "data": {
    "Public": {
      "actor": "PublicKey::Ed25519(4c321f..)",
      "address": {
        "Public": {
          "name": [
            201,36,89,170,131,92,169,118,94,9,43,181,155,
            94,159,203,153,202,173,127,52,138,251,161,88,
            247,249,220,131,54,116,223
          ],
          "tag": 1200
        }
      },
      "data": {
        "{\"path\":[[1,\"PublicKey::Ed25519(4c321f..)\"]]}": {
          "seq": [
            {
              "id": {
                "path": [
                  [
                    1023,
                    "PublicKey::Ed25519(4c321f..)"
                  ]
                ]
              },
              "dot": {
                "actor": "PublicKey::Ed25519(4c321f..)",
                "counter": 1
              },
              "val": [
                109,121,32,115,101,113,32,118,97,108
              ]
            }
          ],
          "gen": {
            "initial_base_bits": 10,
            "boundary": 1,
            "site_id": "PublicKey::Ed25519(4c321f..)"
          },
          "clock": {
            "dots": {
              "PublicKey::Ed25519(4c321f..)": 1
            }
          }
        }
      },
      "policy": {
        "seq": [
          {
            "id": {
              "path": [
                [
                  1,
                  "PublicKey::Ed25519(4c321f..)"
                ]
              ]
            },
            "dot": {
              "actor": "PublicKey::Ed25519(4c321f..)",
              "counter": 1
            },
            "val": [
              {
                "owner": {
                  "Ed25519": "TDIfnZXtbwIhQqzTfqTokNMSAIMKF16gB+hRUAGNpRw="
                },
                "permissions": {
                  "{\"Key\":{\"Ed25519\":\"TDIfnZXtbwIhQqzTfqTokNMSAIMKF16gB+hRUAGNpRw=\"}}": {
                    "append": true,
                    "admin": true
                  }
                }
              },
              null
            ]
          }
        ],
        "gen": {
          "initial_base_bits": 10,
          "boundary": 1,
          "site_id": "PublicKey::Ed25519(4c321f..)"
        },
        "clock": {
          "dots": {
            "PublicKey::Ed25519(4c321f..)": 1
          }
        }
      }
    }
  }
}

This gives some clues where the public-key-as-string value may be. It could be in

  • data.Public.actor
  • data.Public.data.(in a object-as-key).path.0.1
  • data.Public.data.(in the value for object-as-key).seq.0.id.path.0.1
  • data.Public.data.(in the value for object-as-key).seq.0.dot.actor
  • data.Public.data.(in the value for object-as-key).gen.site_id
  • data.Public.data.(in the value for object-as-key).clock.dots.(as a key)
  • data.Public.policy.seq.0.id.path.0.1
  • data.Public.policy.seq.0.dot.actor
  • data.Public.policy.gen.site_id
  • data.Public.policy.clock.dots.(as a key)

In the bincode serialized version the string 4c321f appears 10 times.

The base64 Ed25519 value TDIfnZX... when decoded to bytes and hex encoded is the same as the other hex public key values being displayed 4c321f... But the bincode version does not contain the string TDIfnZX anywhere.

Notice the “data” value has an object for a key

 "{\"path\":[[1,\"PublicKey::Ed25519(4c321f..)\"]]}"

and also permissions has an object as a key

"{\"Key\":{\"Ed25519\":\"TDIfnZXtbwIhQqzTfqTokNMSAIMKF16gB+hRUAGNpRw=\"}}"

Moving to json would be a great exercise. It would give human readable values which is a huge benefit. It will create clarity around the use of enums. It will create clarity around the use of non-strings as map keys.

JSON has no support for enums so that will need some attention to make work. (msgpack has no formal support for enums but the rust implementation can do enums as ints or strings which is handy, as can be seen in the above where there are strings representing enums, such as PublicKey).

From json it’s very easy to move to a more efficient serialization such as messagepack later on.

I feel that the move to json will require a simplification of the data structures, since the way they are now is not convertible to valid json. Moving away from bincode will be a big but imo very important and enlightening job.

For reference here’s the bincode bytes for this sequence chunk (from the file baby-fleming-nodes/sn-node-X/chunks/sequence/<the file>)

[0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 76, 50, 31, 157, 149, 237, 111, 2, 33, 66, 172, 211, 126, 164, 232, 144, 211, 18, 0, 131, 10, 23, 94, 160, 7, 232, 81, 80, 1, 141, 165, 28, 0, 0, 0, 0, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 0, 0, 0, 0, 201, 36, 89, 170, 131, 92, 169, 118, 94, 9, 43, 181, 155, 94, 159, 203, 153, 202, 173, 127, 52, 138, 251, 161, 88, 247, 249, 220, 131, 54, 116, 223, 176, 4, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 255, 3, 0, 0, 0, 0, 0, 0, 1, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0, 10, 0, 0, 0, 0, 0, 0, 0, 109, 121, 32, 115, 101, 113, 32, 118, 97, 108, 10, 1, 0, 0, 0, 0, 0, 0, 0, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 76, 50, 31, 157, 149, 237, 111, 2, 33, 66, 172, 211, 126, 164, 232, 144, 211, 18, 0, 131, 10, 23, 94, 160, 7, 232, 81, 80, 1, 141, 165, 28, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 76, 50, 31, 157, 149, 237, 111, 2, 33, 66, 172, 211, 126, 164, 232, 144, 211, 18, 0, 131, 10, 23, 94, 160, 7, 232, 81, 80, 1, 141, 165, 28, 1, 1, 1, 1, 0, 10, 1, 0, 0, 0, 0, 0, 0, 0, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0, 28, 0, 0, 0, 0, 0, 0, 0, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 0, 0, 0, 0, 0, 0, 0]

and the msgpack bytes for the exact same data (from the additional node logging I added)

[130, 173, 111, 112, 101, 114, 97, 116, 105, 111, 110, 115, 95, 112, 107, 129, 167, 69, 100, 50, 53, 53, 49, 57, 196, 32, 76, 50, 31, 157, 149, 237, 111, 2, 33, 66, 172, 211, 126, 164, 232, 144, 211, 18, 0, 131, 10, 23, 94, 160, 7, 232, 81, 80, 1, 141, 165, 28, 164, 100, 97, 116, 97, 129, 166, 80, 117, 98, 108, 105, 99, 132, 165, 97, 99, 116, 111, 114, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 167, 97, 100, 100, 114, 101, 115, 115, 129, 166, 80, 117, 98, 108, 105, 99, 130, 164, 110, 97, 109, 101, 220, 0, 32, 204, 201, 36, 89, 204, 170, 204, 131, 92, 204, 169, 118, 94, 9, 43, 204, 181, 204, 155, 94, 204, 159, 204, 203, 204, 153, 204, 202, 204, 173, 127, 52, 204, 138, 204, 251, 204, 161, 88, 204, 247, 204, 249, 204, 220, 204, 131, 54, 116, 204, 223, 163, 116, 97, 103, 205, 4, 176, 164, 100, 97, 116, 97, 129, 129, 164, 112, 97, 116, 104, 145, 146, 1, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 131, 163, 115, 101, 113, 145, 131, 162, 105, 100, 129, 164, 112, 97, 116, 104, 145, 146, 205, 3, 255, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 163, 100, 111, 116, 130, 165, 97, 99, 116, 111, 114, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 167, 99, 111, 117, 110, 116, 101, 114, 1, 163, 118, 97, 108, 154, 109, 121, 32, 115, 101, 113, 32, 118, 97, 108, 163, 103, 101, 110, 131, 177, 105, 110, 105, 116, 105, 97, 108, 95, 98, 97, 115, 101, 95, 98, 105, 116, 115, 10, 168, 98, 111, 117, 110, 100, 97, 114, 121, 1, 167, 115, 105, 116, 101, 95, 105, 100, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 165, 99, 108, 111, 99, 107, 129, 164, 100, 111, 116, 115, 129, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1, 166, 112, 111, 108, 105, 99, 121, 131, 163, 115, 101, 113, 145, 131, 162, 105, 100, 129, 164, 112, 97, 116, 104, 145, 146, 1, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 163, 100, 111, 116, 130, 165, 97, 99, 116, 111, 114, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 167, 99, 111, 117, 110, 116, 101, 114, 1, 163, 118, 97, 108, 146, 130, 165, 111, 119, 110, 101, 114, 129, 167, 69, 100, 50, 53, 53, 49, 57, 196, 32, 76, 50, 31, 157, 149, 237, 111, 2, 33, 66, 172, 211, 126, 164, 232, 144, 211, 18, 0, 131, 10, 23, 94, 160, 7, 232, 81, 80, 1, 141, 165, 28, 171, 112, 101, 114, 109, 105, 115, 115, 105, 111, 110, 115, 129, 129, 163, 75, 101, 121, 129, 167, 69, 100, 50, 53, 53, 49, 57, 196, 32, 76, 50, 31, 157, 149, 237, 111, 2, 33, 66, 172, 211, 126, 164, 232, 144, 211, 18, 0, 131, 10, 23, 94, 160, 7, 232, 81, 80, 1, 141, 165, 28, 130, 166, 97, 112, 112, 101, 110, 100, 195, 165, 97, 100, 109, 105, 110, 195, 192, 163, 103, 101, 110, 131, 177, 105, 110, 105, 116, 105, 97, 108, 95, 98, 97, 115, 101, 95, 98, 105, 116, 115, 10, 168, 98, 111, 117, 110, 100, 97, 114, 121, 1, 167, 115, 105, 116, 101, 95, 105, 100, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 165, 99, 108, 111, 99, 107, 129, 164, 100, 111, 116, 115, 129, 188, 80, 117, 98, 108, 105, 99, 75, 101, 121, 58, 58, 69, 100, 50, 53, 53, 49, 57, 40, 52, 99, 51, 50, 49, 102, 46, 46, 41, 1]
1 Like
4

The issue with public keys being stored with their to_string format stems from here:

The changes I made to resolve it and not have truncated public key strings in the chunkstore are

sn_data_types/src/sequenc/mod.rs:L29

-type ActorType = String;
+type ActorType = PublicKey;

sn_client/src/client/sequence_apis.rs

-let mut data = Sequence::new_private(pk, pk.to_string(), name, tag);
+let mut data = Sequence::new_private(pk, pk, name, tag);
...
-let mut data = Sequence::new_public(pk, pk.to_string(), name, tag);
+let mut data = Sequence::new_public(pk, pk, name, tag);

sn_client/src/client/transfer_actor/write_apis.rs

-let data = Sequence::new_public(pk, pk.to_string(), XorName::random(), 33323);
+let data = Sequence::new_public(pk, pk, XorName::random(), 33323);

Some observations / questions

Do we always expect an Actor to be a PublicKey? I understand the spec for crdt may expect an Actor to be a String type (or any arbitrary unique value of any type) but what is most relevant for our specific use case? I feel PublicKey is probably appropriate here. I admit I’m not familiar with the workings of crdt so if this is not appropriate then that’s all good.

edit: looking further into this, ActorType was previously a PublicKey and got changed to a String (see this commit which unfortunately doesn’t have an explanation for the change). In the rust-crdt readme they say " for adding new data you’ll need .derive_add_ctx(<actor_id>) where <actor_id> is a unique identifier of whatever is acting on the CRDT." So it seems PublicKey is ok to use here…? @bochaco is this something you’re familiar with?

The input arguments for Sequence::new_public and Sequence::new_private now have duplicate values for pk (I would probably say pk and pk.to_string() was already sus in the first place). Is it appropriate to simplify that function, or are the valid reasons to have the input operations_pk: PublicKey separate from actor: ActoryType?

If the ActorType should still be String, we could at least change it to be unique as the entire public key in hex, rather than the truncated form which is probably not appropriate or secure.

3 Likes
5

There’s a bug here which is simple to fix (once discovered!).

Simplest way to reproduce the bug:

$ safe seq store "first value"
$ safe seq append "second value" <the xorurl>
$ safe cat <the xorurl>?v=1
# this will go into ~10s 100% cpu usage followed by this error:
sn_cli error: [Error] VersionNotFound - Version '1' is invalid for the Sequence found at "<the xorurl>?v=1"

The bug is in sn_data_types/src/sequence/seq_crdt.rs:in_range. The fix is shown by the diff lines.

/// Gets a list of items which are within the given indices.
pub fn in_range(&self, start: Index, end: Index) -> Option<Entries> {
    let start_index = to_absolute_index(start, self.len() as usize)?;
+   let end_index = to_absolute_index(end, self.len() as usize)?; 
-   let num_items = to_absolute_index(end, self.len() as usize)?; // end_index

    self.current_lseq().map(|lseq| {
        lseq.iter()
-           .take(num_items)
            .enumerate()
-           .take_while(|(i, _)| i >= &start_index)
+           .filter(|(i, _)| i >= &start_index && i < &end_index)
            .map(|(_, entry)| entry.clone())
            .collect::<Entries>()
    })  
}

From a style perspective, it seems odd that a specific version is being fetched using in_range which would always have length 1. Looking at where this happens in sn_api/src/api/app/safe_client.rs:L470 the function takes the index argument as a u64, then ‘calculates’ the start and end range as index and index+1. Why not use the get function from seq_crdt.rs and use index directly? Regardless of this point about style, the in_range function still has the bug so needs fixing.

Maybe also could benefit from an additional comment for the in_range docs about the interval the range spans, ie [start,end) so we know what entries are / are not included.

Also worth looking into (a bit of a code-smell and poor ux) is this loop (snippet below). This was causing 100% cpu for ~10s before the above fix was implemented because it always returned the same error (empty sequence due to the bug above) and we have to wait to timeout. Not sure the best way to improve this but the loop seems slightly dubious to me, could be improved, maybe return the error immediately rather than using loop-as-wait.

// loop in case original data hasn't been propogated to all elders as yet
while res.is_err() {
    res = self
        .safe_client
        .sequence_get_entry(xorname, type_tag, version, is_private)
        .await; 
}

Last bit to look into is the storecost size vs chunkstore size discrepancy. But I won’t get to it today so hopefully I’ll get some time again in the near future. Glad to have explored 2/3 things though.

edit: just wanted to add a big thanks to the whole maidsafe team for all the work on the refactor and getting to a working network! It’s great to be able to do this sort of exploration again!!

7 Likes
6

@mav Again thanks for this, just so you know, your recent posts are this Thursdays HO topic. So we will be going through these points and bugfixes. It’s great we now can pay attention to this area. The logic is close enough for us to tidy and we need to tidy a lot. This last week or so has been Error handling and getting consistent there. It’s an exercise we will never finish but needed to start. It shows where we should “fix” and Error as opposed to passing them all up to main().

In any case we are on the case now and should be able to provide a lot more detail soon. So :bowing_man: for this, it’s all terrific.

4 Likes
7

Let me try to explain it, and please shout if it’s not clear enough or somehow not making sense to you @mav, as this is a key point in CRT and our client side ops and use cases.

As you found out, the ActorType used to be defined as PublicKey as that’s what we started using as a valid initial value for Seq CRDT ops. What we need is basically something which tells which replica is mutating this CRDT object, and since this is user’s data, it would always be a client/app sending a Seq CRDT op.

Now, imagine a user using some app to write, let’s say events, on a Seq, and imagine the user doing so from different app instances and/or devices, simultaneously and concurrently. We need to identify each of those app/client instances/replicas with different actors so all operations sent by them can all be merged properly on each replica of the Seq (both on the network nodes and on clients syncing up this Seq/CRDT object).

First thing you could think of using is just a random number as the actor, but that would mean that the Seq’s vector clocks could be bloated as each operation and/or new client/replica instantiated would start recording CRDT ops with a diff actor, and remember we need to keep each actor in the vector clock as that’s telling us what’s the last op we’ve seen from such replica, so that future operations from that same actor can be merged properly, no matter how long ago we received last op from it.

This is why we started using client’s PK as the actor since that’s unique but at the same time it wouldn’t bloat the CRDT data when the user is using the same client from different moment and from possibly different devices or instances. So this is good enough as a start, but as you may be already realising, this is still not good enough for us, if the user is using let’s say the same application concurrently from two different devices, such CRDT ops would be using the same actor, when in reality they should be different actors with their own op’s Dot/counter to be properly merged. Secondly, even if the public key was good enough for us, that functionality in the sn_data_types crate is generic and not specific for our current client use cases, so eventually we could be having other crate using it and needing the actor to be something else, this is why we decided to strictly keep separate what the ActorType is and the PublicKey used by the replica to sign the operation, the fact that our only use of the Seq CRDT uses a PK for the actor shall be anecdotal only, as in fact the actor is kinda an opaque id.

This is very good catch, I think we just need to make sure we have our PubilcKey type to properly serialise the PK bytes instead of that string which is meant for logging/display purposes. This is because we used to have a BLS pk type only and the to_string was giving us the bytes IIRC, and we recently switched to sn_data_types/src/keys/public_key.rs at master · maidsafe/sn_data_types · GitHub

2 Likes
8

Yep this makes sense to me, thanks for the clear and detailed explanation. I see that PublicKey is not specific enough. We need a combo of the person+device.

I wonder if Vec<u8> might be more appropriate than String? My thinking is if app developers see it’s a string, they’ll type ascii for their identifier and end up using a fairly limited subset of possible char values, which leads to longer identifiers and higher storecost than necessary. I’m thinking that Vec is a way to signal a certain intention for how app-developers should approach the value.

Do you feel there’s added benefit in having a human-readable value (eg hex or base32z)? I personally prefer efficiency for aspects that affect storecost, and using human readable formats only at the presentation layer.

2 Likes
9

True, i think you are right, a Vec sounds better to me too. I agree also about storage bytes, we probably don’t need any human-readable encoding and only wherever we need to log it or show it.

3 Likes