Add the query_txid_plugin as a option on the develop branch #1957

bijianing97 · 2019-08-23T08:50:20Z

The pull request is the solution for the #1954, I rebased on the current develop.

libraries/chain/include/graphene/chain/transaction_entry_object.hpp

libraries/plugins/CMakeLists.txt

libraries/plugins/query_txid/include/graphene/query_txid/query_txid_plugin.hpp

libraries/plugins/query_txid/query_txid_plugin.cpp

oxarbitrage

Looks good, i am going to test it and comment/review further.

In the meantime if you want to fix some minor style adjustments i sent will be great. Thanks!

libraries/plugins/query_txid/include/graphene/query_txid/query_txid_plugin.hpp

bijianing97 · 2019-08-26T02:08:05Z

ok,I will fix them and should I create a new pull request?

bijianing97 · 2019-08-26T06:54:07Z

I have fixed the above questions and hope you will like it.

libraries/chain/include/graphene/chain/transcation_entry_object.hpp

libraries/plugins/query_txid/query_txid_plugin.cpp

oxarbitrage · 2019-08-26T21:37:52Z

Thank you @bijianing97. Looking good.

I noticed that the new plugin files like https://github.com/bitshares/bitshares-core/blob/32ba6bbf1f2ee9b7b781ac7bf53d2eeae1c01cbf/libraries/plugins/query_txid/query_txid_plugin.cpp the indentation is 4 spaces while the Bitshares project use 3 (almost) everywhere. Will be good if you can change these.

I sent also a few more comments. Adding more commits into this pull request is fine and what you should do, by now your commits have all the same name. Will be good if next ones haves a more descriptive name at least to differentiate them.

Overall great work, i am downloading the requirements and testing further. Will send some more comments once there.

CMakeLists.txt

bijianing97 · 2019-08-28T12:51:56Z

OK，I will try to keep it in few days because i have other work to do

oxarbitrage · 2019-08-28T13:39:37Z

There are some compiler warnings about signed/unsigned comparisons that i think it should be easy to remove:

[ 64%] Building CXX object libraries/plugins/query_txid/CMakeFiles/graphene_query_txid.dir/query_txid_plugin.cpp.o
/home/oxarbitrage/CLionProjects/pull1957/bitshares-core/libraries/plugins/query_txid/query_txid_plugin.cpp: In member function ‘void graphene::query_txid::detail::query_txid_plugin_impl::collect_txid_index(const graphene::protocol::signed_block&)’:
/home/oxarbitrage/CLionProjects/pull1957/bitshares-core/libraries/plugins/query_txid/query_txid_plugin.cpp:102:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       for (auto idx = 0; idx < b.transactions.size(); idx++) {
                          ~~~~^~~~~~~~~~~~~~~~~~~~~~~
/home/oxarbitrage/CLionProjects/pull1957/bitshares-core/libraries/plugins/query_txid/query_txid_plugin.cpp: In member function ‘void graphene::query_txid::detail::query_txid_plugin_impl::consume_block()’:
/home/oxarbitrage/CLionProjects/pull1957/bitshares-core/libraries/plugins/query_txid/query_txid_plugin.cpp:128:21: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       while (number > limit_batch) {
              ~~~~~~~^~~~~~~~~~~~~
/home/oxarbitrage/CLionProjects/pull1957/bitshares-core/libraries/plugins/query_txid/query_txid_plugin.cpp:131:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
          for (auto idx = 0; idx < limit_batch; idx++) {
                             ~~~~^~~~~~~~~~~~~
/home/oxarbitrage/CLionProjects/pull1957/bitshares-core/libraries/plugins/query_txid/query_txid_plugin.cpp:149:21: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       if (backupnum > limit_batch)
           ~~~~~~~~~~^~~~~~~~~~~~~
[ 64%] Linking CXX static library libgraphene_query_txid.a
[ 64%] Built target graphene_query_txid

libraries/plugins/query_txid/query_txid_plugin.cpp

…into develop

bijianing97 · 2019-09-05T05:54:27Z

I add the plugin to .travis.yml and Dockerfile ,but failed in the docker ,i can’t see the details and modify it

pmconrad

Please add some unit tests. Need a separate test suite though that gets only built if the plugin is enabled.

.travis.yml

CMakeLists.txt

Dockerfile

pmconrad · 2019-09-05T06:03:35Z

libraries/app/database_api.cpp

@@ -1888,6 +1893,28 @@ std::string database_api::get_transaction_hex_without_sig(
 {
   return my->get_transaction_hex_without_sig(trx);
 }
+optional<query_trx_info> database_api_impl::get_transaction_by_txid(transaction_id_type txid)const
+{
+#ifdef QUERY_TXID_PLUGIN_ABLE


If the plugin is not enabled the method doesn't return anything. IMO it should throw.

pmconrad · 2019-09-05T06:05:14Z

libraries/app/database_api.cpp

+#ifdef QUERY_TXID_PLUGIN_ABLE
+    auto &txid_index = _db.get_index_type<trx_entry_index>().indices().get<by_txid>();
+    auto itor = txid_index.find(txid);
+    auto trx_entry = *itor;


*itor is undefined if itor == txid_index.end()

pmconrad · 2019-09-05T06:22:59Z

but failed in the docker ,i can’t see the details and modify it

Currently it fails because the container doesn't have wget.
But it makes no sense to implement this blindly, if you don't have a local docker installation for testing you can leave it out.

bijianing97 · 2019-09-06T10:29:56Z

I'm sorry that I don't know how to add some unit tests, could you give me some example, I will try to do it in few days.

pmconrad

Here's an example for a unit test suite: https://github.com/bitshares/bitshares-core/blob/3.3.1/tests/performance/performance_tests.cpp

pmconrad · 2019-09-06T12:41:31Z

libraries/app/CMakeLists.txt

                       graphene_chain fc graphene_db graphene_net graphene_utilities graphene_debug_witness )
 target_include_directories( graphene_app
                            PUBLIC "${CMAKE_CURRENT_SOURCE_DIR}/include"
-                            "${CMAKE_CURRENT_SOURCE_DIR}/../egenesis/include" )
+                            "${CMAKE_CURRENT_SOURCE_DIR}/../egenesis/include" 
+                            "${CMAKE_CURRENT_SOURCE_DIR}/../plugins/query_txid_object/include")


I think this is superfluous. The library dependency above should make the plugin include files available to app.

Now I get it. You need the include even when the plugin is disabled, because it declares the structure that is used in the return value of the API calls.

You can work around this by typedefing query_trx_info to some other known structure in that case (the API call should always throw an error, so the actual return type will not be used).

pmconrad · 2019-09-06T12:49:30Z

programs/witness_node/main.cpp

@@ -96,6 +100,9 @@ int main(int argc, char** argv) {
      auto es_objects_plug = node->register_plugin<es_objects::es_objects_plugin>();
      auto grouped_orders_plug = node->register_plugin<grouped_orders::grouped_orders_plugin>();
      auto api_helper_indexes_plug = node->register_plugin<api_helper_indexes::api_helper_indexes>();
+      #ifdef QUERY_TXID_PLUGIN_ABLE


Please let precompiler directives always start at the first column, don't indent them.

pmconrad · 2019-09-06T12:50:22Z

programs/build_helpers/build_and_test

@@ -4,7 +4,7 @@ set -e
 programs/build_helpers/buildstep -s 3500
 ccache -s
 programs/build_helpers/buildstep Prepare 1 "sed -i '/tests/d' libraries/fc/CMakeLists.txt"
-programs/build_helpers/buildstep cmake 5 "cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS=--coverage -DCMAKE_CXX_FLAGS=--coverage -DBoost_USE_STATIC_LIBS=OFF -DCMAKE_CXX_OUTPUT_EXTENSION_REPLACE=ON ."
+programs/build_helpers/buildstep cmake 5 "cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_FLAGS=--coverage -DCMAKE_CXX_FLAGS=--coverage -DBoost_USE_STATIC_LIBS=OFF -DCMAKE_CXX_OUTPUT_EXTENSION_REPLACE=ON -DLOAD_QUERY_TXID_PLUGIN=ON."


There's a space missing after =ON.

lelecommit added 2 commits August 23, 2019 15:25

Add the query_txid_plugin as a option on the develop branch

475a90a

Add the query_txid_plugin as a option on the develop branch

f28c61c

pmconrad mentioned this pull request Aug 23, 2019

Add the query_txid_plugin as a option #1954

Closed

Add the query_txid_plugin as a option on the develop branch

a16942d