Publish milestone together with virtual transactions #188#200
Publish milestone together with virtual transactions #188#200cristina-vasiu wants to merge 7 commits intoHelixNetwork:dev-vtxfrom
Conversation
Take changes from upstream repo
dzhelezov
left a comment
dzhelezov
left a comment
There was a problem hiding this comment.
Major things that we should discuss perhaps:
-- Filler for the empty leaves (if NULL_HASH is good)
-- List<List<Hash>> vs List<Hash> for the Merkle tree representation (and the depth issue)
|
|
||
| public static List<byte[]> buildMerkleTransactionTree(List<Hash> leaves, Hash milestoneHash, Hash address){ | ||
| if (leaves.isEmpty()) { | ||
| leaves.add(Hash.NULL_HASH); |
There was a problem hiding this comment.
There were some concerns from Oliver about using NULL_HASH. Is it kosher here or we should use another filler?
There was a problem hiding this comment.
Regarding NULL_HASH it can be replaced with other hash (but should exists in tangle), here it means that no tips are found, so their merkle root is NULL_HASH, in tangle it will look like this milestone is directly connected to genesis.
There was a problem hiding this comment.
in tangle it will look like this milestone is directly connected to genesis
why I don't think it is kosher.
There was a problem hiding this comment.
can be replaced with other hash (but should exists in tangle)
yes it has to be a valid tx_hash, not sure how, without implementing a(nother) genesis tx.
| private void createAndBroadcastVirtualTransactions(byte[] tipsBytes, List<String> milestoneBundle) { | ||
| TransactionViewModel milestone = getFirstTransactionFromBundle(milestoneBundle); | ||
| if (milestone != null) { | ||
| List<Hash> tips = extractTipsFromData(tipsBytes); |
There was a problem hiding this comment.
Do we have guarantees here that the tips are solid?
There was a problem hiding this comment.
No, milestone for current node is not created yet, so tips are not solid (probably the other milestones from other nodes are in the same place).
I thought that we should broadcast virtual transaction before broadcasting the milestone.
Also here all virtual transaction are broadcasted (should be limitted)
| return buffer; | ||
| } | ||
|
|
||
| public static List<List<Hash>> buildMerkleTree(List<Hash> leaves){ |
There was a problem hiding this comment.
What are the pros and cons of having List<List<Hash>> for the tree representation against a flat array (ArrayList<Hash>) ?
There was a problem hiding this comment.
It depends on what we want to use it:
List<List> is faster if you need to access hashes for a certain level of Merkle tree, but it's slower on traverse all nodes (without any order), or traverse from leves to root.
List is faster if all nodes should be traversed and if merkle level is not relevant
In most cases I saw that only root is used, so maybe we can change the calling methods to call getMerkleRoot instead of the entire tree.
There was a problem hiding this comment.
The trick is to use the compact array representation of a binary tree. If you have say 8 leaves, you pack it into array of size 15 = (2*8) - 1 in the following way:
- leaves have indices 7, 8, ..., 14
- next level indices 3,4,5,6
- next level 1,2
- root 0
The level i has indices from 2^i-1 to 2^{i+1}-2
The benefit of having this representation is that you initially allocate the full size and fill evertyhing with null and as the data becomes available, you can quickly navigate to parents and children by simple bit shifts of the indices, at the same time preserving the order at all levels.
There was a problem hiding this comment.
To be more precise: children of a[i] are a[2*i+1] and a[2*i+2], parent of a[i] is a[(i-1) >> 1]
There was a problem hiding this comment.
I'd suggest introducing a separate interface MerkleTree and MerkleTreeImpl, where this builder can be moved.
Also, I don't think that we should replace null with NULL_HASH at all levels, it should be used only for padding the leaves list so that it's size is an integer of the form 2^i for some i.
There was a problem hiding this comment.
There is no padding to merkle tree leaves, it just take the leaves and pair them 2 by 2 and compute the hashes, when there is an odd number of leaves the last one is not taken into consideration.
oracle58
added
feature
3 participants
No description provided.