Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocks.hpp - Tech debt and potential maintainability issues #375

Closed
Caslship opened this issue Jan 2, 2018 · 0 comments

Comments

Projects
3 participants
@Caslship
Copy link

commented Jan 2, 2018

I've noticed a potential source of future errors in regards to maintanence by future developers. When calculating the static property size in send_block, receive_block, open_block, and change_block, you seem to be calculating the size by summing the size of properties within the block's respective hashables in addition to work and signature properties:

class send_hashables
{
    \\ ...
    rai::block_hash previous;
    rai::account destination;
    rai::amount balance;
}

class send_block : public rai::block
{
    \\ ...
    static size_t constexpr size = sizeof (rai::account) + sizeof (rai::block_hash) + sizeof (rai::amount) + sizeof (rai::signature) + sizeof (uint64_t);
    rai::signature signature;
    uint64_t work;
}

Aside from the fact that how size is computed isn't entirely clear, it's computation is prone to developer error if a developer doesn't make the connection that hashables size is taken into account. An easy fix for this would be...

class send_hashables
{
    \\ ...
    rai::block_hash previous;
    rai::account destination;
    rai::amount balance;
    static size_t constexpr size = sizeof (rai::previous) + sizeof (rai::account) + sizeof (rai::amount);
}

class send_block : public rai::block
{
    \\ ...
    static size_t constexpr size = send_hashables::size + sizeof (rai::signature) + sizeof (uint64_t);
    rai::signature signature;
    uint64_t work;
}

The only other issues I can see is that this size property is repeated throughout the different block child classes which gives the impression that it is required. I would try to take advantage of polymorphism and pure-virtual methods (or just plain virtual if you have a reasonable default method for computing size) to remedy this and keep guidelines clear.

I would shy away from using the sizeof operator on types rather than properties if you can help it; however, for some things like figuring out how many bytes we need for memory allocation then it's OK. This is related to my earlier argument that it doesn't make it very clear how size is computed (e.g. sizeof (uint64_t) vs sizeof (work) -- the former will have developers asking, "Why is sizeof (uint64_t) important when calculating size?").

@rkeene rkeene added this to the V18.0 milestone Aug 23, 2018

@zhyatt zhyatt added this to Unscheduled in V18 Dec 27, 2018

@cryptocode cryptocode moved this from Unscheduled to CP 1 (2018-01-09) in V18 Dec 27, 2018

@cryptocode cryptocode moved this from CP 1 (2018-01-09) to CP 2 (2018-01-16) in V18 Dec 28, 2018

@cryptocode cryptocode moved this from CP 2 (2018-01-16) to CP 1 (2018-01-09) in V18 Dec 28, 2018

@rkeene rkeene closed this in #1528 Jan 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.