MystenLabs/Sui_Owned_Object_Pools

Remove objects from merged pool to prevent double use

Tzal3x opened this issue · 1 comments

Tzal3x commented

This is how merge looks today in the code.

  /*
  Merges the current pool with another pool.
   */
  public merge(poolToMerge: Pool) {
    this._objects = new Map([...this._objects, ...poolToMerge.objects]);
  }

This is how it is in the design doc:

	/**
   * Merges the objects from `that` into `this`, assuming they all belong
   * to the same owner.  Leaves `that` empty to prevent the accidental
   * double-use of its objects.
   */
	merge(that: Pool) {
		if (this.address !== that.address) throw new Error("...");
		this.objects.push(...that.objects.splice(0, that.objects.length));
	}

The important part is that it leaves that empty to prevent the accidental double-use of its objects (from the doc comments). Consider this example, and how it behaves differently with the two implementations of merge:

main = /* ... */;
worker = /* ... */;

main.merge(worker);
await Promise.allSettled([
  worker.signAndExecuteTransactionBlock(T),
  main.signAndExecuteTransactionBlock(U),
]);

In the spec version, the worker call would fail because it no longer contains any objects. In the implemented version it might succeed, but it also risks equivocating the objects that were merged from worker into main.

Tzal3x commented
main = /* ... */;
worker = /* ... */;

main.merge(worker);
await Promise.allSettled([
  worker.signAndExecuteTransactionBlock(T),
  main.signAndExecuteTransactionBlock(U),
]);

Please note that main never calls signAndExecuteTransactionBlock.
Also worker pools whenever they are merged, they are removed from the worker pool.

Therefore the above examples are unlikely impossible to happen, but we will still implement this just to be safe in future changes (maybe in the future main also executes the transaction or 2 pools get merged together apart from main).