Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Depth-first search ledger walking #3324

Merged
merged 22 commits into from
Jul 6, 2021
Merged

Depth-first search ledger walking #3324

merged 22 commits into from
Jul 6, 2021

Conversation

theohax
Copy link
Contributor

@theohax theohax commented Jun 7, 2021

This is the first iteration of the algorithm. Planning to add disk-based hashtables to it and bloom filters at some point but first to get it stable, well tested and profiled.

@theohax theohax requested a review from clemahieu June 7, 2021 22:02
nano/node/ledger_walker.cpp Outdated Show resolved Hide resolved
nano/node/ledger_walker.cpp Outdated Show resolved Hide resolved
nano/core_test/ledger_walker.cpp Outdated Show resolved Hide resolved
nano/core_test/ledger_walker.cpp Outdated Show resolved Hide resolved
@zhyatt zhyatt added this to the V23.0 milestone Jun 22, 2021
theohax added 5 commits June 29, 2021 00:10
Signed-off-by: theohax <theo@nano.org>
Signed-off-by: theohax <theo@nano.org>
Signed-off-by: theohax <theo@nano.org>
Signed-off-by: theohax <theo@nano.org>
Signed-off-by: theohax <theo@nano.org>
CMakeLists.txt Outdated Show resolved Hide resolved
@theohax theohax requested a review from thsfs June 29, 2021 15:10
@theohax
Copy link
Contributor Author

theohax commented Jun 29, 2021

Adding @thsfs at reviewers -- let me know if integrating diskhash from their public repository and building it like this makes sense to you too. And, of course, as per discussed, having our own fork of it does not sound like a bad idea :D.

@thsfs
Copy link
Contributor

thsfs commented Jun 29, 2021

Adding @thsfs at reviewers -- let me know if integrating diskhash from their public repository and building it like this makes sense to you too. And, of course, as per discussed, having our own fork of it does not sound like a bad idea :D.

Having our own repository for this is not required for now. It'd be in case we need to send changes to it (when we do), or for safety, as this code seems to be maintained by a few people.

thsfs
thsfs previously approved these changes Jun 29, 2021
nano::account_info account_info{};
ASSERT_FALSE (node->ledger.store.account.get (transaction, key.pub, account_info));

// TODO: check issue with account head
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not adding it as it won't run, rather we could add an issue on GitHub to track this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are a couple of extra type of checks for unit tests that for some reason do not work as I expected them to. The ledger walker is most likely going to be changed again soon anyways (in different PR) so I don't think this would sit around too much commented out, but yeah maybe I should just take it out? @clemahieu what do you think?

@thsfs thsfs self-requested a review July 5, 2021 15:34
thsfs
thsfs previously approved these changes Jul 5, 2021
@theohax
Copy link
Contributor Author

theohax commented Jul 6, 2021

@thsfs @clemahieu one more review/approval after last pushes please, thx.

@theohax theohax merged commit 2e838b4 into develop Jul 6, 2021
@theohax theohax deleted the dfs-traversal branch July 6, 2021 13:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants