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

Use id instead of index #30

Open
khernyo opened this issue Jun 10, 2018 · 3 comments
Open

Use id instead of index #30

khernyo opened this issue Jun 10, 2018 · 3 comments

Comments

@khernyo
Copy link
Member

khernyo commented Jun 10, 2018

It seems to me that the data referred to as index is not really an index. At least it's not the node index when building the merkle-tree. It's a bit confusing when the nodes are stored in a flat data structure (e.g., a list).

When building the merkle-tree, there is an inherent ordering which is the order in which each hash is computed (using this order requires the least amount of memory to build the merkle-tree):

  1. first data block is received, its hash is computed (A)
  2. second data block is received, its hash is computed (B)
  3. hash of (AB) is computed (C)
  4. third data block is received, hashes computed (D)
  5. fourth data block is received, hashes computed (E)
  6. hash of (DE) is computed (F)
  7. hash of (CF) is computed (G)
    ...

The index for the above hashes in the flat-tree are the following:

  • A: 0
  • B: 2
  • C: 1
  • D: 4
  • E: 6
  • F: 5
  • G: 3

Maybe there is a reason for using index to refer to these nodes, but I could not find one. So, if there is no specific reason for this name, I propose to use id or maybe position instead.

@yoshuawuyts
Copy link
Collaborator

yoshuawuyts commented Jun 11, 2018

In the case of this module I've mostly followed the prior art at https://github.com/mafintosh/flat-tree. I see where you're coming from, but mostly for compat reasons I reckon it might be best to keep things similar.

I personally don't care too much about naming in this case, but probably the easiest approach in case of doubt is to keep things similar so bridging between the two is as straight forward as possible.

Hope that makes sense!

@khernyo
Copy link
Member Author

khernyo commented Jun 11, 2018

Yes, the terminology should be in sync, so I opened the same issue in that repo too: mafintosh/flat-tree#12

I assume there is no other reason against the rename.

@kasperisager
Copy link

I've provided an answer here: mafintosh/flat-tree#12 (comment)

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

No branches or pull requests

3 participants