forked from bitcoin/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 0
Catch up with upstream #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blockchain.cpp has low unit test coverage. This commit is intended to start improving its code coverage to reasonable levels. One or more follow up commits will complete the task that this commit is starting (though the usefulness of this commit is not dependent upon later commits). Note that these tests were not written based upon a specification of how GetDifficulty *should* work, but rather how it actually *does* work. As a result, if there are any bugs in the current GetDifficulty implementation, these unit tests serve to lock them in rather than expose them. -- Why has blockchain.cpp been modified if this is a unit testing change? Since the existing GetDifficulty function relies on a global variable, chainActive, it was not suitable for unit testing purposes. Both the existing GetDifficulty function and the unit tests now call through to a new, more modular version of GetDifficulty that can work on any chain, not just chainActive. -- Why does blockchain_tests.cpp directly include blockchain.cpp instead of blockchain.h? While the new GetDifficulty function's signature is arguably better than the old one's, it still isn't great, and doesn't seem to warrant inclusion as part of the blockchain.h API, especially since only test code is directly using it. If a better way of exposing the new GetDifficulty function to unit tests exists, please mention it and the commit will be updated accordingly. -- Why is the test fixture named blockchain_difficulty_tests rather than blockchain_tests? The Bitcoin Core policy for naming unit test files is to match the the file under test ("blockchain" becomes "blockchain_tests"). While this commit complies with that, blockchain.cpp is a massive file, such that having all of the unit tests in one file will tend towards disorder. Since there will be a lot more tests added to this file, the intention is to divide up different types of tests into different test fixtures within the same file.
This should (marginally) speed up validationinterface queue draining by avoiding a cs_main lock in one client.
Replace the clang patch with a new and improved version that also fixes the build issues with OpenBSD and FreeBSD's clang, and apply it unconditionally. This needs testing on OSX.
Trailing X=Y arguments are supposed to be passed through unchanged to bdb's configure. This was not the case, at least with OpenBSD 6.2's shell. Fix this by not storing the arguments in a temporary variable but passing "$@" through directly.
0.15.0 introduced a new feeest file format, and support for parsing old versions was never fully added. We now simply fail to read the old format, so remove the dead partial-implementation.
node_network_limited had a race condition, since wait_for_block() doesn't do what you might expect. It only checks the most recent block received over the P2P interface (perhaps we should rename the method wait_for_most_recent_block() to avoid future confusion). The test can fail if the node sends us invs for other blocks, we respond with a getdata, and the node sends us one of those blocks in the 0.05 second wait_until loop window. Fix this by not responding to inv messages with getdata messages.
ee5efad [tests] refactor node_network_limited (John Newbery) b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery) 2e02984 [tests] node_network_limited - remove race condition (John Newbery) dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery) 1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery) Pull request description: Fixes race condition in the node_network_limited test case introduced in #11740. Also tidies up the test and removes redundant duplicate tests. Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011
07947ff Merge #9: [tests] Fix BOOST_CHECK_THROW macro ec849d9 [tests] Fix BOOST_CHECK_THROW macro 31bc9f5 Merge #8: Remove unused Homebrew workaround fa04209 Remove HomeBrew workaround a523e08 Merge #7: Declare single-argument (non-converting) constructors "explicit" a9e53b3 Merge #4: Pull upstream 16a1f7f Merge #3: Pull upstream daf1285 Merge pull request #2 from jgarzik/master f32df99 Merge branch '2016_04_unicode' into bitcoin 280b191 Merge remote-tracking branch 'jgarzik/master' into bitcoin 2740c4f Merge branch '2015_11_escape_plan' into bitcoin git-subtree-dir: src/univalue git-subtree-split: 07947ff
8a93543 Replaces numbered place marker %2 with %1. (251) Pull request description: This PR closes #12015 in which @chen610620 suggests to replace numbered place marker `%2` with `%1`. Calling member function`QString::arg()` on a `QString` object with one arbitrary numbered place marker within the range [1,99] works, because `QString::arg()` replaces the _lowest_ numbered place marker in the `QString` object it is called on. [QString::arg documentation:](http://doc.qt.io/qt-5/qstring.html#arg) > Returns a copy of this string with the lowest numbered place marker replaced by string a, i.e., %1, %2, ..., %99. I suspect that the `%2` marker is just an unfortunate typo or the remainder of a string that used to have multiple numbered place markers. This PR replaces the numbered place marker `%2` with `%1` to avoid any confusion in the future. Tree-SHA512: 0bb40cf3b9824e1eeba0a184e72358b30d20e8261e12deb287155b7cc8317ad0b0787ef1d0671325eb8bccc9e51b3037d737015749338c31cf400930840e56b6
…nly addresses 73041c3 RPC Docs: addmultisigaddress is intended for non-watchonly addresses (Gregory Sanders) Pull request description: Spent a couple hours debugging why my p2sh watchonly funds were not appearing in various accounting calls when address was imported via `addmultisigaddress`. Tree-SHA512: 0673e276e5ca8cdc4c9357bd835a29bd5a994520a78179600944932c700917142930288bf179f5e89b0874beaf1a88bd70129f3a297a46df42a10bab847017bb
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa) Pull request description: Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86
Parse JSONRPCException errors, and avoid JSON decode exception if RPC method returns a plain string.
Change TestNodeCLI.__call__() to return a new instance instead of modifying the existing instance. This way, it's possible to create different cli objects that have their own options (for example -rpcwallet options to connect to different wallets), and options set for a single call (`node.cli(options).method(args)`) will no longer leak into future calls.
Support same get_request and batch methods as AuthServiceProxy
test_framework accepts a new --usecli parameter. Running the test with this parameter will cause all RPCs to be sent through bitcoin-cli rather than directly over http. By default, individual test cases do not support --usecli, and self.supports_cli must be set to True in the set_test_params method. We can make supports_cli default to True in future once we know which tests will fail with use_cli.
Add test coverage for bitcoin-cli multiwallet calls.
Avoid creating very small utxos that would violate an assumption in test_non_standard_witness.
Make CKeyStore automatically known about the redeemscripts necessary for P2SH-P2WPKH (and due to the extra checks in IsMine, also P2WPKH) spending.
This introduces two command line flags (-addresstype and -changetype) which control the type of addresses/outputs created by the GUI and RPCs. Certain RPCs allow overriding these (`getnewaddress` and `getrawchangeaddress`). Supported types are "legacy" (P2PKH and P2SH-multisig), "p2sh-segwit" (P2SH-P2WPKH and P2SH-P2WSH-multisig), and "bech32" (P2WPKH and P2WSH-multisig). A few utility functions are added to the wallet to construct different address type and to add the necessary entries to the wallet file to be compatible with earlier versions (see `CWallet::LearnRelatedScripts`, `GetDestinationForKey`, `GetAllDestinationsForKey`, `CWallet::AddAndGetDestinationForScript`).
Improvements and cleanups by John Newbery
91769d6 [Doc] Fix link for bip 159 pull request (azuchi) Pull request description: The link of the pull request for BIP-159 described in bips.md was a different link. Tree-SHA512: 818ea29259ff84a55282df8b0c59fc4ccd3af08d124a104005ac48e67da4155a8b071b980b1d12c35af3f4a008ba77e5b4ee3242292f6c034cb0006b5532ce69
b224a47 Add address_types test (Pieter Wuille) 7ee54fd Support downgrading after recovered keypool witness keys (Pieter Wuille) 940a219 SegWit wallet support (Pieter Wuille) f37c64e Implicitly know about P2WPKH redeemscripts (Pieter Wuille) 57273f2 [test] Serialize CTransaction with witness by default (Pieter Wuille) cf2c0b6 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille) 37c03d3 Support P2WPKH addresses in create/addmultisig (Pieter Wuille) 3eaa003 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille) 30a27dc Expose method to find key for a single-key destination (Pieter Wuille) 985c795 Improve witness destination types and use them more (Pieter Wuille) cbe1974 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille) 0c8ea63 Abstract out IsSolvable from Witnessifier (Pieter Wuille) Pull request description: This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089. Two new configuration options are added: * `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`. * `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used. All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version. The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key. To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used: * All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date. * All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software. * All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work. These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented. `dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now. Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959
New global variables were introduced in #11403 and not setting them causes: test_bitcoin: wallet/wallet.cpp:4199: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed. unknown location(0): fatal error in "ListCoins": signal: SIGABRT (application abort requested) It's possible to reproduce the failure reliably by running: src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug.
2be2b5d Remove the ending slashes from RPC URI format. (Jacky C) Pull request description: This resolves #11861 (A confusion caused by incorrect information in the release notes). More information can be found at #11861. Tree-SHA512: 35f85854b01a84acd5358e0c9deff881205111120277fa7cdf270801933c2603c2ae04fa4d55d233675c7298c2d37cc60c919f89e7e6091f5c61884025775ab0
… g_change_type f765bb3 Fix ListCoins test failure due to unset g_address_type, g_change_type (Russell Yanofsky) Pull request description: New global variables were introduced in #11403 and not setting them causes: ``` test_bitcoin: wallet/wallet.cpp:4199: CTxDestination GetDestinationForKey(const CPubKey&, OutputType): Assertion `false' failed. unknown location(0): fatal error in "ListCoins": signal: SIGABRT (application abort requested) ``` It's possible to reproduce the failure reliably by running: ``` src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins ``` Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug. Example travis failure: https://travis-ci.org/bitcoin/bitcoin/builds/327642495 Tree-SHA512: 3e0875716f66bc0304cf92a26457e6b54ecfe15ed962f4343577b05fc56bb577554422b7f53949ad6085ac5798ad7816b8176c5b01e050ddbfbb925d2732767a
35c2b1f Fix rare failure in p2p-segwit.py (Suhas Daftuar) Pull request description: Avoid creating very small utxos that would violate an assumption in test_non_standard_witness. Fixes #11953 Tree-SHA512: 5fb7ae68f8731df819bab365923a84568b57227e4112f711fc2574767d15be83acd3e99d0d5bac94a42411a958b13a2119468babefed14efcfdda180004d4166
…x_valid.json 18be3ab Adding test case for SINGLE|ANYONECANPAY hash type in tx_valid.json (Chris Stewart) Pull request description: We are missing a test vector for SINGLE|ANYONECANPAY inside of tx_valid.json. This addresses the issue #12060 Tree-SHA512: e3526113477dbf575c4a844cf489dcfa2c037c6d928af6f97413edc1a8d29cdf2143da96471cdfd3de08bf5ed178117ed67926fd70fd42ca391ac0bb0d08f3fd
a14dbff Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9c [tests] allow tests to be run with --usecli (John Newbery) ff9a363 TestNodeCLI batch emulation (Russell Yanofsky) ca9085a Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952 Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in #11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02
…out sys/) 648bdc8 Trivial: Fix #include sys/fcntl.h to just fcntl.h (without sys/) (Jan Sarenik) Pull request description: http://pubs.opengroup.org/onlinepubs/009695399/functions/fcntl.html http://man7.org/linux/man-pages/man2/fcntl.2.html Tree-SHA512: 82c7e0aba55f34a6fec60bdecb712b65c84422461454f0ae9eed5e1bb31bf585c5a65f49bbdd5a89feb59140a998ad6fcd5573ede123a12b2ba2ff677d95cc2b
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.