From d839dd4c1e5ecf890c4fccef8914fa44c2dc68c4 Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Tue, 5 Nov 2019 21:55:38 +1300 Subject: [PATCH 1/9] Merge #17258: Fix issue with conflicted mempool tx in listsinceblock 436ad436434b94982bcb7dc1d13a21949263ef73 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) Pull request description: Closes #8752 by bringing back abandoned #10470. This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future. For more context, #8757 was closed in favor of #10470. ACKs for top commit: instagibbs: utACK https://github.com/bitcoin/bitcoin/pull/17258/commits/436ad436434b94982bcb7dc1d13a21949263ef73 kallewoof: utACK 436ad436434b94982bcb7dc1d13a21949263ef73 jonatack: I'm not qualifed to give an ACK here but 436ad436434b94982bcb7dc1d13a21949263ef73 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp: Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9 --- src/wallet/rpcwallet.cpp | 2 +- test/functional/wallet_listsinceblock.py | 65 ++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 70cf1f10f0..c55f919ea8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1621,7 +1621,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request) for (const std::pair& pairWtx : pwallet->mapWallet) { CWalletTx tx = pairWtx.second; - if (depth == -1 || tx.GetDepthInMainChain() < depth) { + if (depth == -1 || abs(tx.GetDepthInMainChain()) < depth) { ListTransactions(pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */); } } diff --git a/test/functional/wallet_listsinceblock.py b/test/functional/wallet_listsinceblock.py index 18ab146098..94f6f01357 100755 --- a/test/functional/wallet_listsinceblock.py +++ b/test/functional/wallet_listsinceblock.py @@ -5,13 +5,17 @@ """Test the listsincelast RPC.""" from test_framework.test_framework import BitcoinTestFramework +from test_framework.messages import BIP125_SEQUENCE_NUMBER from test_framework.util import ( assert_array_result, assert_equal, assert_raises_rpc_error, connect_nodes, + isolate_node, + reconnect_isolated_node, ) +from decimal import Decimal class ListSinceBlockTest(BitcoinTestFramework): def set_test_params(self): @@ -33,6 +37,7 @@ class ListSinceBlockTest(BitcoinTestFramework): self.test_reorg() self.test_double_spend() self.test_double_send() + self.double_spends_filtered() def test_no_blockhash(self): txid = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1) @@ -289,5 +294,65 @@ class ListSinceBlockTest(BitcoinTestFramework): if tx['txid'] == txid1: assert_equal(tx['confirmations'], 2) + def double_spends_filtered(self): + ''' + `listsinceblock` was returning conflicted transactions even if they + occurred before the specified cutoff blockhash + ''' + spending_node = self.nodes[2] + double_spending_node = self.nodes[3] + dest_address = spending_node.getnewaddress() + + tx_input = dict( + sequence=BIP125_SEQUENCE_NUMBER, **next(u for u in spending_node.listunspent())) + rawtx = spending_node.createrawtransaction( + [tx_input], {dest_address: tx_input["amount"] - Decimal("0.00051000"), + spending_node.getrawchangeaddress(): Decimal("0.00050000")}) + double_rawtx = spending_node.createrawtransaction( + [tx_input], {dest_address: tx_input["amount"] - Decimal("0.00052000"), + spending_node.getrawchangeaddress(): Decimal("0.00050000")}) + + isolate_node(double_spending_node) + + signedtx = spending_node.signrawtransactionwithwallet(rawtx) + orig_tx_id = spending_node.sendrawtransaction(signedtx["hex"]) + original_tx = spending_node.gettransaction(orig_tx_id) + + double_signedtx = spending_node.signrawtransactionwithwallet(double_rawtx) + dbl_tx_id = double_spending_node.sendrawtransaction(double_signedtx["hex"]) + double_tx = double_spending_node.getrawtransaction(dbl_tx_id, 1) + lastblockhash = double_spending_node.generate(1)[0] + + reconnect_isolated_node(double_spending_node, 2) + self.sync_all() + spending_node.invalidateblock(lastblockhash) + + # check that both transactions exist + block_hash = spending_node.listsinceblock( + spending_node.getblockhash(spending_node.getblockcount())) + original_found = False + double_found = False + for tx in block_hash['transactions']: + if tx['txid'] == original_tx['txid']: + original_found = True + if tx['txid'] == double_tx['txid']: + double_found = True + assert_equal(original_found, True) + assert_equal(double_found, True) + + lastblockhash = spending_node.generate(1)[0] + + # check that neither transaction exists + block_hash = spending_node.listsinceblock(lastblockhash) + original_found = False + double_found = False + for tx in block_hash['transactions']: + if tx['txid'] == original_tx['txid']: + original_found = True + if tx['txid'] == double_tx['txid']: + double_found = True + assert_equal(original_found, False) + assert_equal(double_found, False) + if __name__ == '__main__': ListSinceBlockTest().main() From b3488835af0c783a5200a44c0fd80a1fe0b51cda Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 5 Nov 2019 19:25:06 +0100 Subject: [PATCH 2/9] Merge #17044: init: Remove auto-import of `bootstrap.dat` and associated code 104f7de5934f13b837fcf21f6d6b2559799eabe2 remove old bootstrap relevant code (tryphe) Pull request description: This picks up #15954 I fixed the code and added at a functional test utilizing the scripts in `contrib/linearize` as suggested by @MarcoFalke . ACKs for top commit: laanwj: ACK 104f7de5934f13b837fcf21f6d6b2559799eabe2 Tree-SHA512: acac9f285f9785fcbc3afc78118461e45bec2962f90ab90e9f82f3ad28adc90a44f0443b712458ccf486e46d891eb8a67f53e7bee5fa6d89e4387814fe03f117 --- doc/developer-notes.md | 2 +- doc/release-notes-15954.md | 4 ++++ src/init.cpp | 18 +----------------- 3 files changed, 6 insertions(+), 18 deletions(-) create mode 100644 doc/release-notes-15954.md diff --git a/doc/developer-notes.md b/doc/developer-notes.md index df224f89cd..d5f1839d06 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -453,7 +453,7 @@ Threads - ThreadScriptCheck : Verifies block scripts. -- ThreadImport : Loads blocks from blk*.dat files or bootstrap.dat. +- ThreadImport : Loads blocks from `blk*.dat` files or `-loadblock=`. - ThreadDNSAddressSeed : Loads addresses of peers from the DNS. diff --git a/doc/release-notes-15954.md b/doc/release-notes-15954.md new file mode 100644 index 0000000000..f4d2c5688c --- /dev/null +++ b/doc/release-notes-15954.md @@ -0,0 +1,4 @@ +Configuration option changes +----------------------------- + +Importing blocks upon startup via the `bootstrap.dat` file no longer occurs by default. The file must now be specified with `-loadblock=`. diff --git a/src/init.cpp b/src/init.cpp index fed793794b..c3a4afa50e 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -509,7 +509,7 @@ void SetupServerArgs(NodeContext& node) argsman.AddArg("-dbcache=", strprintf("Maximum database cache size MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-debuglogfile=", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-includeconf=", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); - argsman.AddArg("-loadblock=", "Imports blocks from external blk000??.dat file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxorphantxsize=", strprintf("Maximum total size of all orphan transactions in megabytes (default: %u)", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxrecsigsage=", strprintf("Number of seconds to keep LLMQ recovery sigs (default: %u)", llmq::DEFAULT_MAX_RECOVERED_SIGS_AGE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); @@ -900,22 +900,6 @@ static void ThreadImport(ChainstateManager& chainman, std::vector vImp LoadGenesisBlock(chainparams); } - // hardcoded $DATADIR/bootstrap.dat - fs::path pathBootstrap = GetDataDir() / "bootstrap.dat"; - if (fs::exists(pathBootstrap)) { - FILE *file = fsbridge::fopen(pathBootstrap, "rb"); - if (file) { - fs::path pathBootstrapOld = GetDataDir() / "bootstrap.dat.old"; - LogPrintf("Importing bootstrap.dat...\n"); - LoadExternalBlockFile(chainparams, file); - if (!RenameOver(pathBootstrap, pathBootstrapOld)) { - throw std::runtime_error("Rename failed"); - } - } else { - LogPrintf("Warning: Could not open bootstrap file %s\n", pathBootstrap.string()); - } - } - // -loadblock= for (const fs::path& path : vImportFiles) { FILE *file = fsbridge::fopen(path, "rb"); From 0882c487e3cdd75f435a066229076514065deca7 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 6 Nov 2019 00:04:22 +0100 Subject: [PATCH 3/9] Merge #17382: rpc: Remove unused boost::this_thread::interruption_point fa5facd3e72b6d61374b0b93b722b55e2b090020 rpc: Remove unused boost::this_thread::interruption_point (MarcoFalke) Pull request description: There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points However, the rpc threads are `std::thread`, which does not have an `std::thread::interrupt` member function to request interruption: https://dev.visucore.com/bitcoin/doxygen/httpserver_8cpp.html#ae1a63374e18b9abd348eb74e4243ea34 Thus, the interruption points can be removed. ACKs for top commit: laanwj: ACK fa5facd3e72b6d61374b0b93b722b55e2b090020, this does nothing. practicalswift: ACK fa5facd3e72b6d61374b0b93b722b55e2b090020 jamesob: ACK https://github.com/bitcoin/bitcoin/pull/17382/commits/fa5facd3e72b6d61374b0b93b722b55e2b090020 Tree-SHA512: 4e29a44df1f2702cbd1ffdffa559440a8bb800baab64b4116e2c3d27cd64d8d1e8aafe1dc21b1a4e3988470d03be19cae294bd5669f7abf6d487685dc8fd8d7e --- src/node/coinstats.cpp | 2 -- src/rpc/blockchain.cpp | 2 -- test/lint/lint-includes.sh | 1 - 3 files changed, 5 deletions(-) diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index d7e0a9ca12..dc9f044440 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -16,8 +16,6 @@ #include -#include - static uint64_t GetBogoSize(const CScript& scriptPubKey) { return 32 /* txid */ + diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index c56bed66ff..86e476d846 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -47,8 +47,6 @@ #include -#include // boost::thread::interrupt - #include #include #include diff --git a/test/lint/lint-includes.sh b/test/lint/lint-includes.sh index 5f47a7914a..8a5066cfa1 100755 --- a/test/lint/lint-includes.sh +++ b/test/lint/lint-includes.sh @@ -71,7 +71,6 @@ EXPECTED_BOOST_INCLUDES=( boost/test/unit_test.hpp boost/thread.hpp boost/thread/condition_variable.hpp - boost/thread/thread.hpp boost/variant.hpp boost/variant/apply_visitor.hpp boost/variant/static_visitor.hpp From 97308cf85559ad944e1aa2714487170226f4cfc8 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 5 Nov 2019 21:08:31 -0500 Subject: [PATCH 4/9] Merge #17370: doc: Update doc/bips.md with recent changes in master fa7f5a4d2a2581cc25125311892a80efc2c494e2 doc: Update doc/bips.md with recent changes in master (MarcoFalke) Pull request description: Follow-up to #17165 ACKs for top commit: jonatack: ACK fa7f5a4d2a2581cc25125311892a80efc2c494e2. Verified markdown view at https://github.com/MarcoFalke/bitcoin-core/blob/1911-docBips/doc/bips.md and the urls in the links. Some of the PRs are indicated with # and some without, but this is the case over the whole document. laanwj: ACK fa7f5a4d2a2581cc25125311892a80efc2c494e2 fanquake: ACK fa7f5a4d2a2581cc25125311892a80efc2c494e2 Tree-SHA512: 31782b5f1f2f10b1189f05f010f908c183dbe723477ca1c46ad1d3bee5ea483335847008a7fe48d72373ccd39b84e0b950d0d1b23e457cb70f34210c5f2dc6aa --- doc/bips.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/bips.md b/doc/bips.md index eae02ccb8a..3f2d83213a 100644 --- a/doc/bips.md +++ b/doc/bips.md @@ -19,7 +19,11 @@ BIPs that are implemented by Dash Core (up-to-date up to **v18.0**): * [`BIP 65`](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki): The CHECKLOCKTIMEVERIFY softfork was merged in **v0.12.0** ([PR #6351](https://github.com/bitcoin/bitcoin/pull/6351)), and backported to **v0.11.2** and **v0.10.4**. Mempool-only CLTV was added in [PR #6124](https://github.com/bitcoin/bitcoin/pull/6124). * [`BIP 66`](https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki): The strict DER rules and associated version 3 blocks have been implemented since **v0.10.0** ([PR #5713](https://github.com/bitcoin/bitcoin/pull/5713)). * [`BIP 68`](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki): Sequence locks have been implemented as of **v0.12.1** ([PR #7184](https://github.com/bitcoin/bitcoin/pull/7184)), and have been activated since *block 419328*. -* [`BIP 70`](https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki) [`71`](https://github.com/bitcoin/bips/blob/master/bip-0071.mediawiki) [`72`](https://github.com/bitcoin/bips/blob/master/bip-0072.mediawiki): Payment Protocol support has been available in Bitcoin Core GUI since **v0.9.0** ([PR #5216](https://github.com/bitcoin/bitcoin/pull/5216)). Support can be optionally disabled at build time since **v0.18.0** ([PR 4350](https://github.com/dashpay/dash/pull/4350)). +* [`BIP 70`](https://github.com/bitcoin/bips/blob/master/bip-0070.mediawiki) [`71`](https://github.com/bitcoin/bips/blob/master/bip-0071.mediawiki) [`72`](https://github.com/bitcoin/bips/blob/master/bip-0072.mediawiki): + Payment Protocol support has been available in Dash Core GUI since **v0.9.0** ([PR #5216](https://github.com/bitcoin/bitcoin/pull/5216)). + Support can be optionally disabled at build time since **v0.18.0** ([PR 14451](https://github.com/bitcoin/bitcoin/pull/14451)), + and it is disabled by default at build time since **v0.19.0** ([PR #15584](https://github.com/bitcoin/bitcoin/pull/15584)). + It has been removed as of **v0.20.0** ([PR 17165](https://github.com/bitcoin/bitcoin/pull/17165)). * [`BIP 90`](https://github.com/bitcoin/bips/blob/master/bip-0090.mediawiki): Trigger mechanism for activation of BIPs 34, 65, and 66 has been simplified to block height checks since **v0.14.0** ([PR #8391](https://github.com/bitcoin/bitcoin/pull/8391)). * [`BIP 111`](https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki): `NODE_BLOOM` service bit added, and enforced for all peer versions as of **v0.13.0** ([PR #6579](https://github.com/bitcoin/bitcoin/pull/6579) and [PR #6641](https://github.com/bitcoin/bitcoin/pull/6641)). * [`BIP 112`](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki): The CHECKSEQUENCEVERIFY opcode has been implemented since **v0.12.1** ([PR #7524](https://github.com/bitcoin/bitcoin/pull/7524)) and has been activated since *block 419328*. From cb1a2f3d5ea3f4727fbf715f2912a0d129ad2df2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 6 Nov 2019 15:18:34 -0500 Subject: [PATCH 5/9] Merge #17340: Tests: speed up fundrawtransaction test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit af7bae734089f6af0029b0887932ccd9a469e12e [tests] Don't stop-start unnecessarily in rpc_fundrawtransaction.py (John Newbery) 9a8505299ba392acbab4647963113b0c29495f1d [tests] Use -whitelist in rpc_fundrawtransaction.py (John Newbery) 646b593bbd0db113c6e45ab92177b8f5251e8710 [tests] Speed up rpc_fundrawtransaction.py (John Newbery) Pull request description: Speed up rpc_fundrawtransaction.py Most of the time in rpc_fundrawtransaction.py is spent waiting for unconfirmed transactions to propagate. Net processing adds a poisson random delay to the time it will INV transactions with a mean interval of 5 seconds. Calls like the following: ``` self.nodes[2].sendrawtransaction(signedTx['hex']) self.sync_all() self.nodes[1].generate(1) ```` will therefore introduce a delay waiting for the mempools to sync. Instead just generate the block on the node that sent the transaction: ``` self.nodes[2].sendrawtransaction(signedTx['hex']) self.nodes[2].generate(1) ``` rpc_fundrawtransaction.py is not intended to be a test for transaction relay, so it's ok to do this. ACKs for top commit: MarcoFalke: ACK af7bae734089f6af0029b0887932ccd9a469e12e 🛴 Tree-SHA512: db3407d871bfdc99a02e7304b07239dd3585ac47f27f020f1a70608b7f6386b134343c01f3e4d1c246ce734676755897671999695068d6388602fb042d178780 --- test/functional/rpc_fundrawtransaction.py | 34 ++++++----------------- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index bf9a04fb1e..a12910e232 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -29,6 +29,9 @@ class RawTransactionsTest(BitcoinTestFramework): self.num_nodes = 4 self.setup_clean_chain = True self.extra_args = [['-usehd=0']] * self.num_nodes + # This test isn't testing tx relay. Set whitelist on the peers for + # instant tx relay. + self.extra_args = [['-whitelist=127.0.0.1']] * self.num_nodes def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -457,8 +460,7 @@ class RawTransactionsTest(BitcoinTestFramework): # send 12 DASH to msig addr self.nodes[0].sendtoaddress(mSigObj, 12) - self.sync_all() - self.nodes[1].generate(1) + self.nodes[0].generate(1) self.sync_all() oldBalance = self.nodes[1].getbalance() @@ -469,8 +471,7 @@ class RawTransactionsTest(BitcoinTestFramework): signedTx = self.nodes[2].signrawtransactionwithwallet(fundedTx['hex']) self.nodes[2].sendrawtransaction(signedTx['hex']) - self.sync_all() - self.nodes[1].generate(1) + self.nodes[2].generate(1) self.sync_all() # Make sure funds are received at node1. @@ -480,22 +481,6 @@ class RawTransactionsTest(BitcoinTestFramework): self.log.info("Test fundrawtxn with locked wallet") self.nodes[1].encryptwallet("test") - self.stop_nodes() - - self.start_nodes() - # This test is not meant to test fee estimation and we'd like - # to be sure all txns are sent at a consistent desired feerate. - for node in self.nodes: - node.settxfee(self.min_relay_tx_fee) - - connect_nodes(self.nodes[0], 1) - connect_nodes(self.nodes[1], 2) - connect_nodes(self.nodes[0], 2) - connect_nodes(self.nodes[0], 3) - # Again lock the watchonly UTXO or nodes[0] may spend it, because - # lockunspent is memory-only and thus lost on restart. - self.nodes[0].lockunspent(False, [{"txid": self.watchonly_txid, "vout": self.watchonly_vout}]) - self.sync_all() # Drain the keypool. self.nodes[1].getnewaddress() @@ -535,8 +520,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Empty node1, send some small coins from node0 to node1. self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), self.nodes[1].getbalance(), "", "", True) - self.sync_all() - self.nodes[0].generate(1) + self.nodes[1].generate(1) self.sync_all() for i in range(0,20): @@ -564,8 +548,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Again, empty node1, send some small coins from node0 to node1. self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), self.nodes[1].getbalance(), "", "", True) - self.sync_all() - self.nodes[0].generate(1) + self.nodes[1].generate(1) self.sync_all() for i in range(0,20): @@ -582,8 +565,7 @@ class RawTransactionsTest(BitcoinTestFramework): fundedTx = self.nodes[1].fundrawtransaction(rawtx) fundedAndSignedTx = self.nodes[1].signrawtransactionwithwallet(fundedTx['hex']) self.nodes[1].sendrawtransaction(fundedAndSignedTx['hex']) - self.sync_all() - self.nodes[0].generate(1) + self.nodes[1].generate(1) self.sync_all() assert_equal(oldBalance+Decimal('500.19000000'), self.nodes[0].getbalance()) #0.19+block reward From 0a5587c2f7216298b8cafbecbb42a7287458ccdd Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 12 Nov 2019 14:53:31 -0500 Subject: [PATCH 6/9] Merge #17435: test: check custom ancestor limit in mempool_packages.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 49997813a4db388b2810e5e27ef771e8aa6a1f03 test: check custom ancestor limit in mempool_packages.py (Sebastian Falbesoner) Pull request description: The functional test `mempool_packages.py` starts one node with default ancestor/descendant limit settings and one with a custom, reduced ancestor limit (currently `-limitancestorcount=5`). The effect of the latter had not been tested yet though. This is approached in this PR by checking on the expected mempool contents of node1 after the node0 ancestor tests are done, via the following three conditions: - the # of txs in the node1 mempool is equal to the the limit - all txs in node1 mempool are a subset of txs in node0 mempool - the node1 mempool txs match the start of the constructed tx-chain Note that this still doesn't *fully* check the expected mempool of node1 (e.g. that it isn't influenced by `prioritisetransaction` RPC on node0), hence I add another TODO. In the future it would make sense to also set a custom descendant limit when the second TODO about checking node1's mempool is approached: https://github.com/bitcoin/bitcoin/blob/89e93135aedf984f7a98771f047e2beb6cdbdb8e/test/functional/mempool_packages.py#L228 ACKs for top commit: MarcoFalke: ACK 49997813a4db388b2810e5e27ef771e8aa6a1f03 👲 Tree-SHA512: d3a1d19fb49731238ad08ee7c02e2fa81a227e3b4ef3340d68598de42ddb62be9161134f6b8e08fa76b8c9faa02fecfa01111159642e20e9f358292a757b7608 --- test/functional/mempool_packages.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py index da70fc31f7..025f94cf24 100755 --- a/test/functional/mempool_packages.py +++ b/test/functional/mempool_packages.py @@ -10,13 +10,19 @@ from test_framework.messages import COIN from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, assert_raises_rpc_error, satoshi_round +# default limits MAX_ANCESTORS = 25 MAX_DESCENDANTS = 25 +# custom limits for node1 +MAX_ANCESTORS_CUSTOM = 5 class MempoolPackagesTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 2 - self.extra_args = [["-maxorphantxsize=1000"], ["-maxorphantxsize=1000", "-limitancestorcount=5"]] + self.extra_args = [ + ["-maxorphantxsize=1000"], + ["-maxorphantxsize=1000", "-limitancestorcount={}".format(MAX_ANCESTORS_CUSTOM)], + ] def skip_test_if_missing_module(self): self.skip_if_no_wallet() @@ -184,7 +190,14 @@ class MempoolPackagesTest(BitcoinTestFramework): assert_equal(mempool[x]['descendantfees'], descendant_fees * COIN + 2000) assert_equal(mempool[x]['fees']['descendant'], descendant_fees+satoshi_round(0.00002)) - # TODO: check that node1's mempool is as expected + # Check that node1's mempool is as expected (-> custom ancestor limit) + mempool0 = self.nodes[0].getrawmempool(False) + mempool1 = self.nodes[1].getrawmempool(False) + assert_equal(len(mempool1), MAX_ANCESTORS_CUSTOM) + assert set(mempool1).issubset(set(mempool0)) + for tx in chain[:MAX_ANCESTORS_CUSTOM]: + assert tx in mempool1 + # TODO: more detailed check of node1's mempool (fees etc.) # TODO: test ancestor size limits From 640616ff10db18717ad15ddaefe33e754958c485 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 15 Nov 2019 12:46:42 -0500 Subject: [PATCH 7/9] Merge #17469: test: Remove fragile assert_memory_usage_stable fac942ca57dce6cfa5655a3ac8664d6a051bc01f test: Remove fragile assert_memory_usage_stable (MarcoFalke) Pull request description: This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it. Example failures: * https://travis-ci.org/bitcoin/bitcoin/jobs/611497963#L14517 * https://travis-ci.org/MarcoFalke/bitcoin-core/jobs/611029104#L3876 ACKs for top commit: jamesob: ACK https://github.com/bitcoin/bitcoin/pull/17469/commits/fac942ca57dce6cfa5655a3ac8664d6a051bc01f Tree-SHA512: 3577e7ce5891d221cb798454589ba796ed0c06621a26351bb919c23bc6bb46aafcd0b11cb02bbfde64b74d67cb2950da44959a7ecdc436491a34e8b045c1ccf4 --- test/functional/p2p_invalid_messages.py | 31 +++++-------- test/functional/test_framework/test_node.py | 49 --------------------- 2 files changed, 12 insertions(+), 68 deletions(-) diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 5cffdc0802..d9e24dd5fc 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -4,7 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test node responses to invalid network messages.""" import asyncio -import os import struct import sys @@ -66,27 +65,21 @@ class InvalidMessagesTest(BitcoinTestFramework): msg_at_size = msg_unrecognized(str_data="b" * valid_data_limit) assert len(msg_at_size.serialize()) == msg_limit - increase_allowed = 0.5 - if [s for s in os.environ.get("BITCOIN_CONFIG", "").split(" ") if "--with-sanitizers" in s and "address" in s]: - increase_allowed = 3.5 - with node.assert_memory_usage_stable(increase_allowed=increase_allowed): - self.log.info( - "Sending a bunch of large, junk messages to test " - "memory exhaustion. May take a bit...") + self.log.info("Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...") - # Run a bunch of times to test for memory exhaustion. - for _ in range(80): - node.p2p.send_message(msg_at_size) + # Run a bunch of times to test for memory exhaustion. + for _ in range(80): + node.p2p.send_message(msg_at_size) - # Check that, even though the node is being hammered by nonsense from one - # connection, it can still service other peers in a timely way. - for _ in range(20): - conn2.sync_with_ping(timeout=2) + # Check that, even though the node is being hammered by nonsense from one + # connection, it can still service other peers in a timely way. + for _ in range(20): + conn2.sync_with_ping(timeout=2) - # Peer 1, despite serving up a bunch of nonsense, should still be connected. - self.log.info("Waiting for node to drop junk messages.") - node.p2p.sync_with_ping(timeout=320) - assert node.p2p.is_connected + # Peer 1, despite serving up a bunch of nonsense, should still be connected. + self.log.info("Waiting for node to drop junk messages.") + node.p2p.sync_with_ping(timeout=320) + assert node.p2p.is_connected # # 1. diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 1e1d7c8b15..b909563e30 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -148,28 +148,6 @@ class TestNode(): ] return PRIV_KEYS[self.index] - def get_mem_rss_kilobytes(self): - """Get the memory usage (RSS) per `ps`. - - If process is stopped or `ps` is unavailable, return None. - """ - if not (self.running and self.process): - self.log.warning("Couldn't get memory usage; process isn't running.") - return None - - try: - return int(subprocess.check_output( - "ps h -o rss {}".format(self.process.pid), - shell=True, stderr=subprocess.DEVNULL).strip()) - - # Catching `Exception` broadly to avoid failing on platforms where ps - # isn't installed or doesn't work as expected, e.g. OpenBSD. - # - # We could later use something like `psutils` to work across platforms. - except Exception: - self.log.exception("Unable to get memory usage") - return None - def _node_msg(self, msg: str) -> str: """Return a modified msg that identifies this node by its index as a debugging aid.""" return "[node %d] %s" % (self.index, msg) @@ -358,33 +336,6 @@ class TestNode(): time.sleep(0.05) self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log)) - @contextlib.contextmanager - def assert_memory_usage_stable(self, *, increase_allowed=0.03): - """Context manager that allows the user to assert that a node's memory usage (RSS) - hasn't increased beyond some threshold percentage. - - Args: - increase_allowed (float): the fractional increase in memory allowed until failure; - e.g. `0.12` for up to 12% increase allowed. - """ - before_memory_usage = self.get_mem_rss_kilobytes() - - yield - - after_memory_usage = self.get_mem_rss_kilobytes() - - if not (before_memory_usage and after_memory_usage): - self.log.warning("Unable to detect memory usage (RSS) - skipping memory check.") - return - - perc_increase_memory_usage = 1 - (float(before_memory_usage) / after_memory_usage) - - if perc_increase_memory_usage > increase_allowed: - self._raise_assertion_error( - "Memory usage increased over threshold of {:.3f}% from {} to {} ({:.3f}%)".format( - increase_allowed * 100, before_memory_usage, after_memory_usage, - perc_increase_memory_usage * 100)) - @contextlib.contextmanager def profile_with_perf(self, profile_name): """ From 2b035a75f2e2221827c6f68ef748acfb1c4fa795 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 15 Nov 2019 14:43:43 -0500 Subject: [PATCH 8/9] Merge #17455: tests: Update valgrind suppressions d604b4cc8c112a38976c4662cbdc3217a0e5b370 tests: Update valgrind suppressions (practicalswift) Pull request description: Update `valgrind` suppressions. To test this PR: ``` $ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin $ valgrind --suppressions=contrib/valgrind.supp src/bench/bench_bitcoin -evals=1 \ -scaling=0.0 ``` Top commit has no ACKs. Tree-SHA512: 79cb318b5b9171e74d0bd0b89cc688ad4531b134182b06c2942c46058c19b45723c391b781e8ccd157a14fbf6a14588764c7728c5506c73ae237dde9f44db2f6 --- contrib/valgrind.supp | 71 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/contrib/valgrind.supp b/contrib/valgrind.supp index e236514a15..06df5636b5 100644 --- a/contrib/valgrind.supp +++ b/contrib/valgrind.supp @@ -6,7 +6,14 @@ # Example use: # $ valgrind --suppressions=contrib/valgrind.supp src/test/test_dash # $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \ -# --show-leak-kinds=all src/test/test_dash --log_level=test_suite +# --show-leak-kinds=all src/test/test_dash +# +# To create suppressions for found issues, use the --gen-suppressions=all option: +# $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \ +# --show-leak-kinds=all --gen-suppressions=all --show-reachable=yes \ +# --error-limit=no src/test/test_dash +# +# Note that suppressions may depend on OS and/or library versions. { Suppress libstdc++ warning - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65434 Memcheck:Leak @@ -26,6 +33,14 @@ obj:*/libdb_cxx-*.so fun:__log_put_record } +{ + Suppress libdb warning + Memcheck:Param + pwrite64(buf) + fun:pwrite + fun:__os_io + obj:*/libdb_cxx-*.so +} { Suppress leveldb warning (leveldb::InitModule()) - https://github.com/google/leveldb/issues/113 Memcheck:Leak @@ -41,3 +56,57 @@ ... fun:_ZN7leveldbL14InitDefaultEnvEv } +{ + Suppress wcsnrtombs glibc SSE4 warning (could be related: https://stroika.atlassian.net/browse/STK-626) + Memcheck:Addr16 + fun:__wcsnlen_sse4_1 + fun:wcsnrtombs +} +{ + Suppress boost::filesystem warning (fixed in boost 1.70: https://github.com/boostorg/filesystem/commit/bbe9d1771e5d679b3f10c42a58fc81f7e8c024a9) + Memcheck:Cond + fun:_ZN5boost10filesystem6detail28directory_iterator_incrementERNS0_18directory_iteratorEPNS_6system10error_codeE + fun:_ZN5boost10filesystem6detail28directory_iterator_constructERNS0_18directory_iteratorERKNS0_4pathEPNS_6system10error_codeE + obj:*/libboost_filesystem.so.* +} +{ + Suppress boost::filesystem warning (could be related: https://stackoverflow.com/questions/9830182/function-boostfilesystemcomplete-being-reported-as-possible-memory-leak-by-v) + Memcheck:Leak + match-leak-kinds: reachable + fun:_Znwm + fun:_ZN5boost10filesystem8absoluteERKNS0_4pathES3_ +} +{ + Suppress boost still reachable memory warning + Memcheck:Leak + match-leak-kinds: reachable + fun:_Znwm + ... + fun:_M_construct_aux + fun:_M_construct + fun:basic_string + fun:path +} +{ + Suppress LogInstance still reachable memory warning + Memcheck:Leak + match-leak-kinds: reachable + fun:_Znwm + fun:_Z11LogInstancev +} +{ + Suppress secp256k1_context_create still reachable memory warning + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:secp256k1_context_create +} +{ + Suppress BCLog::Logger::StartLogging() still reachable memory warning + Memcheck:Leak + match-leak-kinds: reachable + fun:malloc + ... + fun:_ZN5BCLog6Logger12StartLoggingEv +} From bd642f66d806c070091c36e345d5eb22200ef0f3 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 18 Nov 2019 14:23:33 +0100 Subject: [PATCH 9/9] Merge #17488: test: fix "bitcoind already running" warnings on macOS 1c23ea5fe67b88fd72a1ff640dd1bbb21a34fbf4 test: fix bitcoind already running warnings on macOS (fanquake) Pull request description: On macOS, `pidof` installed via brew returns b'' rather than None. Account for this, to remove spurious warnings from the test_runner. ACKs for top commit: laanwj: ACK 1c23ea5fe67b88fd72a1ff640dd1bbb21a34fbf4 Tree-SHA512: 640f4323d4105eac5c7abb52daf80486d5d3b4a074720490ceeb97c3dd8d73a3de9a988d2550f1e2076c620bb10d452b2959d8b723d2ee64f499878909824e31 --- test/functional/test_runner.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index e8c17a1009..002a25679c 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -394,7 +394,8 @@ def main(): def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage=False, args=None, combined_logs_len=0,failfast=False, runs_ci=False): args = args or [] - # Warn if dashd is already running (unix only) + # Warn if dashd is already running + # pidof might fail or return an empty string if bitcoind is not running try: pidof_output = subprocess.check_output(["pidof", "dashd"]) if not (pidof_output is None or pidof_output == b''):