initial impl of UpdateData #13
Conversation
src/accumulator/util.rs
Outdated
| pub fn left_sibling(position: u64) -> u64 { | ||
| (position | 1) ^ 1 | ||
| } | ||
| // rootsToDestory returns the empty roots that get written over after numAdds |
There was a problem hiding this comment.
Since you're using the variable name num_adds in the function, it'd make sense to also to change the comment as well.
src/accumulator/stump.rs
Outdated
| while (leaves >> h) & 1 == 1 { | ||
| let root = roots.pop(); | ||
| if let Some(root) = root { | ||
| updated_subtree.push((util::left_sibling(pos), root)); |
There was a problem hiding this comment.
The newly added leaves are unique to each subtree but there may be duplicates so I handled this by having separate subtrees and merging them at the end. I see that the code here is adding all the new positions to a single tree.
Could you go through how the de-duplication is handled in this code?
There was a problem hiding this comment.
Duplicated additions?
There was a problem hiding this comment.
Not duplicated additions but when you add multiple things, you calculate the same position multiple times.
For example, when you add 05 to the tree below, the "new" positions are [05, 10].
12 12
|-------\ |-------\
08 09 ==> 08 09 10
|---\ |---\ |---\ |---\ |---\
02 03 04 02 03 04 05
In my implementation in go, I keep track of all the positions and their hashes used during the add.
For example, when you add 04 to the tree below, the used positions are [04].
12 12
|-------\ |-------\
08 09 ==> 08 09
|---\ |---\ |---\ |---\
02 03 02 03 04
Then when you add 05, the used positions are [04, 05, 10]
12 12
|-------\ |-------\
08 09 ==> 08 09 10
|---\ |---\ |---\ |---\ |---\
02 03 04 02 03 04 05
So when we add two elements to the below tree, we get:
[04]
[04, 05, 10]
Where there are two 04s.
12
|-------\
08 09
|---\ |---\
02 03
From what I can see in this PR, the process is mostly the same as my go implementation but I don't
see where the duplicates are handled. Thus I'm not certain if the code is correct.
| "dbc1b4c900ffe48d575b5da5c638040125f65db0fe3e24494b76ea986457d986", | ||
| "084fed08b978af4d7d196a7446a86b58009e636b611db16211b65a9aadff29c5", | ||
| "02242b37d8e851f1e86f46790298c7097df06893d6226b7c1453c213e91717de", | ||
| "9576f4ade6e9bc3a6458b506ce3e4e890df29cb14cb5d3d887672aef55647a2b", |
There was a problem hiding this comment.
Just leaving a note:
The go library currently also returns the roots as well so if we don't return them here, the behaviors won't be the same.
Not sure if returning the root or not returning the root is better.
There was a problem hiding this comment.
Do you mean the current set of roots?
There was a problem hiding this comment.
Not the current set of roots. I mean the roots that were generated by the additions.
So here, the final root after adding all 6 elements was df46b17be5f66f0750a4b3efa26d4679db170a72d41eb56c3e4ff75a58c65386.
In the go implementation, this root would also be returned.
Davidson-Souza
force-pushed
the
feature/update-stump-for-cache
branch
from
e79f3dd
to
360e7ab
Compare
|
Rebased with main, addressed all comments and added some new tests. |
|
|
||
| roots | ||
| } | ||
| updated_subtree.sort(); |
There was a problem hiding this comment.
In the go one I had a vector of subtrees because you could avoid the sort & dedupe here.
Maybe in rust those operations aren't as slow but just noting here for future reference.
| ], | ||
| "to_destroy": [] | ||
| }, | ||
| { |
There was a problem hiding this comment.
It'd be nice to know how you got to these roots. For others I can kinda guess but for this one I can't really tell.
Maybe a "comment" field and add a simple description like "add 6, delete [0, 4]" or something similar would be nice. If we decide to change the hash function used in utreexo we'd need to recreate these so it'd be good to know how to generate these.
|
ACK 360e7ab Verified the cache tests on the go library. The stump add algorithm looks ok to me. Could you rebase on main again and address the comment I left on the tests? |
Davidson-Souza
force-pushed
the
feature/update-stump-for-cache
branch
from
360e7ab
to
7d818ec
Compare
Done with 7d818ec |
test_values/cache_tests.json
Outdated
| "to_destroy": [] | ||
| }, | ||
| { | ||
| "initial_stump": "Add six leaves [0, 1, 2, 3, 4, 5] and remove them", |
There was a problem hiding this comment.
Could you specify which ones you removed here? It's not possible to have two none empty roots here if you remove all six leaves
There was a problem hiding this comment.
Ops, my mistake. This one has no deletions. Fixed!
UpdateData will be useful for caching wallet's utxos. This commit changes `update`'s API to return all data modified while processing a block. It also returns what roots changed during addition.
Davidson-Souza
force-pushed
the
feature/update-stump-for-cache
branch
from
7d818ec
to
a337fa4
Compare
|
ACK a337fa4 |
* Add all missing electrum methods * Refactor to only use get_arg macro * Fix a divergence with json-rpc spec This codebase used to treat id as i32, but the spec says it can be any json-valid object. * Fix a clippy warn
This PR updates
modify'sapi to also return what have changed during this update. For now, only roots changed during addition are returned. This is part of the steps in #12.