Merge #735: [0.18] Reduce Travis errors

f7a41c5ff finalizecompactblock: Fix it, make test easier to fail (Gregory Sanders)
8bbd5a3aa Fix finalizecompactblock filling in more found txs from mempool (Gregory Sanders)
e7b77b757 Don't run normal functional tests twice. (Gregory Sanders)
5f628d542 Sprinkle a bunch of syncs to avoid peg-in failure hanging (Gregory Sanders)
87e80602f feature_fedpeg.py: just use testmempoolaccept checking blinded peg-in transactions (Gregory Sanders)
244cccd09 Take mempool locks when doing compact block RPC calls touching mempool (Gregory Sanders)
e66aca4d5 Disable functional tests for sanitzer runs which just OOM (Gregory Sanders)
12eb9b961 Don't run unit tests for compatibility run(which is a NOP) (Gregory Sanders)

Pull request description:

  1) Don't run unit tests for compatibility run(redundant)
  2) Disable functional tests for sanitizer runs which have been OOM for a while now
  3) only use mempool sync in fedpeg test to avoid connection complexity, add a bunch of sync calls to avoid hanging validation failure corner cases
  4) run functional tests exactly once
  5) fix finalizecompactblock RPC

Tree-SHA512: 2c682d3855cd474fb6e8c039a0cc308cf68321da0f57adbbf2aa8e3aa17b06e244942be7e2f8518581ad972fedfa80f4d898e1cd72a95e9660463413a3b47efe

.travis.yml

@@ -154,6 +154,7 @@ jobs:
         DOCKER_NAME_TAG=ubuntu:16.04
         PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libssl-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
         NO_DEPENDS=1
+        RUN_FUNCTIONAL_TESTS=false
         GOAL="install"
         BITCOIN_CONFIG="--enable-zmq --disable-wallet --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=thread --disable-hardening --disable-asm CC=clang CXX=clang++"
 
@@ -163,6 +164,7 @@ jobs:
         HOST=x86_64-unknown-linux-gnu
         PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
         NO_DEPENDS=1
+        RUN_FUNCTIONAL_TESTS=false
         GOAL="install"
         BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=qt5 CPPFLAGS=-DDEBUG_LOCKORDER --with-sanitizers=integer,undefined CC=clang CXX=clang++"
 # x86_64 Linux, No wallet, no QT(build timing out with for some reason)
@@ -188,10 +190,12 @@ jobs:
 
 # x86_64 Linux w/ Bitcoin functional tests (Qt5 & system libs)
     - stage: test
+      name: 'x86_64 Linux  [GOAL: install]  [bitcoin functional]'
       env: >-
         HOST=x86_64-unknown-linux-gnu
         PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
         NO_DEPENDS=1
+        RUN_UNIT_TESTS=false
         RUN_BITCOIN_TESTS=true
         RUN_FUNCTIONAL_TESTS=false
         GOAL="install"
@@ -199,6 +203,7 @@ jobs:
 
 # x86_64 Linux w/ single fedpeg test that uses upstream bitcoind as mainchain
     - stage: test
+      name: 'x86_64 Linux  [GOAL: install]  [bitcoind fedpeg test]'
       env: >-
         HOST=x86_64-unknown-linux-gnu
         PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools libssl1.0-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
@@ -211,6 +216,7 @@ jobs:
         BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --enable-glibc-back-compat --enable-reduce-exports --with-gui=no --disable-tests --disable-bench CPPFLAGS=-DDEBUG_LOCKORDER"
 # x86_64 Linux (uses qt5 dev package instead of depends Qt to speed up build and avoid timeout), no functional tests, LIQUID BUILD
     - stage: test
+      name: 'x86_64 Linux  [GOAL: install]  [liquid build]'
       env: >-
         HOST=x86_64-unknown-linux-gnu
         PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools protobuf-compiler libdbus-1-dev libharfbuzz-dev libprotobuf-dev"

.travis/test_06_script_a.sh

@@ -47,36 +47,4 @@ BEGIN_FOLD build
 DOCKER_EXEC make $MAKEJOBS $GOAL || ( echo "Build failure. Verbose build follows." && DOCKER_EXEC make $GOAL V=1 ; false )
 END_FOLD
 
-if [ "$RUN_UNIT_TESTS" = "true" ]; then
-  BEGIN_FOLD unit-tests
-  DOCKER_EXEC LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib make $MAKEJOBS check VERBOSE=1
-  END_FOLD
-fi
-
-if [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then
-  extended="--extended --exclude feature_pruning"
-fi
-
-if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
-  BEGIN_FOLD functional-tests
-  DOCKER_EXEC test/functional/test_runner.py --ci --combinedlogslen=4000 --coverage --quiet --failfast ${extended}
-  END_FOLD
-fi
-
-if [ "$RUN_BITCOIN_TESTS" = "true" ]; then
-    BEGIN_FOLD bitcoin-functional-tests
-    DOCKER_EXEC test/bitcoin_functional/functional/test_runner.py --combinedlogslen=4000 --coverage --quiet --failfast ${extended}
-    END_FOLD
-fi
-
-if [ "$RUN_FEDPEG_BITCOIND_TEST" = "true" ]; then
-    BEGIN_FOLD fedpeg-bitcoind-test
-    BITCOIND_VERSION=0.18.0
-    BITCOIND_ARCH=x86_64-linux-gnu
-    DOCKER_EXEC curl -O https://bitcoincore.org/bin/bitcoin-core-$BITCOIND_VERSION/bitcoin-$BITCOIND_VERSION-$BITCOIND_ARCH.tar.gz
-    DOCKER_EXEC tar -zxf bitcoin-$BITCOIND_VERSION-$BITCOIND_ARCH.tar.gz
-    DOCKER_EXEC test/functional/feature_fedpeg.py --parent_bitcoin --parent_binpath $(pwd)/bitcoin-$BITCOIND_VERSION/bin/bitcoind
-    END_FOLD
-fi
-
 cd ${TRAVIS_BUILD_DIR} || (echo "could not enter travis build dir $TRAVIS_BUILD_DIR"; exit 1)

.travis/test_06_script_b.sh

@@ -25,3 +25,29 @@ if [ "$RUN_FUZZ_TESTS" = "true" ]; then
   DOCKER_EXEC test/fuzz/test_runner.py -l DEBUG ${DIR_FUZZ_IN}
   END_FOLD
 fi
+
+if [ "$RUN_UNIT_TESTS" = "true" ]; then
+  BEGIN_FOLD unit-tests
+  DOCKER_EXEC LD_LIBRARY_PATH=$TRAVIS_BUILD_DIR/depends/$HOST/lib make $MAKEJOBS check VERBOSE=1
+  END_FOLD
+fi
+
+if [ "$RUN_BITCOIN_TESTS" = "true" ]; then
+    BEGIN_FOLD bitcoin-functional-tests
+    DOCKER_EXEC test/bitcoin_functional/functional/test_runner.py --combinedlogslen=4000 --coverage --quiet --failfast ${extended}
+    END_FOLD
+fi
+
+if [ "$RUN_FEDPEG_BITCOIND_TEST" = "true" ]; then
+    BEGIN_FOLD fedpeg-bitcoind-test
+    BITCOIND_VERSION=0.18.0
+    BITCOIND_ARCH=x86_64-linux-gnu
+    DOCKER_EXEC curl -O https://bitcoincore.org/bin/bitcoin-core-$BITCOIND_VERSION/bitcoin-$BITCOIND_VERSION-$BITCOIND_ARCH.tar.gz
+    DOCKER_EXEC tar -zxf bitcoin-$BITCOIND_VERSION-$BITCOIND_ARCH.tar.gz
+    DOCKER_EXEC test/functional/feature_fedpeg.py --parent_bitcoin --parent_binpath $(pwd)/bitcoin-$BITCOIND_VERSION/bin/bitcoind
+    END_FOLD
+fi
+
+if [ "$TRAVIS_EVENT_TYPE" = "cron" ]; then
+  extended="--extended --exclude feature_pruning"
+fi

src/rpc/mining.cpp

@@ -1245,6 +1245,7 @@ UniValue consumecompactsketch(const JSONRPCRequest& request)
     CBlockHeaderAndShortTxIDs cmpctblock;
     ssBlock >> cmpctblock;
 
+    LOCK(mempool.cs);
     PartiallyDownloadedBlock partialBlock(&mempool);
     const std::vector<std::pair<uint256, CTransactionRef>> dummy;
     ReadStatus status = partialBlock.InitData(cmpctblock, dummy);
@@ -1367,7 +1368,7 @@ UniValue finalizecompactblock(const JSONRPCRequest& request)
 
     // Cached transactions
     std::vector<unsigned char> found_tx(ParseHex(request.params[2].get_str()));
-    CDataStream ssFound(block_tx, SER_NETWORK, PROTOCOL_VERSION);
+    CDataStream ssFound(found_tx, SER_NETWORK, PROTOCOL_VERSION);
 
     std::vector<CTransactionRef> found;
     ssFound >> found;
@@ -1375,13 +1376,25 @@ UniValue finalizecompactblock(const JSONRPCRequest& request)
     // Make mega-list
     found.insert(found.end(), transactions.txn.begin(), transactions.txn.end());
 
-    // Now construct the final block!
-    PartiallyDownloadedBlock partialBlock(&mempool);
+    // Now construct the final block! (use dummy mempool here, otherwise reconstruction may fail)
+    CTxMemPool dummy_pool;
+    PartiallyDownloadedBlock partialBlock(&dummy_pool);
 
-    const std::vector<std::pair<uint256, CTransactionRef>> dummy;
+    // "Extra" list is really our combined list that will be put into place using InitData
+    std::vector<std::pair<uint256, CTransactionRef>> extra_txn;
+    for (const auto& found_tx : found) {
+        extra_txn.push_back(std::make_pair(found_tx->GetWitnessHash(), found_tx));
+    }
     std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
-    if (partialBlock.InitData(cmpctblock, dummy) != READ_STATUS_OK || partialBlock.FillBlock(*pblock, found, false /* pow_check*/) != READ_STATUS_OK) {
+    if (partialBlock.InitData(cmpctblock, extra_txn) != READ_STATUS_OK) {
+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "compact_hex appears malformed.");
+    }
+    const std::vector<CTransactionRef> dummy_missing;
+    auto result = partialBlock.FillBlock(*pblock, dummy_missing, false /* pow_check*/);
+    if (result == READ_STATUS_FAILED) {
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Failed to complete block though all transactions were apparently found. Could be random short ID collision; requires full block instead.");
+    } else if (result != READ_STATUS_OK) {
+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Failed to complete block though all transactions were apparently found.");
     }
 
     CDataStream ssBlock(SER_NETWORK, PROTOCOL_VERSION);

test/functional/feature_blocksign.py

@@ -109,7 +109,7 @@ class BlockSignTest(BitcoinTestFramework):
 
         # Make a few transactions to make non-empty blocks for compact transmission
         if make_transactions:
-            for i in range(5):
+            for i in range(20):
                 miner.sendtoaddress(miner_next.getnewaddress(), int(miner.getbalance()['bitcoin']/10), "", "", True)
         # miner makes a block
         block = miner.getnewblockhex()

test/functional/feature_fedpeg.py

@@ -6,7 +6,6 @@ from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     connect_nodes_bi,
-    disconnect_nodes,
     get_auth_cookie,
     get_datadir_path,
     rpc_port,
@@ -538,6 +537,7 @@ class FedPegTest(BitcoinTestFramework):
         # Watch the address so we can get tx without txindex
         parent.importaddress(mainchain_addr)
         claim_block = parent.generatetoaddress(50, mainchain_addr)[0]
+        self.sync_all(self.node_groups)
         block_coinbase = parent.getblock(claim_block, 2)["tx"][0]
         claim_txid = block_coinbase["txid"]
         claim_tx = block_coinbase["hex"]
@@ -553,11 +553,13 @@ class FedPegTest(BitcoinTestFramework):
 
         # 50 more blocks to allow wallet to make it succeed by relay and consensus
         parent.generatetoaddress(50, parent.getnewaddress())
+        self.sync_all(self.node_groups)
         # Wallet still doesn't want to for 2 more confirms
         assert_equal(sidechain.createrawpegin(claim_tx, claim_proof)["mature"], False)
         # But we can just shoot it off
         claim_txid = sidechain.sendrawtransaction(signed_pegin)
         sidechain.generatetoaddress(1, sidechain.getnewaddress())
+        self.sync_all(self.node_groups)
         assert_equal(sidechain.gettransaction(claim_txid)["confirmations"], 1)
 
         # Test a confidential pegin.
@@ -569,6 +571,7 @@ class FedPegTest(BitcoinTestFramework):
         txid_fund = parent.sendtoaddress(pegin_addr, 10)
         # 10+2 confirms required to get into mempool and confirm
         parent.generate(11)
+        self.sync_all(self.node_groups)
         proof = parent.gettxoutproof([txid_fund])
         raw = parent.gettransaction(txid_fund)["hex"]
         raw_pegin = sidechain.createrawpegin(raw, proof)['hex']
@@ -579,6 +582,8 @@ class FedPegTest(BitcoinTestFramework):
         blind_addr = sidechain.getnewaddress("", "blech32")
         sidechain.sendtoaddress(blind_addr, 15)
         sidechain.generate(6)
+        # Make sure sidechain2 knows about the same input
+        self.sync_all(self.node_groups)
         unspent = [u for u in sidechain.listunspent(6, 6) if u["amount"] == 15][0]
         assert(unspent["spendable"])
         assert("amountcommitment" in unspent)
@@ -615,23 +620,14 @@ class FedPegTest(BitcoinTestFramework):
             assert(final_decoded["vout"][1]["commitmentnonce_fully_valid"])
             assert("value" in final_decoded["vout"][2])
             assert("asset" in final_decoded["vout"][2])
-            # check that it is accepted in the mempool
+            # check that it is accepted in either mempool
             accepted = sidechain.testmempoolaccept([pegin_signed["hex"]])[0]
+            if not accepted["allowed"]:
+                raise Exception(accepted["reject-reason"])
+            accepted = sidechain2.testmempoolaccept([pegin_signed["hex"]])[0]
             if not accepted["allowed"]:
                 raise Exception(accepted["reject-reason"])
             print("Blinded transaction looks ok!") # need this print to distinguish failures in for loop
-        # check if they get mined; since we're trying to mine two double spends, disconnect the nodes
-        disconnect_nodes(sidechain, 3)
-        disconnect_nodes(sidechain2, 2)
-        txid1 = sidechain.sendrawtransaction(pegin_signed1["hex"])
-        blocks = sidechain.generate(3)
-        assert_equal(sidechain.getrawtransaction(txid1, True, blocks[0])["confirmations"], 3)
-        txid2 = sidechain2.sendrawtransaction(pegin_signed2["hex"])
-        blocks = sidechain2.generate(3)
-        assert_equal(sidechain2.getrawtransaction(txid2, True, blocks[0])["confirmations"], 3)
-        # reconnect in case we extend the test
-        connect_nodes_bi(self.nodes, 2, 3)
-        sidechain.generate(10)
 
         print('Success!')