use correct minimum size when reading block / state headers #6263
Conversation
`sizeof` also includes padding between fields, while SSZ defines `fixedPortionSize` (on type) or `sszSize` (on value) to denote required bytes to encode. Switch forked block/state readers to SSZ size. As blocks/states are much larger than the padding, this doesn't affect practical use cases but is slightly more correct this way.
|
Updating to latest |
beacon_chain/spec/forks.nim
Outdated
|
|
||
| # TODO https://github.com/nim-lang/Nim/issues/19357 | ||
| result = readSszForkedHashedBeaconState(cfg, header.slot, data) | ||
| readSszForkedHashedBeaconState(cfg, header.slot, data) |
There was a problem hiding this comment.
In theory, this result = is to work around nim-lang/Nim#19357 despite its being less-than-ideal code style. #3050 appears to have introduced this, and notes:
- fix several cases of states being stored on stack in tests, causing
random failures on some platforms
Maybe it's not necessary, but the referenced Nim issue has not been resolved/closed.
If it's not necessary anymore in this situation, that would be better, but that would require some evidence of the codegen improvement.
There was a problem hiding this comment.
finding out requires going to the c code and looking for copies - it's a messy process - at some point, this should probably be pursued as a task, ie codegen quality review, with upstream .. including tests.
| raise (ref MalformedSszError)(msg: "Not enough data for BeaconState header") | ||
| const numHeaderBytes = fixedPortionSize(BeaconStateHeader) | ||
| if data.len() < numHeaderBytes: | ||
| raise (ref MalformedSszError)(msg: "Incomplete BeaconState header") |
There was a problem hiding this comment.
do we even need this raise? feels like SSZ.decode should already raise when there's not enough data
There was a problem hiding this comment.
Yes, we only pass toOpenArray(0, numHeaderBytes - 1), which would possibly Defect without the check.
sizeofalso includes padding between fields, while SSZ definesfixedPortionSize(on type) orsszSize(on value) to denote required bytes to encode. Switch forked block/state readers to SSZ size. As blocks/states are much larger than the padding, this doesn't affect practical use cases but is slightly more correct this way.