Page MenuHomePhabricator

[avalanche] Add some safety to AvalancheBlockUpdate using static asserts

Authored by deadalnix on Dec 23 2018, 15:23.



AvalancheBlockUpdate rely on the two lowers bits of a CBlockIndex* to be zero to work properly. While we know this is true, enforcing this is the case using a static assert is preferable.

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

deadalnix created this revision.Dec 23 2018, 15:23
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 23 2018, 15:23
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Dec 28 2018, 22:33
jasonbcox added a subscriber: jasonbcox.

Nits on the variable naming to make it explicit what is being masked. Looks good otherwise.

150 ↗(On Diff #6419)


151 ↗(On Diff #6419)


This revision now requires changes to proceed.Dec 28 2018, 22:33
jasonbcox added inline comments.Jan 1 2019, 00:06
153 ↗(On Diff #6419)

See the build failure:

In file included from ../../src/avalanche.cpp:5:0:
../../src/avalanche.h:153:52: error: expected ‘,’ before ‘)’ token
     static_assert(alignof(CBlockIndex) >= 1 << BITS);

static_assert(statement, message) needs an error message

deadalnix updated this revision to Diff 6495.Jan 3 2019, 21:28
  • Add a comment on the static assert.
Fabien requested changes to this revision.Jan 4 2019, 09:21
Fabien added a subscriber: Fabien.

I don't think you're checking anything useful here.
If the arch does not support unaligned access, the static_assert is redundant with the compiler behavior.
If the address is forced unaligned (e.g. using packing), the static_assert will not detect it anyway, and your status and block index will get corrupted.

You can only check at runtime that pindexIn is aligned to a multiple of (1 << STATUS_BITS).

This revision now requires changes to proceed.Jan 4 2019, 09:21
deadalnix requested review of this revision.Jan 4 2019, 14:42

Unaligned pointers is undefined behavior. See:

Fabien accepted this revision.Jan 4 2019, 15:59

Got explanation from offline discussion : this check will prevent breaking the code if CBlockIndex get modified so thath its size gets <(1 << STATUS_BITS)

jasonbcox accepted this revision.Jan 6 2019, 02:16
This revision is now accepted and ready to land.Jan 6 2019, 02:16
This revision was automatically updated to reflect the committed changes.