From fe64b22d6b49762c6049296caac88ce428ed233e Mon Sep 17 00:00:00 2001
From: Lin Huang <lin.huang@eosnetwork.com>
Date: Wed, 3 May 2023 20:58:47 -0400
Subject: [PATCH 1/5] Do not check execution window related configurations if
 --read-only-threads is not configured; better error message for invalid
 --read-only-write-window-time-us

---
 plugins/producer_plugin/producer_plugin.cpp | 36 ++++++++++-----------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp
index 0bc4b1a91c..fad678deba 100644
--- a/plugins/producer_plugin/producer_plugin.cpp
+++ b/plugins/producer_plugin/producer_plugin.cpp
@@ -1126,6 +1126,7 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_
       }
    }
    EOS_ASSERT( test_mode_ || my->_ro_thread_pool_size == 0 || my->_producers.empty(), plugin_config_exception, "--read-only-threads not allowed on producer node" );
+
    // only initialize other read-only options when read-only thread pool is enabled
    if ( my->_ro_thread_pool_size > 0 ) {
 #ifdef EOSIO_EOS_VM_OC_RUNTIME_ENABLED
@@ -1159,26 +1160,25 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_
       }
 #endif
       EOS_ASSERT( my->_ro_thread_pool_size <= my->_ro_max_threads_allowed, plugin_config_exception, "--read-only-threads (${th}) greater than number of threads allowed (${allowed})", ("th", my->_ro_thread_pool_size) ("allowed", my->_ro_max_threads_allowed) );
-   }
-
-   my->_ro_write_window_time_us = fc::microseconds( options.at( "read-only-write-window-time-us" ).as<uint32_t>() );
-   my->_ro_read_window_time_us = fc::microseconds( options.at( "read-only-read-window-time-us" ).as<uint32_t>() );
-   EOS_ASSERT( my->_ro_read_window_time_us > my->_ro_read_window_minimum_time_us, plugin_config_exception, "minimum of --read-only-read-window-time-us (${read}) must be ${min} microseconds", ("read", my->_ro_read_window_time_us) ("min", my->_ro_read_window_minimum_time_us) );
-   my->_ro_read_window_effective_time_us = my->_ro_read_window_time_us - my->_ro_read_window_minimum_time_us;
 
-   // Make sure a read-only transaction can finish within the read
-   // window if scheduled at the very beginning of the window.
-   // Use _ro_read_window_effective_time_us instead of _ro_read_window_time_us
-   // for safety margin
-   if ( my->_max_transaction_time_ms.load() > 0 ) {
-      EOS_ASSERT( my->_ro_read_window_effective_time_us > fc::milliseconds(my->_max_transaction_time_ms.load()), plugin_config_exception, "--read-only-read-window-time-us (${read}) must be greater than --max-transaction-time ${trx_time} ms plus a margin of ${min} us", ("read", my->_ro_read_window_time_us) ("trx_time", my->_max_transaction_time_ms.load()) ("min", my->_ro_read_window_minimum_time_us) );
-      my->_ro_max_trx_time_us = fc::milliseconds(my->_max_transaction_time_ms.load());
-   } else {
-      // _max_transaction_time_ms can be set to negative in testing (for unlimited)
-      my->_ro_max_trx_time_us = my->_ro_read_window_effective_time_us;
+      my->_ro_write_window_time_us = fc::microseconds( options.at( "read-only-write-window-time-us" ).as<uint32_t>() );
+      my->_ro_read_window_time_us = fc::microseconds( options.at( "read-only-read-window-time-us" ).as<uint32_t>() );
+      EOS_ASSERT( my->_ro_read_window_time_us > my->_ro_read_window_minimum_time_us, plugin_config_exception, "minimum of --read-only-read-window-time-us (${read}) must be ${min} microseconds", ("read", my->_ro_read_window_time_us) ("min", my->_ro_read_window_minimum_time_us) );
+      my->_ro_read_window_effective_time_us = my->_ro_read_window_time_us - my->_ro_read_window_minimum_time_us;
+
+      // Make sure a read-only transaction can finish within the read
+      // window if scheduled at the very beginning of the window.
+      // Add _ro_read_window_minimum_time_us for safety margin.
+      if ( my->_max_transaction_time_ms.load() > 0 ) {
+         EOS_ASSERT( my->_ro_read_window_time_us > ( fc::milliseconds(my->_max_transaction_time_ms.load()) + my->_ro_read_window_minimum_time_us ), plugin_config_exception, "--read-only-read-window-time-us (${read} us) must be greater than --max-transaction-time (${trx_time} us) plus ${min} us, required: ${read} us > (${trx_time} us + ${min} us).", ("read", my->_ro_read_window_time_us) ("trx_time", my->_max_transaction_time_ms.load() * 1000) ("min", my->_ro_read_window_minimum_time_us) );
+         my->_ro_max_trx_time_us = fc::milliseconds(my->_max_transaction_time_ms.load());
+      } else {
+         // _max_transaction_time_ms can be set to negative in testing (for unlimited)
+         my->_ro_max_trx_time_us = my->_ro_read_window_effective_time_us;
+      }
+      ilog("ro_thread_pool_size ${s}, ro_write_window_time_us ${ww}, ro_read_window_time_us ${rw}, ro_max_trx_time_us ${t}, ro_read_window_effective_time_us ${w}",
+           ("s", my->_ro_thread_pool_size)("ww", my->_ro_write_window_time_us)("rw", my->_ro_read_window_time_us)("t", my->_ro_max_trx_time_us)("w", my->_ro_read_window_effective_time_us));
    }
-   ilog("ro_thread_pool_size ${s}, ro_write_window_time_us ${ww}, ro_read_window_time_us ${rw}, ro_max_trx_time_us ${t}, ro_read_window_effective_time_us ${w}",
-        ("s", my->_ro_thread_pool_size)("ww", my->_ro_write_window_time_us)("rw", my->_ro_read_window_time_us)("t", my->_ro_max_trx_time_us)("w", my->_ro_read_window_effective_time_us));
 
    my->_incoming_block_sync_provider = app().get_method<incoming::methods::block_sync>().register_provider(
          [this](const signed_block_ptr& block, const std::optional<block_id_type>& block_id, const block_state_ptr& bsp) {

From ce0260f8105dddc0fb4ae4bd3d171721fe47c07d Mon Sep 17 00:00:00 2001
From: Lin Huang <lin.huang@eosnetwork.com>
Date: Wed, 3 May 2023 21:02:06 -0400
Subject: [PATCH 2/5] test case for read-only trx related options not validated
 if read-only-threads is 0

---
 .../producer_plugin/test/test_read_only_trx.cpp  | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/plugins/producer_plugin/test/test_read_only_trx.cpp b/plugins/producer_plugin/test/test_read_only_trx.cpp
index 4717c939b9..da32cd1a7d 100644
--- a/plugins/producer_plugin/test/test_read_only_trx.cpp
+++ b/plugins/producer_plugin/test/test_read_only_trx.cpp
@@ -50,7 +50,8 @@ auto make_unique_trx( const chain_id_type& chain_id ) {
 
 BOOST_AUTO_TEST_SUITE(read_only_trxs)
 
-void error_handling_common(std::vector<const char*>& specific_args) {
+// app_init_status: true -- succeeded, false -- failed
+void test_configs_common(std::vector<const char*>& specific_args, bool app_init_status) {
    appbase::scoped_app app;
    fc::temp_directory temp;
    auto temp_dir_str = temp.path().string();
@@ -59,19 +60,26 @@ void error_handling_common(std::vector<const char*>& specific_args) {
    std::vector<const char*> argv =
       {"test", "--data-dir", temp_dir_str.c_str(), "--config-dir", temp_dir_str.c_str()};
    argv.insert( argv.end(), specific_args.begin(), specific_args.end() );
-   BOOST_CHECK_EQUAL( app->initialize<producer_plugin>( argv.size(), (char**) &argv[0]), false );
+   BOOST_CHECK_EQUAL( app->initialize<producer_plugin>( argv.size(), (char**) &argv[0]), app_init_status );
 }
 
 // --read-only-thread not allowed on producer node
 BOOST_AUTO_TEST_CASE(read_only_on_producer) {
    std::vector<const char*> specific_args = {"-p", "eosio", "-e", "--read-only-threads", "2" };
-   error_handling_common(specific_args);
+   test_configs_common(specific_args, false /* failed */);
 }
 
 // read_window_time must be greater than max_transaction_time + 10ms
 BOOST_AUTO_TEST_CASE(invalid_read_window_time) {
    std::vector<const char*> specific_args = { "--read-only-threads", "2", "--max-transaction-time", "10", "--read-only-write-window-time-us", "50000", "--read-only-read-window-time-us", "20000" }; // 20000 not greater than --max-transaction-time (10ms) + 10000us (minimum margin)
-   error_handling_common(specific_args);
+   test_configs_common(specific_args, false /* failed */);
+}
+
+// if --read-only-threads is not configured, read-only trx related configs should
+// not be checked
+BOOST_AUTO_TEST_CASE(not_check_configs_if_no_read_only_threads) {
+   std::vector<const char*> specific_args = { "--max-transaction-time", "10", "--read-only-write-window-time-us", "50000", "--read-only-read-window-time-us", "20000" }; // 20000 not greater than --max-transaction-time (10ms) + 10000us (minimum margin)
+   test_configs_common(specific_args, true /* succeeded  */);
 }
 
 void test_trxs_common(std::vector<const char*>& specific_args) {

From 4e98b2e9ded3e67d51dbd564ab5685139c410597 Mon Sep 17 00:00:00 2001
From: Lin Huang <lin.huang@eosnetwork.com>
Date: Thu, 4 May 2023 09:32:42 -0400
Subject: [PATCH 3/5] make sure _ro_max_trx_time_us is always initialized

---
 plugins/producer_plugin/producer_plugin.cpp | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp
index fad678deba..31e83c498e 100644
--- a/plugins/producer_plugin/producer_plugin.cpp
+++ b/plugins/producer_plugin/producer_plugin.cpp
@@ -1171,14 +1171,20 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_
       // Add _ro_read_window_minimum_time_us for safety margin.
       if ( my->_max_transaction_time_ms.load() > 0 ) {
          EOS_ASSERT( my->_ro_read_window_time_us > ( fc::milliseconds(my->_max_transaction_time_ms.load()) + my->_ro_read_window_minimum_time_us ), plugin_config_exception, "--read-only-read-window-time-us (${read} us) must be greater than --max-transaction-time (${trx_time} us) plus ${min} us, required: ${read} us > (${trx_time} us + ${min} us).", ("read", my->_ro_read_window_time_us) ("trx_time", my->_max_transaction_time_ms.load() * 1000) ("min", my->_ro_read_window_minimum_time_us) );
-         my->_ro_max_trx_time_us = fc::milliseconds(my->_max_transaction_time_ms.load());
-      } else {
-         // _max_transaction_time_ms can be set to negative in testing (for unlimited)
-         my->_ro_max_trx_time_us = my->_ro_read_window_effective_time_us;
       }
-      ilog("ro_thread_pool_size ${s}, ro_write_window_time_us ${ww}, ro_read_window_time_us ${rw}, ro_max_trx_time_us ${t}, ro_read_window_effective_time_us ${w}",
-           ("s", my->_ro_thread_pool_size)("ww", my->_ro_write_window_time_us)("rw", my->_ro_read_window_time_us)("t", my->_ro_max_trx_time_us)("w", my->_ro_read_window_effective_time_us));
+      ilog("ro_write_window_time_us ${ww}, ro_read_window_time_us ${rw}, ro_read_window_effective_time_us ${w}",
+           ("ww", my->_ro_write_window_time_us)("rw", my->_ro_read_window_time_us)("w", my->_ro_read_window_effective_time_us));
+   }
+
+   // Make sure _ro_max_trx_time_us is alwasys set.
+   if ( my->_max_transaction_time_ms.load() > 0 ) {
+      my->_ro_max_trx_time_us = fc::milliseconds(my->_max_transaction_time_ms.load());
+   } else {
+      // In integration tests, _max_transaction_time_ms can be set to negative
+      // for unlimited time
+     my->_ro_max_trx_time_us = fc::microseconds::maximum();
    }
+   ilog("ro_thread_pool_size ${s}, ro_max_trx_time_us ${t}}", ("s", my->_ro_thread_pool_size)("t", my->_ro_max_trx_time_us));
 
    my->_incoming_block_sync_provider = app().get_method<incoming::methods::block_sync>().register_provider(
          [this](const signed_block_ptr& block, const std::optional<block_id_type>& block_id, const block_state_ptr& bsp) {

From 2b5095736da1c624aa6512a287dc8c8da3ff2464 Mon Sep 17 00:00:00 2001
From: Lin Huang <lin.huang@eosnetwork.com>
Date: Thu, 4 May 2023 10:03:37 -0400
Subject: [PATCH 4/5] use enum instead of bool for better status indication

---
 .../producer_plugin/test/test_read_only_trx.cpp | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/plugins/producer_plugin/test/test_read_only_trx.cpp b/plugins/producer_plugin/test/test_read_only_trx.cpp
index da32cd1a7d..5f1a92cdc2 100644
--- a/plugins/producer_plugin/test/test_read_only_trx.cpp
+++ b/plugins/producer_plugin/test/test_read_only_trx.cpp
@@ -50,8 +50,9 @@ auto make_unique_trx( const chain_id_type& chain_id ) {
 
 BOOST_AUTO_TEST_SUITE(read_only_trxs)
 
-// app_init_status: true -- succeeded, false -- failed
-void test_configs_common(std::vector<const char*>& specific_args, bool app_init_status) {
+enum class app_init_status { failed, succeeded };
+
+void test_configs_common(std::vector<const char*>& specific_args, app_init_status expected_status) {
    appbase::scoped_app app;
    fc::temp_directory temp;
    auto temp_dir_str = temp.path().string();
@@ -60,26 +61,30 @@ void test_configs_common(std::vector<const char*>& specific_args, bool app_init_
    std::vector<const char*> argv =
       {"test", "--data-dir", temp_dir_str.c_str(), "--config-dir", temp_dir_str.c_str()};
    argv.insert( argv.end(), specific_args.begin(), specific_args.end() );
-   BOOST_CHECK_EQUAL( app->initialize<producer_plugin>( argv.size(), (char**) &argv[0]), app_init_status );
+
+   // app->initialize() returns a boolean. BOOST_CHECK_EQUAL cannot compare
+   // a boolean with a app_init_status directly
+   bool rc = (expected_status == app_init_status::succeeded) ? true : false;
+   BOOST_CHECK_EQUAL( app->initialize<producer_plugin>( argv.size(), (char**) &argv[0]), rc );
 }
 
 // --read-only-thread not allowed on producer node
 BOOST_AUTO_TEST_CASE(read_only_on_producer) {
    std::vector<const char*> specific_args = {"-p", "eosio", "-e", "--read-only-threads", "2" };
-   test_configs_common(specific_args, false /* failed */);
+   test_configs_common(specific_args, app_init_status::failed);
 }
 
 // read_window_time must be greater than max_transaction_time + 10ms
 BOOST_AUTO_TEST_CASE(invalid_read_window_time) {
    std::vector<const char*> specific_args = { "--read-only-threads", "2", "--max-transaction-time", "10", "--read-only-write-window-time-us", "50000", "--read-only-read-window-time-us", "20000" }; // 20000 not greater than --max-transaction-time (10ms) + 10000us (minimum margin)
-   test_configs_common(specific_args, false /* failed */);
+   test_configs_common(specific_args, app_init_status::failed);
 }
 
 // if --read-only-threads is not configured, read-only trx related configs should
 // not be checked
 BOOST_AUTO_TEST_CASE(not_check_configs_if_no_read_only_threads) {
    std::vector<const char*> specific_args = { "--max-transaction-time", "10", "--read-only-write-window-time-us", "50000", "--read-only-read-window-time-us", "20000" }; // 20000 not greater than --max-transaction-time (10ms) + 10000us (minimum margin)
-   test_configs_common(specific_args, true /* succeeded  */);
+   test_configs_common(specific_args, app_init_status::succeeded);
 }
 
 void test_trxs_common(std::vector<const char*>& specific_args) {

From 11ad31e62ca7f0c5df3461a04821937ff3f2c586 Mon Sep 17 00:00:00 2001
From: Lin Huang <lin.huang@eosnetwork.com>
Date: Thu, 4 May 2023 11:40:24 -0400
Subject: [PATCH 5/5] log configuration options and other values from the user
 perspective

---
 plugins/producer_plugin/producer_plugin.cpp | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/plugins/producer_plugin/producer_plugin.cpp b/plugins/producer_plugin/producer_plugin.cpp
index 31e83c498e..9f41ccc885 100644
--- a/plugins/producer_plugin/producer_plugin.cpp
+++ b/plugins/producer_plugin/producer_plugin.cpp
@@ -1125,7 +1125,7 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_
          }
       }
    }
-   EOS_ASSERT( test_mode_ || my->_ro_thread_pool_size == 0 || my->_producers.empty(), plugin_config_exception, "--read-only-threads not allowed on producer node" );
+   EOS_ASSERT( test_mode_ || my->_ro_thread_pool_size == 0 || my->_producers.empty(), plugin_config_exception, "read-only-threads not allowed on producer node" );
 
    // only initialize other read-only options when read-only thread pool is enabled
    if ( my->_ro_thread_pool_size > 0 ) {
@@ -1156,23 +1156,23 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_
          num_threads_supported -= 2;
          auto actual_threads_allowed = std::min(my->_ro_max_threads_allowed, num_threads_supported);
          ilog("vm total in kb: ${total}, vm used in kb: ${used}, number of EOS VM OC threads supported ((vm total - vm used)/4.2 TB - 2): ${supp}, max allowed: ${max}, actual allowed: ${actual}", ("total", vm_total_kb) ("used", vm_used_kb) ("supp", num_threads_supported) ("max", my->_ro_max_threads_allowed)("actual", actual_threads_allowed));
-         EOS_ASSERT( my->_ro_thread_pool_size <= actual_threads_allowed, plugin_config_exception, "--read-only-threads (${th}) greater than number of threads allowed for EOS VM OC (${allowed})", ("th", my->_ro_thread_pool_size) ("allowed", actual_threads_allowed) );
+         EOS_ASSERT( my->_ro_thread_pool_size <= actual_threads_allowed, plugin_config_exception, "read-only-threads (${th}) greater than number of threads allowed for EOS VM OC (${allowed})", ("th", my->_ro_thread_pool_size) ("allowed", actual_threads_allowed) );
       }
 #endif
-      EOS_ASSERT( my->_ro_thread_pool_size <= my->_ro_max_threads_allowed, plugin_config_exception, "--read-only-threads (${th}) greater than number of threads allowed (${allowed})", ("th", my->_ro_thread_pool_size) ("allowed", my->_ro_max_threads_allowed) );
+      EOS_ASSERT( my->_ro_thread_pool_size <= my->_ro_max_threads_allowed, plugin_config_exception, "read-only-threads (${th}) greater than the number of threads allowed (${allowed})", ("th", my->_ro_thread_pool_size) ("allowed", my->_ro_max_threads_allowed) );
 
       my->_ro_write_window_time_us = fc::microseconds( options.at( "read-only-write-window-time-us" ).as<uint32_t>() );
       my->_ro_read_window_time_us = fc::microseconds( options.at( "read-only-read-window-time-us" ).as<uint32_t>() );
-      EOS_ASSERT( my->_ro_read_window_time_us > my->_ro_read_window_minimum_time_us, plugin_config_exception, "minimum of --read-only-read-window-time-us (${read}) must be ${min} microseconds", ("read", my->_ro_read_window_time_us) ("min", my->_ro_read_window_minimum_time_us) );
+      EOS_ASSERT( my->_ro_read_window_time_us > my->_ro_read_window_minimum_time_us, plugin_config_exception, "read-only-read-window-time-us (${read}) must be at least greater than  ${min} us", ("read", my->_ro_read_window_time_us) ("min", my->_ro_read_window_minimum_time_us) );
       my->_ro_read_window_effective_time_us = my->_ro_read_window_time_us - my->_ro_read_window_minimum_time_us;
 
       // Make sure a read-only transaction can finish within the read
       // window if scheduled at the very beginning of the window.
       // Add _ro_read_window_minimum_time_us for safety margin.
       if ( my->_max_transaction_time_ms.load() > 0 ) {
-         EOS_ASSERT( my->_ro_read_window_time_us > ( fc::milliseconds(my->_max_transaction_time_ms.load()) + my->_ro_read_window_minimum_time_us ), plugin_config_exception, "--read-only-read-window-time-us (${read} us) must be greater than --max-transaction-time (${trx_time} us) plus ${min} us, required: ${read} us > (${trx_time} us + ${min} us).", ("read", my->_ro_read_window_time_us) ("trx_time", my->_max_transaction_time_ms.load() * 1000) ("min", my->_ro_read_window_minimum_time_us) );
+         EOS_ASSERT( my->_ro_read_window_time_us > ( fc::milliseconds(my->_max_transaction_time_ms.load()) + my->_ro_read_window_minimum_time_us ), plugin_config_exception, "read-only-read-window-time-us (${read} us) must be greater than max-transaction-time (${trx_time} us) plus ${min} us, required: ${read} us > (${trx_time} us + ${min} us).", ("read", my->_ro_read_window_time_us) ("trx_time", my->_max_transaction_time_ms.load() * 1000) ("min", my->_ro_read_window_minimum_time_us) );
       }
-      ilog("ro_write_window_time_us ${ww}, ro_read_window_time_us ${rw}, ro_read_window_effective_time_us ${w}",
+      ilog("read-only-write-window-time-us: ${ww} us, read-only-read-window-time-us: ${rw} us, effective read window time to be used: ${w} us",
            ("ww", my->_ro_write_window_time_us)("rw", my->_ro_read_window_time_us)("w", my->_ro_read_window_effective_time_us));
    }
 
@@ -1180,11 +1180,10 @@ void producer_plugin::plugin_initialize(const boost::program_options::variables_
    if ( my->_max_transaction_time_ms.load() > 0 ) {
       my->_ro_max_trx_time_us = fc::milliseconds(my->_max_transaction_time_ms.load());
    } else {
-      // In integration tests, _max_transaction_time_ms can be set to negative
-      // for unlimited time
+      // max-transaction-time can be set to negative for unlimited time
      my->_ro_max_trx_time_us = fc::microseconds::maximum();
    }
-   ilog("ro_thread_pool_size ${s}, ro_max_trx_time_us ${t}}", ("s", my->_ro_thread_pool_size)("t", my->_ro_max_trx_time_us));
+   ilog("read-only-threads ${s}, max read-only trx time to be enforced: ${t} us}", ("s", my->_ro_thread_pool_size)("t", my->_ro_max_trx_time_us));
 
    my->_incoming_block_sync_provider = app().get_method<incoming::methods::block_sync>().register_provider(
          [this](const signed_block_ptr& block, const std::optional<block_id_type>& block_id, const block_state_ptr& bsp) {