Page MenuHomePhabricator

Add nTimeReceived to CBlockIndex for tracking recevied time of blocks

Authored by jasonbcox on Apr 14 2018, 17:15.



D1196 for prototype implementation. Partial work for T208.
nTimeReceived will be used to break ties for blocks as the same height.

Test Plan

make check

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

323 ↗(On Diff #3505)

Maybe it's better to put the 'right' value for the field when deserializing ?

deadalnix requested changes to this revision.Apr 15 2018, 20:53

Also, we should probably serialize/deserialize this from disk as well, but this can wait.

This revision now requires changes to proceed.Apr 15 2018, 20:53
jimpo added inline comments.
237 ↗(On Diff #3505)

Move this below nSequenceId for better alignment. Putting it here adds 4 bytes of padding above it in the struct. You can see by inspecting (char*)(&block_index.nTimeReceived) - (char*)(&block_index) with nTimeReceived in this location or below. On my machine, the offset is the same.

Optimizing alignment on this struct is important because there are many of them stored in memory all the time.

Also, we should probably serialize/deserialize this from disk as well, but this can wait.

T196 appears to be the beginning of this discussion. Please comment if you have any additional thoughts.

jasonbcox marked 2 inline comments as done.

Fixed according to feedback.

237 ↗(On Diff #3505)

Good callout. Fixed.

323 ↗(On Diff #3505)

By deserializing, I assume you mean in the constructor that takes CBlockHeader as input, since I couldn't find any other deserialization functions for CBlockIndex. Done.

This also means that nTimeReceived being 0 would be easier to debug since it's expect to be set when a new CBlockIndex is initialized from a non-CBlockHeader source. I think this is a good thing.

For de-serialization, I see that it comes from the block header (only the block header is serialized/deserialized), so there is no way to do what I wanted, at least not easily. So let's not block that diff for it.

323 ↗(On Diff #3505)


This revision is now accepted and ready to land.Apr 18 2018, 23:12
This revision was automatically updated to reflect the committed changes.