Refactor protocol definitions into separate library #1506
Comments
pmconrad
added
1b User Story
|
I agree with this. It is legal right now to include for example an object definition into a protocol header file for whatever reason. I only realized this was not a good idea after seeing this was not done anywhere else. |
|
Of course it is possible, I've (unwittingly) done it. Fortunately, it didn't pass code review. This simple change helps developers see the lines between components. |
|
In principle I agree that this is a good move. If this is done, the only downside I can think of, is it's harder to track old changes done on the relocated files. |
|
I too support any effort to refactor our code to be more readable and reinforce best practices. I notice this received the |
pmconrad
added
good first issue
2c Ready for Development
|
I think it requires more knowledge of cmake and build processes than of the internals of our codebase. Added the "good first issue" label. |
|
I'll take this issue, if no one else already wants it. I think it shouldn't be too difficult, but I'm going to explore some other changes simultaneously that I think are worth doing, most notably:
As we work to further modularize Bitshares, I envision a proliferation of independently building subprojects, which most or all depend on FC. I would like to see these projects live in separate repos with independent build systems, as placing them all in a single repo with a single build system allows the pieces to interact in less well-defined ways, which somewhat defeats the purpose of modularization. We may still have a parent project that connects all the subprojects together for a single build, but the subprojects shouldn't require this. Having FC as a submodule of each of these subprojects is clearly undesirable, and having all of the subprojects depend on an overarching parent project/build which has an FC submodule is, again, purpose defeating. The above discussion takes a somewhat mid- to long-term view, but in the shorter term, we want to break out the protocol library, which requires FC, as too will the main bitshares repo. So treating FC as an external dependency becomes relevant now and paves the road for further modularization later on. As to the conversion from CMake to Meson, perhaps I will initially create both build systems for the protocol library to allow for a direct side-by-side comparison at small scale, at which point I predict the advantages of Meson to be apparent. Thoughts? |
|
So far I find the following violations of the rule that files in
So... I can resolve the The
I'm now being inundated with thousands of errors to do with the various This opens the question: if we bust out the Has there been any serious inquiry into switching to chainbase? |
|
IMO keeping FC as a submodule keeps the build process simpler than making it an external dependency. (I had a separate fc RPM package in my openSUSE repo, with the idea that it could be used in multiple graphene-based builds. It turned out that each graphene package required its own specific version of fc, which completely defeated the purpose. I'm now building it inline with my current BTS package.) Not familiar with Meson - what would be the advantage?
I think A The Currently I don't see any advantages in moving to a different internal database. My experience with chainbase in particular is that its performance goes down the drain when its size exceeds physical RAM, which makes it quite pointless IMO. Edit: the evil includes in |
|
I assigned @nathanhourt and attempted to pull out a task list from the previous comments, now within the Description. I'll work with @nathanhourt directly about how we can estimate/update as we progress. I read some Thanks! |
|
The results are fairly unanimous on chainbase, nobody likes it, so we don't run with it. I'll just make I'm also thinking that reorganizing the build system is probably biting off more than I should chew for this particular issue. I maintain that it would be beneficial, but it's not as high a priority as just moving the protocol to its own library. |
|
Regarding replacing CMAKE: I'll remove it from the Task List here. I'll attempt to create a new Issue for it specifically so we retain it, but mark it low. Added #1568 |
|
Alright, so here's a question: all of the Funny part about this is, these types are only used for template magic; they have no impact on the compiled code really, so as far as the So I can see three alternative solutions to this:
The first two solutions suck as they violate the separation of implementation and protocol... Either way, the The last solution is the "correct" solution, but IIRC this is a monumental task, if it's possible at all. The So I'm requesting input here... Should I just kludge the namespaces? Or am I overestimating the difficulty of moving the protocol-level objects to the |
|
Additional note: I've already made a |
|
Hm, tricky. Side note: I recently learned that the object_id_types raw-serialize only the instance number, not the space nor type id. So technically the database ID (i. e. the tuple I can't estimate how much work solution 3 would be. IMO we should use a solution that requires not too much effort. IMO either solution is fine as a first step and can be improved upon later, if we feel the effort is justified. Actually wrt to the side node above, the ultimate separation would be to use a different ID type at the protocol level that has the same serialization as an object_id_type and can easily be converted from/to database IDs as needed. Not sure if it's worth it. |
I like it. I'll see if it plays |
|
sigh You're right, I get ahead of myself. Replacing fc, even if just in But swapping the dependency from |
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.
|
That was an amazingly trivial diff for how much cleaner it makes things conceptually. OK, I'll call this ready to merge once we get the fc change merged (or figure out how to render it irrelevant) |
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.
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.
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.
Resolve #1506: Break protocol off into its own library
|
Fixed by #1737. |
This adds the most important updates to Graphene from BitShares. Most notably, bitshares/bitshares-core#1506 Second most notably, it updates Peerplays' FC to be in sync with BitShares FC. This is a squash commit of several subcommits. The subcommit messages are reproduced below: Replace fc::uint128 with boost::multiprecision::uint128_t replace smart_ref with shared_ptr Fixes/Remove Unused Remove NTP time Remove old macro This macro is now in FC, so no need to define it here anymore Replaced fc::array with std::array Separate exception declaration and implementation Adapted to fc promise changes Fixes Add back in some of Peter's fixes that got lost in the cherry pick _hash endianness fixes Remove all uses of fc/smart_ref It's gone, can't use it anymore Replace improper static_variant operator overloads with comparators Fixes Remove boost::signals from build system; it's header-only so it's not listed in cmake anymore. Also remove some unused hashing code Impl. pack/unpack functions for extension class Ref #1506: Isolate chain/protocol to its own library Ref #1506: Add object_downcast_t Allows the more concise expression `object_downcast_t<xyz>` instead of the old `typename object_downcast<xyz>::type` Ref #1506: Move ID types from db to protocol 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. Ref #1506: Isolate chain/protocol to its own library Remove commented-out index code Wrap overlength line Remove unused key types Probably fix Docker build Fix build after rebase Ref #1506/#1737: Some requested changes Ref #1506/#1737: Macro-fy ID type definitions 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. Ref #1506/#1737: Fix clean_name() Ref #1506/#1737: Oops Fix .gitignore Externalized serialization in protocol library Fix compile sets Delete a couple of ghost files that were in the tree but not part of the project (I accidentally added them to CMakeLists while merging, but they're broken and not part of the Peerplays code), and add several files that got dropped from the build during merge. General fixes Fix warnings, build issues, unused code, etc. Fix #1772 by decprecating cli_wallet -H More fixes Fix errors and warnings and generally coax it to build Fix test I'm pretty sure this didn't break from what I did... But I can't build the original code, so I can't tell. Anyways, this one now passes... Others still fail... Small fix Fix crash in auth checks Final fixes Last round of fixes following the rebase to Beatrice Rename project in CMakeLists.txt The CMakeLists.txt declared this project as BitShares and not Peerplays, which makes it confusing in IDEs. Rename it to be clear which project is open. Resolve peerplays-network#374 Replace all object refs in macros with IDs, and fix affected tests to look up objects by ID rather than using invalidated refs. A full audit of all tests should be performed to eliminate any further usage of invalidated object references. Resolve peerplays-network#373: Add object notifiers Various fixes Fixes to various issues, primarily reflections, that cropped up during merge conflict resolution Fix startup bug in Bookie plugin Bookie plugin was preventing the ndoe from starting up because it registered its secondary indexes to create objects in its own primary indexes to track objects being created in other primary indexes, and did so during its `initialize()` step, which is to say, before the database was loaded from disk at startup. This caused the secondary indexes to create tracker objects when the observed indexes were loading objects from disk. This then caused a failure when these tracker indexes were later loaded from disk, and the first object IDs collided. This is fixed by refraining from defining secondary indexes until the `startup()` stage rather than the `initialize()` stage. Primary indexes are registered in `initialize()`, secondary indexes are registered in `startup()`. I have no idea how this was working before I got here... Fix egenesis install Fixes after updates Rebase on updated develop branch and fix conflicts
This adds the most important updates to Graphene from BitShares. Most notably, bitshares/bitshares-core#1506 Second most notably, it updates Peerplays' FC to be in sync with BitShares FC. This is a squash commit of several subcommits. The subcommit messages are reproduced below: Replace fc::uint128 with boost::multiprecision::uint128_t replace smart_ref with shared_ptr Fixes/Remove Unused Remove NTP time Remove old macro This macro is now in FC, so no need to define it here anymore Replaced fc::array with std::array Separate exception declaration and implementation Adapted to fc promise changes Fixes Add back in some of Peter's fixes that got lost in the cherry pick _hash endianness fixes Remove all uses of fc/smart_ref It's gone, can't use it anymore Replace improper static_variant operator overloads with comparators Fixes Remove boost::signals from build system; it's header-only so it's not listed in cmake anymore. Also remove some unused hashing code Impl. pack/unpack functions for extension class Ref #1506: Isolate chain/protocol to its own library Ref #1506: Add object_downcast_t Allows the more concise expression `object_downcast_t<xyz>` instead of the old `typename object_downcast<xyz>::type` Ref #1506: Move ID types from db to protocol 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. Ref #1506: Isolate chain/protocol to its own library Remove commented-out index code Wrap overlength line Remove unused key types Probably fix Docker build Fix build after rebase Ref #1506/#1737: Some requested changes Ref #1506/#1737: Macro-fy ID type definitions 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. Ref #1506/#1737: Fix clean_name() Ref #1506/#1737: Oops Fix .gitignore Externalized serialization in protocol library Fix compile sets Delete a couple of ghost files that were in the tree but not part of the project (I accidentally added them to CMakeLists while merging, but they're broken and not part of the Peerplays code), and add several files that got dropped from the build during merge. General fixes Fix warnings, build issues, unused code, etc. Fix #1772 by decprecating cli_wallet -H More fixes Fix errors and warnings and generally coax it to build Fix test I'm pretty sure this didn't break from what I did... But I can't build the original code, so I can't tell. Anyways, this one now passes... Others still fail... Small fix Fix crash in auth checks Final fixes Last round of fixes following the rebase to Beatrice Rename project in CMakeLists.txt The CMakeLists.txt declared this project as BitShares and not Peerplays, which makes it confusing in IDEs. Rename it to be clear which project is open. Resolve peerplays-network#374 Replace all object refs in macros with IDs, and fix affected tests to look up objects by ID rather than using invalidated refs. A full audit of all tests should be performed to eliminate any further usage of invalidated object references. Resolve peerplays-network#373: Add object notifiers Various fixes Fixes to various issues, primarily reflections, that cropped up during merge conflict resolution Fix startup bug in Bookie plugin Bookie plugin was preventing the node from starting up because it registered its secondary indexes to create objects in its own primary indexes to track objects being created in other primary indexes, and did so during its `initialize()` step, which is to say, before the database was loaded from disk at startup. This caused the secondary indexes to create tracker objects when the observed indexes were loading objects from disk. This then caused a failure when these tracker indexes were later loaded from disk, and the first object IDs collided. This is fixed by refraining from defining secondary indexes until the `startup()` stage rather than the `initialize()` stage. Primary indexes are registered in `initialize()`, secondary indexes are registered in `startup()`. This also involved adding a new method, "add_secondary_index()", to `object_database`, as before there was no way to do this because you couldn't get a non-const index from a non-const database. I have no idea how this was working before I got here... Fix egenesis install Fixes after updates Rebase on updated develop branch and fix conflicts
This adds the most important updates to Graphene from BitShares. Most notably, bitshares/bitshares-core#1506 Second most notably, it updates Peerplays' FC to be in sync with BitShares FC. This is a squash commit of several subcommits. The subcommit messages are reproduced below: Replace fc::uint128 with boost::multiprecision::uint128_t replace smart_ref with shared_ptr Fixes/Remove Unused Remove NTP time Remove old macro This macro is now in FC, so no need to define it here anymore Replaced fc::array with std::array Separate exception declaration and implementation Adapted to fc promise changes Fixes Add back in some of Peter's fixes that got lost in the cherry pick _hash endianness fixes Remove all uses of fc/smart_ref It's gone, can't use it anymore Replace improper static_variant operator overloads with comparators Fixes Remove boost::signals from build system; it's header-only so it's not listed in cmake anymore. Also remove some unused hashing code Impl. pack/unpack functions for extension class Ref #1506: Isolate chain/protocol to its own library Ref #1506: Add object_downcast_t Allows the more concise expression `object_downcast_t<xyz>` instead of the old `typename object_downcast<xyz>::type` Ref #1506: Move ID types from db to protocol 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. Ref #1506: Isolate chain/protocol to its own library Remove commented-out index code Wrap overlength line Remove unused key types Probably fix Docker build Fix build after rebase Ref #1506/#1737: Some requested changes Ref #1506/#1737: Macro-fy ID type definitions 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. Ref #1506/#1737: Fix clean_name() Ref #1506/#1737: Oops Fix .gitignore Externalized serialization in protocol library Fix compile sets Delete a couple of ghost files that were in the tree but not part of the project (I accidentally added them to CMakeLists while merging, but they're broken and not part of the Peerplays code), and add several files that got dropped from the build during merge. General fixes Fix warnings, build issues, unused code, etc. Fix #1772 by decprecating cli_wallet -H More fixes Fix errors and warnings and generally coax it to build Fix test I'm pretty sure this didn't break from what I did... But I can't build the original code, so I can't tell. Anyways, this one now passes... Others still fail... Small fix Fix crash in auth checks Final fixes Last round of fixes following the rebase to Beatrice Rename project in CMakeLists.txt The CMakeLists.txt declared this project as BitShares and not Peerplays, which makes it confusing in IDEs. Rename it to be clear which project is open. Resolve peerplays-network#374 Replace all object refs in macros with IDs, and fix affected tests to look up objects by ID rather than using invalidated refs. A full audit of all tests should be performed to eliminate any further usage of invalidated object references. Resolve peerplays-network#373: Add object notifiers Various fixes Fixes to various issues, primarily reflections, that cropped up during merge conflict resolution Fix startup bug in Bookie plugin Bookie plugin was preventing the node from starting up because it registered its secondary indexes to create objects in its own primary indexes to track objects being created in other primary indexes, and did so during its `initialize()` step, which is to say, before the database was loaded from disk at startup. This caused the secondary indexes to create tracker objects when the observed indexes were loading objects from disk. This then caused a failure when these tracker indexes were later loaded from disk, and the first object IDs collided. This is fixed by refraining from defining secondary indexes until the `startup()` stage rather than the `initialize()` stage. Primary indexes are registered in `initialize()`, secondary indexes are registered in `startup()`. This also involved adding a new method, "add_secondary_index()", to `object_database`, as before there was no way to do this because you couldn't get a non-const index from a non-const database. I have no idea how this was working before I got here... Fix egenesis install Fixes after updates Rebase on updated develop branch and fix conflicts
This adds the most important updates to Graphene from BitShares. Most notably, bitshares/bitshares-core#1506 Second most notably, it updates Peerplays' FC to be in sync with BitShares FC. This is a squash commit of several subcommits. The subcommit messages are reproduced below: Replace fc::uint128 with boost::multiprecision::uint128_t replace smart_ref with shared_ptr Fixes/Remove Unused Remove NTP time Remove old macro This macro is now in FC, so no need to define it here anymore Replaced fc::array with std::array Separate exception declaration and implementation Adapted to fc promise changes Fixes Add back in some of Peter's fixes that got lost in the cherry pick _hash endianness fixes Remove all uses of fc/smart_ref It's gone, can't use it anymore Replace improper static_variant operator overloads with comparators Fixes Remove boost::signals from build system; it's header-only so it's not listed in cmake anymore. Also remove some unused hashing code Impl. pack/unpack functions for extension class Ref #1506: Isolate chain/protocol to its own library Ref #1506: Add object_downcast_t Allows the more concise expression `object_downcast_t<xyz>` instead of the old `typename object_downcast<xyz>::type` Ref #1506: Move ID types from db to protocol 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. Ref #1506: Isolate chain/protocol to its own library Remove commented-out index code Wrap overlength line Remove unused key types Probably fix Docker build Fix build after rebase Ref #1506/#1737: Some requested changes Ref #1506/#1737: Macro-fy ID type definitions 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. Ref #1506/#1737: Fix clean_name() Ref #1506/#1737: Oops Fix .gitignore Externalized serialization in protocol library Fix compile sets Delete a couple of ghost files that were in the tree but not part of the project (I accidentally added them to CMakeLists while merging, but they're broken and not part of the Peerplays code), and add several files that got dropped from the build during merge. General fixes Fix warnings, build issues, unused code, etc. Fix #1772 by decprecating cli_wallet -H More fixes Fix errors and warnings and generally coax it to build Fix test I'm pretty sure this didn't break from what I did... But I can't build the original code, so I can't tell. Anyways, this one now passes... Others still fail... Small fix Fix crash in auth checks Final fixes Last round of fixes following the rebase to Beatrice Rename project in CMakeLists.txt The CMakeLists.txt declared this project as BitShares and not Peerplays, which makes it confusing in IDEs. Rename it to be clear which project is open. Resolve peerplays-network#374 Replace all object refs in macros with IDs, and fix affected tests to look up objects by ID rather than using invalidated refs. A full audit of all tests should be performed to eliminate any further usage of invalidated object references. Resolve peerplays-network#373: Add object notifiers Various fixes Fixes to various issues, primarily reflections, that cropped up during merge conflict resolution Fix startup bug in Bookie plugin Bookie plugin was preventing the node from starting up because it registered its secondary indexes to create objects in its own primary indexes to track objects being created in other primary indexes, and did so during its `initialize()` step, which is to say, before the database was loaded from disk at startup. This caused the secondary indexes to create tracker objects when the observed indexes were loading objects from disk. This then caused a failure when these tracker indexes were later loaded from disk, and the first object IDs collided. This is fixed by refraining from defining secondary indexes until the `startup()` stage rather than the `initialize()` stage. Primary indexes are registered in `initialize()`, secondary indexes are registered in `startup()`. This also involved adding a new method, "add_secondary_index()", to `object_database`, as before there was no way to do this because you couldn't get a non-const index from a non-const database. I have no idea how this was working before I got here... Fix egenesis install Fixes after updates Rebase on updated develop branch and fix conflicts
User Story
As a software architect I want to enforce a clear separation of the BitShares protocol definition and the BitShares Core implementation.
The
libraries/chaindirectory currently contains aprotocolsubdirectory, which forms a weak architectural boundary. This is also reflected in a separateprotocolsubdirectory for the relevant include files. It is an unwritten (?) rule that code in theprotocoldirectory must not depend on code or definitions from the upper levels. This rule can easily be violated. It should therefore be enforced by moving the protocol-specific files into a separate library of their own.(Steem did the same a long time ago.)
Impacts
Software architecture
CORE TEAM TASK LIST
protocoldbThe text was updated successfully, but these errors were encountered: