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

Fix random polNodes sticking around #288

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

kcalvinalvin
Copy link
Member

@kcalvinalvin kcalvinalvin commented Jul 7, 2021

This PR is related with #287

EDIT:

Second commit adds pollard size to the stats, making it easier to keep track of the pollard size.

@adiabat
Copy link
Contributor

adiabat commented Jul 12, 2021

try:

if h!=0 {
    leftRoot.niece, n.niece = n.niece, leftRoot.niece          // swap
}

and for n.prune() it can take an h==0 bool as an argument, which if true gives this different functionality:

if n.niece[0].deadEnd() && n.niece[1].deadEnd() {
	n.niece[0], n.niece[1] = nil, nil
}

@dergoegge
Copy link
Contributor

The last part of my comment here might explain why there are random nodes sticking around.
I think the simplest solution to remembering proofs is to use a dedicated polnode that leaves point to if they are supposed to be remembered.

@kcalvinalvin kcalvinalvin marked this pull request as ready for review August 14, 2021 04:23
@kcalvinalvin kcalvinalvin changed the title WIP: Fix random polNodes sticking around Fix random polNodes sticking around Aug 14, 2021
@kcalvinalvin kcalvinalvin marked this pull request as draft August 14, 2021 06:09
@kcalvinalvin kcalvinalvin marked this pull request as ready for review August 14, 2021 08:54
polnodes that are to be remembered are no longer marked with n.niece[0]
= n but with a new remember bool in the polNode struct. TestCache is
modified to test for polNodes that aren't supposed to be cached, being
cached.
@kcalvinalvin kcalvinalvin force-pushed the polnode-caching branch 2 times, most recently from 271769e to bd9e174 Compare August 15, 2021 04:42
Copy link
Contributor

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

I feel like creating a special rememberNode would be the easier patch with less side effects.
rememberNode could either be a global or a field on Pollard.

accumulator/pollardutil.go Outdated Show resolved Hide resolved
// All the leaves to be deleted should be set to be not remembered.
// TODO there's probably a better way to do this than calling readPos
// a whole lot.
for _, del := range dels {
Copy link
Contributor

Choose a reason for hiding this comment

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

it would probably be better to set remember=false when swapping the leaves

Copy link
Member Author

Choose a reason for hiding this comment

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

I did try to get that to work but TestCache() kept failing and I bet there's a bunch of edge cases to handle. I'm leaning more towards just getting this in and then working on setting remember to false during swaps since that's an optimization.

niece [2]*polNode
data Hash
niece [2]*polNode
remember bool
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe creating a polLeaf type would be good because the remember bool is stored on all nodes now but really only the leaves need it.
Both polNode and polLeaf would need to inherit from the same interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yeah. I also hate the fact that niece isn't nieces for roots and leaves. Maybe we can solve all these problems at once.

Pollard stats now also return the count of all the PolNodes in the
entire pollard.
@adiabat
Copy link
Contributor

adiabat commented Aug 23, 2021

OK - this fixes the bug but at the cost of increasing the memory usage around 17% (extra 8 bytes for the bool on all nodes).
We need to fix that space usage, but will merge this for now because it does fix the caching.
We might fix this by going to pointing to a (global) "memorable" dummy node to point to. Or some other larger fix to pollard reprsentation.

@adiabat adiabat merged commit 5f26803 into mit-dci:master Aug 23, 2021
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

Successfully merging this pull request may close these issues.

3 participants