664657ed13 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)
Pull request description:
Motivation:
* ranged descriptors MUST not be able to have label (current impl allows it)
* external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)
Repro steps:
* create blank wallet and import descriptors
* external has `label=test` (not internal)
```
conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
passphrase=None, avoid_reuse=False, descriptors=True)
descriptors = [
{
"timestamp": "now",
"label": "test",
"active": True,
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
"internal": False
},
{
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
"active": True,
"internal": True,
"timestamp": "now"
},
]
r = conn.importdescriptors(descriptors)
print(r)
```
response:
```
[{'error': {'code': -8,
'message': 'Internal addresses should not have a label'},
'success': False,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
But in above, ONLY external has a label.
If you remove `internal: False` from external descriptor import object - it will import no problem:
```
[{'success': True,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.
ACKs for top commit:
achow101:
ACK 664657ed13
rkrux:
lgtm crACK 664657ed13
Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2
0465574c12 test: Fixes send_blocks_and_test docs (Sergi Delgado Segura)
09c95f21e7 test: Adds block tiebreak over restarts tests (Sergi Delgado Segura)
18524b072e Make nSequenceId init value constants (Sergi Delgado Segura)
8b91883a23 Set the same best tip on restart if two candidates have the same work (Sergi Delgado Segura)
5370bed21e test: add functional test for complex reorgs (Pieter Wuille)
ab145cb3b4 Updates CBlockIndexWorkComparator outdated comment (Sergi Delgado Segura)
Pull request description:
This PR grabs some interesting bits from https://github.com/bitcoin/bitcoin/pull/29284 and fixes some edge cases in how block tiebreaks are dealt with.
## Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated https://github.com/bitcoin/bitcoin/pull/29284#discussion_r1522023578 (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
## New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1994317785 for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within `CBlockIndex` is `nSequenceId`, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the same `nSequenceId`: 0.
Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the `CBlockIndexWorkComparator` has to default to its last rule: whatever block is loaded first (has a smaller memory address).
This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way `nSequenceId` is initialized is changed to:
- 0 for blocks that belong to the previously known best chain
- 1 to all other blocks loaded from disk
ACKs for top commit:
sipa:
utACK 0465574c12
TheCharlatan:
ACK 0465574c12
furszy:
Tested ACK 0465574c12.
Tree-SHA512: 161da814da03ce10c34d27d79a315460a9c98d019b85ee35bc5daa991ed3b6a2e69a829e421fc70d093a83cf7a2e403763041e594df39ed1991445e54c16532a
51877f2fc5 test: Update BIP324 test vectors (Tim Ruffing)
Pull request description:
This updates the hardcoded test vectors from BIP324. The test vectors had to be regenerated (in the aux files of the BIP) because there was a bug in the script used for generating them (https://github.com/bitcoin/bips/pull/2016).
ACKs for top commit:
jonatack:
ACK 51877f2fc5
theStack:
ACK 51877f2fc5
Tree-SHA512: 59f4075e286067b11fce98667c860f3083b6cca8a2e49da8783ccdce8e32c34fd3e1943191d24dcf5bb68d8a2540726d99f7c29e8b0f104032ccb82423ca8d82
- Also move them to policy/fees/ and update includes
- Note: the block_policy_estimator_args.h include in block_policy_estimator_args.cpp was done manually.
5ded99a7f0 fuzz: MockMempoolMinFee in wallet_fees (brunoerg)
c9a7a198d9 test: move MockMempoolMinFee to util/txmempool (brunoerg)
adf67eb21b fuzz: create FeeEstimatorTestingSetup to set fee_estimator (brunoerg)
ff10a37e99 fuzz: mock CBlockPolicyEstimator in wallet_fuzz (brunoerg)
f591c3beca fees: make estimateSmartFee/HighestTargetTracked virtual for mocking (brunoerg)
19273d0705 fuzz: set mempool options in wallet_fees (brunoerg)
Pull request description:
Some functions in `wallet/fees.cpp` (fuzzed by the wallet_fees target) depends on some mempool stuff - e.g. relay current min fee, smart fee and max blocks estimation, relay dust fee and other ones. For better fuzzing of it, it would be great to have these values/interactions. That said, this PR enhances the `wallet_fees` target by:
- Setting mempool options - `min_relay_feerate`, `dust_relay_feerate` and `incremental_relay_feerate` - when creating the `CTxMemPool`.
- Creates a `ConsumeMempoolMinFee` function which is used to have a mempool min fee (similar approach from `MockMempoolMinFee` from unit test).
- Mock `CBlockPolicyEstimator` - estimateSmartFee/HighestTagretTracket functions, especifically. It's better to mock it then trying to interact to CBlockPolicyEstimator in order to have some effective values due to performance.
Note that I created `FeeEstimatorTestingSetup` because we cannot set `m_node.fee_estimator` in `ChainTestingSetup` since fae8c73d9e.
ACKs for top commit:
maflcko:
re-ACK 5ded99a7f0🎯
ismaelsadeeq:
Code review ACK 5ded99a7f0
Tree-SHA512: 13d2af042098afd237ef349437021ea841069d93d4c3e3a32e1b562c027d00c727f375426709d34421092993398caf7ba8ff19077982cb6f470f8938a44e7754
45bd891465 log: split assumevalid ancestry-failure-reason message (Lőrinc)
6c13a38ab5 log: separate script verification reasons (Lőrinc)
f2ea6f04e7 refactor: untangle assumevalid decision branches (Lőrinc)
9bc298556c validation: log initial script verification state (Lőrinc)
4fad4e992c test: add assumevalid scenarios scaffold (Lőrinc)
91ac64b0a6 log: reword `signature validations` to `script verification` in `assumevalid` log (Lőrinc)
Pull request description:
### Summary
Users can encounter cases where script checks are unexpectedly enabled (e.g. after reindex, or when `assumevalid`/`minimumchainwork` gates fail). Without an explicit line, they must infer state from the absence of a message, which is incomplete and error-prone.
The existing "Assuming ancestors of block …" line does not reliably indicate whether script checks are actually enabled, which makes debugging/benchmarking confusing.
### What this changes
We make the initial **script-verification** state explicit and log **why** checks are enabled to avoid confusion.
* Always log the first script-verification state on startup, **before** the first `UpdateTip`.
* Flatten the nested `assumevalid` conditionals into a linear gating sequence for readability.
* Extend the functional test to assert the old behavior with the new reason strings.
This is a **logging-only** test change it shouldn't change any other behavior.
### Example output
The state (with reason) is logged at startup and whenever the reason changes, e.g.:
* `Disabling script verification at block #904336 (000000000000000000014106b2082b1a18aaf3091e8b337c6fed110db8c56620).`
* `Enabling script verification at block #912527 (000000000000000000010bb6aa3ecabd7d41738463b6c6621776c2e40dbe738a): block too recent relative to best header.`
* `Enabling script verification at block #912684 (00000000000000000001375cf7b90b2b86e559d05ed92ca764d376702ead3858): block height above assumevalid height.`
------
Follow-up to https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037
ACKs for top commit:
Eunovo:
re-ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/45bd8914658a675d00aa9c83373d6903a8a9ece8
achow101:
ACK 45bd891465
hodlinator:
re-ACK 45bd891465
yuvicc:
ACK 45bd891465
andrewtoth:
ACK 45bd891465
ajtowns:
ACK 45bd891465
Tree-SHA512: 58328d7c418a6fe18f1c7fe1dd31955bb6fce8b928b0df693f6200807932eb5933146300af886a80a1d922228d93faf531145186dae55ad4ad1f691970732eca
b63428ac9c rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0d refactor: increase string_view usage (stickies-v)
b3bf18f0ba rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](https://github.com/bitcoin/bitcoin/pull/32845/commits/d127b25199118756122232d9aafff19f1922869b#diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32)) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9c🎉
achow101:
ACK b63428ac9c
pablomartin4btc:
re-ACK [b63428a](https://github.com/bitcoin/bitcoin/commit/b63428ac9ce2c903670409b3e47b9f6730917ae8)
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/32983/commits/b63428ac9ce2c903670409b3e47b9f6730917ae8
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
This refactor does not change behavior, but it has a few benefits:
* The shellcheck SC2086 warning is disabled for the whole command, but
is only needed for the CI_CONTAINER_CAP env var. So in Python, only
pass this one env var to shlex.split() for proper word splitting.
* Future logic improvements can be implemented in Python.
The comments are moved, which can be checked via the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
fa70e23de7 ci: Drop libFuzzer from msan fuzz task (MarcoFalke)
Pull request description:
libFuzzer is mostly unmaintained (https://llvm.org/docs/LibFuzzer.html#status), and it isn't really needed by the CI tasks. While it provides some additional stats like rss or the max input byte size, they are not essential. Dropping libFuzzer here would also drop the "60 seconds sanity check" for empty folders, but I think this is an acceptable price to pay to silence false-positives that were hit for years.
Also, there seems to be a history of intermittent false-positive msan warnings (https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3391921802).
It is unclear what exactly is causing the false-positives, so just disable libFuzzer in this task for now, to work around them.
ACKs for top commit:
kevkevinpal:
ACK [fa70e23](https://github.com/bitcoin/bitcoin/pull/33666/commits/fa70e23de75baaf8c1ef6836ffe8ca73562c8937)
dergoegge:
ACK fa70e23de7
Tree-SHA512: c3e5958b8378ba30f51d923f97a84dec2ee60af8b9c2a4f13bc8de486a490031468371120e421384aa198ffec591db554e636935ab3c6d4de5e870238f5079f2
fa37153288 util: Abort on failing CHECK_NONFATAL in debug builds (MarcoFalke)
fa0dc4bdff test: Allow testing of check failures (MarcoFalke)
faeb58fe66 refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD (MarcoFalke)
Pull request description:
A failing `CHECK_NONFATAL` will throw an exception. This is fine and even desired in production builds, because the program may catch the exception and give the user a way to easily report the bug upstream.
However, in debug development builds, exceptions for internal bugs are problematic:
* The exception could accidentally be caught and silently ignored
* The exception does not include a full stacktrace, possibly making debugging harder
Fix all issues by turning the exception into an abort in debug builds.
This can be tested by reverting the hunks to `src/rpc/node.cpp` and `test/functional/rpc_misc.py` and then running the functional or fuzz tests.
ACKs for top commit:
achow101:
ACK fa37153288
ryanofsky:
Code review ACK fa37153288, just catching subprocess.CalledProcessError in test fixing up a comment since last review
stickies-v:
ACK fa37153288
Tree-SHA512: 2d892b838ccef6f9b25a066e7c2f6cd6f5acc94aad1d91fce62308983bd3f5c5d724897a76de4e3cc5c3678ddadc87e2ee8c87362965373526038e598dfb0101
cc5dda1de3 headerssync: Make HeadersSyncState more flexible and move constants (Hodlinator)
8fd1c2893e test(headerssync): Test returning of pow_validated_headers behavior (Hodlinator)
7b00643ef5 test(headerssync): headers_sync_chainwork test improvements (Hodlinator)
04eeb9578c doc(test): Improve comments (Hodlinator)
fe896f8faa refactor(test): Store HeadersSyncState on the stack (Hodlinator)
f03686892a refactor(test): Break up headers_sync_state (Hodlinator)
e984618d0b refactor(headerssync): Process spans of headers (Hodlinator)
a4ac9915a9 refactor(headerssync): Extract test constants ahead of breakup into functions (Hodlinator)
Pull request description:
### Background
As part of the release process we often run *contrib/devtools/headerssync-params.py* and increase the values of the constants `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` in *src/headerssync.cpp* as per *doc/release-process.md* (example: 11a2d3a63e). This helps fine tune the memory consumption per `HeadersSyncState`-instance in the face of malicious peers.
(The `REDOWNLOAD_BUFFER_SIZE`/`HEADER_COMMITMENT_PERIOD` ratio determines how many Headers Sync commitment bits must match between PRESYNC & REDOWNLOAD phases before we start permanently storing headers from a peer. For more details see comments in *src/headerssync.h* and *contrib/devtools/headerssync-params.py*).
### Problem: Not feeding back headers until completing sync
During v30 release process #33274 made `REDOWNLOAD_BUFFER_SIZE` exceed the `target_blocks` constant used to control the length of chains generated for testing Headers Sync (`15000`, *headers_sync_chainwork_tests.cpp*).
The `HeadersSyncState::m_redownloaded_headers`-buffer now does not reach the `REDOWNLOAD_BUFFER_SIZE`-threshold during those unit tests. As a consequence `HeadersSyncState::PopHeadersReadyForAcceptance()` will not start feeding back headers until the PoW threshold has been met. While this will not cause the unit test to start failing on master, it means we have gone from testing behavior that resembles mainnet (way more than `REDOWNLOAD_BUFFER_SIZE` headers to reach the PoW limit), to behavior that is not possible/expected there.
### Solution
Avoid testing this unrealistic condition of completing Headers Sync before reaching `REDOWNLOAD_BUFFER_SIZE` by making tests able to define their own values through the new `HeadersSyncParams` instead of having them hard-coded for all chains & tests.
### Commits
* First 6 commits refactor and improve the unit tests in order to clarify latter changes.
* We then add checks for the behavior around the `REDOWNLOAD_BUFFER_SIZE` threshold.
* The main change: we extract the section from *headerssync.cpp* containing the constants to *kernel/chainparams.cpp*, making `HeadersSyncState` no longer hard-coded to mainnet.
### Notes
This PR used to be called "headerssync: Preempt unrealistic unit test behavior".
ACKs for top commit:
l0rinc:
reACK cc5dda1de3
marcofleon:
code review ACK cc5dda1de3
danielabrozzoni:
reACK cc5dda1de3
Tree-SHA512: ccc824dcbbb8ad5ae98c3bf5808b38467aac0230739898a758c9b939eecd74f982df088fa0ba81cc1c1732f19a607b135a6e9577bb9fcf7f8570567ce92f66e6
d0e1bbad01 test: repeat block malleability test with relayable block over P2P (Musa Haruna)
Pull request description:
This PR adds a functional test to repeat the existing malleability check for oversized coinbase witness nonce size using a block that is small enough to be relayed over the P2P network.
This addresses the TODO in test_block_malleability by ensuring behavior is consistent between submitblock RPC and P2P relay.
ACKs for top commit:
maflcko:
lgtm ACK d0e1bbad01
janb84:
re ACK d0e1bbad01
glozow:
utACK d0e1bbad01
Tree-SHA512: 05aec4fade5af8043f40274a8d2f3cf3f540acd038138975bdefbbbc81e105792d6d2588256a2ee5ddb1e05b37fe2d0b3d287160d2dbe86e1aac7cfa9cc02116
8f7673257a miner: fix empty mempool case for waitNext() (Sjors Provoost)
Pull request description:
Block template fees are calculated by looping over `new_tmpl->vTxFees` and return (early) once the `fee_threshold` is exceeded.
This left an edge case when the mempool is empty, which this commit fixes and adds a test for.
Also update `test/functional/interface_ipc.py` to reflect the new behavior,
Fixes https://github.com/Sjors/sv2-tp/issues/9
ACKs for top commit:
optout21:
ACK 8f7673257a
cedwies:
tACK 8f76732
sipa:
utACK 8f7673257a
zaidmstrr:
Concept ACK [8f76732](https://github.com/bitcoin/bitcoin/pull/33566/commits/8f7673257a1a86717c1d83770dc857fc254df107)
Tree-SHA512: ef200fe95e96f810e425283bc37f945c4bf5efa16f4b74820b8a07968f30c5146bca213a372124be84b48beead5dfd35f2b5d10d188d0a465f847ebab61de10a
e9cd45e3d3 test: set number of RPC server threads to 2 (furszy)
Pull request description:
The default `-rpcthreads` value spawns 16 HTTP server threads for each node.
Running the functional test suite with default `rpcthreads` can exhaust file
descriptors or hit other resource limits very easily (more when tests are run
in parallel).
Furthermore, having 16 threads is unnecessary since they are mostly idle. We
run RPC calls on a single RPC connection and wait for it result synchronously.
There is (almost) never two RPC calls occurring concurrently.
Because of this, the threads are mostly idle, so we can safely limit the number
of them to two.
Note for reviewers:
I checked this does not introduce any timing regression but would be good
to double-check it on your end too. We could add another thread if needed.
Just the 16 threads default value is too high and unnecessary.
ACKs for top commit:
maflcko:
lgtm ACK e9cd45e3d3
l0rinc:
ACK e9cd45e3d3
kevkevinpal:
ACK [e9cd45e](https://github.com/bitcoin/bitcoin/pull/33679/commits/e9cd45e3d3c7592265ebf67387090b3df1501df4)
andrewtoth:
ACK e9cd45e3d3
Tree-SHA512: a777286f4a890fb87f5df72cd2ccfdc628657206a4b3e995044e5a0d12987b8c78a7cf7d684cc4e92605aa782aaeebc44e9f754752c3a524152fac94fa30f4b5
The default `-rpcthreads` value spawns 16 HTTP server threads for each node.
Running the functional test suite with default `rpcthreads` can exhaust file
descriptors or hit other resource limits very easily.
Moreover, having 16 threads is unnecessary since they are mostly idle. We
run RPC calls on a single RPC connection and wait for it result synchronously.
There is (almost) never two RPC calls occurring concurrently.
Because of this, the threads are mostly idle, so we can safely limit the number
of them to two.
The build scripts inside the image retry after a failure. However, there
may be some rare network failures inside the container engine. For
example, when pulling the underlying base image, or when pulling the
docker cache.
Thus, retry after a failure once, which should hopefully fix
https://github.com/bitcoin/bitcoin/issues/33640.
The other code in this file is using this pattern to throw when a key is
unset, instead of silently returning a None when using os.getenv(key)
with no default value specified.
So use the pattern here as well. As the env vars are always set, this
should be a refactor that does not change the behavior.
fabe0e07de ci: Only write docker build images to Cirrus cache (MarcoFalke)
fab64a5d6f ci: Move buildx command to python script (MarcoFalke)
fa72a2bd5c ci: Remove unused MAYBE_CPUSET (MarcoFalke)
Pull request description:
The `DOCKER_BUILD_CACHE_ARG` env var holds the options on how to use cache providers. Storing the image layers is useful for the Cirrus cache provider, because it offers 10GB per runner (https://cirrus-runners.app/setup/#speeding-up-the-cache). The cached image layers can help to avoid issues when the upstream package manager infra (apt native, apt llvm, pip, apk, git clone, ...) has outages or network issues.
However, on the GitHub Actions cache provider, a *total* cache of 10GB is offered for the whole repo. This cache must be shared with the depends cache, and the ccache, as well as the previous releases cache. So it is already full and trying to put the docker build layers into it will lead to an overflow.
Fix it by only writing to the docker cache on Cirrus.
Also, `DOCKER_BUILD_CACHE_ARG` requires a `shellcheck disable=SC2086` on the full build command. Fix that as well by using `shlex.split` from Python on just this variable.
ACKs for top commit:
m3dwards:
ACK fabe0e07de
cedwies:
reACK fabe0e0
l0rinc:
Code review ACK fabe0e07de
willcl-ark:
ACK fabe0e07de
Tree-SHA512: 4f471f080007fdd0c3bc97b0cfe0e9c0457e5029a7ccde1d784d30eb4752e5eb309cd4b122b182bce31f1b986c8a9f3e9a49da1768bedbb2b1f64f70183680ba
9610b0d1e2 randomenv: Fix MinGW dllimport warning for `environ` (Lőrinc)
Pull request description:
Related to https://github.com/bitcoin/bitcoin/pull/33550#issuecomment-3378978210
Extends 7703884 to guard environ declaration on all Windows builds, not just MSVC.
In the `mingw-w64` headers (used by `llvm-mingw`), `environ` is defined as a macro which expands through [`_environ`](https://github.com/msys2-contrib/mingw-w64/blob/cdb052f1d4056cd510cb83197b55868427b87476/mingw-w64-headers/crt/stdlib.h#L262-L264) to `(* __p__environ())`, a call to a `dllimport` function, causing the same inconsistent linkage warning as MSVC.
Use `WIN32` instead of `_MSC_VER` to match the platform-specific guards already used throughout the file.
The warning occurs with `llvm-mingw` (both `UCRT` and `MSVCRT` variants as tested by Hebasto), but not with the `mingw-w64` toolchain currently used in CI (as mentioned by fanquake).
----
The error was reproduced by adding a temporary [nightly build](https://github.com/l0rinc/bitcoin-core-nightly/pull/4) pointing to https://github.com/l0rinc/bitcoin/pull/45. On `master` the failure can be seen in https://github.com/l0rinc/bitcoin-core-nightly/pull/2
before:
https://github.com/l0rinc/bitcoin-core-nightly/actions/runs/18327936488/job/52196728885?pr=2
<details>
<summary>Details</summary>
```
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/randomenv.cpp:61:15: warning: '__p__environ' redeclared without 'dllimport' attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
61 | extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/aarch64-w64-mingw32/include/stdlib.h:656:17: note: expanded from macro 'environ'
656 | #define environ _environ
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/aarch64-w64-mingw32/include/stdlib.h:225:21: note: expanded from macro '_environ'
225 | #define _environ (* __p__environ())
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/aarch64-w64-mingw32/include/stdlib.h:221:27: note: previous declaration is here
221 | _CRTIMP char ***__cdecl __p__environ(void);
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/aarch64-w64-mingw32/include/stdlib.h:221:3: note: previous attribute is here
221 | _CRTIMP char ***__cdecl __p__environ(void);
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/aarch64-w64-mingw32/include/_mingw.h:52:40: note: expanded from macro '_CRTIMP'
52 | # define _CRTIMP __attribute__ ((__dllimport__))
| ^
1 warning generated.
```
</details>
after:
https://github.com/l0rinc/bitcoin-core-nightly/actions/runs/18329616268/job/52201940831?pr=4
<details>
<summary>Details</summary>
```
[ 28%] Building CXX object src/util/CMakeFiles/bitcoin_util.dir/__/randomenv.cpp.obj
```
</details>
Note that there are some other remaining warnings in the logs that will be fixed in separate PRs
ACKs for top commit:
sipa:
utACK 9610b0d1e2 if this makes the compilers happy
laanwj:
Code review ACK 9610b0d1e2
hebasto:
re-ACK 9610b0d1e2.
Tree-SHA512: a9e39d288b663ed24cbbbae228850e6f02d417d8781a3ac3d0b3db0b7ff734bbd62fddb9f57b8f77daab4e9c016ff66906ebc5fb2de7635ef539ef7f4dc2eaba
fa20275db3 test: Use unassigned p2p_port instead of hardcoded 60000 in p2p_i2p_ports.py (MarcoFalke)
Pull request description:
The goal is to fix https://github.com/bitcoin/bitcoin/issues/30030.
The root cause it unclear. However, hard-coding the port to 60000 does not seem ideal anyway. This could break in an unlikely setting where so many functional tests are run, such that the port is occupied. Also, it could fail when `TEST_RUNNER_PORT_MIN` is set sufficiently high. (This is purely theoretical, as I don't think anyone would run a command like this, but on current master it fails, and on this pull it passes: `TEST_RUNNER_PORT_MIN=60000 ./bld-cmake/test/functional/p2p_i2p_ports.py --portseed=0`)
So fix those issues (and hopefully also 30030) by using an unoccupied p2p_port.
The logic is similar to the `extra_port()` logic in the `feature_bind_extra.py` test.
ACKs for top commit:
laanwj:
Code review ACK fa20275db3
mzumsande:
ACK fa20275db3
Tree-SHA512: ac5487ca195db9ca746b78b8add91d0b9ef59cc3be0cdb7fbd9f76d42549eea68a61c32b4f5a162e01f3777959110f9f8d56ff05af6a13a9f61ea5be5b7d8631
c864a4c194 Simplify fs::path by dropping filename() and make_preferred() overloads (Ryan Ofsky)
b0113afd44 Fix windows libc++ fs::path fstream compile errors (Ryan Ofsky)
Pull request description:
Drop support for passing `fs::path` directly to `std::ifstream` and `std::ofstream` constructors and `open()` functions, because as reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545, after https://wg21.link/lwg3430 there is no way this can continue to work in windows builds, and there are already compile errors compiling for windows with newer versions of libc++.
Instead, add an `fs::path::std_path()` method that returns `std::filesystem::path` references and use it where needed.
ACKs for top commit:
hebasto:
ACK c864a4c194.
l0rinc:
Code review ACK c864a4c194
maflcko:
re-ACK c864a4c194 🌥
Tree-SHA512: d22372692ab86244e2b2caf4c5e9c9acbd9ba38df5411606b75e428474eabead152fc7ca1afe0bb0df6b818351211a70487e94b40a17b68db5aa757604a0ddf6
This has a few benefits:
* The shellcheck SC2086 warning is disabled for the whole command, but
is only needed for the DOCKER_BUILD_CACHE_ARG env var. So in Python,
only pass this one env var to shlex.split() for proper word splitting.
* Future logic improvements can be implemented in Python.
The comments are moved, which can be checked via the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The option is currently unused. If it is used again in the future, it
could trivially be added back.
Also, the logic is just a single undocumented python command one-liner.
So remove it for now.
4b41f99d57 build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script (Henry Romp)
Pull request description:
Remove `CMAKE_SKIP_INSTALL_RPATH` from CMakeLists.txt and add `CMAKE_SKIP_RPATH` to the Guix build script. This keeps build-environment-specific settings in the build scripts rather than hardcoded in the CMake configuration.
ACKs for top commit:
purpleKarrot:
ACK 4b41f99d57
janb84:
re ACK 4b41f99d57
Tree-SHA512: 74d6af382476d731f10f9833978d670e9981c160ba306d0e9d4b1ad1e9b9960b8d03a3b9b608e234edb1c0c2c7a2b4f9f606a2a7887b7a153792159e71ae9b21
Ava Chow