Ozan/merkletree #32
Ozan/merkletree #32
Conversation
| * @param n number of elements to create | ||
| * @return a new skip list with n random elements | ||
| */ | ||
| public static MerkleTree createSkipList(int n) { |
There was a problem hiding this comment.
Seems like there are still residues of SkipList in this PR, please check and fix.
| /** | ||
| * Encapsulates tests for an authenticated and concurrent implementation of SkipList ADS. | ||
| */ | ||
| public class MerkleTreeTest { |
There was a problem hiding this comment.
Please add a comment for each test briefly explaining what it evaluates.
| if (e == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Invalid input must trigger an IllegalArgumentException, the get should explicitly list the exceptions it throws, and those exceptions must be bubbled up to the caller, literally safely fail the main. Also, please add a test covering this test case.
| } | ||
| if (proof == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
On a happy path, do we expect to have a null proof? if we don't, then this line steps into an illegal state, and hence must throw an IllegalStateException. (Also, please add test covering this test case).
| } | ||
|
|
||
| private Proof getProof(Identifier id) { | ||
| Integer idx = leafNodesHashTable.get(hasher.computeHash(id)); |
There was a problem hiding this comment.
ids are unique, i.e., they correspond to the hash of entities they keep, hence, there is no need to hash them again, right?
| if (e == null) { | ||
| lock.writeLock().unlock(); | ||
| return null; |
There was a problem hiding this comment.
Isn't it an IllegalArgumentException case?
| } | ||
|
|
||
| @Override | ||
| public AuthenticatedEntity put(Entity e) { |
There was a problem hiding this comment.
A general comment about this and some subsequent methods, where unlocking the lock is happening on several disjoint execution paths. It makes the implementation daunting to keep track of lock-unlock state transitions and makes it bug-prone regarding the liveness issues. Hence, please use try-finally blocks to lock and unlock only once.
| AuthenticatedEntity put(Entity e); | ||
|
|
||
| AuthenticatedEntity get(Identifier id); | ||
| AuthenticatedEntity get(Entity e); |
There was a problem hiding this comment.
This is wrong! get is utilized only when we have the id of the entity and not the entity itself. Please abide to the original interface.
| * @return SHA3-256 hash object of the commutative concatenation (i.e. max(b1,b2) || min(b1,b2)) | ||
| * of the two byte arrays. | ||
| */ | ||
| public Sha3256Hash computeHash(byte[] b1, byte[] b2) { |
| Entity entity = new EntityFixture(); | ||
| try { | ||
| merkleTree.put(entity); | ||
| AuthenticatedEntity authenticatedEntity = merkleTree.get(entity); |
There was a problem hiding this comment.
get should be done in a separate for loop (sequentially), once all put threads are done.
* implements merkle tree implementation and insertion to it * implements merkle tree authentication * implements tests * implements hash table for searching in merkle tree * implements thread safety to merkle tree implementation * fixes lint & spotbugs issues * renames Merkle Tree creation function in MerkleTreeFixture.java * suppresses EI_EXPOSE_REP warnings in spotbugs * suppresses warnings in spotbugs * adds comments for unit tests * handles exceptions on MerkleTree.java * changes how id of an entity is stored on the merkle tree, changed from hash(id) to id * handles exceptions better for locks * changes get method in a way it gets the AuthenticatedEntity by id not Entity * improves thread safety check * improves exception handling1 * improves the hashing and how proofs are created in merkle tree * applies revisions * applies revisions * applies revisions * applies revisions * fixes lint * removes unnecessary setters * removes unnecessary setters * fixes a java doc * applies revisions * fixes lint * applies revisions Co-authored-by: Yahya <yhassanzadeh13@ku.edu.tr>
No description provided.