MastForest maximum node length invariant

[sergerad]

Closes #1392

• Enforce invariant that MastForest node length is < 2^30
• Return errors from add_node() and enforce_node()
• Map errors to AssemblyError

[sergerad]

@plafer LMK if this makes sense.

And are we happy panicking in add_node()? Or do we want to return error and refactor usage?

[plafer]

@sergerad Thank you for taking this on, it is super appreciated!

The current implementation looks great; however, could you rebase it on the plafer-mast-forest-serialization branch? We can then wait for that PR to be merged before we merge this one. When I created #1392, I assumed the PR would be merged by the time anyone started working on it 😅.

Hence, the current implementation can stay the same, and the new MastNodeId::from_u32_safe() could be modified to use the new TryFrom that you implemented.

And are we happy panicking in add_node()? Or do we want to return error and refactor usage?

If you are willing to do the work, returning an error would be much better as it's definitely the direction we want to go in. This would also require a similar change to MastForestBuilder::ensure_node().

[plafer]

Actually, thinking about it a bit more, I think it makes more sense for MastForest to know about the $2^{30}$ limit on number of nodes as opposed to the MastNodeId (a constraint that ultimately comes from the MastForest serialization format). So:

• MastNodeId::from_u32_safe() remains unchanged
• MastForest::add_node() contains the $2^{30}$ node limit check, and returns an error instead of panicking

[sergerad]

Thanks very much for that guidance @plafer. I will make those changes including the removal of the panic.

[sergerad]

@plafer I have added the node count invariant logic to MastForest and updated the relevant fns to return errors.

Most of the diffs are due to adding unwrap() in test files. All other files should be handling the new errors, not unwrapping.

I have done my best to handle those errors appropriately and map to AssemblyError. Let me know if and how to change the approach I have there with MastForestError.

[bobbinth]

Looks good! Thank you for tackling this! I left a few small comments inline.

[plafer]

Looks good! Just left a few minor comments

[sergerad]

Think I have covered all the current comments. Thanks!

[plafer]

LGTM! (let's fix clippy and wait for #1370 to be merged so that we can merge this into next)

[plafer]

Merged #1370, and changed the target branch to next

[sergerad]

Fixed clippy and changelog 👍

[plafer]

🙏

[bobbinth]

Looks good! Thank you! I left one small comment inline.

[bobbinth]

All looks good! Thank you!