5555bce994 ci: Document why IN_GETOPT_BIN env var is needed on macOS (MarcoFalke)
fabe516440 ci: Export the container id in python script (MarcoFalke)
fa6aa9f42f ci: Retry image building once on failure (MarcoFalke)
fa4dbe04d7 ci: Allow overwriting check option in run() helper (MarcoFalke)
fa8e4de5c3 ci: Use os.environ[key] access when value must be set (MarcoFalke)
Pull request description:
This should fix https://github.com/bitcoin/bitcoin/issues/33640.
It also contains a few refactor cleanups, which are explained in the corresponding commits.
ACKs for top commit:
l0rinc:
Code review reACK 5555bce994
kevkevinpal:
ACK [5555bce](5555bce994)
davidgumberg:
crACK 5555bce994
Tree-SHA512: f1ea95b0650e57d6a9f97c575a11ee461832c0715c3d1a24dbfe12ccc5366f295639d4c4827f1d01da460ddf00917ecaa627e7dbd12e405770db6c53c3778a9c
53b34c80c6 ci: use pycapnp 2.2.1 in mac native job (fanquake)
865432869c ci: remove Python version comment from mac config (fanquake)
Pull request description:
Switch to using v2.2.1 in the mac native job. Remove the git clone & install step.
ACKs for top commit:
maflcko:
lgtm ACK 53b34c80c6
l0rinc:
crACK 53b34c80c6
hebasto:
ACK 53b34c80c6.
Tree-SHA512: e756694c14431aacb3e48104331da88285c7500b4c4599c698f50d721d428ffe61258be075ef526b93c15aa3331f38535ca95249a2ef3ebfc804f61479095d9b
53e4951a5b Switch to ANSI Windows API in `fsbridge::fopen()` function (Hennadii Stepanov)
dbe770d921 Switch to ANSI Windows API in `Win32ErrorString()` function (Hennadii Stepanov)
06d0be4e22 Remove no longer necessary `WinCmdLineArgs` class (Hennadii Stepanov)
f366408492 cmake: Set process code page to UTF-8 on Windows (Hennadii Stepanov)
dccbb17806 Set minimum supported Windows version to 1903 (May 2019 Update) (Hennadii Stepanov)
Pull request description:
The main goal is to remove [deprecated](https://github.com/bitcoin/bitcoin/issues/32361) code (removed in C++26).
This PR employs Microsoft's modern [approach](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page) to handling UTF-8:
> Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.
TODO:
- [x] Handle application manifests properly when building with MSVC.
- [x] Bump the minimum supported Windows version to 1903 (May 2019 Update).
- [x] Remove all remaining use cases of the deprecated `std:wstring_convert`.
- The instance in `subprocess.h` will be addressed in a follow-up PR, as additional tests are likely needed.
- The usage in `common/system.cpp` is handled in https://github.com/bitcoin/bitcoin/pull/32566.
Resolves partially https://github.com/bitcoin/bitcoin/issues/32361.
ACKs for top commit:
laanwj:
re-ACK 53e4951a5b
hodlinator:
re-ACK 53e4951a5b
davidgumberg:
untested crACK 53e4951a5b
Tree-SHA512: 0dbe9badca8b979ac2b4814fea6e4a7e53c423a1c96cb76ce894253137d3640a87631a5b22b9645e8f0c2a36a107122eb19ed8e92978c17384ffa8b9ab9993b5
57f7c68821 test: add functional test for `TestShell` (matching doc example) (Sebastian Falbesoner)
53874f7934 doc: test: update TestShell example instructions/options (Sebastian Falbesoner)
Pull request description:
This PR adds a functional framework test for the `TestShell` class. The primary motivation for this is to avoid that the example instructions for the interactive Python shell in `test-shell.md` get outdated or broken without noticing, a problem we had already several times in the past (see #26520, #27906, #31415). Having a copy is still not perfect, as docs and functional test have to be kept in sync, but I don't expect this to be a problem in practice, assuming the hint in the functional test comment is hopefully noticed if changes are made.
Alternatively, the example instructions in `test-shell.md` could be removed with a hint to the functional test (I tend to prefer to keep the docs as-is though, showing the full REPL interaction).
The first commit contain two small removal fix-ups in `test-shell.md` regarding the result of the `createwallet` RPC call and the mentioning of the `noshutdown` option that was removed earlier (see #31620). Could be backported to v30.
ACKs for top commit:
brunoerg:
ACK 57f7c68821
stratospher:
ACK 57f7c68.
pinheadmz:
ACK 57f7c68821
Tree-SHA512: 25c35af46b742bbefce7838708437529bbf613fa3d1f0fba590cacef0acdde82b3a78c7a01459c73eaac26ce3f1041e54092d098f0fc94a8c76ac0b4f4970338
1a7fb5eeee fees: return current block height in estimateSmartFee (ismaelsadeeq)
ab49480d9b fees: rename fees_args to block_policy_estimator_args (ismaelsadeeq)
06db08a435 fees: refactor: rename fees to block_policy_estimator (ismaelsadeeq)
6dfdd7e034 fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp (ismaelsadeeq)
Pull request description:
This PR is a simple refactoring that does four things:
1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.
**Motivation**
In preparation for adding a new fee estimator, the `fees` directory is created so we can organize code into `block_policy_estimator` and `mempool` because
a) It would be clunky to add more code directly under `fees`.
b) Having `policy/fees.{h,cpp}` and `policy/mempool.{h,cpp}` would also be undesirable.
Therefore, it makes sense to structure the it as `policy/fees/block_policy_estimator`, `policy/fees/mempool`, etc.
Hence test file were also updated accordingly.
The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.
ACKs for top commit:
maflcko:
re-ACK 1a7fb5eeee🐤
polespinasa:
re ACK 1a7fb5eeee
willcl-ark:
ACK 1a7fb5eeee
janb84:
re ACK 1a7fb5eeee
Tree-SHA512: fef7ace2a9f262ec0361fb7a46df5108afc46b5c4b059caadf2fd114740aefbb2592389d11646c13d0e28bf0ef2cfcfbab3e659c4d4288b8ebe64725fd1963c0
944e5ff848 doc: mention key removal in rpc interface modification (rkrux)
Pull request description:
A discussion in a previous PR 32618 prompted me to add this note: https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2181951390
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
maflcko:
lgtm ACK 944e5ff848
stickies-v:
ACK 944e5ff848
glozow:
lgtm ACK 944e5ff848
Tree-SHA512: f64c086c99e7c73a3ae7d60b2e8e06c8e7a3a49305a66d5c5a96db9b4ebbd01928ab5ccbcbdac26f400d16662f84469c448625e1f55ec2a9a920eff8a05fc379
02d2b5a11c ci, iwyu: Treat warnings as errors for specific directories (Hennadii Stepanov)
57a3eac387 refactor: Fix includes in `index` directory (Hennadii Stepanov)
bdb8eadcdc refactor: Fix includes in `crypto` directory (Hennadii Stepanov)
56f2a689a2 ci: Do not patch `leveldb` to workaround UB in "tidy" CI job (Hennadii Stepanov)
Pull request description:
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
ACKs for top commit:
maflcko:
re-ACK 02d2b5a11c💮
ryanofsky:
Code review ACK 02d2b5a11c. Just rebased and update tidy patch comment again since last review
willcl-ark:
ACK 02d2b5a11c
Tree-SHA512: 1c966e01c47bf3e7d225faa3b819367f757430e2d71e9582fa82d67307aabe3f0d76f69346ee180192e7f5ab194ecc58d2b8ecf178eab26ba3309a6b55bff4b6
59c4898994 guix: remove python-pydantic-core input from LIEF (fanquake)
9f2a6927d3 guix: use Clang & LLVM 19 for macOS build (fanquake)
9570ddbec9 guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2 (fanquake)
7b5cc276aa guix: patch around riscv issue with newer (2.40+) binutils (fanquake)
91b5cbaabb ci: use Debian Trixie for macOS cross job (fanquake)
Pull request description:
5cb84f2013 isn't super recent, but it's enough to get access to some newer packages, such as LLVM 19, and avoids having to add any further work arounds for things that we know are fixed later (i.e nsis). Once things upstream have stabilized a bit more (the `core-updates` branch was fairly recently merged), we could look at bumping to something newer.
Package updates:
(base) glibc 2.35 -> 2.39
binutils 2.38 -> 2.41
diffutils 3.8 -> 3.10
gawk 5.2.1 -> 5.3.0
git-minimal 2.45.2 -> 2.46.0
grep 3.8 -> 3.11
gzip 1.12 -> 1.13
linux-headers 6.1.106 -> 6.1.119
make 4.3 -> 4.4.1
xz 5.2.8 -> 5.4.5
CMake 3.30 becomes available.
Clang/LLVM 19 becomes available.
Could be used for #32764.
ACKs for top commit:
hebasto:
re-ACK 59c4898994.
willcl-ark:
ACK 59c4898994
Tree-SHA512: c44965d5a315e4c862f5e40d8e98c645713405fec72a61055f95b6c68b7d2dcc69a61a084e397a4556d4c1df18f1cfa7a905234643fe4a7df9c58d486e26c097
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 45bd891465
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` ](d127b25199 (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](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
65a10fc3c5 p2p: add assertion for BlockTransactionsRequest indexes (frankomosh)
58be359f6b fuzz: add a target for DifferenceFormatter Class (frankomosh)
Pull request description:
Adds a fuzz test for the [`DifferenceFormatter`](e3f416dbf7/src/blockencodings.h (L22-L42)) (used in [`BlockTransactionsRequest`](https://github.com/bitcoin/bitcoin/blob/master/src/blockencodings.h#L44-L54), [BIP 152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki)). The DifferenceFormatter class implements differential encoding for compact block transactions (BIP 152). This PR ensures that its strictly-monotonic property is maintained. It complements the tests in [`blocktransactionsrequest_deserialize`](9703b7e6d5/src/test/fuzz/deserialize.cpp (L314)).
Additionally, there's an added invariant check after GETBLOCKTXN deserialization in `net_processing.cpp`.
ACKs for top commit:
Crypt-iQ:
tACK 65a10fc3c5
achow101:
ACK 65a10fc3c5
dergoegge:
Code review ACK 65a10fc3c5
Tree-SHA512: 70659cf045e99bb5f753763c7ddac094cb2883c202c899276cbe616889afa053b2d5e831f99d6386d4d1e4118cd35fa0b14b54667853fe067f6efe2eb77b4097
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](fa70e23de7)
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
faa9d10c84 refactor: Construct g_verify_flag_names on first use (MarcoFalke)
Pull request description:
The current usage of the `g_verify_flag_names` map seems fine and I can not see a static initialization order fiasco here.
However, it seems brittle to hope this remains the case in the future. Also, it triggers a msan false-positive in the fuzz CI task. (C.f https://github.com/bitcoin-core/qa-assets/actions/runs/18352815555/job/52413137315?pr=241#step:7:5245)
So just apply the "Construct on first use" idiom.
ACKs for top commit:
kevkevinpal:
ACK [faa9d10](faa9d10c84)
ajtowns:
ACK faa9d10c84
janb84:
lgtm ACK faa9d10c84
stickies-v:
ACK faa9d10c84
Tree-SHA512: 6685dfc91c99a8245722e07fac99a7a6d58586c30964be7ccd74a176dfbf00c6255c8594621e2909640763924f51d3efd4ce65ed65eaeeb1d05c2fd01fe63604
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](8f7673257a)
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](e9cd45e3d3)
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`](cdb052f1d4/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
