Support read-after-write consistency for `NodeGraph` bucket operations

[joshuakarp]

Specification

There's currently an issue of "read-after-write" consistency with sequences of batch operations in the NodeGraph buckets database, as setNode operations rely on the state of the bucket on insertion.

Recall that a bucket has a fixed capacity of k nodes. If we attempt to add a new node to a full bucket, we find the least active node in the bucket and remove it, in favour of the new node.

However, we come across an issue if we want to create a batch operation of multiple setNodeOps within the same bucket.

Because the ops are created before any additions, the bucket value is statically defined, based on the initial state of the bucket.

For example, if I were to batch add 3 nodes (A, B, C) into the same bucket (that already has some node Z, we'd have the following array of ops:

    [
      {
        type: 'put',
        domain: this.nodeGraphBucketsDbDomain,
        key: bucketIndex,
        value: {'Z':9.9.9.9, 'A':1.1.1.1},
      },
      {
        type: 'put',
        domain: this.nodeGraphBucketsDbDomain,
        key: bucketIndex,
        value: {'Z':9.9.9.9, 'B':2.2.2.2},
      },
      {
        type: 'put',
        domain: this.nodeGraphBucketsDbDomain,
        key: bucketIndex,
        value: {'Z':9.9.9.9, 'C':3.3.3.3},
      },
    ]

That is, the final setNode op would overwrite the entire bucket with only nodes Z and C in the bucket.

This problem can be solved by utilising the same system in js-encryptedfs. We'd need to use:

• separate sublevels for each individual bucket index
• Transaction snapshot system

In this process, it would also be worthwhile to change our keys to be buffers.

Additional context

• original thread from MR that brought up this issue https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/209#note_680113619
• mid-sprint meeting has some good discussions of this https://vimeo.com/609500458:
  • overall summary at ~1:00:30
  • Transaction overview at 11:00
• original nodes claims issue Creating a cryptolink claim from node to node (cross-signing claims) #213

Tasks

1. Investigate the snapshot system in the EFS Transaction class
2. Change buckets to use individual sublevels for each bucket index (instead of a single sublevel indexed by bucket indexes - see how RFS)
3. Add "higher" metadata level for storing the size of a particular bucket. See INodeManager sublevel vs INodeManager dir sublevel. The INodeManager sublevel is capable of storing metadata.
4. Incorporate transaction system into NodeGraph operations
5. Remove lexicographic-integer dependency from PK (see original comments regarding this here https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_707121227)

[CMCDragonkai]

Because the new db being integrated into PK, you should probably incorporate this into the vaults refactoring MR.

…

On 9/21/21 12:05 PM, Josh wrote: Specification There's currently an issue of "read-after-write" consistency with sequences of batch operations in the |NodeGraph| buckets database, as |setNode| operations rely on the state of the bucket on insertion. Recall that a bucket has a fixed capacity of |k| nodes. If we attempt to add a new node to a full bucket, we find the least active node in the bucket and remove it, in favour of the new node. *However*, we come across an issue if we want to create a batch operation of multiple |setNodeOps| within the same bucket. *Because the |ops| are created before any additions, the |bucket| value is statically defined, based on the initial state of the bucket.* For example, if I were to batch add 3 nodes (|A|, |B|, |C|) into the same bucket (that already has some node |Z|, we'd have the following array of ops: |[ { type: 'put', domain: this.nodeGraphBucketsDbDomain, key: bucketIndex, value: {'Z':9.9.9.9, 'A':1.1.1.1}, }, { type: 'put', domain: this.nodeGraphBucketsDbDomain, key: bucketIndex, value: {'Z':9.9.9.9, 'B':2.2.2.2}, }, { type: 'put', domain: this.nodeGraphBucketsDbDomain, key: bucketIndex, value: {'Z':9.9.9.9, 'C':3.3.3.3}, }, ] | That is, the final |setNode| op would overwrite the entire bucket with only nodes |Z| and |C| in the bucket. This problem can be solved by utilising the same system in js-encryptedfs. We'd need to use: * separate sublevels for each individual bucket index * |Transaction| snapshot system In this process, it would also be worthwhile to change our keys to be buffers. Additional context * original thread from MR that brought up this issue https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/209#note_680113619 <https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/209#note_680113619> * mid-sprint meeting has some good discussions of this https://vimeo.com/609500458 <https://vimeo.com/609500458>: o overall summary at ~1:00:30 o |Transaction| overview at 11:00 Tasks 1. Investigate the snapshot system in the EFS |Transaction| class 2. Change buckets to use individual sublevels for each bucket index (instead of a single sublevel indexed by bucket indexes - see how RFS) 3. Add "higher" metadata level for storing the size of a particular bucket. See INodeManager sublevel vs INodeManager dir sublevel. The INodeManager sublevel is capable of storing metadata. 4. Incorporate transaction system into |NodeGraph| operations — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#244>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHKGTUL443PSZIZEGTTUC7R73ANCNFSM5ENH7MJA>.

[joshuakarp]

This is currently within #291, but it should be considered low priority. The only function that requires this concurrent operation is refreshBuckets, and I've already implemented a work-around to resolve it within the function itself:

  public async refreshBuckets(): Promise<void> {
    return await this._transaction(async () => {
      const ops: Array<DBOp> = [];
      // Get a local copy of all the buckets
      const buckets = await this.getAllBuckets();
      // Wrap as a batch operation. We want to rollback if we encounter any
      // errors (such that we don't clear the DB without re-adding the nodes)
      // 1. Delete every bucket
      for await (const k of this.nodeGraphBucketsDb.createKeyStream()) {
        const hexBucketIndex = k as string;
        ops.push({
          type: 'del',
          domain: this.nodeGraphBucketsDbDomain,
          key: hexBucketIndex,
        });
      }
      const tempBuckets: Record<string, NodeBucket> = {};
      // 2. Re-add all the nodes from all buckets
      for (const b of buckets) {
        for (const n of Object.keys(b)) {
          const nodeId = n as NodeId;
          const newIndex = this.getBucketIndex(nodeId);
          let expectedBucket = tempBuckets[newIndex];
          // The following is more or less copied from setNodeOps
          if (expectedBucket == null) {
            expectedBucket = {};
          }
          const bucketEntries = Object.entries(expectedBucket);
          // Add the old node
          expectedBucket[nodeId] = {
            address: b[nodeId].address,
            lastUpdated: b[nodeId].lastUpdated,
          };
          // If, with the old node added, we exceed the limit...
          if (bucketEntries.length > this.maxNodesPerBucket) {
            // Then, with the old node added, find the least active and remove
            const leastActive = bucketEntries.reduce((prev, curr) => {
              return prev[1].lastUpdated < curr[1].lastUpdated ? prev : curr;
            });
            delete expectedBucket[leastActive[0]];
          }
          // Add this reconstructed bucket (with old node) into the temp storage
          tempBuckets[newIndex] = expectedBucket;
        }
      }
      // Now that we've reconstructed all the buckets, perform batch operations
      // on a bucket level (i.e. per bucket, instead of per node)
      for (const bucketIndex in tempBuckets) {
        ops.push({
          type: 'put',
          domain: this.nodeGraphBucketsDbDomain,
          key: bucketIndex,
          value: tempBuckets[bucketIndex],
        });
      }
      await this.db.batch(ops);
    });
  }

That is to say, this issue is only required for making any potential, future concurrent operations on the NodeGraph safe.

[CMCDragonkai]

The sublevels used in INodeManager in EFS does not care about the order of the sublevels. This is why we could just do this:

    const dataDb = await this.db.level(ino.toString(), this.dataDb);

But in the buckets of the NodeGraph, the order of the sublevels do matter. So we want to ensure that the sublevel prefix ordering is the same as the numeric order of the bucket index.

To do this, you need to lexi int encode the bucket index before you call this.db.level.

Note that iteration over buckets is not efficient, but iteration over key-values of all buckets will be efficient.

// this becomes difficult (you need a way to indicate every possible bucket)
for (const bucket of ...) {
}

// this is easy
for (const o of ALLBUCKETS) {
}

// this is also easy
const bucket = await this.db.level(lexint(bucketIndex), nodeGraphDb);
for (const o of bucket) {
}

[CMCDragonkai]

Fixing this is also about correctness, not just performance. The read after write consistency can be assured by using the DBTransaction.

[joshuakarp]

Something that's also arisen as I've been writing the getAll command for getting all buckets in #326. A bucket at the moment is the following structure:

bucket {
  'nodeId1': {
    address: { host: ..., port: ... },
    lastUpdated: ...,
  },
  'nodeId2': {
    address: { host: ..., port: ... },
    lastUpdated: ...,
  },
  ...
}

The NodeBucket type we currently use for this is as follows (from src/nodes/types):

type NodeBucket = {
  [key: string]: {
    address: NodeAddress;
    lastUpdated: Date;
  };
};

I found it interesting that the key is a string. So, taking a look at a specific example of a bucket:

    bucket {
      '\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x05': {
        address: { host: '227.1.1.1', port: 4567 },
        lastUpdated: '2022-02-17T23:04:27.678Z'
      }
    }

We can see that we're getting the NodeId "buffer" as a string. (Can just translate this into the raw NodeId buffer with IdInternal.fromString<NodeId>(nodeId)). I can't quite remember if this was the intention of when we refactored the node ID. This may just be how leveldb stores the buffer, but I'm not 100% sure.

Anyway, this may be worthwhile to revisit and investigate when looking into separating the buckets into separate leveldb instances.

[CMCDragonkai]

That problem becomes non-problem once you move to using sublevels.

However in the meantime, any POJO keys have to be strings so it may be using NodeIdString and not NodeIdEncoded.

I usually prefer storing any external data db or network as the encoded version, not string version.

[joshuakarp]

I've added the CommandGetAll in #326 for retrieving all of the buckets at the CLI level: 366c9dd.

Once the buckets are moved to their own sublevels, it would also be worthwhile to take a look at that command in this issue. It was a bit of a quick and dirty addition (the outputting of the buckets is very rough, and the getAllBuckets call doesn't retain the bucket index nicely, so have had to recalculate bucket index after retrieval), and doesn't currently have any tests either.

The getAllBuckets function in NodeGraph that it calls is likely to change in this issue anyway, so can throw that all together.