-
Notifications
You must be signed in to change notification settings - Fork 60
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
accumulator: Add CacheSim type that can simulate pollard modification #185
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I think the concept is good but do you have tests that you used on this code? Proof code is consensus critical and we've caught one consensus bug here before. I think some tests here are needed.
// Figure out which targets are known/cached and which aren't. | ||
var knownTargets, unknownTargets []uint64 | ||
for _, target := range targets { | ||
if c.cached.Bit(int(target)) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit since it'll be eons until we get to 1 << 63 leaves but this could error out right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kcalvinalvin are you saying it will be the int casting that overflows? in that case would it be a problem on 32-bit platforms? are those supposed to be supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a 64-bit system it would cast to a signed int64 and target
is unsigned, so the casted int could be negative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't know what happens on a 32-bit system with all the uint64s we are using 😄
|
||
// Simulate simulates one modification to the pollard. | ||
// Takes a slice of target positions and a slice of bools representing new leaves (true means "cache the leaf"). | ||
// Returns the proof positions that are needed to prove the targets (including the unkown targets). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another nit but there's a spelling error on unkown
var proofPositions, computedPositions []uint64 | ||
for row := uint8(0); row < forestRows; row++ { | ||
computedPositions = append(computedPositions, targets...) | ||
if numLeaves&(1<<row) > 0 && len(targets) > 0 && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numLeaves&(1<<row) > 0
is checking to see if a root exists right? Comment or maybe even splitting this up to make something like rootExists := numLeaves&(1<<row) > 0
could be better for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably do this in a lot of places or maybe we create a function that checks for roots that we can call in various places, pretty sure the go compiler would inline simple functions like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't there some example very recently where Go didn't inline a really simple function? probably something i read from @kcalvinalvin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should inline all functions that meet these criteria.
we can also verify that it inlines them using -gcflags=-m
when building.
For sure, I haven't really tested this yet. I will mark this as a draft for now and add some tests. |
While writing the unit test i found that this does not work as it currently stands or at least it's not optimal. It still reduces the proof sizes in a lot of cases though so maybe it's ok for now. Example for targets A and B: if A is cached and B is not but the proof paths of A and B overlap then the combined proof size for A and B can reduced because the overlap of A can be retrieved from cache. This is similar to how you can batch proofs for multiple targets. It would probably be tricky to make this optimal... one way would be to keep all nodes in the bit set and not just the leaves but then you would have to do the entire remove transformation which could get tricky because it's a bit set and not a data structure like the pollard with pointers. (also the bit set would be 2x in size) |
was part of my experiments in #180.
Simulation of pollard modification will be helpful to derive partial proofs without adding extra round trips for every block during IBD.
This PR also introduces the
ProofPositions
function which most of the other changes in #180 depend on.