Reduce Preprepare size #1924
Reduce Preprepare size #1924
Conversation
| if err != nil { | ||
| return err | ||
| } | ||
| if proposal, ok := propIndex[im.ProposalHash]; ok { |
There was a problem hiding this comment.
could im.ProposalHash be nil here?
There was a problem hiding this comment.
Only if someone is sending a malicious message, I think. But in this case, as the propIndex is generated with valid proposals, it shouldn't find anything and in that case
| mess := make([]Message, len(iMess)) | ||
| for i, im := range iMess { | ||
| mess[i] = Message{ | ||
| Code: im.Message.Code, |
There was a problem hiding this comment.
could im.Message be nil here?
There was a problem hiding this comment.
Mmm. As much as before, if an element of the list had been nil. Not sure if it's great not to check, but it's not worse than before
| } | ||
|
|
||
| // Iterate values. RLP does not support maps | ||
| proposals := make([]*types.Block, len(proposalsMap)) |
There was a problem hiding this comment.
Would it be helpful to log the number of proposals we're seeing here (e.g. with trace logging)?
If this does not fix the issue, it would be nice to get some fresh logging data to see what the composition of the round change certificate looks like.
|
|
||
| if proposal != nil { | ||
| // we don't use the height since we know they MUST be the same | ||
| proposalsMap[proposal.Hash()] = proposal |
There was a problem hiding this comment.
Since we can't RLP a map, is there a set data structure we can use here?
There was a problem hiding this comment.
We are using that map to build the proposal list. As the same proposal will be set in the same hash, that result array will be already a set
…-blockchain into hbandura/roundchange_boost
hbandura
requested review from
gastonponti and
nategraf
and removed request for
a team
* Log line from Warn to Debug * Bump version to 1.5.8 * New encoding for RoundChangeCertificate * clean up NewIndexedRoundChangeMessage * Use pointers whenever possible to reduce call by value costs * Merge * Fix compile error * Added comment * Added docs * Add error checking * Add error * Clean up encoding logic * more cleanup * Fix compile errors * Added doc * Fix dummy object in test * Fix * Fix tests * Remove nil * fix proposal not rlp serializable * fix test * Add tests * Fix lint * Fix test * fix test * Fix test * Fix * Add log * Revert change * cleanup (#1925) * cleanup * revert * Fix debug log Co-authored-by: Pasto <[email protected]> Co-authored-by: Gaston Ponti <[email protected]>
Cherry pick reduce preprepare size from release/1.5.x * Log line from Warn to Debug * Bump version to 1.5.8 * New encoding for RoundChangeCertificate * clean up NewIndexedRoundChangeMessage * Use pointers whenever possible to reduce call by value costs * Merge * Fix compile error * Added comment * Added docs * Add error checking * Add error * Clean up encoding logic * more cleanup * Fix compile errors * Added doc * Fix dummy object in test * Fix * Fix tests * Remove nil * fix proposal not rlp serializable * fix test * Add tests * Fix lint * Fix test * fix test * Fix test * Fix * Add log * Revert change * cleanup (#1925) * cleanup * revert * Fix debug log Co-authored-by: Pasto <[email protected]> Co-authored-by: Gaston Ponti <[email protected]> Co-authored-by: Mariano Cortesi <[email protected]> Co-authored-by: Gaston Ponti <[email protected]>
Description
This PR modifies
RoundChangeCertificateencoding so that we don't serializeNtimes the proposal, instead we do that only once.Backwards compatibility
Change is not backward compatible, requires 2/3 of validators to use the new version at that same time