Merge branch '210-update-lastnodelist-only-after-response-is-verified…

…' into 'develop'

Resolve "Update lastNodeList only after response is verified"

Closes #210

See merge request in3/c/in3-core!160

c/src/api/utils/api_utils_priv.c

@@ -91,7 +91,7 @@ void set_error(int err, const char* msg) {
 }
 
 void api_set_error(int err, const char* msg) {
-  return set_error_impl(err, msg);
+  return set_error_impl(err, msg ? msg : "unknown error");
 }
 
 d_token_t* get_result(in3_ctx_t* ctx) {
@@ -110,6 +110,6 @@ d_token_t* get_result(in3_ctx_t* ctx) {
   t = d_get(ctx->responses[0], K_ERROR); // we we have an error...
   api_set_error(ETIMEDOUT, !t
                                ? "No result or error in response"
-                               : (d_type(t) == T_OBJECT ? d_string(t) : d_get_stringk(t, K_MESSAGE)));
+                               : (d_type(t) != T_OBJECT ? d_string(t) : d_get_stringk(t, K_MESSAGE)));
   return NULL;
 }

c/src/core/client/execute.c

@@ -396,10 +396,7 @@ static in3_ret_t find_valid_result(in3_ctx_t* ctx, int nodes_count, in3_response
           vc.config  = ctx->requests_configs + i;
 
           if ((vc.proof = d_get(ctx->responses[i], K_IN3))) {
-
             // vc.proof is temporary set to the in3-section. It will be updated to real proof in the next lines.
-            check_autoupdate(ctx, chain, vc.proof, node);
-
             vc.last_validator_change = d_get_longk(vc.proof, K_LAST_VALIDATOR_CHANGE);
             vc.currentBlock          = d_get_longk(vc.proof, K_CURRENT_BLOCK);
             vc.proof                 = d_get(vc.proof, K_PROOF);
@@ -431,6 +428,10 @@ static in3_ret_t find_valid_result(in3_ctx_t* ctx, int nodes_count, in3_response
       }
     }
 
+    // check auto update opts only if this node wasn't blacklisted (due to wrong result/proof)
+    if (!is_blacklisted(node) && d_get(ctx->responses[0], K_IN3))
+      check_autoupdate(ctx, chain, d_get(ctx->responses[0], K_IN3), node);
+
     // !node_weight is valid, because it means this is a internaly handled response
     if (!node || !is_blacklisted(node))
       return IN3_OK; // this reponse was successfully verified, so let us keep it.

c/test/unit_tests/test_nodelist.c

@@ -584,6 +584,82 @@ static void test_nodelist_update_7() {
   in3_free(c);
 }
 
+// Scenario 8: Newer `lastNodeList` with wrong response
+// Begin with 3 nodes in first nodeList update (always taken from a boot node) which happened at block 87989012 (i.e. `lastBlockNumber`).
+// After 200 secs, `eth_blockNumber` returns an error result but reports a nodeList update happened at 87989038 (i.e. `lastNodeList`).
+// Since the result was not valid, we must not accept the `lastNodeList` and should not trigger a nodeList update.
+// A request now should trigger a nodeList update.
+static void test_nodelist_update_8() {
+  in3_t* c                = in3_init_test(ETH_CHAIN_ID_MAINNET);
+  c->proof                = PROOF_NONE;
+  c->replace_latest_block = DEF_REPL_LATEST_BLK;
+  c->max_attempts         = 0;
+
+  // start time
+  uint64_t t = 1;
+  in3_time(&t);
+
+  // reset rand to be deterministic
+  int s = 0;
+  in3_rand(&s);
+
+  // begin with 3 nodes, i.e. one node more than the usual boot nodes
+  ADD_RESPONSE_BLOCK_NUMBER("87989038", "87989050", "0x53E9B3A");
+  ADD_RESPONSE_NODELIST_3("87989038");
+
+  t = 200;
+  in3_time(&t);
+  TEST_ASSERT_NOT_EQUAL(0, eth_blockNumber(c));
+
+  // reset rand to be deterministic
+  s = 0;
+  in3_rand(&s);
+
+  // test that nodelist_upd8_params are not set when we have an error response
+  add_response("eth_blockNumber",
+               "[]",
+               NULL,
+               "\"Error: Internal server error\"",
+               "{"
+               "  \"lastValidatorChange\": 0,"
+               "  \"lastNodeList\": 87989049,"
+               "  \"execTime\": 59,"
+               "  \"rpcTime\": 59,"
+               "  \"rpcCount\": 1,"
+               "  \"currentBlock\": 87989092,"
+               "  \"version\": \"2.0.0\""
+               "}");
+  TEST_ASSERT_EQUAL(0, eth_blockNumber(c));
+  in3_chain_t* chain = in3_find_chain(c, ETH_CHAIN_ID_MAINNET);
+  TEST_ASSERT_NULL(chain->nodelist_upd8_params);
+
+  // test that nodelist_upd8_params are not set when we have a response without proof
+  c->proof = PROOF_STANDARD;
+  add_response("eth_getBalance",
+               "[\"0x000000000000000000000000000000000000dead\",\"latest\"]",
+               "\"0x2a595770eb8ed0c5827\"",
+               NULL,
+               "{"
+               "  \"lastValidatorChange\": 0,"
+               "  \"lastNodeList\": 87989049,"
+               "  \"execTime\": 59,"
+               "  \"rpcTime\": 59,"
+               "  \"rpcCount\": 1,"
+               "  \"currentBlock\": 87989092,"
+               "  \"version\": \"2.0.0\""
+               "}");
+
+  address_t addr;
+  hex_to_bytes("0x000000000000000000000000000000000000dead", -1, addr, 20);
+  long double balance = as_double(eth_getBalance(c, addr, BLKNUM_LATEST()));
+  TEST_ASSERT_EQUAL(0, balance);
+
+  chain = in3_find_chain(c, ETH_CHAIN_ID_MAINNET);
+  TEST_ASSERT_NULL(chain->nodelist_upd8_params);
+
+  in3_free(c);
+}
+
 /*
  * Main
  */
@@ -600,5 +676,6 @@ int main() {
   RUN_TEST(test_nodelist_update_5);
   RUN_TEST(test_nodelist_update_6);
   RUN_TEST(test_nodelist_update_7);
+  RUN_TEST(test_nodelist_update_8);
   return TESTS_END();
 }