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

Block builder #1451

Merged
merged 5 commits into from Dec 28, 2018

Conversation

Projects
4 participants
@cryptocode
Copy link
Collaborator

commented Dec 14, 2018

Implements a builder pattern for all block types.

@cryptocode cryptocode force-pushed the cryptocode:improvement/block-builders branch 2 times, most recently from 4ca7027 to ba1b4cd Dec 14, 2018

@rkeene rkeene added this to the V18.0 milestone Dec 17, 2018

@rkeene rkeene requested review from rkeene and SergiySW Dec 18, 2018

@rkeene

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

What if someone does something like:

auto block = builder.state().clear().sign(key.prv, key.pub).balance(1).build();

Maybe an assert that can catch these kinds of errors, or nullify members which become invalid after updating ?

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 18, 2018

@rkeene The sign function returns a base class type so attempted to call balance will be a compile error.

The param-less build() will assert if there's an internal error. We just have to decide what those errors should be. It's currently decoding errors. To catch your example, we might have to track some state. I'm fine with that, just need a list of things to catch.

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2018

Can we use sentinel values to pack unset/invalid_decoding information in the value itself? We should have a way to query can_build if the block has all the appropriate items set, based on its type. I think build() should assert if can_build is false and make it a precondition to being called.

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 20, 2018

The whole domain for some values (like destination pubkey) seems to be valid so no sentinel will do IIUC. Could we use an uint8_t bitmap in the builder which is updated while building, and then validated on build() ?

edit: Actually, the std::error_code might be usable for this while building to save space.

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2018

Since it's a 256bit value , if we use a random internal sentinel it shouldn't be a problem. Globals create random values for not-a-block or not-an-account.

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 20, 2018

@clemahieu that makes sense, thanks!

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 20, 2018

There are work (64-bit) and amount (128-bit) values too though. The 64-bit one is in the realm of collisions (maybe max-1 works? 0 is used in tests)

@cryptocode

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 21, 2018

After discussing it a bit, currently reworking this to use builder state instead of sentinels. This allows for fast validation through per-block-type masks.

edit: updated

@cryptocode cryptocode force-pushed the cryptocode:improvement/block-builders branch from ba1b4cd to 53338dd Dec 21, 2018

@cryptocode cryptocode force-pushed the cryptocode:improvement/block-builders branch from 53338dd to 80e57d0 Dec 21, 2018

cryptocode added some commits Dec 21, 2018

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

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

@rkeene

rkeene approved these changes Dec 28, 2018

Copy link
Contributor

left a comment

Looks good to me !

@rkeene rkeene merged commit 5fb4617 into nanocurrency:master Dec 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cryptocode cryptocode deleted the cryptocode:improvement/block-builders branch Dec 29, 2018

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

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.