Resolve #1506: Break protocol off into its own library #1737

nathanielhourt · 2019-04-26T19:52:13Z

This pull moves the chain/protocol folder into its own library, formalizing the separation between the protocol and the implementation. #1506

Notable architectural changes:

Remove xyz_object types from object_id<> template and replace with a mapping of object_id<> to xyz_object type which is defined in the header that defines xyz_object
Move object_id_type and object_id<> from db library to protocol, make db depend on protocol rather than vice versa

libraries/plugins/account_history/include/graphene/account_history/account_history_plugin.hpp

libraries/protocol/include/graphene/protocol/object_id.hpp

libraries/protocol/include/graphene/protocol/types.hpp

pmconrad · 2019-04-27T09:16:39Z

Docker build fails with

[ 60%] Building CXX object libraries/chain/CMakeFiles/graphene_chain.dir/db_notify.cpp.o
In file included from /bitshares-core/libraries/chain/db_notify.cpp:7:0:
/bitshares-core/libraries/chain/include/graphene/chain/withdraw_permission_object.hpp:43:41: error: ‘protocol_ids’ was not declared in this scope
static const uint8_t space_id = protocol_ids;
^
/bitshares-core/libraries/chain/include/graphene/chain/withdraw_permission_object.hpp:43:41: note: suggested alternative:
In file included from /bitshares-core/libraries/protocol/include/graphene/protocol/authority.hpp:25:0,
from /bitshares-core/libraries/chain/db_notify.cpp:3:
/bitshares-core/libraries/protocol/include/graphene/protocol/types.hpp:121:5: note: ‘protocol_ids’
protocol_ids = 1,
^

pmconrad · 2019-04-27T09:50:42Z

The travis build works. Both docker and travis are on Ubuntu-16.04 with gcc-5.4.x and boost-1.58.0.
Most notable difference is that travis builds with type Debug whereas docker builds with type Release.
I can reproduce the build failure locally with type RelWithDebInfo (my standard), on openSUSE with gcc-7.4 and boost-1.69. ~~Trying Debug build now.~~ Debug build fails just the same. I wonder why travis works.

nathanielhourt · 2019-05-01T19:04:36Z

OK, requested changes applied, and I think the build should be fixed.

Allows the more concise expression `object_downcast_t<xyz>` instead of the old `typename object_downcast<xyz>::type`

The ID types, object_id and object_id_type, were defined in the db library, and the protocol library depends on db to get these types. Technically, the ID types are defined by the protocol and used by the database, and not vice versa. Therefore these types should be in the protocol library, and db should depend on protocol to get them. This commit makes it so.

nathanielhourt · 2019-05-01T23:18:50Z

....aaaand Travis timed out....

libraries/protocol/include/graphene/protocol/fba.hpp

abitmore

Mostly looks good. Thanks a lot!

libraries/chain/evaluator.cpp

libraries/chain/include/graphene/chain/config.hpp

libraries/chain/include/graphene/chain/exceptions.hpp

libraries/chain/include/graphene/chain/types.hpp

libraries/chain/include/graphene/chain/special_authority.hpp

abitmore · 2019-05-04T11:15:04Z

libraries/protocol/include/graphene/protocol/config.hpp

+
+#define GRAPHENE_MAX_SIG_CHECK_DEPTH 2
+
+#define GRAPHENE_IRREVERSIBLE_THRESHOLD                      (70 * GRAPHENE_1_PERCENT)


IMHO this doesn't belong to protocol.

It determines fork validity... I think it would be interesting to a third party implementation. Otherwise they would have to replicate this number in their own project, or risk their implementations following a fork that the reference implementation won't follow.

The line between protocol and implementation is somewhat blurred. We have lots of consensus logic in the implementation, and all of it determines block validity.

I lean a bit towards @abitmore in this case. The threshold is only helpful for the local node, in that it defines any fork that goes back beyond LIB to be invalid. But an independent observer given two forks cannot decide which one is "real". Therefore the value is irrelevant wrt the protocol.

libraries/protocol/include/graphene/protocol/config.hpp

libraries/protocol/include/graphene/protocol/types.hpp

libraries/wallet/wallet.cpp

abitmore

See above.

Define macros to fully de-boilerplate ID type definitions. Externalities: - Rename transaction_object -> transaction_history_object - Rename impl_asset_dynamic_data_type -> impl_asset_dynamic_data_object_type - Rename impl_asset_bitasset_data_type -> impl_asset_bitasset_data_object_type The first is to avoid a naming collision on transaction_id_type, and the other two are to maintain consistency with the naming of the other types.

nathanielhourt · 2019-05-04T21:31:37Z

OK, I've made some macros that eliminate all the boilerplate around the ID types, enums, and typename reflection. How does that look?

abitmore · 2019-05-04T22:40:24Z

Looks good to me.
Restarted Travis.
Noticed some warnings when running tests, e.g.

1435270ms th_a reflect_util.hpp:51 clean_name ] don't know how to clean name: graphene::protocol::override_transfer_operation

Fixed.

nathanielhourt · 2019-05-04T23:20:40Z

Huh, never seen that before... Easy fix, though. :)

abitmore · 2019-05-05T00:00:22Z

cli_tests fails.

nathanielhourt · 2019-05-05T00:17:12Z

I have no idea how that happened... Oops!

pmconrad · 2019-05-05T12:03:35Z

Docker build timed out. Restarted. :-/

nathanielhourt · 2019-05-05T15:17:44Z

Did it time out again? At least travis finally passed.

.gitignore

abitmore

Looks good to me. Don't know why Docker Cloud failed.

pmconrad reviewed Apr 27, 2019

View reviewed changes

libraries/plugins/account_history/include/graphene/account_history/account_history_plugin.hpp Outdated Show resolved Hide resolved

pmconrad reviewed Apr 27, 2019

View reviewed changes

libraries/protocol/include/graphene/protocol/object_id.hpp Outdated Show resolved Hide resolved

pmconrad reviewed Apr 27, 2019

View reviewed changes

libraries/protocol/include/graphene/protocol/types.hpp Outdated Show resolved Hide resolved

pmconrad mentioned this pull request Apr 27, 2019

c++14 upgrade #847

Closed

abitmore added this to the 3.2.0 - Feature Release milestone Apr 28, 2019

nathanielhourt force-pushed the issue-1506 branch from 5ba55c7 to 6d51696 Compare May 1, 2019 18:49

nathanielhourt requested a review from pmconrad May 1, 2019 19:03

nathanielhourt and others added 10 commits May 1, 2019 16:04

Ref bitshares#1506: Isolate chain/protocol to its own library

eec1da5

Ref bitshares#1506: Add object_downcast_t

dae82b1

Allows the more concise expression `object_downcast_t<xyz>` instead of the old `typename object_downcast<xyz>::type`

Ref bitshares#1506: Isolate chain/protocol to its own library

90f33c1

Bump FC

38b4acd

Remove commented-out index code

c15b507

Wrap overlength line

ea4d28b

Remove unused key types

683f5d5

Probably fix Docker build

1405345

Fix build after rebase

1997b5c

nathanielhourt force-pushed the issue-1506 branch from 452c253 to 1997b5c Compare May 1, 2019 22:04

pmconrad suggested changes May 2, 2019

View reviewed changes

libraries/protocol/include/graphene/protocol/fba.hpp Show resolved Hide resolved

Ref bitshares#1506: Bumb DB_VERSION

d6a4c17

pmconrad previously approved these changes May 4, 2019

View reviewed changes

abitmore self-requested a review May 4, 2019 09:40

abitmore reviewed May 4, 2019

View reviewed changes

abitmore previously requested changes May 4, 2019

View reviewed changes

Ref bitshares#1506/bitshares#1737: Some requested changes

29d4dde

nathanielhourt dismissed pmconrad’s stale review via 29d4dde May 4, 2019 17:33

Ref bitshares#1506/bitshares#1737: Fix clean_name()

22bc1e2

Ref bitshares#1506/bitshares#1737: Oops

2c6af3f

pmconrad reviewed May 6, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Fix .gitignore

25e168e

nathanielhourt requested review from pmconrad and abitmore May 7, 2019 13:05

abitmore approved these changes May 7, 2019

View reviewed changes

nathanielhourt merged commit a779ea0 into bitshares:develop May 7, 2019

nathanielhourt deleted the issue-1506 branch May 7, 2019 18:49

abitmore mentioned this pull request May 7, 2019

Refactor protocol definitions into separate library #1506

Closed

13 tasks

pmconrad mentioned this pull request Jun 20, 2019

Release Notes: BitShares Core 3.2.0 #1808

Closed

oxarbitrage mentioned this pull request Nov 13, 2019

[GRPH 92] Build optimization peerplays-network/peerplays#137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve #1506: Break protocol off into its own library #1737

Resolve #1506: Break protocol off into its own library #1737

nathanielhourt commented Apr 26, 2019 •

edited by abitmore

Loading

pmconrad commented Apr 27, 2019

pmconrad commented Apr 27, 2019 •

edited

Loading

nathanielhourt commented May 1, 2019

nathanielhourt commented May 1, 2019

abitmore left a comment

abitmore May 4, 2019

nathanielhourt May 4, 2019

pmconrad May 5, 2019

abitmore left a comment

nathanielhourt commented May 4, 2019

abitmore commented May 4, 2019 •

edited

Loading

nathanielhourt commented May 4, 2019

abitmore commented May 5, 2019

nathanielhourt commented May 5, 2019

pmconrad commented May 5, 2019

nathanielhourt commented May 5, 2019

abitmore left a comment


		#define GRAPHENE_MAX_SIG_CHECK_DEPTH 2

		#define GRAPHENE_IRREVERSIBLE_THRESHOLD (70 * GRAPHENE_1_PERCENT)

Resolve #1506: Break protocol off into its own library #1737

Resolve #1506: Break protocol off into its own library #1737

Conversation

nathanielhourt commented Apr 26, 2019 • edited by abitmore Loading

pmconrad commented Apr 27, 2019

pmconrad commented Apr 27, 2019 • edited Loading

nathanielhourt commented May 1, 2019

nathanielhourt commented May 1, 2019

abitmore left a comment

Choose a reason for hiding this comment

abitmore May 4, 2019

Choose a reason for hiding this comment

nathanielhourt May 4, 2019

Choose a reason for hiding this comment

pmconrad May 5, 2019

Choose a reason for hiding this comment

abitmore left a comment

Choose a reason for hiding this comment

nathanielhourt commented May 4, 2019

abitmore commented May 4, 2019 • edited Loading

nathanielhourt commented May 4, 2019

abitmore commented May 5, 2019

nathanielhourt commented May 5, 2019

pmconrad commented May 5, 2019

nathanielhourt commented May 5, 2019

abitmore left a comment

Choose a reason for hiding this comment

nathanielhourt commented Apr 26, 2019 •

edited by abitmore

Loading

pmconrad commented Apr 27, 2019 •

edited

Loading

abitmore commented May 4, 2019 •

edited

Loading