[Merged by Bors] - Tortoise: only count base block opinion that is within hdist #2453
Conversation
y0sher
force-pushed
the
trtl_inf_opinions_leak
branch
from
6a15ba1
to
f83c048
Compare
tortoise/verifying_tortoise_test.go
Outdated
| @@ -188,6 +196,14 @@ func turtleSanity(t *testing.T, layers types.LayerID, blocksPerLayer, voteNegati | |||
| for l = mesh.GenesisLayer().Index() + 1; l <= layers; l++ { | |||
| turtleMakeAndProcessLayer(t, l, trtl, blocksPerLayer, msh, hm) | |||
| fmt.Println("Handled ", l, "========================================================================") | |||
| lastlyr := trtl.BlockOpinionsByLayer[l] | |||
| for _, v := range lastlyr { | |||
| fmt.Println("Block opinoin map size") | |||
There was a problem hiding this comment.
| fmt.Println("Block opinoin map size") | |
| fmt.Println("Block opinion map size") |
There was a problem hiding this comment.
this doesn't actually print the size
tortoise/verifying_tortoise_test.go
Outdated
| @@ -324,21 +340,42 @@ func createTurtleLayer(l types.LayerID, msh *mesh.DB, bbp baseBlockProvider, ivp | |||
| return lyr | |||
| } | |||
|
|
|||
| //func size(m map[types.LayerID]map[types.BlockID]Opinion) int { | |||
tortoise/verifying_tortoise.go
Outdated
| @@ -395,6 +399,18 @@ func (t *turtle) processBlock(block *types.Block) error { | |||
| // TODO: save and vote against blocks that exceed the max exception list size (DoS prevention) | |||
| opinion := make(map[types.BlockID]vec) | |||
| for blk, vote := range baseBlockOpinion.BlocksOpinion { | |||
| fblk, err := t.bdp.GetBlock(blk) | |||
| if err != nil { | |||
| return fmt.Errorf("voted block not in db") | |||
There was a problem hiding this comment.
I will be cooll to have in error messages layer id also.
There was a problem hiding this comment.
@lrettig - it will probably be useful for these cases if we could create a structure with a few fields and return it, right now it will be returned in string form..
@sudachen - well, it is problematic, since we don't really know the LayerID of this block since we couldn't find it in our DB.. we could print the original processed block id and layerid as well if that helps..
There was a problem hiding this comment.
@y0sher re: the block's layer - if you print the voting block's layer, then in most cases the voted-for block will be in the previous layer. This is not always the case, but can help the investigation.
Having an error structure with fields shouldn't be terribly hard. We can create a type that implements the error interface and pass that along, then in log.Err() we can do a type assertion and extract the fields if relevant. Only problem is that log.Err() returns a single field - would be nicer if we could log those extracted fields separately. I have an idea how to do that, but it starts getting complicated... If we make sure we always use fmt.Errorf("bla: %w", err) when propagating errors then we won't lose the wrapped fields either.
There was a problem hiding this comment.
we need a solution to pass multiple fields as error.. for sure not a part of this PR, would appreciate if you could create an issue about it and mention this code, we can outsource it to the community.
|
bors merge |
## Motivation This was causing a memory leak where a new layer arrived and according to the original description of the SMIP we copied the base block opinion and added latest diffs, this resulted in an adding a larger and larger map to the table on each layer saving the entire history of votes from all preceding base blocks. the memory leak wasn't easy to spot since the above map exist for each block for each layer and structured as - `map[LayerID]map[BlockID]OpinionMap(map[BlockID]opinon)` the opinion map would be bigger each time though it will be cleared by eviction, a larger map will be added for next layer and so on. ## Changes only count base block opinions that is within hdist. this results in much lower and consistent memory size of the tortoise. ## Test Plan Added unit test check to ensure our table size stays persistent ## TODO - [ ] - discuss with research team
|
Build failed: |
|
bors merge |
## Motivation This was causing a memory leak where a new layer arrived and according to the original description of the SMIP we copied the base block opinion and added latest diffs, this resulted in an adding a larger and larger map to the table on each layer saving the entire history of votes from all preceding base blocks. the memory leak wasn't easy to spot since the above map exist for each block for each layer and structured as - `map[LayerID]map[BlockID]OpinionMap(map[BlockID]opinon)` the opinion map would be bigger each time though it will be cleared by eviction, a larger map will be added for next layer and so on. ## Changes only count base block opinions that is within hdist. this results in much lower and consistent memory size of the tortoise. ## Test Plan Added unit test check to ensure our table size stays persistent ## TODO - [ ] - discuss with research team
|
Pull request successfully merged into develop. Build succeeded: |
bors
bot
changed the title
Motivation
This was causing a memory leak where a new layer arrived and according to the original description of the SMIP we copied the base block opinion and added latest diffs, this resulted in an adding a larger and larger map to the table on each layer saving the entire history of votes from all preceding base blocks.
the memory leak wasn't easy to spot since the above map exist for each block for each layer and structured as -
map[LayerID]map[BlockID]OpinionMap(map[BlockID]opinon)the opinion map would be bigger each time though it will be cleared by eviction, a larger map will be added for next layer and so on.
Changes
only count base block opinions that is within hdist.
this results in much lower and consistent memory size of the tortoise.
Test Plan
Added unit test check to ensure our table size stays persistent
TODO