Skip to content

fix: validate branch node must have both children#656

Closed
yihuang wants to merge 2 commits intocosmos:masterfrom
yihuang:master
Closed

fix: validate branch node must have both children#656
yihuang wants to merge 2 commits intocosmos:masterfrom
yihuang:master

Conversation

@yihuang
Copy link

@yihuang yihuang commented Jan 9, 2023

This is what confused me when reading the code, to my understanding branch node always have two children, but the code seems to allow single child branch nodes?

marked as draft to wait for feedbacks first.

@yihuang yihuang requested a review from a team January 9, 2023 05:07
@yihuang yihuang changed the title validate branch node should have two children fix: validate branch node should have two children Jan 9, 2023
@yihuang yihuang changed the title fix: validate branch node should have two children fix: validate branch node should have both children Jan 9, 2023
@yihuang yihuang changed the title fix: validate branch node should have both children fix: validate branch node must have both children Jan 9, 2023
@yihuang yihuang marked this pull request as draft January 9, 2023 08:08
@yihuang yihuang marked this pull request as ready for review January 10, 2023 02:46
@cool-develope
Copy link
Contributor

yeah, I also thought this is weird, but this will be updated in adr-001 imp. It allows having one child nodekey, even zero nodekey.
IMO, we will keep them at this moment.

@yihuang
Copy link
Author

yihuang commented Jan 11, 2023

yeah, I also thought this is weird, but this will be updated in adr-001 imp. It allows having one child nodekey, even zero nodekey. IMO, we will keep them at this moment.

"It allows having one child nodekey", can you elaborate on that? do you mean one child branch node?

@cool-develope
Copy link
Contributor

cool-develope commented Jan 11, 2023

no, what I mean is it is possible to have one or zero node-key, since we are using the version + path as the node key, we can remove the child node keys for the same versions from the node structure, right?
because we can determine the child path like this left child path = node path + '0', right child path = node path + '1'

I know this is a bit far from this topic, but it will be updated in new imp, so I'd like to keep it at this time. tbh I am facing conflicts in my open PRs so frequently.

@yihuang
Copy link
Author

yihuang commented Jan 11, 2023

no, what I mean is it is possible to have one or zero node-key, since we are using the version + path as the node key, we can remove the child node keys for the same versions from the node structure, right? because we can determine the child path like this left child path = node path + '0', right child path = node path + '1'

I know this is a bit far from this topic, but it will be updated in new imp, so I'd like to keep it at this time. tbh I am facing conflicts in my open PRs so frequently.

BTW, we have some new ideas regarding iavl storage recently.

@cool-develope
Copy link
Contributor

we can close this one, @yihuang @tac0turtle ?

@yihuang yihuang closed this Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants