The descriptor inference logic would previously always use a dummy
signing provider and would never analyze the witness script of a P2WSH
scriptPubKey.
Note even a valid Miniscript might not always be decodable from Script
without more contextual information (for instance the key preimage for a
pk_h).
b093f5619f Fill out dust limit unit test for known types except bare multisig (Greg Sanders)
Pull request description:
Having the constants checked explicitly in a single spot helps with possible regressions and also useful for documentation.
In addition, add a check for undefined v1 witness programs.
ACKs for top commit:
theStack:
Code-review ACK b093f5619f
MarcoFalke:
review ACK b093f5619f🥉
Tree-SHA512: 1421f75471739d29b9ef59b0a925b6b07e4e9af92822dbe56eedfb590be9a00fb0c34312146c7c1b5211906461ed00bfa2eb53c88595c6e5a27694b2dc21df38
fab9f7d1bd test: Use std::unique_ptr over manual delete in coins_tests (MarcoFalke)
Pull request description:
Makes the code smaller and easier to read
ACKs for top commit:
stickies-v:
ACK fab9f7d1bd
john-moffett:
ACK fab9f7d1bd
Tree-SHA512: 30d2d2097906e61fdef47a52fc6a0c5ce2417bc41c3c82dafc1b216c655f31dabf9c1c13759575a696f61bbdfdba3f442be032d5e5145b7a54fae2a927824621
fa6986a66b ci: Print iwyu patch in git diff format (MarcoFalke)
Pull request description:
Seems more dev friendly to also have a patch to copy-paste
ACKs for top commit:
hebasto:
ACK fa6986a66b, tested on Ubuntu 22.04 locally.
fanquake:
ACK fa6986a66b - did not test but example CI output looks ok.
stickies-v:
utACK fa6986a66b
Tree-SHA512: 7cfd8584bf12e03c28af23f4712c6bcafd648d87ddb92788b9cd35455b2db49f4bd4aef8ad4711f75c7f11ad2bb2492c2eb6044007086c20e36016575c060603
fa486de212 ci: Cache package manager install step (MarcoFalke)
Pull request description:
Use the local podman or docker image cache to skip the slow `apt` step
ACKs for top commit:
jamesob:
ACK fa486de212 ([`jamesob/ackr/26976.1.MarcoFalke.ci_cache_package_manager`](https://github.com/jamesob/bitcoin/tree/ackr/26976.1.MarcoFalke.ci_cache_package_manager))
Tree-SHA512: 3495346c6c862b63296d2691cc492bf52a0a99ee7fae798887c792609904546013eba788045cd508a5f669f2c52e3479c122c18a5275c87af38237a1b5c9da17
576f7b8614 Fix misleading RPC console wallet message (John Moffett)
Pull request description:
## Misleading message from RPCConsole window ##
In certain circumstances, the GUI console will display the message 'Executing command without any wallet' when it is, in fact, using the currently loaded wallet. For instance:
In RPC calls, if no wallet is explicitly selected and there is exactly one wallet loaded, the [default](39363a4b94/src/wallet/rpc/util.cpp (L71-L93)) is to act on that loaded wallet.
The GUI console acts that way in reality, but sometimes erroneously reports that it's not acting on any particular wallet. The root issue is due to the logic that prevents changing the selected wallet if the RPCConsole is visible:
39363a4b94/src/qt/rpcconsole.cpp (L783-L786)
This PR removes that unnecessary logic. This does have some ramifications. Prior to this PR, if a user opened the console window without any wallets loaded, then opened two or more wallets, the RPC console would select "None" of the wallets and any wallet-specific RPCs would fail. However, the behavior was different if the user hadn't had the console window open. In that case, if they opened the RPC Console window _after_ loading at least the first wallet, it would select the first-loaded wallet. This context-dependent behavior is (IMO) undesirable, and this PR changes it to be consistent.
ACKs for top commit:
hebasto:
ACK 576f7b8614, tested on Ubuntu 22.04 (Qt 5.15.3).
Tree-SHA512: 627da186025ba4f4e8df7fdd1b10363f923c4ecc50f023bbf2aece6e2593da65c45147c933effaca9040f813a6e46f034fc2d1ee2fb0f401000a3a6221a0e36e
08209c039f Correctly limit overview transaction list (John Moffett)
Pull request description:
Fixes#703
The way the main overview page limits the number of transactions displayed (currently 5) is not an appropriate use of Qt. Our subclassed transaction sort/filter proxy model returns a maximum of `5` in `rowCount()`. However, the model itself actually may hold significantly more. While this has _worked_, it breaks the contract of `rowCount()`.
If `bitcoin-qt` is run with a DEBUG build of Qt, it'll result in an assert-crash in certain relatively common situations (see #703 for details). Instead of artificially limiting the `rowCount()` in the subclassed filter, we can hide/unhide the rows in the displaying `QListView` upon any changes in the sorted proxy filter.
I loaded a wallet with 20,000 transactions and did not notice any performance differences between master and this branch.
For reference, this is the list I'm referring to:
<img width="934" alt="image" src="https://user-images.githubusercontent.com/116917595/214947304-3f289380-3510-487b-80e8-d19428cf2f0f.png">
ACKs for top commit:
Sjors:
tACK 08209c039f
hebasto:
ACK 08209c039f, tested on Ubuntu 22.04.
Tree-SHA512: c2a7b1a2a6e6ff30694830d7c722274c4c47494a81ce9ef25f8e5587c24871b02343969f4437507693d4fd40ba7a212702b159cf54b3357d8d76c02bc8245113
fa47b28dfc refactor: Remove unused CDataStream SerializeMany constructor (MarcoFalke)
Pull request description:
Seems odd to have an unused method. Moreover, the function is fragile and dangerous, because one could have a `std::vector vec_a` and type `CDataStream{vec_a, 0, 0}.size()` and `CDataStream{0, 0, vec_a}.size()`, assuming they are the same thing, when in fact they are not. (The first takes over the memory as is, the second serializes the vector).
So my suggestion would be to remove the unused method and introduce a new method when this functionality is needed. For example: `static DataStream FromMany(Args&&... args)`.
ACKs for top commit:
stickies-v:
ACK fa47b28dfc
Tree-SHA512: 9593a034b997e33a0794f779f76f02425b1097b218cf8cb1facb7f874fa69da328ce567a79138015baeebe004ae7d103dda4f64f83e8ad375b6dae6b66d3d950
fad7af700e Use steady clock for logging timer (MarcoFalke)
Pull request description:
The logging timer has many issues:
* The underlying clock is mockable, meaning that benchmarks are useless when mocktime was set at the beginning or end of the benchmark.
* The underlying clock is not monotonic, meaning that benchmarks are useless when the system time was changed during the benchmark.
Fix all issues in this patch.
ACKs for top commit:
stickies-v:
Approach ACK fad7af700e
john-moffett:
ACK fad7af700e
Tree-SHA512: bec8da0f338ed4611e1807937575e1b2afda25139d88015b1c29fa7d13946fbfbc4ee589b576c0508d505df5e5fafafcbc07d63ce4bab4b01475260d9d5d2107
71383f2fad ci: avoid using -Werror for older compilers (fanquake)
Pull request description:
Don't enable `-Werror` (in the CI) for compilers at least older than our current release compiler (GCC 10). It provides little-to-no value, other than turning compiler bugs & false positives into build failures, and we aren't going to mutate perfectly fine/working code, for the sake of avoid a warning that shouldn't even exist.
I also do not see the point of playing whack-a-mole and turning off various warnings/trying to further work around the broken compiler, just to acheive warningless builds for the sake of warningless builds.
One anecdote from ["How SQLite Is Tested"](https://www.sqlite.org/testing.html):
> Static analysis has found a few bugs in SQLite, but those are the
> exceptions. More bugs have been introduced into SQLite while trying
> to get it to compile without warnings than have been found by static
> analysis.
ACKs for top commit:
hebasto:
ACK 71383f2fad
jarolrod:
ACK 71383f2fad
Tree-SHA512: 20ed3dcf54fb17a7d9f0d8ca68c0ad2ee8f171f8bd61673a428f3123ab322c24cd9833f65915489bc8cebeffc37fd683a30e9669684b219960e69ddc7adae5bd
6d31900e52 wallet: migrate wallet, exit early if no legacy data exist (furszy)
Pull request description:
The process first creates a backup file then return an error,
without removing the recently created file, when notices that
the db is already running sqlite.
ACKs for top commit:
john-moffett:
ACK 6d31900e52
achow101:
ACK 6d31900e52
ishaanam:
crACK 6d31900e52
Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a
dc70c1eb08 addrman: Use std::nullopt instead of {} (Martin Zumsande)
59cc66abb9 test: Remove AddrMan unit test that fails consistency checks (Martin Zumsande)
Pull request description:
Two fixups for #26847:
* Now that `AddrMan::Size()` performs internal consistency tests (it didn't before), we can't call it in the `load_addrman_corrupted` unit tests, where we deal with an artificially corrupted `AddrMan`. This would fail the test when using `-checkaddrman=1` (leading to spurious CI fails). Therefore remove the tests assertion, which is not particularly helpful anyway (in production we abort init when peers.dat is corrupted instead of querying AddrMan in its corrupted state).
(See https://github.com/bitcoin/bitcoin/pull/26847#issuecomment-1411458339)
* Use `std::nullopt` instead of `{}` for default args (suggested in https://github.com/bitcoin/bitcoin/pull/26847#discussion_r1090643603)
ACKs for top commit:
MarcoFalke:
lgtm ACK dc70c1eb08
Tree-SHA512: dd8a988e23d71a66d3dd30560bb653c9ad17db6915abfa5f722818b0ab18921051ec9223bfbc75d967df8bcd204dfe473d680bf68e8a8e4e4998fbb91dc973c5
fa451d4b60 Fix clang-tidy readability-const-return-type violations (MarcoFalke)
Pull request description:
This comes up during review, so instead of wasting review cycles on this, just enforce it via CI
ACKs for top commit:
stickies-v:
utACK fa451d4b6
hebasto:
ACK fa451d4b60.
Tree-SHA512: 144a85612f00ec43f7ea1fdaa11901ca981a9f0465a8849745712d741b201b9c3307118172ee0b8efd12bebf25bc6f32a6e2c908495e371f9ada0a917994f44e
Don't enable `-Werror` (in the CI) for compilers at least older than
our current release compiler (GCC 10). It provides little-to-no value,
other than turning compiler bugs & false positives into build failures,
and we aren't going to mutate perfectly fine/working code, for the sake
of avoid a warning that shouldn't even exist.
I also do not see the point of playing whack-a-mole and turning off various
warnings/trying to further work around the broken compiler, just to
acheive warningless builds for the sake of warningless builds.
One anecdote from "How SQLite Is Tested":
> Static analysis has found a few bugs in SQLite, but those are the
> exceptions. More bugs have been introduced into SQLite while trying
> to get it to compile without warnings than have been found by static
> analysis.
https://www.sqlite.org/testing.html.
Now that Size() performs internal consistency checks,
it will rightfully fail (and assert) when dealing with
a corrupted AddrMan. Therefore remove this check.
87f11ef47f refactor: use `Hash` helper for double-SHA256 calculations (Sebastian Falbesoner)
Pull request description:
We have two helper templates `Hash(const T& in1)` and `Hash(const T& in1, const T& in2)` available for calculating the double-SHA256 hash of one object or two concatenated objects, respectively:
b5868f4b1f/src/hash.h (L74-L89)
This PR uses them in order to increase readability and simplify the code. As in #15294 (which inspired this PR, doing the same for RIPEMD160), the helper is not utilized in validation.cpp and script/interpreter.cpp to avoid touching consensus-relevant code.
ACKs for top commit:
john-moffett:
ACK 87f11ef47f
stickies-v:
ACK 87f11ef47f
MarcoFalke:
review ACK 87f11ef47f😬
Tree-SHA512: 11d7e3d00c89685107784010fbffb33ccafb4d1b6a76c4dceb937b29bb234ef4d54581b16bd0737c8d2994a90cf4fe10a9738c7cc5b6d085c6a819f06176dab9
7a820cee0e test, build: Separate `read_json` function into its own module (Hennadii Stepanov)
Pull request description:
Currently, 4 source files rely on the definition of the `read_json` function provided in `src/test/script_tests.cpp`.
This PR breaks this entanglement, improves code structure and maintainability.
ACKs for top commit:
fanquake:
ACK 7a820cee0e
Tree-SHA512: f1567989f76cb54ab86cc48927851a8c424b08a9483d02d4918b629e0c792108bad4ccf7fa341d57b0921d91e84bf8fa3b9c07e5fdf12c64d9d5da83e4e464fb
b0e916913c clang-tidy: Force to check all headers (Hennadii Stepanov)
96ee992ac3 clang-tidy: Fix `modernize-use-default-member-init` in headers (Hennadii Stepanov)
Pull request description:
This PR:
- fixes the only [remained](https://github.com/bitcoin/bitcoin/pull/26705#issuecomment-1353742082) check in headers, i.e., `modernize-use-default-member-init`
- forces `clang-tidy` check all headers
Closesbitcoin/bitcoin#26703.
ACKs for top commit:
MarcoFalke:
review ACK b0e916913c🍹
Tree-SHA512: 4d33fe873094914541ae81968cdb4e7a7a01b3fdd4f25bc6daa8a53f45dab80565a5b3607ddc338f122369ca5a0a2d0d09c8e78cabe1beb6bd50c115bc5c5210
dfc01ccd73 net: simplify the call to vProcessMsg.splice() (Vasil Dimov)
Pull request description:
At the time when
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
```
is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the call equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
```
which is equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
```
Thus, use the latter. Further, maybe irrelevant, but the latter has constant complexity while the original code is `O(length of vRecvMsg)`.
ACKs for top commit:
theStack:
Code-review ACK dfc01ccd73
MarcoFalke:
review ACK dfc01ccd73🐑
jonatack:
Light review ACK dfc01ccd73
Tree-SHA512: 9f4eb61d1caf4af9a61ba2f54b915fcfe406db62c58ab1ec42f736505b6792e9379a83d0458d6cc04f289edcec070b7c962f94a920ab51701c3cab103152866f
80f39c99ef addrman, refactor: combine two size functions (Amiti Uttarwar)
4885d6f197 addrman, refactor: move count increment into Create() (Martin Zumsande)
c77c877a8e net: Load fixed seeds from reachable networks for which we don't have addresses (Martin Zumsande)
d35595a78a addrman: add function to return size by network and table (Martin Zumsande)
Pull request description:
AddrMan currently doesn't track the number of its entries by network, it only knows the total number of addresses. This PR makes AddrMan keep track of these numbers, which would be helpful for multiple things:
1. Allow to specifically add fixed seeds to AddrMan of networks where we don't have any addresses yet - even if AddrMan as a whole is not empty (partly fixing #26035). This is in particular helpful if the user abruptly changes `-onlynet` settings (such that addrs that used to be reachable are no longer and vice versa), in which case they currently could get stuck and not find any outbound peers. The second commit of this PR implements this.
1. (Future work): Add logic for automatic connection management with respect to networks - such as making attempts to have at least one connection to each reachable network as suggested [here](https://github.com/bitcoin/bitcoin/issues/26035#issuecomment-1249420209). This would involve requesting an address from a particular network from AddrMan, and expanding its corresponding function `AddrMan::Select()` to do this requires internal knowledge of the current number of addresses for each network and table to avoid getting stuck in endless loops.
1. (Future work): Perhaps display the totals to users. At least I would find this helpful to debug, the existing option (`./bitcoin-cli -addrinfo`) is rather indirect by doing the aggregation itself in each call, doesn't distinguish between new and tried, and being based on `AddrMan::GetAddr()` it's also subject to a quality filter which we probably don't want in this spot.
ACKs for top commit:
naumenkogs:
utACK 80f39c9
stratospher:
ACK 80f39c9
achow101:
ACK 80f39c99ef
vasild:
ACK 80f39c99ef
Tree-SHA512: 6359f2e3f4db7c120c0789d92d74cb7d87a2ceedb7d6a34b5eff20c7f55c5c81092d10ed94efe29afc1c66947820a0d9c14876ee0c8d1f8e068a6df4e1131927
6d58117a31 build: Build minisketch test in `make check`, not in `make` (Hennadii Stepanov)
Pull request description:
On master (d1e42659bb):
```
$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
$ make 2>&1 | grep LD | grep -v .la
CXXLD bitcoind
CXXLD bitcoin-cli
CXXLD bitcoin-tx
CXXLD bitcoin-util
CXXLD test/test_bitcoin
CXXLD bench/bench_bitcoin
CXXLD minisketch/test
CXXLD test/fuzz/fuzz
CXXLD univalue/test/object
CXXLD univalue/test/unitester
$ make check 2>&1 | grep LD
CCLD exhaustive_tests
CCLD tests
```
With this PR:
```
$ ./autogen.sh && ./configure --without-gui --disable-wallet && make clean
$ make 2>&1 | grep LD | grep -v .la
CXXLD bitcoind
CXXLD bitcoin-cli
CXXLD bitcoin-tx
CXXLD bitcoin-util
CXXLD test/test_bitcoin
CXXLD bench/bench_bitcoin
CXXLD test/fuzz/fuzz
CXXLD univalue/test/object
CXXLD univalue/test/unitester
$ make check 2>&1 | grep LD
CXXLD minisketch/test
CCLD exhaustive_tests
CCLD tests
```
In fact, this PR restores behavior that was before bitcoin/bitcoin#22646, and that behavior looks more optimal.
As an outcome, the `contrib/guix/libexec/build.sh` does not spend resources to build binaries which are not a part of the release package.
ACKs for top commit:
TheCharlatan:
ACK 6d58117a31
Tree-SHA512: 4957c8f88a01aca005813bf4c1c26f433756bf68ea0c958481c638ead229fa8e23ecae3a8ac31ea555876ba6f2cc10ecd91caf2e2f664de5cb529ec05fb38fa7
a24e633339 refactor: rpc: set TxToJSON default verbosity to SHOW_DETAILS (stickies-v)
Pull request description:
`TxToJSON()` and `TxToUniv()` are only to be called when we want to decode the transaction (i.e. its details) into JSON. If `TxVerbosity` is `SHOW_TXID`, the function should not have been (and currently is not) called in the first place.
There is no behaviour change, current logic simply assumes anything less than `TxVerbosity::SHOW_DETAILS_AND_PREVOUT` equals `TxVerbosity::SHOW_DETAILS`. With this change, the assumptions and intent become more explicit.
ACKs for top commit:
w0xlt:
ACK a24e633339
Tree-SHA512: b97235adae49b972bdbe10aca1438643fb35ec66a4e57166b1975b3015bc5a06a711feebe4453a8fefe71781e484b21ef80847d8e8a33694a3abcc863accd4d7
d4c59da8d6 build: Avoid `BOOST_NO_CXX98_FUNCTION_BASE` macro redefinition (Hennadii Stepanov)
Pull request description:
With GCC 12 and Boost 1.81 (from depends) having multiple warnings:
```
In file included from /home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/config.hpp:48:
/home/hebasto/bitcoin/depends/x86_64-pc-linux-gnu/include/boost/config/stdlib/libstdcpp3.hpp:397:9: warning: 'BOOST_NO_CXX98_FUNCTION_BASE' macro redefined [-Wmacro-redefined]
#define BOOST_NO_CXX98_FUNCTION_BASE
^
<command line>:8:9: note: previous definition is here
#define BOOST_NO_CXX98_FUNCTION_BASE 1
^
1 warning generated.
```
This PR fixes those warnings.
Defining of the `BOOST_NO_CXX98_FUNCTION_BASE` macro was introduced in https://github.com/bitcoin/bitcoin/pull/25436, but since https://github.com/boostorg/config/pull/430, it is required to check it before adding.
ACKs for top commit:
fanquake:
ACK d4c59da8d6 - it works now.
Tree-SHA512: 53b9ddcf8dad729638ed41251e30c80f2d7d1ae3ffe47466865834f1f10184fe0881abeb339b3e46c270c3eb11fb63d19ab12cc9461bf5c2be12b4763c1b1c34
b530d9605d test: refactor: introduce `replace_in_config` helper (Sebastian Falbesoner)
Pull request description:
Currently two functional tests (p2p_permissions.py and wallet_crosschain.py) include quite similar code for substituting strings in a TestNode's bitcoind configuration file, so refactoring that out to a dedicated helper method seems to make sense (probably other tests could need that too in the future).
ACKs for top commit:
kouloumos:
ACK b530d9605d
Tree-SHA512: 5ca65a2ef3292460e5720d5c6acf7326335001858e8f71ab054560117f9479dbadb1dacd8c9235f67f3fcfd08dbc761b62704f379cbf619bba8804f16a25bc7b
56a03f1834 depends: ensure we are appending to sqlite cflags (fanquake)
Pull request description:
Otherwise we'll just override other flags passed in (i.e msan).
Should fix https://cirrus-ci.com/task/6598922274078720?logs=ci#L3661.
ACKs for top commit:
achow101:
ACK 56a03f1834
TheCharlatan:
ACK 56a03f1834
Tree-SHA512: 5890018cfc5deaef18b0f01a3a0396f803e97f9a9785bf6873ef48bc13b74b644315f0f29cf11d3522964a6396f74e1f080bb4e412bc302956a651fed28b27df
faba08b5b4 refactor: Remove stray cs_main redundant declaration (MarcoFalke)
fa02591edf doc: Export threadsafety.h from sync.h (MarcoFalke)
Pull request description:
Looks like this was forgotten when introducing kernel/cs_main ?
Also, there is a commit to export threadsafety.h from sync.h.
ACKs for top commit:
hebasto:
ACK faba08b5b4
Tree-SHA512: 0aa58e7693b6fcd504f9da7339f8baa463a6407f67b27f68002db705f4642321ac3765f16c3d906c925ee24085591b79160a62fa5f4aaf6f2e5dcc788411800d
1d7935b45a test: add test for coins view flush behavior using Sync() (James O'Beirne)
2c3cbd6c00 test: add use of Sync() to coins tests (James O'Beirne)
6d8affca96 test: refactor: clarify the coins simulation (James O'Beirne)
79cedc36af coins: add Sync() method to allow flush without cacheCoins drop (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
In certain circumstances, we may want to flush chainstate data to disk without
emptying `cacheCoins`, which affects performance. UTXO snapshot
activation is one such case, as we populate `cacheCoins` with the snapshot
contents and want to persist immediately afterwards but also enter IBD.
See also #15265, which makes the case that under normal operation a
flush-without-erase doesn't necessarily add much benefit. I open this PR
even in light of the previous discussion because (i) flush-without-erase
almost certainly provides benefit in the case of snapshot activation (especially
on spinning disk hardware) and (ii) this diff is fairly small and gives us convenient
options for more granular cache management without changing existing policy.
See also #15218.
ACKs for top commit:
sipa:
ACK 1d7935b45a
achow101:
ACK 1d7935b45a
Sjors:
tACK 1d7935b45a
Tree-SHA512: 897583963e98661767d2d09c9a22f6019da24125558cd88770bfe2d017d924f23a9075b729e4b1febdec5b0709a38e8fa1ef94d62aa88650556b06cb4826c845
eeee61065f Use AutoFile and HashVerifier where possible (MarcoFalke)
fa961141f7 Add HashVerifier (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `AutoFile` and `HashVerifier`. `CAutoFile` and `CHashVerifier` remain in places where it is not yet possible.
ACKs for top commit:
stickies-v:
ACK eeee61065f
Tree-SHA512: 93786778c309ecfdc1ed43552d24ff9d966954d69a47f66faaa6de24daacd25c651f3f62bde5abbb362700298fb3c04ffbd3207a0dd13d0bd8bff7fd6d07dcf8
a3a2bd9e8a ci: Drop no longer needed package-specific flags (Hennadii Stepanov)
071eef1e97 build: Propagate user-defined flags to host packages (Hennadii Stepanov)
Pull request description:
On master (4f8b1f8759) `{CPP,C,CXX,LD}FLAGS` that are specified in the command line are not propagated to packages:
```
$ make --no-print-directory -C depends print-libevent_cxxflags CXXFLAGS=-some-fancy-flag
libevent_cxxflags=-pipe -O2
```
This PR:
- propagates `{CPP,C,CXX,LD}FLAGS` to host packages:
```
$ make --no-print-directory -C depends print-libevent_cxxflags CXXFLAGS=-some-fancy-flag
libevent_cxxflags= -some-fancy-flag
```
- does not propagate `{CPP,C,CXX,LD}FLAGS` to native packages:
```
$ make --no-print-directory -C depends print-native_b2_cxxflags CXXFLAGS=-some-fancy-flag
native_b2_cxxflags=
```
- actually addresses the https://github.com/bitcoin/bitcoin/pull/23551#issuecomment-973896518
ACKs for top commit:
TheCharlatan:
Code review ACK a3a2bd9e8a
Tree-SHA512: 243d6b1b0e9c5de46debc36de62a77b6b4d6f638940fd530040c219956ec624e321b0c25290fed164e3a8c88befa7b97b20f765d7b9a428c269b3720f21da099
d51f0fa4b7 doc: add release notes for 26896 (fanquake)
2b248798d9 build: remove --enable-upnp-default from configure (fanquake)
02f5a5e7b5 build: remove --enable-natpmp-default from configure (fanquake)
25a0e8ba0b Remove configure-time setting of DEFAULT_UPNP (fanquake)
06562e5fa7 Remove configure-time setting of DEFAULT_NATPMP (fanquake)
Pull request description:
This PR removes the `--enable-upnp-default` and `--enable-natpmp-default` options from configure.
It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default.
I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a `.conf` file, can't or don't want to pass command line options at runtime, and also don't want to modify the source code?
ACKs for top commit:
hebasto:
ACK d51f0fa4b7, rebased and comments have been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/26896#pullrequestreview-1273910740).
TheCharlatan:
ACK d51f0fa4b7
Tree-SHA512: 481decd8bddd8b03b7319591e3acf189f7b6b96c9a9a8c5bc1a3f8ec00d0b8f9b52d2f5c28a298a2ec947cfe9611cfd184e393ccb2e4e21bfce86ca7d4de60d3
At the time when
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
```
is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the
call equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
```
which is equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
```
Thus, use the latter. Further, maybe irrelevant, but the latter has
constant complexity while the original code is `O(length of vRecvMsg)`.
