Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement in_mem_block_tree #1786

Closed
wants to merge 1 commit into from
Closed

Implement in_mem_block_tree #1786

wants to merge 1 commit into from

Conversation

ImplOfAnImpl
Copy link
Contributor

This is the generalized version of the "in-memory block tree" from #1768 . The corresponding Trees/Tree/TreeRef structs are in tree_wrappers.rs. However, I also had to wrap all indextree's primitives (which are in the primitives module), so that they can be exposed to the user and still maintain the required guarantees (i.e. that a parent/child nodes will always correspond to parent/child blocks).

@@ -0,0 +1,413 @@
// Copyright (c) 2021-2024 RBB S.r.l
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module needs its own tests (in particular for the ensure_node_in_subtree calls).

Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to its own crate in utils?

Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too generic for my taste, but I'm OK with it. But please move it to utils in its own crate... this doens't belong in primitives.

@ImplOfAnImpl
Copy link
Contributor Author

Can we move this to its own crate in utils?

in_mem_block_tree depends on common because it needs primitives related to blocks, such as Id and GenBlock. So, utils is not a good place for it.
But I agree that the current place is not the best for it either.

@ImplOfAnImpl
Copy link
Contributor Author

Can we move this to its own crate in utils?

in_mem_block_tree depends on common because it needs primitives related to blocks, such as Id and GenBlock. So, utils is not a good place for it. But I agree that the current place is not the best for it either.

I've abstracted Id<GenBlock> away from it and then moved it to utils.
Some caveats:

  1. I've renamed it to in_mem_item_tree, because the crate has nothing to do with blocks anymore; I don't really like this new name, but I couldn't come up with anything better.
  2. utils/in_mem_item_tree depends on utils and not vice versa, like its other crates. This doesn't look nice, but we already have similar inconsistency in other places, e.g. in the wallet, so I guess it's ok?

@TheQuantumPhysicist
Copy link
Contributor

That's alright.

/// This is the observer that only holds the item-id-to-node-id map in debug builds, to perform
/// the corresponding runtime checks.
pub struct DebugOnlyTrackingModificationObserver<T: DataItem> {
#[cfg(debug_assertions)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just note that the overuse of cfg like this complicates testing. I wouldn't recommend doing this. I would prefer to have a run-time configurable flag. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just note that the overuse of cfg like this complicates testing. I wouldn't recommend doing this. I would prefer to have a run-time configurable flag. Up to you.

Since you don't insist, I'll leave it as is. IMO it won't complicate testing of the user code, because it basically adds some checks in debugs builds.

)]
NotParentChild {
child_item_id: String,
childs_parent_item_id: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children?

It's supposed to be read as child's.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the possessive 's', but not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants