8b56fc0b91 [qa] Test that v0 segwit outputs can't be spent pre-activation (Suhas Daftuar)
ccb8ca42a4 Always enforce SCRIPT_VERIFY_WITNESS with P2SH (Suhas Daftuar)
5c31b20a35 [qa] Remove some pre-activation segwit tests (Suhas Daftuar)
95749a5836 Separate NULLDUMMY enforcement from SEGWIT enforcement (Suhas Daftuar)
ce650182f4 Use P2SH consensus rules for all blocks (Suhas Daftuar)
Pull request description:
As discussed at the IRC meeting back in October (https://botbot.me/freenode/bitcoin-core-dev/2017-10-12/?msg=92231929&page=2), I had looked into the feasibility of enforcing P2SH and SCRIPT_VERIFY_WITNESS back to the genesis block.
The P2SH change is pretty straightforward -- there was only one historical block on mainnet that violated the rule, so I carved out an exception to it, similar to the way we have exceptions for the BIP30 violators.
The segwit change is not entirely as clear. The code changes themselves are relatively straightforward: we can just always turn on SCRIPT_VERIFY_WITNESS whenever P2SH is active. However conceptually, this amounts to splitting up BIP141 into two parts, the part that implements new script rules, and the part that handles witness commitments in blocks.
Arguably though the script rules are really defined in BIP 143 anyway, and so this really amounts to backdating BIP 143 -- script rules for v0 segwit outputs -- back to genesis. So maybe conceptually this isn't so bad...
I don't feel strongly about this change in either direction; I started working on it because I was searching for a way to simplify the way we understand and implement the consensus rules around segwit, but I'm not yet sure whether I think this achieves anything toward that goal.
ping @TheBlueMatt
Tree-SHA512: 73551d4a983eb9792c7ac67f56005822528ac4d1fd52c27cee6d305ebee953f69687ef4ddee8bdc0fec77f77e6b5a9d669750793efee54c076533a095e233042
2c084a6 net: Minor accumulated cleanups (Thomas Snider)
Pull request description:
From now-derelict larger changes I had been working on, here are a series of DRY refactors/cleanups. Net loss of 35 lines of code - a small step in the good fight.
In particular I think operator!= should only ever be implemented as a negation of operator==. Lower chance for errors, and removes the possibility of divergent behavior.
Tree-SHA512: 58bf4b542a4e8e5bc465b508aaa16e9ab51448c3f9bee52cd9db0a64a5c6c5a13e4b4286d0a5aa864934fc58064799f6a88a40a87154fd3a4bd731a72e254393
1bf3f33b46 node: Removed unused wallet-related methods from the Node interface. (Thomas Snider)
b38200459f benchmark: Removed bench/perf.cpp (Thomas Snider)
Pull request description:
Not sure if these should be separate PRs.
First is removal of a platform abstraction for getting cycle counters where possible. Since the benchmarking switch to counting number of iterations over a fixed window instead of counting cycles per iteration, these are unused.
Second is removal of a few methods from the Node interface that seem vestigial from when the concepts of wallet/node were not as clearly separated.
Tree-SHA512: de1460a7d4473ca19db4e2ca845185c63c765d12462c2685044a1f27dedab266cd908bc52235a881a7ad98bc251a4abf4eae523e5f599c169e3511e489f19a0d
Seems providing at least minimal visibility to the failure is a good practice.
The only remaining ignored state is in LoadExternalBlockFile, where logging
would likely be spammy.
To reflect its actual status as a member rather than a global value.
g_failed_blocks was previously global: 2862aca40f
-BEGIN VERIFY SCRIPT-
sed -i 's/g_failed_blocks/m_failed_blocks/g' src/validation.cpp
-END VERIFY SCRIPT-
7d8a8cc Avoid launching as admin when NSIS installer ends. (JeremyRand)
Pull request description:
The Bitcoin Core NSIS script runs with elevated privileges. Unfortunately, this means that it launches Bitcoin Core itself with elevated privileges when the user chooses to launch Bitcoin Core at the end of the installation procedure. This PR works around the issue by having `explorer.exe` launch Bitcoin Core. Seems to be a similar approach to what http://nsis.sourceforge.net/ShellExecAsUser_plug-in does, but without a plugin.
I've tested this with Sysinternals Process Explorer on Windows 10 32-bit. I wouldn't expect any differences in behavior on other Windows releases, but if anyone would like to test on other Windows releases, feel free.
h/t to "UK" at https://mdb-blog.blogspot.se/2013/01/nsis-lunch-program-as-user-from-uac.html?showComment=1410158039989#c2463780017054126736 for the sample code.
Fixes#7990.
Tree-SHA512: f40d6b6e5bb72952dcfbf223b68bfeb9a03bd5638f41b1700f4651f6452ce3fe7468129f6652c4f546210a5fd2521b2574c4b6068c5aea01ed2d719a8a838cd8
aff16fd depends: Add 'make clean' and 'make clean-all' rules (Henrik Jonsson)
Pull request description:
It's useful to have a standard way to clean up the work done by the
depends system when testing changes to it.
Tree-SHA512: d56686ca810c2531d852049420f242178d124d45a2508feeac0acf80408e8a95fc9915fb777ff0cbc4edc496990f5fdecf699c87d7986164c74b8fbf418ebf49
9e50c337c Note new weight field in release-notes. (Matt Corallo)
d0d9112b7 Test new weight field in p2p_segwit (Matt Corallo)
2874709a9 Expose a transaction's weight via RPC (Matt Corallo)
Pull request description:
This seems like an obvious oversight.
Tree-SHA512: defd047de34fb06a31f589e1a4eef68fcae85095cc67b7c8fb434237bb40300d7f3f97e852d3e7226330e26b96943846b7baf6da0cfc79db8d56e9c1f7848ad9
fae58eca93 tests: Avoid copies of CTransaction (MarcoFalke)
Pull request description:
Avoid the copy (or move) constructor of `CTransaction` in test code, whereever a simple reference can be used instead.
Tree-SHA512: 8ef2077a277d6182996f4671722fdc01a90909ae7431c1e52604aab8ed028910615028caf9b4cb07a9b15fdc04939dea2209cc3189dde7d38271256d9fe1076c
This commit finalizes the deprecation of the wallet 'accounts' API by
removing all account arguments and return values.
RPC behaviour is slightly different if the 'accounts' or 'labels' API is
being used. Those behaviour changes are fully documented in the RPC help
text.
All account RPC methods are now deprecated and can only be called if
bitcoind has been started with the -deprecatedrpc=accounts switch.
Affected RPC methods are:
- getaccount
- getaccountaddress
- getaddressesbyaccount
- getreceivedbyaccount
- listaccouts
- listreceivedbyaccount
- move
- setaccount
6a3b0d3 Print to console by default when not run with -daemon (Evan Klitzke)
Pull request description:
Cherry-picked ef6fa1c38e1bd115d1cce155907023d79da379d8 from the "up for grabs" PR: "Smarter default behavior for -printtoconsole" (#12689).
See previous review in #12689.
Tree-SHA512: 8923a89b9c8973286d53e960d3c464b1cd026cd5a5911ba62f9f972c83684417dc4004101815dfe987fc1e1baaec1fdd90748a0866bb5548e974d77b3135d43b
2eb5036c3 macOS: Prevent Xcode 9.3 build warnings (Akio Nakamura)
Pull request description:
This PR intends to solve #12867
1. clang (Apple LLVM version 9.1.0 (clang-902.0.39.1)) warns unused argument '-pie' during compilation. This parameter should pass to the linker, so use '-Wl,-pie' style in configure.ac.
2. The other hand, when linking libbitcoinconsensus.la with passing '-Wl,-pie', ld warns that '-pie' being ignored because not link a main executable. So remove it from $libbitcoinconsensus_la_LDFLAGS in src/Makefile.am.
- In order to apply this change to Makefile, you need to run autogen.sh and ./configure.
- I think no need to add if-clause by target-os because also g++ can recognize/handle '-Wl,-pie'.
- I confirmed that the build was successful on macos10.13.4/XCode9.3 and ubuntu 17.10/g++7.2.0 and that warning was not issued.
Tree-SHA512: 45b7c3881f3ad92172eb2ff6fa90c4dc70a42037450ea4b6fd87613b10c0aa90ebcffd54e7c50b70ba876b7c2e356825950fbf5a7a4e8e25118688d98d7b6ee0
c25321f Add config changes to release notes (Anthony Towns)
5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns)
005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns)
608415d [tests] Unit tests for network-specific config entries (Anthony Towns)
68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns)
d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns)
8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns)
30f9407 [tests] Unit tests for config file sections (Anthony Towns)
95eb66d ArgsManager: support config file sections (Anthony Towns)
4d34fcc ArgsManager: drop m_negated_args (Anthony Towns)
3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns)
Pull request description:
The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) discussed allowing options to bitcoin to have some sensitivity to what network is in use. @theuni suggested having sections in the config file:
<cfields> an alternative to that would be sections in a config file. and on the
cmdline they'd look like namespaces. so, [testnet] port=5. or -testnet::port=5.
This approach is (more or less) supported by `boost::program_options::detail::config_file_iterator` -- when it sees a `[testnet]` section with `port=5`, it will treat that the same as "testnet.port=5". So `[testnet] port=5` (or `testnet.port=5` without the section header) in bitcoin.conf and `-testnet.port=5` on the command line.
The other aspect to this question is possibly limiting some options so that there is no possibility of accidental cross-contamination across networks. For example, if you're using a particular wallet.dat on mainnet, you may not want to accidentally use the same wallet on testnet and risk reusing keys.
I've set this up so that the `-addnode` and `-wallet` options are `NETWORK_ONLY`, so that if you have a bitcoin.conf:
wallet=/secret/wallet.dat
upnp=1
and you run `bitcoind -testnet` or `bitcoind -regtest`, then the `wallet=` setting will be ignored, and should behave as if your bitcoin.conf had specified:
upnp=1
[main]
wallet=/secret/wallet.dat
For any `NETWORK_ONLY` options, if you're using `-testnet` or `-regtest`, you'll have to add the prefix to any command line options. This was necessary for `multiwallet.py` for instance.
I've left the "default" options as taking precedence over network specific ones, which might be backwards. So if you have:
maxmempool=200
[regtest]
maxmempool=100
your maxmempool will still be 200 on regtest. The advantage of doing it this way is that if you have `[regtest] maxmempool=100` in bitcoin.conf, and then say `bitcoind -regtest -maxmempool=200`, the same result is probably in line with what you expect...
The other thing to note is that I'm using the chain names from `chainparamsbase.cpp` / `ChainNameFromCommandLine`, so the sections are `[main]`, `[test]` and `[regtest]`; not `[mainnet]` or `[testnet]` as might be expected.
Thoughts? Ping @MeshCollider @laanwj @jonasschnelli @morcos
Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db
Future commits will deprecate the accounts RPC methods, arguments and
return objects. Set the -deprecatedrpc=accounts switch now so tests
don't break in intermediate commits.
9db48c5634 tests: Remove redundant bytes² (practicalswift)
Pull request description:
This is a follow-up to #12993. As @jnewbery noted `bytes()` is idempotent.
Tree-SHA512: 0eb25e0c2c46f9abaac30f964c5eb422bece1414c840a717d86794424294cb19d995a6db7c8df2a2f4ec84776b05274a637f2c111738f397051f510e57184752
643aad17fa Enable additional flake8 rules (practicalswift)
f020aca297 Minor Python cleanups to make flake8 pass with the new rules enabled (practicalswift)
Pull request description:
Enabled rules:
```
* E242: tab after ','
* E266: too many leading '#' for block comment
* E401: multiple imports on one line
* E402: module level import not at top of file
* E701: multiple statements on one line (colon)
* E901: SyntaxError: invalid syntax
* E902: TokenError: EOF in multi-line string
* F821: undefined name 'Foo'
* W293: blank line contains whitespace
* W606: 'async' and 'await' are reserved keywords starting with Python 3.7
```
Note to reviewers:
* In general we don't allow whitespace cleanups to existing code, but in order to allow for enabling Travis checking for these rules a few smaller whitespace cleanups had to made as part of this PR.
* Use [this `?w=1` link](https://github.com/bitcoin/bitcoin/pull/12987/files?w=1) to show a diff without whitespace changes.
Before this commit:
```
$ flake8 -qq --statistics --ignore=B,C,E,F,I,N,W --select=E112,E113,E115,E116,E125,E131,E133,E223,E224,E242,E266,E271,E272,E273,E274,E275,E304,E306,E401,E402,E502,E701,E702,E703,E714,E721,E741,E742,E743,F401,E901,E902,F402,F404,F406,F407,F601,F602,F621,F622,F631,F701,F702,F703,F704,F705,F706,F707,F811,F812,F821,F822,F823,F831,F841,W292,W293,W504,W601,W602,W603,W604,W605,W606 .
5 E266 too many leading '#' for block comment
4 E401 multiple imports on one line
6 E402 module level import not at top of file
5 E701 multiple statements on one line (colon)
1 F812 list comprehension redefines 'n' from line 159
4 F821 undefined name 'ConnectionRefusedError'
28 W293 blank line contains whitespace
```
After this commit:
```
$ flake8 -qq --statistics --ignore=B,C,E,F,I,N,W --select=E112,E113,E115,E116,E125,E131,E133,E223,E224,E242,E266,E271,E272,E273,E274,E275,E304,E306,E401,E402,E502,E701,E702,E703,E714,E721,E741,E742,E743,F401,E901,E902,F402,F404,F406,F407,F601,F602,F621,F622,F631,F701,F702,F703,F704,F705,F706,F707,F811,F812,F821,F822,F823,F831,F841,W292,W293,W504,W601,W602,W603,W604,W605,W606 .
$
```
Tree-SHA512: fc7d5e752298a50d4248afc620ee2c173135b4ca008e48e02913ac968e5a24a5fd5396926047ec62f1d580d537434ccae01f249bb2f3338fa59dc630bf97ca7a
While this isn't a supported build configuration, some build
systems need to build without going through our autotools steps,
so defaulting to something sane may make it easier to build.
Specifically, this fixes the inability to build
rust-bitcoinconsensus on some non-x86 platforms. It needs to build
without our autotools/configure steps to ensure correct compile
args are passed from the rust build system to gcc. Converting the
args from the rust build system to gcc would be a lot of
unmaintainable work.
