ssz icon indicating copy to clipboard operation
ssz copied to clipboard

get() Root should not return a sub Tree

Open dapplion opened this issue 3 years ago • 2 comments

When getting a property from a tree backed ssz type this if decides to return the "raw" value or tree backed.

https://github.com/ChainSafe/ssz/blob/d6efe104a3bdc2d2beebc47af8234a3e40d414f4/src/backings/tree/container.ts#L138

In my opinion I would assume that a Root will not return a sub Tree, but just the raw value. However this is not the case

const stateTree = config.types.phase0.BeaconState.tree.deserialize(stateBytes);
console.log({
      epoch: stateTree.finalizedCheckpoint.epoch,
      root: stateTree.finalizedCheckpoint.root,
});
// stdout of above code
{
  epoch: 0,
  root: Tree { _node: LeafNode { _root: [Uint8Array] }, hook: [Function] }
}

Due to the Tree hook anyone that holds any root part of a state will prevent to garbage collect that state, causing this memory issues https://github.com/ChainSafe/lodestar/issues/2181#issuecomment-803656088

dapplion avatar Mar 21 '21 20:03 dapplion

I propose to add constructor options to composite types so you can choose not to return a Tree (using shouldNotReturnSubTree for clarity, but we should use a better name)

export abstract class CompositeType<T extends object> extends Type<T> {
  shouldNotReturnSubTree: boolean;
  constructor(opts) {
    this.shouldNotReturnSubTree = opts.shouldNotReturnSubTree ?? false
  }

Then tree backing can use that info to not return a tree for those types

// ssz/src/backings/tree/container.ts:138
if (!isCompositeType(fieldType) || fieldType.shouldNotReturnSubTree) {
   // return raw value
} else {
  // return tree
}

This behavior can be customized in lodestar-types for roots, and won't affect other users.

export const Root = new ByteVectorType({length: 32, shouldNotReturnSubTree: true});

dapplion avatar Mar 21 '21 21:03 dapplion

right now, we use valueOf to return the primitive value. We use this a lot to get an actual Uint8Array from a Root.

{
  root: stateTree.finalizedCheckpoint.root.valueOf(),
}

I agree that a Root should always return a Uint8Array, since roots are treated as basic types and never partially updated. IMO we can just update RootType.tree_getProperty to return a Uint8Array.

wemeetagain avatar Mar 22 '21 16:03 wemeetagain