0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1 wallet, refactor: Convert uint256 to Txid in wallet (marcofleon)
c8ed51e62be998f16b8b06201b1df92b73c4220d wallet, refactor: Convert uint256 to Txid in wallet interfaces (marcofleon)
b3214cefe6d880838f36e7801bc2c3068bd98d96 qt, refactor: Convert uint256 to Txid in the GUI (marcofleon)
Pull request description:
This is part of https://github.com/bitcoin/bitcoin/pull/32189.
Converts all instances of transactions from `uint256` to `Txid` in the wallet, GUI, and related interfaces.
ACKs for top commit:
stickies-v:
re-ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase.
achow101:
ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1
furszy:
Code review ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1
Tree-SHA512: 9fd4675db63195c4eed2d14c25015a1821fb597f51404674e4879a44a9cf18f475021a97c5f62f3926b7783ade5a38567386f663acba9f5861f1f59c1309ed60
This reverts commit b2d536100282bd901d3e0be7f7f4a6966e0ef817.
Also, adjust the doc to reflect the new minimum version. Versions 17.6
or 17.11 (or anything in between) may still work on a best-effor basis,
but it is not checked by CI or by developers.
a0eed55398f882d9390e50582b10272d18f2b836 run_command: Enable close_fds option to avoid lingering fds (Luke Dashjr)
c7c356a448657e154e6bad6c39a296cfd6dce30c cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux (Luke Dashjr)
4f5e04da135080291853f71e6f81dd0302224c3a Revert "remove unneeded close_fds option from cpp-subprocess" (Luke Dashjr)
Pull request description:
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).
> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and starting bitcoind again will fail). It's also a potential security issue if a child process is intended to be sandboxed at some point. Not to mention plain ugly :)
>
> cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in https://github.com/bitcoin/bitcoin/pull/29961 rather than fixing this during the migration, but this PR restores it, enables it for RunCommandParseJSON, and optimises it by iterating over /proc/self/fd/ like most other libraries do these days ([eg, glib]> (487b1fd20c/glib/gspawn.c (L1094))) since iterating all possible fd numbers [has been found to be problematic](https://bugzilla.redhat.com/show_bug.cgi?id=1537564).
>
> (Equivalent to https://github.com/bitcoin/bitcoin/pull/22417 was for boost::process)
ACKs for top commit:
achow101:
ACK a0eed55398f882d9390e50582b10272d18f2b836
hebasto:
ACK a0eed55398f882d9390e50582b10272d18f2b836, tested on Ubuntu 25.04:
vasild:
ACK a0eed55398f882d9390e50582b10272d18f2b836
Tree-SHA512: 7dc1cb6cc1f45ff7c4f53512e400baad1a033b4ebf14ba6f6ffa38588314932d6d01ef67b197f081e8202bb802659ac6a87998277797721d6d7b20efde8e9a6b
5bf91ba8800d23402536d758f02198eac0fd7d61 wallet: Drop unused fFromMe from CWalletTx (David Gumberg)
Pull request description:
This has been unused since commit fe52346, this is a re-opening of #9351.
ACKs for top commit:
maflcko:
lgtm ACK 5bf91ba8800d23402536d758f02198eac0fd7d61
achow101:
ACK 5bf91ba8800d23402536d758f02198eac0fd7d61
Tree-SHA512: b9a84f27b6cfe7796dcf629be6a8e01a97d931ea81ef088951d54d6691ffe79d22138baacc632375093cf3176a22c265e30a80f1f63c3bc620d08bf16f6a488f
faf9082a5f689e2e51a474bf654e4e9b6ca29685 test: Fix whitespace in prevector_tests.cpp (MarcoFalke)
fa7f04c8a7b7cbb4a1728bf2c9c6c7c8408b432a refactor: Remove UB in prevector reverse iterators (MarcoFalke)
Pull request description:
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils such as `std::reverse_iterator{it}` or `std::views::reverse` can be used out of the box.
So remove them, along with the ubsan suppressions, that are no longer used.
I've tagged this a refactor, because the code was always dead (unused outside of tests). And since commit 2925bd537cbd8c70594e23f6c4298b7101f7f73d it was completely dead. Also, I could not find a sanitizer that detects this type of UB.
ACKs for top commit:
l0rinc:
tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
achow101:
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
stickies-v:
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685, nice find.
theuni:
utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
Tree-SHA512: 31511d520a1c0fdd65c2e5f1a8ef6fd17464303b6bff88a5d9d9577adfee849d431deb510882b6f4e15e8fb7168861bc0d26fca3bed4278f57a9d6e7b1235dce
62fc42d475df4f23bd93313f95ee7b7eb0d4683f interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq)
c39ca9d4f7bc9ca155692ac949be2e61c0598a97 interfaces: move getTip implementation to miner (Sjors Provoost)
720f201e652885b9e0aec8e62a1bf9590052b320 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq)
e6c2f4ce7a841153510971f0236c527d1a499649 interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq)
02d4bc776bbe002ee624ec2c09d7c3f981be1b17 interfaces: remove redundant coinbase fee check in `waitNext` (ismaelsadeeq)
Pull request description:
#### Motivation
In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](https://github.com/bitcoin/bitcoin/blob/master/src/wallet) and just exposed in [src/interfaces/](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces) instead of being implemented there, so it can be more modular and accessible to unit tests.
However the some methods in the newly added `BlockTemplateImpl` and `MinerImpl` classes partially enforces this guideline, as the implementations of the `submitSolution`, `waitNext`, and `waitTipChanged` methods reside within the class itself.
#### What the PR Does
This PR introduces a simple refactor by moving certain method implementations from `BlockTemplateImpl` into the miner module. It introduces three new functions:
1. Remove rundundant coinbase fee check in `waitNext`
2. **`AddMerkleRootAndCoinbase`**: Computes the block's Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by `submitSolution` before the block is submitted for processing.
3. **`WaitAndCreateNewBlock`**: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns `nullptr`. The `waitNext` method in `BlockTemplateImpl` now simply wraps this function.
4. Move `GetTip` implementation to miner.
5. **`WaitTipChanged`**: Returns the tip when the chain it changes, or `nullopt` if a timeout or interrupt occurs. The `waitTipChanged` method in `MinerImpl` now calls `GetTip` after invoking `ChainTipChanged`, and returns the tip.
#### Behavior Change
- We now only `Assert` for a valid chainman and notifications pointer once.
ACKs for top commit:
achow101:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
Sjors:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
ryanofsky:
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
Tree-SHA512: 502632f94ced81f576b2c43cf015f1527e2c259e6ca253f670f5a6889171e2246372b4e709575701afa3f01d488d6633557fef54f48fe83bbaf1836ac5326c4f
a04f17a1882407db09b0a07338e12877ac1d9e92 doc: warn that CheckBlock() underestimates sigops (Sjors Provoost)
Pull request description:
Counting sigops in the witness requires context that `CheckBlock()` does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
The commit message contains some historical context.
ACKs for top commit:
ismaelsadeeq:
ACK a04f17a1882407db09b0a07338e12877ac1d9e92
ryanofsky:
Code review ACK a04f17a1882407db09b0a07338e12877ac1d9e92
Tree-SHA512: 26528367a7f3cfa8540ef0b90f7aa912c8f0bc057428f20a1fd1d4e232dac77747bc20044f0fcb0ffab8a2e1fb3dbe3dab46be749553a917744ddc7a829025cb
- This commit creates a function `WaitTipChanged` that waits for the connected
tip to change until timeout elapsed.
- This function is now used by `waitTipChanged`
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
fa427ffceeefd368a1ade273501ce4b01133ad4d fuzz: Properly setup wallet in wallet_fees target (MarcoFalke)
Pull request description:
`g_wallet_ptr` is destructed after the `testing_setup`. This is not supported and will lead to issues such as https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2863875857 or https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932.
This could be fixed by fixing the initialization order.
However, the global wallet is also modified in the fuzz target, which is bad fuzzing practise.
So instead fix it by constructing a fresh wallet for each fuzz iteration.
ACKs for top commit:
brunoerg:
code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d
hebasto:
ACK fa427ffceeefd368a1ade273501ce4b01133ad4d, this change fixes the issue when building the "Debug" configuration with MSVC on Windows.
marcofleon:
Code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d
Tree-SHA512: 161b93fc39a609cb16d9ffea7366c5e339bd01712577f0782aedff46c00f79edd2a907807ac83f9fcec687b4bbbe0fd6e6f75e32169639a310e4e7b771078b3b
rend() creates a pointer with offset -1. This is UB, according to the
C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils
such as std::reverse_iterator{it} or std::views::reverse can be used out
of the box.
So remove them, along with the ubsan suppressions, that are no longer
used.
1e0de7a6ba926487c8a075856b74af2a3a0eb8ef fees: document non-monotonic estimation edge case (willcl-ark)
Pull request description:
Closes: https://github.com/bitcoin/bitcoin/issues/11800
In scenarios where data is available for higher targets but not for lower ones, this method *may* return lower fee rates for higher confirmation targets. This could occur if `estimateCombinedFee` returns no valid data (`-1`) for some estimates for a low target, but **does** return valid data for a higher target.
Users of this function should be aware of this potential, if unlikely, inconsistency in behaviour in data-sparse scenarios.
ACKs for top commit:
adamandrews1:
Code review ACK 1e0de7a
ismaelsadeeq:
Code review ACK 1e0de7a6ba926487c8a075856b74af2a3a0eb8ef
glozow:
ACK 1e0de7a6ba926487c8a075856b74af2a3a0eb8ef
Tree-SHA512: 161e5dafdd131570853a89491753ae39a7b725d1a86cab5a7294c2a5939da1a9a5f2c4aca0900e9ad810e828b6e0e636f256384e3d1fda6dd552da189bbbe747
0750249289c092fc8e2e29669fec73a58b873767 mining: document gbt_rule_value helper (Sjors Provoost)
5e87c3ec094d68a7a27dfb7ae665b225ff4dfdb6 scripted-diff: rename gbt_force and gbt_force_name (Sjors Provoost)
Pull request description:
The term "force" is ambiguous and not used in [BIP9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes) where there ! rule prefix is introduced.
E.g. this code is hard to read:
```cpp
if (!gbt_force) {
s.insert(s.begin(), '!');
```
Additionally, #29039 renamed `gbt_vb_name` to `gbt_force_name` which, at least for me, further increased the confusion.
This is a pure (variable rename) refactor (plus documentation) and does not change behavior.
Reminder of how to verify a scripted diff:
```sh
test/lint/commit-script-check.sh origin/master..HEAD
```
ACKs for top commit:
achow101:
ACK 0750249289c092fc8e2e29669fec73a58b873767
janb84:
ACK [0750249](0750249289)
musaHaruna:
ACK [0750249](0750249289)
glozow:
ACK 0750249289c092fc8e2e29669fec73a58b873767, seems sensible
Tree-SHA512: 8c88a273a3b36040f6c641843bd20579d0065b051aad4b39fc14f0d2af2808690dff6772bd8b1a4d9699b72279a700d2661012651bc315433a123dcc8996adaa
8673e8f01917b48a5f5476792f759f44ea49d5a5 txgraph: Special-case singletons in chunk index (optimization) (Pieter Wuille)
abdd9d35a34dd9cd589c1b925ab4241d15e6446c txgraph: Skipping end of cluster has no impact (optimization) (Pieter Wuille)
604acc2c289fb187f55439a29dca02d39163df25 txgraph: Reuse discarded chunkindex entries (optimization) (Pieter Wuille)
c734081454d7d7552b1388a27de648bcdd2e4b3a txgraph: Introduce TxGraph::GetWorstMainChunk (feature) (Pieter Wuille)
394dbe21427ee30cca73d2b1e31f18a00a76a1a1 txgraph: Introduce BlockBuilder interface (feature) (Pieter Wuille)
883df3648ee92841e515bb6d8d259cb7054493b1 txgraph: Generalize GetClusterRefs to support subsections (preparation) (Pieter Wuille)
c28a602e007fcc0275bc34f58690820ebe4c9162 txgraph: Introduce TxGraphImpl observer tracking (preparation) (Pieter Wuille)
9095d8ac1c319cdf1b9d44a1fa5993e9a55ccb21 txgraph: Maintain chunk index (preparation) (Pieter Wuille)
87e74e1242ece5fc30e65d4881f24be3a0d02e44 txgraph: abstract out transaction ordering (refactor) (Pieter Wuille)
2614fea17fe3c179ca3a256fadf498ae8cbc03e0 txgraph: Add GetMainStagingDiagrams function (feature) (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289.
This adds more functionality to the txgraph module, specifically:
* `TxGraph::GetMainStagingDiagrams()`, a function to obtain feerate diagrams for both the main graph and the staged changes to it, including only the clusters that differ between the two.
* `TxGraph::GetBlockBuilder()`, a function to obtain an object which can efficiently iterate the chunks of the (main) graph from high to low chunk feerate, allowing each to be skipped or included.
* `TxGraph::GetWorstMainChunk()`, a function to obtain the last chunk that would be returned by `GetBlockBuilder()`'s returned object, intended for eviction.
ACKs for top commit:
monlovesmango:
reACK 8673e8f01917b48a5f5476792f759f44ea49d5a5
instagibbs:
reACK 8673e8f01917b48a5f5476792f759f44ea49d5a5
glozow:
reACK 8673e8f0191
Tree-SHA512: 5c98c54919c44eb2f9545dfc130e54dfc25b5b54d43cf5ca9bcf46e019b9fd405a572fcd70e71e2a7c5b4b096cfd540a4d09ef1f52ba188504418682f1dfc4af
- Create a new function `AddMerkleRootAndCoinbase` that compute the
block's merkle root, insert the coinbase transaction and the merkle
root into the block.
This interface lets one iterate efficiently over the chunks of the main
graph in a TxGraph, in the same order as CompareMainOrder. Each chunk
can be marked as "included" or "skipped" (and in the latter case,
dependent chunks will be skipped).
This is preparation for a next commit which will introduce a class whose
objects hold references to internals in TxGraphImpl, which disallows
modifications to the graph while such objects exist.
3bbdbc0a5e1b409969cedaf249d1d01dea9bcf73 qt, docs: Unify term "clipboard" (Hennadii Stepanov)
Pull request description:
A translator on Transifex noticed:
> The term "system clipboard" appears twice. The term "clipboard" appears 10 times. Perhaps we could standardize on just saying "clipboard"?
This PR addresses this issue.
ACKs for top commit:
davidgumberg:
ACK 3bbdbc0a5e
pablomartin4btc:
ACK 3bbdbc0a5e1b409969cedaf249d1d01dea9bcf73
Tree-SHA512: 61a100f60890d81122a4b8ce3e2cb7d355c7fb643de3196573f7f9107c6f52fa0b3e7a4f743ce2833e8c67b9cdad3568b761d730fef5c9781f5e1c45252888c4
002b792b9a85100d89e47706c29cf1fd355d9727 gui: decouple WalletModel from RPCExecutor (furszy)
Pull request description:
A more comprehensive fix for the issue described in #837.
Since the `WalletModel` class is unavailable when compiling without wallet support
`(-DENABLE_WALLET=0)`, the RPC executor class should not be coupled to it.
This decoupling ensures GUI compatibility with builds that omit wallet support.
This also drops an extra `#ifdef ENABLE_WALLET` block which is always good.
ACKs for top commit:
w0xlt:
Code Review ACK 002b792b9a
pablomartin4btc:
tACK 002b792b9a85100d89e47706c29cf1fd355d9727
BrandonOdiwuor:
tACK 002b792b9a85100d89e47706c29cf1fd355d9727
hebasto:
ACK 002b792b9a85100d89e47706c29cf1fd355d9727, I have reviewed the code and it looks OK.
Tree-SHA512: a8e6b7e9d88dd8e0ff5e2d0de91be2f85fd0559265267d3bf6cae5a37606cf1ab6bc7415d5817a11006008de362f2ca3557ba772b4e1bd9fbef5f564be3b53bb
ab878a7e741073574336c9c4b1d41c6cf80b0d6a build: simplify *ifaddr handling (fanquake)
Pull request description:
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
`iffaddrs.h` that implements one, but not the other.
ACKs for top commit:
hebasto:
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a. Only addressed my [comment](https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126) and rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189).
TheCharlatan:
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a
Tree-SHA512: 7667305df9fef4728526c7217f85b51e739ec63b38e808da51d6ae65cb6f2696afa5ba82e5a72ed4a7a9b79ffa2402640448af4392587253027122eab7618e30
a58cb3b1c12c8cb75a87375c50f94c4605bb805d qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a55448c003b3f7f3021955fbb351caa miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf343526760fa8f3ed969ac3713212a5b6 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b6f03b761a0ef97e1b8cd133b934714 qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b6cf67a24eb03c51230a4f2b2bf2d64 contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc400ad0adb41a78c65b8abb971e0d622 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
achow101:
ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
TheCharlatan:
Re-ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
1ee698fde2e8652d8eb083749edb8029c003b8db test: refactor: negate signature-s using libsecp256k1 (Sebastian Falbesoner)
Pull request description:
This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by https://github.com/bitcoin-core/secp256k1/pull/69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see https://github.com/bitcoin-core/secp256k1/pull/408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general for scalars modulo the group order.
ACKs for top commit:
achow101:
ACK 1ee698fde2e8652d8eb083749edb8029c003b8db
laanwj:
Code review ACK 1ee698fde2e8652d8eb083749edb8029c003b8db
w0xlt:
ACK 1ee698fde2
rkrux:
tACK 1ee698fde2e8652d8eb083749edb8029c003b8db
Tree-SHA512: dc36ea1572b538d11ae34e1871f310a1cda8083ffb753e93e7ee9d56e91ebd8ec78d35758dfb700254720914b734ef7a071eeef71b6239f19e1e2fb289fb5435
It is confusing that the chain client flush happens between
StopHTTPServer and StopMapPort. Also, it is unused code. Seems best to
just add it back properly when it is needed again.
Counting sigops in the witness and for p2sh requires
context that CheckBlock() does not have, so it only
counts a subset of sigops.
The check here was introduced by Satoshi as a "cleanup" in
f1e1fb4bdef878c8fc1564fa418d44e7541a7e83. With the attempted
introduction of OP_EVAL, it was replaced by the check in
ConnectBlock(). Commit e679ec969c8b22c676ebb10bea1038f6c8f13b33
marked this code as a placeholder for backward compatibility.
Then when P2SH replaced OP_EVAL in 922e8e2929a2e78270868385aa46f96002fbcff3
the phrase "compatibility-breaking" was replaced by a simple
observation that before v0.6 this is how sigops were counted.
It's unclear why the check was kept and there were no review comments
about it.
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
iffaddrs.h that implements one, but not the other.
