Resolve pubkeys from state change in dupe check in the tx_pool

(#707)

* Resolve public keys from state changes for the TX pool to avoid dupes

* Update copy pasted error message for quorum index OOB

* Code review

.travis.yml

@@ -9,7 +9,7 @@ cache:
   - $HOME/.ccache
 env:
   global:
-    - MAKEJOBS=-j2
+    - MAKEJOBS=-j3
     - CCACHE_SIZE=100M
     - CCACHE_TEMPDIR=/tmp/.ccache-temp
     - CCACHE_COMPRESS=1
@@ -18,6 +18,14 @@ env:
     - SDK_URL=https://bitcoincore.org/depends-sources/sdks
     - DOCKER_PACKAGES="build-essential libtool cmake autotools-dev automake pkg-config bsdmainutils curl git ca-certificates ccache"
   matrix:
+# ARM v7
+    - HOST=arm-linux-gnueabihf PACKAGES="python3 gperf g++-arm-linux-gnueabihf"
+# ARM v8
+    - HOST=aarch64-linux-gnu PACKAGES="python3 gperf g++-aarch64-linux-gnu"
+# i686 Win
+# - HOST=i686-w64-mingw32 DEP_OPTS="NO_QT=1" PACKAGES="python3 g++-mingw-w64-i686 qttools5-dev-tools"
+# i686 Linux
+# - HOST=i686-pc-linux-gnu PACKAGES="gperf cmake g++-multilib python3-zmq"
 # Win64
     - HOST=x86_64-w64-mingw32 DEP_OPTS="NO_QT=1" PACKAGES="cmake python3 g++-mingw-w64-x86-64 qttools5-dev-tools"
 # x86_64 Linux
@@ -47,8 +55,11 @@ script:
     - export TRAVIS_COMMIT_LOG=`git log --format=fuller -1`
     - OUTDIR=$BASE_OUTDIR/$TRAVIS_PULL_REQUEST/$TRAVIS_JOB_NUMBER-$HOST
     - if [ -z "$NO_DEPENDS" ]; then $DOCKER_EXEC ccache --max-size=$CCACHE_SIZE; fi
+    # TODO(loki): Tests push the build > 50mins and so doesn't give us useful metrics on the CI.
+    # - if [ "$RUN_TESTS" = true ]; then $DOCKER_EXEC bash -c "mkdir build && cd build && cmake -D BUILD_TESTS=ON CMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=$TRAVIS_BUILD_DIR/contrib/depends/$HOST/share/toolchain.cmake .. && make $MAKEJOBS && make $MAKEJOBS ARGS=\"-E libwallet_api_tests\" test"; fi
+    # - if [ "$RUN_TESTS" = false ]; then $DOCKER_EXEC bash -c "mkdir build && cd build && cmake -DCMAKE_TOOLCHAIN_FILE=$TRAVIS_BUILD_DIR/contrib/depends/$HOST/share/toolchain.cmake .. && make $MAKEJOBS"; fi
     - $DOCKER_EXEC bash -c "mkdir build && cd build && cmake -DCMAKE_TOOLCHAIN_FILE=$TRAVIS_BUILD_DIR/contrib/depends/$HOST/share/toolchain.cmake .. && make $MAKEJOBS"
     - export LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/contrib/depends/$HOST/lib
 after_script:
     - echo $TRAVIS_COMMIT_RANGE
-    - echo $TRAVIS_COMMIT_LOG
+    - echo $TRAVIS_COMMIT_LOG

CONTRIBUTING.md

@@ -88,26 +88,26 @@ C4 is meant to provide a reusable optimal collaboration model for open source so
 
 ### Preliminaries
 
-- The project SHALL use the git distributed revision control system.
-- The project SHALL be hosted on github.com or equivalent, herein called the "Platform".
-- The project SHALL use the Platform issue tracker.
+- The project MUST use the git distributed revision control system.
+- The project MUST be hosted on github.com or equivalent, herein called the "Platform".
+- The project MUST use the Platform issue tracker.
   - Non-GitHub example:
     - "Platform" could be a vanilla git repo and Trac hosted on the same machine/network.
     - The Platform issue tracker would be Trac.
 - The project SHOULD have clearly documented guidelines for code style.
 - A "Contributor" is a person who wishes to provide a patch, being a set of commits that solve some clearly identified problem.
 - A "Maintainer" is a person who merges patches to the project. Maintainers are not developers; their job is to enforce process.
-- Contributors SHALL NOT have commit access to the repository unless they are also Maintainers.
-- Maintainers SHALL have commit access to the repository.
-- Everyone, without distinction or discrimination, SHALL have an equal right to become a Contributor under the terms of this contract.
+- Contributors MUST NOT have commit access to the repository unless they are also Maintainers.
+- Maintainers MUST have commit access to the repository.
+- Everyone, without distinction or discrimination, MUST have an equal right to become a Contributor under the terms of this contract.
 
 ### Licensing and ownership
 
-- The project SHALL use a share-alike license, such as BSD-3, the GPLv3 or a variant thereof (LGPL, AGPL), or the MPLv2.
-- All contributions to the project source code ("patches") SHALL use the same license as the project.
-- All patches are owned by their authors. There SHALL NOT be any copyright assignment process.
-- The copyrights in the project SHALL be owned collectively by all its Contributors.
-- Each Contributor SHALL be responsible for identifying themselves in the project Contributor list.
+- The project MUST use a share-alike license, such as BSD-3, the GPLv3 or a variant thereof (LGPL, AGPL), or the MPLv2.
+- All contributions to the project source code ("patches") MUST use the same license as the project.
+- All patches are owned by their authors. There MUST NOT be any copyright assignment process.
+- The copyrights in the project MUST be owned collectively by all its Contributors.
+- Each Contributor MUST be responsible for identifying themselves in the project Contributor list.
 
 ### Patch requirements
 
@@ -116,27 +116,27 @@ C4 is meant to provide a reusable optimal collaboration model for open source so
 - A patch SHOULD be a minimal and accurate answer to exactly one identified and agreed problem.
 - A patch MUST adhere to the code style guidelines of the project if these are defined.
 - A patch MUST adhere to the "Evolution of Public Contracts" guidelines defined below.
-- A patch SHALL NOT include non-trivial code from other projects unless the Contributor is the original author of that code.
+- A patch MUST NOT include non-trivial code from other projects unless the Contributor is the original author of that code.
 - A patch MUST compile cleanly and pass project self-tests on at least the principle target platform.
 - A patch commit message SHOULD consist of a single short (less than 50 character) line summarizing the change, optionally followed by a blank line and then a more thorough description.
 - A "Correct Patch" is one that satisfies the above requirements.
 
 ### Development process
 
-- Change on the project SHALL be governed by the pattern of accurately identifying problems and applying minimal, accurate solutions to these problems.
+- Change on the project MUST be governed by the pattern of accurately identifying problems and applying minimal, accurate solutions to these problems.
 - To request changes, a user SHOULD log an issue on the project Platform issue tracker.
 - The user or Contributor SHOULD write the issue by describing the problem they face or observe.
 - The user or Contributor SHOULD seek consensus on the accuracy of their observation, and the value of solving the problem.
-- Users SHALL NOT log feature requests, ideas, or suggestions unrelated to Monero code or Monero's dependency code or Monero's potential/future dependency code or research which successfully implements Monero.
-- Users SHALL NOT log any solutions to problems (verifiable or hypothetical) of which are not explicitly documented and/or not provable and/or cannot be reasonably proven.
-- Thus, the release history of the project SHALL be a list of meaningful issues logged and solved.
-- To work on an issue, a Contributor SHALL fork the project repository and then work on their forked repository.
-- To submit a patch, a Contributor SHALL create a Platform pull request back to the project.
-- A Contributor SHALL NOT commit changes directly to the project.
+- Users MUST NOT log feature requests, ideas, or suggestions unrelated to Monero code or Monero's dependency code or Monero's potential/future dependency code or research which successfully implements Monero.
+- Users MUST NOT log any solutions to problems (verifiable or hypothetical) of which are not explicitly documented and/or not provable and/or cannot be reasonably proven.
+- Thus, the release history of the project MUST be a list of meaningful issues logged and solved.
+- To work on an issue, a Contributor MUST fork the project repository and then work on their forked repository.
+- To submit a patch, a Contributor MUST create a Platform pull request back to the project.
+- A Contributor MUST NOT commit changes directly to the project.
 - To discuss a patch, people MAY comment on the Platform pull request, on the commit, or elsewhere.
-- To accept or reject a patch, a Maintainer SHALL use the Platform interface.
+- To accept or reject a patch, a Maintainer MUST use the Platform interface.
 - Maintainers SHOULD NOT merge their own patches except in exceptional cases, such as non-responsiveness from other Maintainers for an extended period (more than 30 days) or unless urgent as defined by the Monero Maintainers Team.
-- Maintainers SHALL NOT make value judgments on correct patches unless the Maintainer (as may happen in rare circumstances) is a core code developer.
+- Maintainers MUST NOT make value judgments on correct patches unless the Maintainer (as may happen in rare circumstances) is a core code developer.
 - Maintainers MUST NOT merge pull requests in less than 168 hours (1 week) unless deemed urgent by at least 2 people from the Monero Maintainer Team.
 - The Contributor MAY tag an issue as "Ready" after making a pull request for the issue.
 - The user who created an issue SHOULD close the issue after checking the patch is successful.
@@ -146,27 +146,27 @@ C4 is meant to provide a reusable optimal collaboration model for open source so
 
 ### Creating stable releases
 
-- The project SHALL have one branch ("master") that always holds the latest in-progress version and SHOULD always build.
-- The project SHALL NOT use topic branches for any reason. Personal forks MAY use topic branches.
-- To make a stable release someone SHALL fork the repository by copying it and thus become maintainer of this repository.
+- The project MUST have one branch ("master") that always holds the latest in-progress version and SHOULD always build.
+- The project MUST NOT use topic branches for any reason. Personal forks MAY use topic branches.
+- To make a stable release someone MUST fork the repository by copying it and thus become maintainer of this repository.
 - Forking a project for stabilization MAY be done unilaterally and without agreement of project maintainers.
-- A patch to a stabilization project declared "stable" SHALL be accompanied by a reproducible test case.
+- A patch to a stabilization project declared "stable" MUST be accompanied by a reproducible test case.
 
 ### Evolution of public contracts
 
-- All Public Contracts (APIs or protocols) SHALL be documented.
+- All Public Contracts (APIs or protocols) MUST be documented.
 - All Public Contracts SHOULD have space for extensibility and experimentation.
 - A patch that modifies a stable Public Contract SHOULD not break existing applications unless there is overriding consensus on the value of doing this.
 - A patch that introduces new features to a Public Contract SHOULD do so using new names.
 - Old names SHOULD be deprecated in a systematic fashion by marking new names as "experimental" until they are stable, then marking the old names as "deprecated".
 - When sufficient time has passed, old deprecated names SHOULD be marked "legacy" and eventually removed.
-- Old names SHALL NOT be reused by new features.
+- Old names MUST NOT be reused by new features.
 - When old names are removed, their implementations MUST provoke an exception (assertion) if used by applications.
 
 ### Project administration
 
-- The project founders SHALL act as Administrators to manage the set of project Maintainers.
-- The Administrators SHALL ensure their own succession over time by promoting the most effective Maintainers.
-- A new Contributor who makes a correct patch SHALL be invited to become a Maintainer.
+- The project founders MUST act as Administrators to manage the set of project Maintainers.
+- The Administrators MUST ensure their own succession over time by promoting the most effective Maintainers.
+- A new Contributor who makes a correct patch MUST be invited to become a Maintainer.
 - Administrators MAY remove Maintainers who are inactive for an extended period of time, or who repeatedly fail to apply this process accurately.
 - Administrators SHOULD block or ban "bad actors" who cause stress and pain to others in the project. This should be done after public discussion, with a chance for all parties to speak. A bad actor is someone who repeatedly ignores the rules and culture of the project, who is needlessly argumentative or hostile, or who is offensive, and who is unable to self-correct their behavior when asked to do so by others.

contrib/epee/src/mlog.cpp

@@ -100,7 +100,7 @@ static const char *get_default_categories(int level)
   switch (level)
   {
     case 0:
-      categories = "*:WARNING,net:FATAL,net.http:FATAL,net.ssl:FATAL,net.p2p:FATAL,net.cn:FATAL,global:INFO,verify:FATAL,serialization:FATAL,stacktrace:INFO,logging:INFO,msgwriter:INFO";
+      categories = "*:WARNING,net:FATAL,net.http:FATAL,net.ssl:FATAL,net.p2p:FATAL,net.cn:FATAL,global:INFO,verify:FATAL,serialization:FATAL,logging:INFO,msgwriter:INFO";
       break;
     case 1:
       categories = "*:INFO,global:INFO,stacktrace:INFO,logging:INFO,msgwriter:INFO,perf.*:DEBUG";

src/cryptonote_core/blockchain.cpp

@@ -96,7 +96,7 @@ struct hard_fork_record
 
 // TODO(doyle): Move this out into a globally accessible object
 // version 7 from the start of the blockchain, inhereted from Monero mainnet
-static const hard_fork_record mainnet_hard_forks[] =
+static const hard_fork_record mainnet_hard_forks[] = // SEEME
 {
   { network_version_7,                   1,		0, 1513046577 },
   { network_version_8,                   101,	0, 1534006000 },
@@ -687,7 +687,7 @@ block Blockchain::pop_block_from_blockchain()
       // that might not be always true. Unlikely though, and always relaying
       // these again might cause a spike of traffic as many nodes re-relay
       // all the transactions in a popped block when a reorg happens.
-      bool r = m_tx_pool.add_tx(tx, tvc, true, true, false, version);
+      bool r = m_tx_pool.add_tx(tx, tvc, true, true, false, version, m_service_node_list);
       if (!r)
       {
         LOG_ERROR("Error returning transaction to tx_pool");
@@ -1820,6 +1820,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
     difficulty_type current_diff = get_next_difficulty_for_alternative_chain(alt_chain, bei);
     CHECK_AND_ASSERT_MES(current_diff, false, "!!!!!!! DIFFICULTY OVERHEAD !!!!!!!");
     crypto::hash proof_of_work = null_hash;
+	get_block_longhash(this, bei.bl, proof_of_work, bei.height, 0); // SEEME
 //    if (b.major_version >= RX_BLOCK_VERSION)
 //    {
 //      crypto::hash seedhash = null_hash;
@@ -1842,7 +1843,7 @@ bool Blockchain::handle_alternative_block(const block& b, const crypto::hash& id
 //      get_altblock_longhash(bei.bl, proof_of_work, get_current_blockchain_height(), bei.height, seedheight, seedhash);
 //    } else
 //    {
-      get_block_longhash(this, bei.bl, proof_of_work, bei.height, 0);
+//      get_block_longhash(this, bei.bl, proof_of_work, bei.height, 0);
 //    }
     if(!check_hash(proof_of_work, current_diff))
     {
@@ -3208,29 +3209,34 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc,
         return false;
       }
 
-      auto quorum = m_service_node_list.get_testing_quorum(service_nodes::quorum_type::obligations, state_change.block_height);
-      if (!quorum)
+      auto const quorum_type  = service_nodes::quorum_type::obligations;
+      auto const quorum       = m_service_node_list.get_testing_quorum(quorum_type, state_change.block_height);
       {
-        MERROR_VER("State change TX could not get quorum for height: " << state_change.block_height);
-        return false;
-      }
+        if (!quorum)
+        {
+          MERROR_VER("could not get obligations quorum for recent state change tx");
+          return false;
+        }
 
-      if (!service_nodes::verify_tx_state_change(state_change, get_current_blockchain_height(), tvc, *quorum, hf_version))
-      {
-        // will be set by the above on serious failures (i.e. illegal value), but not for less
-        // serious ones like state change heights slightly outside of allowed bounds:
-        //tvc.m_verifivation_failed = true;
-        MERROR_VER("tx " << get_transaction_hash(tx) << ": state change tx could not be completely verified reason: " << print_vote_verification_context(tvc.m_vote_ctx));
-        return false;
+        if (!service_nodes::verify_tx_state_change(state_change, get_current_blockchain_height(), tvc, *quorum, hf_version))
+        {
+          // will be set by the above on serious failures (i.e. illegal value), but not for less
+          // serious ones like state change heights slightly outside of allowed bounds:
+          //tvc.m_verifivation_failed = true;
+          MERROR_VER("tx " << get_transaction_hash(tx) << ": state change tx could not be completely verified reason: " << print_vote_verification_context(tvc.m_vote_ctx));
+          return false;
+        }
       }
 
-      const uint64_t height                = state_change.block_height;
-      constexpr size_t num_blocks_to_check = service_nodes::STATE_CHANGE_TX_LIFETIME_IN_BLOCKS;
+      crypto::public_key const &state_change_service_node_pubkey = quorum->workers[state_change.service_node_index];
+      const uint64_t height                                      = state_change.block_height;
+      constexpr size_t num_blocks_to_check                       = service_nodes::STATE_CHANGE_TX_LIFETIME_IN_BLOCKS;
 
       std::vector<std::pair<cryptonote::blobdata,block>> blocks;
       std::vector<cryptonote::blobdata> txs;
       if (!get_blocks(height, num_blocks_to_check, blocks, txs))
       {
+        MERROR_VER("Failed to get historical blocks to check against previous state changes for de-duplication");
         return false;
       }
 
@@ -3253,15 +3259,15 @@ bool Blockchain::check_tx_inputs(transaction& tx, tx_verification_context &tvc,
           continue;
         }
 
-        const auto existing_quorum = m_service_node_list.get_testing_quorum(service_nodes::quorum_type::obligations, existing_state_change.block_height);
-        if (!existing_quorum)
-        {
-          MERROR_VER("could not get obligations quorum for recent state change tx");
+        crypto::public_key existing_state_change_service_node_pubkey;
+        if (!m_service_node_list.get_quorum_pubkey(quorum_type,
+                                                   service_nodes::quorum_group::worker,
+                                                   existing_state_change.block_height,
+                                                   existing_state_change.service_node_index,
+                                                   existing_state_change_service_node_pubkey))
           continue;
-        }
 
-        if (existing_quorum->workers[existing_state_change.service_node_index] ==
-            quorum->workers[state_change.service_node_index])
+        if (existing_state_change_service_node_pubkey == state_change_service_node_pubkey)
         {
           MERROR_VER("Already seen this state change tx (aka double spend)");
           tvc.m_double_spend = true;
@@ -3625,7 +3631,7 @@ void Blockchain::return_tx_to_pool(std::vector<std::pair<transaction, blobdata>>
     // all the transactions in a popped block when a reorg happens.
     const size_t weight = get_transaction_weight(tx.first, tx.second.size());
     const crypto::hash tx_hash = get_transaction_hash(tx.first);
-    if (!m_tx_pool.add_tx(tx.first, tx_hash, tx.second, weight, tvc, true, true, false, version))
+    if (!m_tx_pool.add_tx(tx.first, tx_hash, tx.second, weight, tvc, true, true, false, version, m_service_node_list))
     {
       MERROR("Failed to return taken transaction with hash: " << get_transaction_hash(tx.first) << " to tx_pool");
     }

src/cryptonote_core/cryptonote_core.cpp

@@ -1373,7 +1373,7 @@ namespace cryptonote
     }
 
     uint8_t version = m_blockchain_storage.get_current_hard_fork_version();
-    return m_mempool.add_tx(tx, tx_hash, blob, tx_weight, tvc, keeped_by_block, relayed, do_not_relay, version);
+    return m_mempool.add_tx(tx, tx_hash, blob, tx_weight, tvc, keeped_by_block, relayed, do_not_relay, version, m_service_node_list);
   }
   //-----------------------------------------------------------------------------------------------
   bool core::relay_txpool_transactions()