Skip to content

feat: add KeyTree::from_nodes() with consistency check#425

Open
ygd58 wants to merge 2 commits intologos-blockchain:mainfrom
ygd58:feat/key-tree-from-nodes
Open

feat: add KeyTree::from_nodes() with consistency check#425
ygd58 wants to merge 2 commits intologos-blockchain:mainfrom
ygd58:feat/key-tree-from-nodes

Conversation

@ygd58
Copy link
Copy Markdown
Contributor

@ygd58 ygd58 commented Apr 1, 2026

Summary

Implements the TODO at key_tree/mod.rs:62.

Refs #209.

Changes

Added KeyTree::from_nodes(nodes: Vec<(ChainIndex, N)>) -> Result<Self>:

// Create a tree from existing nodes
    (ChainIndex::root(), root_node),
    (root.nth_child(0), child_node),
])?;

Consistency Checks

  • List must not be empty
  • Root node (ChainIndex::root()) must be present
  • Every non-root node must have its parent present in the list

Tests

  • from_nodes_empty_fails
  • from_nodes_missing_root_fails
  • from_nodes_roundtrip

All 40 existing tests still pass.

Refs #209

Implements the TODO at key_tree/mod.rs:62.

Creates a KeyTree from a list of (ChainIndex, N) pairs with
consistency checks:
- List must not be empty
- Root node must be present
- Every non-root node must have its parent present

3 new tests:
- from_nodes_empty_fails
- from_nodes_missing_root_fails
- from_nodes_roundtrip

Refs logos-blockchain#209
Copy link
Copy Markdown
Collaborator

@schouhy schouhy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I wouldn't add functions to our code that are not needed. Would this allow to simplify our code if used? and where?
@Pravdyvy tagging you here because this is related to a TODO added by you some time ago.

@ygd58
Copy link
Copy Markdown
Contributor Author

ygd58 commented Apr 22, 2026

Thanks for the review. Looking at the codebase, the most natural use case would be in wallet storage/recovery scenarios when a KeyTree needs to be reconstructed from persisted flat node data (e.g. from JSON storage or a database).

The consistency checks (non-empty, root present, all parents present) provide safety guarantees that raw serde deserialization does not.

That said, if there is no current code path that would immediately benefit from this, I am happy to close this PR. The TODO at key_tree/mod.rs:62 was the motivating reference @Pravdyvy, could you clarify the intended use case?

@Pravdyvy
Copy link
Copy Markdown
Collaborator

@ygd58 @schouhy In wallet we construct trees from file via multiple iterations of following:

public_tree.insert(data.account_id, data.chain_index, data.data);

There is no check for tree consistency.
If you modify your storage, or, more probably, key protocol is changed, tree will not signal about it there.

@ygd58
Copy link
Copy Markdown
Contributor Author

ygd58 commented Apr 24, 2026

Thanks @Pravdyvy! That is exactly the use case.

In chain_storage.rs (line 66-84), nodes are inserted one by one into the tree via multiple insert() calls with no consistency validation. If storage is corrupted or the key protocol changes, the tree could be built in an inconsistent state silently.

KeyTree::from_nodes() would allow replacing the iterative insert pattern with a single call that validates:

  • Root node is present
  • All parent nodes exist before children
  • Tree is non-empty

This would make storage loading fail-fast with a clear error instead of silently building a broken tree.

I can update the PR to show a concrete refactor of chain_storage.rs using from_nodes() if that would help demonstrate the value.

…validation

Previously chain_storage.rs built KeyTree via multiple iterative insert()
calls with no consistency validation. Corrupted storage or key protocol
changes would produce a broken tree silently.

Now uses from_nodes() which validates:
- Tree is non-empty
- Root node is present
- All parent nodes exist before children

Fails fast with a clear error on malformed storage data.
@ygd58
Copy link
Copy Markdown
Contributor Author

ygd58 commented Apr 24, 2026

Updated the PR to include a concrete refactor of chain_storage.rs using from_nodes().

The iterative insert() pattern is replaced with a single from_nodes() call that validates tree consistency on storage load. This makes the wallet fail-fast with a clear error if storage is corrupted or the key protocol changes, instead of silently building a broken tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants