search

filecoin-project / consensus

Filecoin consensus work
Other
42 stars 5 forks source link

BlockTree bugs? #50

Closed sternhenri closed 5 years ago

sternhenri commented 5 years ago

@ZenGround0 should https://github.com/filecoin-project/consensus/blob/master/code/ec-sim-zs/utils/blocktree.py#L37 not instead read

length = self.BlocksByHeight.keys().sort()[len(self.BlocksByHeight) - 1] ?

Further don't we have the deeper issue that length and weight are not the same, so you will get the longest chain here but not necessarily the heaviest.

Do you agree or am I missing something?

ZenGround0 commented 5 years ago

First point: I believe that the first change will not cause any difference in behavior because the json file contains null blocks too, so all rounds should have at least one block in them. It is implicit and sloppy as it is though and you should work through this yourself to double check my thinking.

Second point: I think you are right. It depends on how the simulation writes null blocks to json. If it is the case that null blocks are only written when a real block is written then this is indeed a bug.

sternhenri commented 5 years ago

1) fair point on the null blocks piece. Makes sense. 2) I'm not sure I follow here. Null blocks contribute to length (as in height) but not weight. Likewise, depending on what tipset you pick, two blocks at same height could have different weight, so I think the bug here is real.

Will fix.

sternhenri commented 5 years ago

fixed here: https://github.com/filecoin-project/consensus/commit/04e3d8f731c6127625a9313460b719dc0748a83a