[Easy] Bad tests in betting_tests #374
Comments
nathanielhourt
added a commit
to nathanielhourt/peerplays
that referenced
this issue
Aug 27, 2020
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.
nathanielhourt
added a commit
to nathanielhourt/peerplays
that referenced
this issue
Sep 2, 2020
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.
nathanielhourt
added a commit
to nathanielhourt/peerplays
that referenced
this issue
Oct 26, 2020
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
nathanielhourt
added a commit
to nathanielhourt/peerplays
that referenced
this issue
Nov 21, 2020
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
nathanielhourt
added a commit
to nathanielhourt/peerplays
that referenced
this issue
Nov 21, 2020
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
MichelSantos
pushed a commit
to MichelSantos/peerplays
that referenced
this issue
Nov 13, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Bug Description
The tests in
betting_tests.cppmake extensive use of the macros near the top of the file that set up a canned test scenario so that individual test cases can test various subsequent operations within that canned scenario.Unfortunately, these macros are improperly written leading to abuse of RAM and spurious test failures. The macros, over the course of several blocks, create several objects and store references to these objects for later use. This is not permitted: Graphene object references are invalidated at each block, so it is not valid to keep a reference (or pointer) across block boundaries. Instead, keep the ID, and look up the object by ID after each block.
These macros (and the tests) must be rewritten to store IDs rather than references.
Impacts
Describe which portion(s) of Peerplays may be impacted by this bug. Please tick at least one box.
PBSA / Developer tasks
The text was updated successfully, but these errors were encountered: