We need to determine if CJDNS is reachable before we parse any IPv6
addresses (for example, by the -rpcallowip setting) or an RFC4193
address might get flipped to CJDNS, which can not be used with subnets
fab5a3c803 test: Remove unused verify_flags suppression (MarcoFalke)
Pull request description:
`static bool verify_flags(unsigned)` was removed in commit 80f8b92f4f
ACKs for top commit:
fanquake:
ACK fab5a3c803
hebasto:
ACK fab5a3c803, I have reviewed the code and it looks OK.
Tree-SHA512: da0cfc6ee253419c0aef316cd9c8366b744231261b755a95618ca0e777c1d95cecba8199db5486fd35079ded89c64c1a9f5b056f01dada4176b815b0d97261b7
8f4ba90b8f build: document why we check for std::system (fanquake)
Pull request description:
It's probably debatable if we support targets like iOS, but for now, document why we are checking for this standard library feature.
Trying to use `std::system` for a `aarch64-darwin-ios` target results in:
```bash
test.cpp:7:10: error: 'system' is unavailable: not available on iOS
7 | std::system("some_command");
| ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/_stdlib.h:203:6: note: 'system' has been explicitly marked unavailable here
203 | int system(const char *) __DARWIN_ALIAS_C(system);
| ^
1 error generated.
```
ACKs for top commit:
Sjors:
ACK 8f4ba90b8f
Tree-SHA512: 219cac205b36004c607194f6956c2ce6153f192bd4349e505b52c4e511e403e195ce0f462ae10c515e67f1e95d4b1636d526c8e4376004044853b574a84df427
516f0689b5 refactor: re-enable UBSan implicit-sign-change in serialize.h (Lőrinc)
5827e93507 refactor: use consistent size type for serialization template parameters (Lőrinc)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/32154, the main goal of this PR is to reenable sanitizer checks for `serialize.h` since it's modified in a few other PRs.
ACKs for top commit:
maflcko:
review ACK 516f0689b5🎈
stickies-v:
ACK 516f0689b5, nice cleanup!
Tree-SHA512: 63da9bf1988a5b68e3c053b0ed786b8735f8f75b05763511522d1601b728b55798006e063137447615c266582622642d3226318fa83e488bd363f1756f8811e8
486bc91790 depends: bump to latest config.sub (Sebastian Falbesoner)
6880383427 depends: bump to latest config.guess (Sebastian Falbesoner)
Pull request description:
Noticed that these files were last updated from [upstream](https://git.savannah.gnu.org/gitweb/?p=config.git) quite a while ago (in 2023, see #28781), so bump them again.
Can be verified via e.g.
```
$ git clone https://git.savannah.gnu.org/git/config.git /tmp/config.git
$ diff /tmp/config.git/config.guess ./depends/config.guess
$ diff /tmp/config.git/config.sub ./depends/config.sub
```
ACKs for top commit:
fanquake:
ACK 486bc91790
Tree-SHA512: cbfd21a351a2404e5821610b6ef84d1050ea1a8045da8bfb535ef1ed49b5ad3f4140e8332d1eed545332f96d3117adc531d73aa83e19e7154fe382d041102c93
301993ebf7 init: drop -upnp (fanquake)
Pull request description:
This was slated for removal in `30.0`, so remove it.
ACKs for top commit:
i-am-yuvi:
ACK 301993ebf7
maflcko:
review ACK 301993ebf7
darosior:
tACK 301993ebf7
Tree-SHA512: 635e374c013fa08c4cda7caadc465c89bb376d3ee2c66f67a27e3ed9031844674d3e996169aaffb9b65a67b0d44d92aaec000aaf69abe3dd10fce2f4138f3e27
8f4fed7ec7 symbol-check: Add check for application manifest in Windows binaries (Hennadii Stepanov)
2bb6ab8f1b ci: Add "Get bitcoind manifest" steps to Windows CI jobs (Hennadii Stepanov)
282b4913c7 cmake: Add application manifests when cross-compiling for Windows (Hennadii Stepanov)
Pull request description:
Windows [application manifests ](https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests) provide several benefits—such as enhanced security settings, and the ability to set a process-wide code page (required for https://github.com/bitcoin/bitcoin/pull/32380), as well as granular control over supported Windows versions. Most of these benefits lie beyond the scope of this PR and will be evaluated separately.
On the current master branch @ fc6346dbc8, the linker generates and embeds a manifest only when building with MSVC:
```xml
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
<security>
<requestedPrivileges>
<requestedExecutionLevel level="asInvoker" uiAccess="false"></requestedExecutionLevel>
</requestedPrivileges>
</security>
</trustInfo>
</assembly>
```
However, this manifest fails validation:
```
> mt.exe -nologo -inputresource:build\bin\Release\bitcoind.exe -validate_manifest
mt.exe : general error 10100ba: The manifest is missing the definition identity.
```
This PR unifies manifest embedding for both native and cross-compilation builds.
Here is the change in the manifest on Windows:
```diff
--- bitcoind-master.manifest
+++ bitcoind-pr.manifest
@@ -1,5 +1,6 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+ <assemblyIdentity type="win32" name="org.bitcoincore.bitcoind" version="29.99.0.0"></assemblyIdentity>
<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
<security>
<requestedPrivileges>
```
which effectively resolves the "missing the definition identity" error.
Finally, “Get bitcoind manifest” steps have been added to the Windows CI jobs to ensure the manifest is embedded and validated.
ACKs for top commit:
sipsorcery:
re-tACK 8f4fed7ec7.
hodlinator:
re-ACK 8f4fed7ec7
davidgumberg:
Reviewed and tested ACK 8f4fed7ec7
Tree-SHA512: 6e2dbdc77083eafdc242410eb89a6678e37b11efd786505dcd7844f0bac8f44d68625e62924a03b26549bdb4aaec5330dc608e6b4d66789f0255092e23aef6cb
0671d66a8e wallet, refactor: Convert uint256 to Txid in wallet (marcofleon)
c8ed51e62b wallet, refactor: Convert uint256 to Txid in wallet interfaces (marcofleon)
b3214cefe6 qt, refactor: Convert uint256 to Txid in the GUI (marcofleon)
Pull request description:
This is part of https://github.com/bitcoin/bitcoin/pull/32189.
Converts all instances of transactions from `uint256` to `Txid` in the wallet, GUI, and related interfaces.
ACKs for top commit:
stickies-v:
re-ACK 0671d66a8e, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase.
achow101:
ACK 0671d66a8e
furszy:
Code review ACK 0671d66a8e
Tree-SHA512: 9fd4675db63195c4eed2d14c25015a1821fb597f51404674e4879a44a9cf18f475021a97c5f62f3926b7783ade5a38567386f663acba9f5861f1f59c1309ed60
fa2c662362 build: Revert "Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC" (MarcoFalke)
Pull request description:
Now that GitHub Actions has a fixed version and the Windows developers have updated their compiler, the workaround is no longer needed.
ACKs for top commit:
davidgumberg:
reACK fa2c662362
hodlinator:
ACK fa2c662362
Tree-SHA512: 952b36c917c91d78d82b5013e1df338b23f860fad7be43327150581f403050e61f748fc75557ec96fb2b115a2cc0246a506bc2ddc25e05f5a41339bd466c4b1a
This reverts commit b2d5361002.
Also, adjust the doc to reflect the new minimum version. Versions 17.6
or 17.11 (or anything in between) may still work on a best-effor basis,
but it is not checked by CI or by developers.
e62423d6f1 doc: Improve dependencies.md documentation (Nicola Leonardo Susca)
a3520f9d56 doc: Add dependency self-compilation info (Nicola Leonardo Susca)
d1fdc84c54 doc: Remove Linux Kernel from dep. table (Nicola Leonardo Susca)
Pull request description:
Small improvements to the `dependencies.md` documentation as a follow-up for #31634.
**Linux Kernel** does not need to be in the dependencies as it is not required for cross-compiling from other systems, and users building on Linux should not expect they can build using any EOL kernel, see: https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957123270
**Runtime dependencies** can be in a separate table to improve readability. See: https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550
**Version used** is redundant as the depends package definition is already linked in the table and can thus be removed, see: https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972
ACKs for top commit:
maflcko:
lgtm ACK e62423d6f1🛄
hebasto:
ACK e62423d6f1.
jonatack:
ACK e62423d6f1
Tree-SHA512: 586c450aec7ece5d543bcb12796a2bb7ff459e15c8813a7b5104a38d09fc51e7e902363ff023be48273ae2b1a1b0807a439c8523b4ea2e398b76b7c9a48d0dfb
fa981b90f5 ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32493
For some reason terminate or kill do not work inside the CI system under valgrind.
So disable the test for now, until a solution is found.
ACKs for top commit:
fanquake:
ACK fa981b90f5
mzumsande:
utACK fa981b90f5
Tree-SHA512: ce591fa7ffffbf757e2c15744e36a9e57300edf743400938e49fd02291f3977c551a3af1635bc7a6ccc1900d5ea150a64ee2ace46c1d765019ab11bd51035139
- Remove the "Version used" column from the dependencies tables as the
depends package definition which defines the version used is already
linked. In case a developer is interested in which PR introduced this
file/version they can use `git blame` on the package definition as
usual. This removes doc. maintenance overhead and eliminates the risk
of stale information about the "Version used", see comment:
https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2063356972
- Separate dependency tables into build-time and run-time tables for
easier distinction of the two and to avoid repeating the same
information ("No"/"Yes") for better readability.
- Order dependencies alphabetically
The `dependencies.md` should mention that it is possible to self-compile
the dependencies and reference `depends/README.md` for instructions.
Also mention full path to `/doc/build-*.md` for clarity.
Windows application manifests provide several benefits. However, on the
master branch, the linker generates and embeds manifests only when
building with MSVC.
This change unifies manifest embedding for both native and
cross-compilation.
75a185ea3d test: add skip_if_running_under_valgrind() (fanquake)
Pull request description:
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
ACKs for top commit:
maflcko:
lgtm ACK 75a185ea3d
willcl-ark:
ACK 75a185ea3d
Tree-SHA512: 7f45c3049ab39cc514024067bd6ac26598e99202c114b48459834c26c2e1273fa58af693878298e628a10c561b954850e49e76b39567b771bb0c0534a063a524
3b824169c7 doc: remove Carls substitute server from Guix docs (fanquake)
Pull request description:
This no-longer exists. Use one of the other Guix servers in the example.
ACKs for top commit:
achow101:
ACK 3b824169c7
hebasto:
ACK 3b824169c7, the listed substitute servers are the same as in https://guix.gnu.org/manual/en/html_node/Official-Substitute-Servers.html.
Tree-SHA512: dc3a362ccaa9ce8039d3c02158de9cd71082eb4dd790368bfb11c2942a5aae57e67779b5ff3108b532c4fb765811bd9e145eedb390fc48b52b43d334d5864865
a0eed55398 run_command: Enable close_fds option to avoid lingering fds (Luke Dashjr)
c7c356a448 cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux (Luke Dashjr)
4f5e04da13 Revert "remove unneeded close_fds option from cpp-subprocess" (Luke Dashjr)
Pull request description:
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).
> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and starting bitcoind again will fail). It's also a potential security issue if a child process is intended to be sandboxed at some point. Not to mention plain ugly :)
>
> cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in https://github.com/bitcoin/bitcoin/pull/29961 rather than fixing this during the migration, but this PR restores it, enables it for RunCommandParseJSON, and optimises it by iterating over /proc/self/fd/ like most other libraries do these days ([eg, glib]> (487b1fd20c/glib/gspawn.c (L1094))) since iterating all possible fd numbers [has been found to be problematic](https://bugzilla.redhat.com/show_bug.cgi?id=1537564).
>
> (Equivalent to https://github.com/bitcoin/bitcoin/pull/22417 was for boost::process)
ACKs for top commit:
achow101:
ACK a0eed55398
hebasto:
ACK a0eed55398, tested on Ubuntu 25.04:
vasild:
ACK a0eed55398
Tree-SHA512: 7dc1cb6cc1f45ff7c4f53512e400baad1a033b4ebf14ba6f6ffa38588314932d6d01ef67b197f081e8202bb802659ac6a87998277797721d6d7b20efde8e9a6b
5bf91ba880 wallet: Drop unused fFromMe from CWalletTx (David Gumberg)
Pull request description:
This has been unused since commit fe52346, this is a re-opening of #9351.
ACKs for top commit:
maflcko:
lgtm ACK 5bf91ba880
achow101:
ACK 5bf91ba880
Tree-SHA512: b9a84f27b6cfe7796dcf629be6a8e01a97d931ea81ef088951d54d6691ffe79d22138baacc632375093cf3176a22c265e30a80f1f63c3bc620d08bf16f6a488f
faf9082a5f test: Fix whitespace in prevector_tests.cpp (MarcoFalke)
fa7f04c8a7 refactor: Remove UB in prevector reverse iterators (MarcoFalke)
Pull request description:
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils such as `std::reverse_iterator{it}` or `std::views::reverse` can be used out of the box.
So remove them, along with the ubsan suppressions, that are no longer used.
I've tagged this a refactor, because the code was always dead (unused outside of tests). And since commit 2925bd537c it was completely dead. Also, I could not find a sanitizer that detects this type of UB.
ACKs for top commit:
l0rinc:
tested ACK faf9082a5f
achow101:
ACK faf9082a5f
stickies-v:
ACK faf9082a5f, nice find.
theuni:
utACK faf9082a5f
Tree-SHA512: 31511d520a1c0fdd65c2e5f1a8ef6fd17464303b6bff88a5d9d9577adfee849d431deb510882b6f4e15e8fb7168861bc0d26fca3bed4278f57a9d6e7b1235dce
4b24186756 test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
8ba245cb83 test: add constants for MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.
ACKs for top commit:
achow101:
ACK 4b24186756
rkrux:
re-ACK 4b24186
Tree-SHA512: f12919f71b3fff74df1d7ddaa8db455b1b139f7abd51d7f3fa5d750fc7dd613454b438c4e0dedad679476d414fa1da43ef1121e486b0bdfd97d5ef8bdf37f060
62fc42d475 interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq)
c39ca9d4f7 interfaces: move getTip implementation to miner (Sjors Provoost)
720f201e65 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq)
e6c2f4ce7a interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq)
02d4bc776b interfaces: remove redundant coinbase fee check in `waitNext` (ismaelsadeeq)
Pull request description:
#### Motivation
In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](https://github.com/bitcoin/bitcoin/blob/master/src/wallet) and just exposed in [src/interfaces/](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces) instead of being implemented there, so it can be more modular and accessible to unit tests.
However the some methods in the newly added `BlockTemplateImpl` and `MinerImpl` classes partially enforces this guideline, as the implementations of the `submitSolution`, `waitNext`, and `waitTipChanged` methods reside within the class itself.
#### What the PR Does
This PR introduces a simple refactor by moving certain method implementations from `BlockTemplateImpl` into the miner module. It introduces three new functions:
1. Remove rundundant coinbase fee check in `waitNext`
2. **`AddMerkleRootAndCoinbase`**: Computes the block's Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by `submitSolution` before the block is submitted for processing.
3. **`WaitAndCreateNewBlock`**: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns `nullptr`. The `waitNext` method in `BlockTemplateImpl` now simply wraps this function.
4. Move `GetTip` implementation to miner.
5. **`WaitTipChanged`**: Returns the tip when the chain it changes, or `nullopt` if a timeout or interrupt occurs. The `waitTipChanged` method in `MinerImpl` now calls `GetTip` after invoking `ChainTipChanged`, and returns the tip.
#### Behavior Change
- We now only `Assert` for a valid chainman and notifications pointer once.
ACKs for top commit:
achow101:
ACK 62fc42d475
Sjors:
ACK 62fc42d475
ryanofsky:
Code review ACK 62fc42d475. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
Tree-SHA512: 502632f94ced81f576b2c43cf015f1527e2c259e6ca253f670f5a6889171e2246372b4e709575701afa3f01d488d6633557fef54f48fe83bbaf1836ac5326c4f
a04f17a188 doc: warn that CheckBlock() underestimates sigops (Sjors Provoost)
Pull request description:
Counting sigops in the witness requires context that `CheckBlock()` does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
The commit message contains some historical context.
ACKs for top commit:
ismaelsadeeq:
ACK a04f17a188
ryanofsky:
Code review ACK a04f17a188
Tree-SHA512: 26528367a7f3cfa8540ef0b90f7aa912c8f0bc057428f20a1fd1d4e232dac77747bc20044f0fcb0ffab8a2e1fb3dbe3dab46be749553a917744ddc7a829025cb
Made every signed/unsigned conversion in the serialization helpers explicit so the UBSan `implicit-sign-change` check passes and the `serialize.h` suppression can be dropped.
For consistency, a few other simple changes were also applied to the serialization helpers:
* remove redundant `inline` on function templates;
* unify formatting to make the differences between similar methods obvious.
- This commit creates a function `WaitTipChanged` that waits for the connected
tip to change until timeout elapsed.
- This function is now used by `waitTipChanged`
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
e7ad86e1ca test: fix another intermittent failure in wallet_basic.py (Martin Zumsande)
07350e204d test: Fix intermittent failure in wallet_basic.py (Martin Zumsande)
Pull request description:
Fixes two rare failures that happened in the CI:
#27249:
There could be a race with outstanding TxAddedToMempool notifications being applied to the soon-to-be created wallet:
1. importdescriptors during rescan sets status to `TxStateConfirmed`
2. old `transactionAddedToMempool` notification changes status back to `TxStateInMempool`
3. If the listunspent call happens here the test will fail
4. blockConnected notification will change the status back to `TxStateConfirmed` (so it's not a persistent failure)
I could reproduce this by adding a 100 microsecond sleep to `AddToWallet()`, the fix is to add a sync, so `transactionAddedToMempool` notifications won't affect the new wallet anymore.
#32456:
During init, the test framework will start using rpc after the mempool was loaded.
It will not wait for `start()` / `postInitProcess` or outstanding `transactionAddedToMempool` notifications (which would both set the status to `TxStateInMempool`), leading to
a possible race, in which `listunspent` can be called while the tx is still in `Inactive` status.
Can be reproduced by adding two sleeps: To init before calling `start()` for the chain clients, plus to `transactionAddedToMempool` in `wallet.cpp`.
Prevent this by processing outstanding notifications.
Fixes#27249Fixes#32456
ACKs for top commit:
maflcko:
review ACK e7ad86e1ca🎩
Tree-SHA512: 1f1a11e5c8e1c6d3c39a49401c2c5122befdbbec25c0451953f5bfe8dfb53221ada552a68006e266570addda12bb16c1b9b1e49ad2198c33d91c4b96b764d73e
fa427ffcee fuzz: Properly setup wallet in wallet_fees target (MarcoFalke)
Pull request description:
`g_wallet_ptr` is destructed after the `testing_setup`. This is not supported and will lead to issues such as https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2863875857 or https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932.
This could be fixed by fixing the initialization order.
However, the global wallet is also modified in the fuzz target, which is bad fuzzing practise.
So instead fix it by constructing a fresh wallet for each fuzz iteration.
ACKs for top commit:
brunoerg:
code review ACK fa427ffcee
hebasto:
ACK fa427ffcee, this change fixes the issue when building the "Debug" configuration with MSVC on Windows.
marcofleon:
Code review ACK fa427ffcee
Tree-SHA512: 161b93fc39a609cb16d9ffea7366c5e339bd01712577f0782aedff46c00f79edd2a907807ac83f9fcec687b4bbbe0fd6e6f75e32169639a310e4e7b771078b3b
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
It's probably debatable if we support targets like iOS, but for now,
document why we are checking for this standard library feature.
Trying to use `std::system` for a `aarch64-darwin-ios` target results in
```bash
test.cpp:7:10: error: 'system' is unavailable: not available on iOS
7 | std::system("some_command");
| ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/_stdlib.h:203:6: note: 'system' has been explicitly marked unavailable here
203 | int system(const char *) __DARWIN_ALIAS_C(system);
| ^
1 error generated.
```
rend() creates a pointer with offset -1. This is UB, according to the
C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils
such as std::reverse_iterator{it} or std::views::reverse can be used out
of the box.
So remove them, along with the ubsan suppressions, that are no longer
used.
1e0de7a6ba fees: document non-monotonic estimation edge case (willcl-ark)
Pull request description:
Closes: https://github.com/bitcoin/bitcoin/issues/11800
In scenarios where data is available for higher targets but not for lower ones, this method *may* return lower fee rates for higher confirmation targets. This could occur if `estimateCombinedFee` returns no valid data (`-1`) for some estimates for a low target, but **does** return valid data for a higher target.
Users of this function should be aware of this potential, if unlikely, inconsistency in behaviour in data-sparse scenarios.
ACKs for top commit:
adamandrews1:
Code review ACK 1e0de7a
ismaelsadeeq:
Code review ACK 1e0de7a6ba
glozow:
ACK 1e0de7a6ba
Tree-SHA512: 161e5dafdd131570853a89491753ae39a7b725d1a86cab5a7294c2a5939da1a9a5f2c4aca0900e9ad810e828b6e0e636f256384e3d1fda6dd552da189bbbe747
0750249289 mining: document gbt_rule_value helper (Sjors Provoost)
5e87c3ec09 scripted-diff: rename gbt_force and gbt_force_name (Sjors Provoost)
Pull request description:
The term "force" is ambiguous and not used in [BIP9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes) where there ! rule prefix is introduced.
E.g. this code is hard to read:
```cpp
if (!gbt_force) {
s.insert(s.begin(), '!');
```
Additionally, #29039 renamed `gbt_vb_name` to `gbt_force_name` which, at least for me, further increased the confusion.
This is a pure (variable rename) refactor (plus documentation) and does not change behavior.
Reminder of how to verify a scripted diff:
```sh
test/lint/commit-script-check.sh origin/master..HEAD
```
ACKs for top commit:
achow101:
ACK 0750249289
janb84:
ACK [0750249](0750249289)
musaHaruna:
ACK [0750249](0750249289)
glozow:
ACK 0750249289, seems sensible
Tree-SHA512: 8c88a273a3b36040f6c641843bd20579d0065b051aad4b39fc14f0d2af2808690dff6772bd8b1a4d9699b72279a700d2661012651bc315433a123dcc8996adaa
2aa63d511a test: Use uninvolved pruned node in feature_pruning undo test (enoch)
772ba7f9ce test: Fix nTimes typo in feature_pruning test (enoch)
Pull request description:
This PR contains two commits:
1. Fixes a typo in feature_pruning.py where 'nTimes' was incorrectly
used instead of 'nTime'. This typo caused the test to always reset
mine_large_blocks.nTime to 0, rather than only on the first run.
2. Fixes the test failure exposed by the typo fix. The
test_pruneheight_undo_presence test was failing because it was using
node 2, which is involved in reorg testing and could be on a
different chain than other nodes. The solution switches to using
node 5, which is also a pruned node but isn't involved in reorg
testing.
Testing:
- Ran test/functional/feature_pruning.py multiple times to verify
consistent passing
- Verified that the test now passes with the correct nTime variable name
- Confirmed the test behavior matches the intended functionality of
verifying pruned block availability
- Ran the full test suite to ensure the changes did not introduce any
regressions or affect other tests
Thanks to fjahr for his assistance in diagnosing the issue and
suggesting the solution.
This fixes the test failure reported in #32249
ACKs for top commit:
fjahr:
tACK 2aa63d511a
maflcko:
lgtm ACK 2aa63d511a
naiyoma:
tACK 2aa63d511a
stratospher:
tested ACK 2aa63d5. verified that `nTime` is being incremented now.
Tree-SHA512: a543528fd4eeb30e978c0b43cfa109768252edaf1f94679dbbc7fe684122c00da34224e2cc1abd2a265af1b267eef1cd34246207946cf7d8e93d2c0f11aa56d8
