The notifications are used for notifying on connected blocks and on
warning and fatal error conditions.
The user of the C header may define callbacks that gets passed to the
internal notification object in the
`kernel_NotificationInterfaceCallbacks` struct.
Each of the callbacks take a `user_data` argument that gets populated
from the `user_data` value in the struct. It can be used to recreate the
structure containing the callbacks on the user's side, or to give the
callbacks additional contextual information.
As a first option, add the chainparams. For now these can only be
instantiated with default values. In future they may be expanded to take
their own options for regtest and signet configurations.
This commit also introduces a unique pattern for setting the option
values when calling the `*_set(...)` function.
The context introduced here holds the objects that will be required for
running validation tasks, such as the chosen chain parameters, callbacks
for validation events, and interrupt handling. These will be used by the
chainstate manager introduced in subsequent commits.
This commit also introduces conventions for defining option objects. A
common pattern throughout the C header will be:
```
options = object_option_create();
object = object_create(options);
```
This allows for more consistent usage of a "builder pattern" for
objects where options can be configured independently from
instantiation.
Exposing logging in the kernel library allows users to follow
operations. Users of the C header can use
`kernel_logging_connection_create(...)` to pass a callback function to
Bitcoin Core's internal logger. Additionally the level and category can
be globally configured.
By default, the logger buffers messages until
`kernel_loggin_connection_create(...)` is called. If the user does not
want any logging messages, it is recommended that
`kernel_disable_logging()` is called, which permanently disables the
logging and any buffering of messages.
Co-authored-by: stringintech <stringintech@gmail.com>
As a first step, implement the equivalent of what was implemented in the
now deprecated libbitcoinconsensus header. Also add a test binary to
exercise the header and library.
Unlike the deprecated libbitcoinconsensus the kernel library can now use
the hardware-accelerated sha256 implementations thanks for its
statically-initialzed context. The functions kept around for
backwards-compatibility in the libbitcoinconsensus header are not ported
over. As a new header, it should not be burdened by previous
implementations. Also add a new error code for handling invalid flag
combinations, which would otherwise cause a crash.
The macros used in the new C header were adapted from the libsecp256k1
header.
To make use of the C header from C++ code, a C++ header is also
introduced for wrapping the C header. This makes it safer and easier to
use from C++ code.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
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
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
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
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
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
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
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
24d861da78 coins: only adjust `cachedCoinsUsage` on `EmplaceCoinInternalDANGER` insert (Lőrinc)
d7c9d6c291 coins: fix `cachedCoinsUsage` accounting to prevent underflow (Lőrinc)
39cf8bb3d0 refactor: remove redundant usage tracking from `CoinsViewCacheCursor` (Lőrinc)
67cff8bec9 refactor: assert newly-created parent cache entry has zero memory usage (Lőrinc)
Pull request description:
### Summary
This PR fixes `cachedCoinsUsage` accounting bugs in `CCoinsViewCache` that caused UBSan `unsigned-integer-overflow` violations during testing. The issues stemmed from incorrect decrement timing in `AddCoin()`, unconditional reset in `Flush()` on failure, and incorrect increment in `EmplaceCoinInternalDANGER()` when insertion fails.
### Problems Fixed
**1. `AddCoin()` underflow on exception**
- Previously decremented `cachedCoinsUsage` *before* the `possible_overwrite` validation
- If validation threw, the map entry remained unchanged but counter was decremented
- This corrupted accounting and later caused underflow
- **Impact**: Test-only in current codebase, but unsound accounting that could affect future changes
**2. `Flush()` accounting drift on failure**
- Unconditionally reset `cachedCoinsUsage` to 0, even when `BatchWrite()` failed
- Left the map populated while the counter read zero
- **Impact**: Test-only (production `BatchWrite()` returns `true`), but broke accounting consistency
**3. Cursor redundant usage tracking**
- `CoinsViewCacheCursor::NextAndMaybeErase()` subtracted usage when erasing spent entries
- However, `SpendCoin()` already decremented and cleared the `scriptPubKey`, leaving `DynamicMemoryUsage()` at 0
- **Impact**: Redundant code that obscured actual accounting behavior
**4. `EmplaceCoinInternalDANGER()` double-counting**
- Incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key)
- Inflated the counter on duplicate attempts
- **Impact**: Mostly test-reachable (AssumeUTXO doesn't overwrite in production), but incorrect accounting
### Testing
To reproduce the historical UBSan failures on the referenced baseline and to verify the fix, run:
```
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh
```
The change was tested with the related unit and fuzz test, and asserted before/after each `cachedCoinsUsage` change (in production code and fuzz) that the calculations are still correct by recalculating them from scratch.
<details>
<summary>Details</summary>
```C++
bool CCoinsViewCache::CacheUsageValid() const
{
size_t actual{0};
for (auto& entry : cacheCoins | std::views::values) actual += entry.coin.DynamicMemoryUsage();
return actual == cachedCoinsUsage;
}
```
or
```patch
diff --git a/src/coins.cpp b/src/coins.cpp
--- a/src/coins.cpp(revision fd3b1a7f4bb2ac527f23d4eb4cfa40a3215906e5)
+++ b/src/coins.cpp(revision 872a05633bfdbd06ad82190d7fe34b42d13ebfe9)
@@ -96,6 +96,7 @@
fresh = !it->second.IsDirty();
}
if (!inserted) {
+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
}
it->second.coin = std::move(coin);
@@ -133,6 +134,7 @@
bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
CCoinsMap::iterator it = FetchCoin(outpoint);
if (it == cacheCoins.end()) return false;
+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACEPOINT(utxocache, spent,
outpoint.hash.data(),
@@ -226,10 +228,12 @@
if (itUs->second.IsFresh() && it->second.coin.IsSpent()) {
// The grandparent cache does not have an entry, and the coin
// has been spent. We can just delete it from the parent cache.
+ Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
cacheCoins.erase(itUs);
} else {
// A normal modification.
+ Assert(cachedCoinsUsage >= itUs->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
if (cursor.WillErase(*it)) {
// Since this entry will be erased,
@@ -279,6 +283,7 @@
{
CCoinsMap::iterator it = cacheCoins.find(hash);
if (it != cacheCoins.end() && !it->second.IsDirty() && !it->second.IsFresh()) {
+ Assert(cachedCoinsUsage >= it->second.coin.DynamicMemoryUsage());
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
TRACEPOINT(utxocache, uncache,
hash.hash.data(),
```
</details>
ACKs for top commit:
optout21:
reACK 24d861da78
andrewtoth:
ACK 24d861da78
sipa:
ACK 24d861da78
w0xlt:
ACK 24d861da78
Tree-SHA512: ff1b756b46220f278ab6c850626a0f376bed64389ef7f66a95c994e1c7cceec1d1843d2b24e8deabe10e2bdade2a274d9654ac60eb2b9bf471a71db8a2ff496c
3a10d700bc test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH` flag (brunoerg)
Pull request description:
This PR adds a test case for `GetTransactionSigOpCost` to check that P2SH sig ops are only counted when `SCRIPT_VERIFY_P2SH` flag is set.
Kills the following [mutant](https://corecheck.dev/mutation/src/consensus/tx_verify.cpp#L150):
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 9d09872597..cc7cdaaf8f 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
if (tx.IsCoinBase())
return nSigOps;
- if (flags & SCRIPT_VERIFY_P2SH) {
+ if (1==1) {
nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
}
```
ACKs for top commit:
l0rinc:
Tested ACK 3a10d700bc
maflcko:
re-lgtm ACK 3a10d700bc
instagibbs:
ACK 3a10d700bc
janb84:
tested ACK 3a10d700bc
Tree-SHA512: f560b4f9f2ce5c5fdd0a86e7e1f8ea27a8c6fda0327a6186a0c21e2c06ef13beeb017686db1688cace68812a01701abe46e8e1a095afefc6f2aed6ed96ba8288
ac599c4a9c test: Test MuSig2 in the wallet (Ava Chow)
68ef954c4c wallet: Keep secnonces in DescriptorScriptPubKeyMan (Ava Chow)
4a273edda0 sign: Create MuSig2 signatures for known MuSig2 aggregate keys (Ava Chow)
258db93889 sign: Add CreateMuSig2AggregateSig (Ava Chow)
bf69442b3f sign: Add CreateMuSig2PartialSig (Ava Chow)
512b17fc56 sign: Add CreateMuSig2Nonce (Ava Chow)
82ea67c607 musig: Add MuSig2AggregatePubkeys variant that validates the aggregate (Ava Chow)
d99a081679 psbt: MuSig2 data in Fill/FromSignatureData (Ava Chow)
4d8b4f5336 signingprovider: Add musig2 secnonces (Ava Chow)
c06a1dc86f Add MuSig2SecNonce class for secure allocation of musig nonces (Ava Chow)
9baff05e49 sign: Include taproot output key's KeyOriginInfo in sigdata (Ava Chow)
4b24bfeab9 pubkey: Return tweaks from BIP32 derivation (Ava Chow)
f14876213a musig: Move synthetic xpub construction to its own function (Ava Chow)
fb8720f1e0 sign: Refactor Schnorr sighash computation out of CreateSchnorrSig (Ava Chow)
a4cfddda64 tests: Clarify why musig derivation adds a pubkey and xpub (Ava Chow)
39a63bf2e7 descriptors: Add a doxygen comment for has_hardened output_parameter (Ava Chow)
2320184d0e descriptors: Fix meaning of any_key_parsed (Ava Chow)
Pull request description:
This PR implements MuSig2 signing so that the wallet can receive and spend from imported `musig(0` descriptors.
The libsecp musig module is enabled so that it can be used for all of the MuSig2 cryptography.
Secnonces are handled in a separate class which holds the libsecp secnonce object in a `secure_unique_ptr`. Since secnonces must not be used, this class has no serialization and will only live in memory. A restart of the software will require a restart of the MuSig2 signing process.
ACKs for top commit:
fjahr:
tACK ac599c4a9c
rkrux:
lgtm tACK ac599c4a9c
theStack:
Code-review ACK ac599c4a9c🗝️
Tree-SHA512: 626b9adc42ed2403e2f4405321eb9ce009a829c07d968e95ab288fe4940b195b0af35ca279a4a7fa51af76e55382bad6f63a23bca14a84140559b3c667e7041e
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. It does so by using std::accumulate instead
of manual loops.
Also update interface_ipc.py to account for the new behavior.
Co-authored-by: Raimo33 <claudio.raimondi@protonmail.com>
`EmplaceCoinInternalDANGER()` incremented `cachedCoinsUsage` even when `try_emplace` did not insert (duplicate key), inflating the counter.
This is mostly reachable in tests today since `AssumeUTXO` does not overwrite.
Increment only on successful insert, and capture `coin.DynamicMemoryUsage()` before the move so accounting uses the correct value.
Fuzz: add an `EmplaceCoinInternalDANGER` path to exercise insert-only accounting.
Unit test: emplace two different coins at the same outpoint (with different `DynamicMemoryUsage()`), verify `SelfTest()` passes and `AccessCoin(outpoint)` returns the first coin.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: w0xlt <woltx@protonmail.com>
Move the `cachedCoinsUsage` subtract in `AddCoin()` to after the `possible_overwrite` check.
Previously a throw before assignment decremented the counter without changing the entry, which corrupted accounting and later underflowed.
In `Flush()`, reset `cachedCoinsUsage` to `0` only when `BatchWrite()` succeeds and `cacheCoins` is actually cleared. In production `BatchWrite()` returns `true`, so this mostly affects tests. On failure, leave the counter unchanged to keep it in sync with the cache.
The existing `Flush()` workaround in fuzzing was also removed now that the source of the problem was fixed, so the fuzzer no longer needs `coins_view_cache.Flush()` to realign `cachedCoinsUsage` after an exception.
Replace the prior `expected_code_path` tracking with direct assertions. The role of the variable was to verify that code execution follows only expected paths, either successful addition, or if it's an exception, the message is verified and checked that overwrite was disallowed.
With these changes the counter stays consistent across success and exception paths, so we can finally remove the `UBSan` suppressions for `CCoinsViewCache` that were masking the issue.
Included a unit test as well, attempting to add a different coin to the same outpoint without allowing overwrites and make sure it throws.
We use `SelfTest()` to validates accounting, and check that the cache remains usable.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: w0xlt <woltx@protonmail.com>
When a coin is spent via `SpendCoin()`, `cachedCoinsUsage` is already decremented and the coin's `scriptPubKey` is cleared, so `DynamicMemoryUsage()` is `0`.
`CoinsViewCacheCursor::NextAndMaybeErase()` was subtracting usage again when erasing spent entries.
Replace it with an assert that documents spent coins have zero dynamic memory usage by the time the cursor encounters them.
Remove the now-unnecessary `usage` reference from the cursor's constructor and member variables.
652424ad16 test: additional test coverage for script_verify_flags (Anthony Towns)
417437eb01 script/verify_flags: extend script_verify_flags to 64 bits (Anthony Towns)
3cbbcb66ef script/interpreter: make script_verify_flag_name an ordinary enum (Anthony Towns)
bddcadee82 script/verify_flags: make script_verify_flags type safe (Anthony Towns)
a5ead122fe script/interpreter: introduce script_verify_flags typename (Anthony Towns)
4577fb2b1e rpc: have getdeploymentinfo report script verify flags (Anthony Towns)
a3986935f0 validation: export GetBlockScriptFlags() (Anthony Towns)
5db8cd2d37 Move mapFlagNames and FormatScriptFlags logic to script/interpreter.h (Anthony Towns)
Pull request description:
We currently use 21 of 32 possible bits for `SCRIPT_VERIFY_*` flags, with open PRs that may use 8 more (#29247, #31989, #32247, #32453). The mutinynet fork that has included many experimental soft fork features is [already reusing bits here](d4a86277ed/src/script/interpreter.h (L175-L195)). Therefore, bump this to 64 bits.
In order to make it easier to update this logic in future, this PR also introduces a dedicated type for the script flags, and disables implicit conversion between that type and the underlying integer type. To make verifying that this change doesn't cause flags to disappear, this PR also resurrects the changes from #28806 so that the script flags that are consensus enforced on each block can be queried via getdeploymentinfo.
ACKs for top commit:
instagibbs:
reACK 652424ad16
achow101:
ACK 652424ad16
darosior:
ACK 652424ad16
theStack:
Code-review ACK 652424ad16🎏
Tree-SHA512: 7b30152196cdfdef8b9700b571b7d7d4e94d28fbc5c26ea7532788037efc02e4b1d8de392b0b20507badfdc26f5c125f8356a479604a9149b8aae23a7cf5549f
3635d62f5a chain: make use of pskip in LastCommonAncestor (optimization) (Pieter Wuille)
2e09d66fbb tests: add unit tests for CBlockIndex::GetAncestor and LastCommonAncestor (Pieter Wuille)
Pull request description:
In theory, the `LastCommonAncestor` function in chain.cpp can take $\mathcal{O}(n)$ time, walking over the entire chain, if the forking point is very early, which could take ~milliseconds. I expect this to be very rare in normal occurrences, but it seems nontrivial to reason about worst cases as it's accessible from several places in net_processing.
This PR modifies the algorithm to make use of the `CBlockIndex::pskip` skip pointers to find the forking point in sublinear time (a simulation shows that for heights up to $34 \cdot 4^k - 2$ and $k \geq 8$, no more than $k^2 + 10k + 13$ steps are ever needed), in a way that should be nearly free - at worst the same number of memory accesses should be made, with a tiny increase in computation.
As it appears we didn't really have tests for this function, unit tests are added for that function as well as `CBlockIndex::GetAncestor()`.
This is inspired by https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2394877881
ACKs for top commit:
optout21:
ACK 3635d62f5a
achow101:
ACK 3635d62f5a
vasild:
ACK 3635d62f5a
mzumsande:
Code Review ACK 3635d62f5a
furszy:
ACK 3635d62f5a
stratospher:
ACK 3635d62f5a.
Tree-SHA512: f9b7dea1e34c1cc1ec1da3fb9e90c4acbf4aaf0f04768844f538201efa6b11eeeefc97b720509e78c21878977192e2c4031fd8974151667e2e756247002b8164
1aaaaa078b fuzz: Drop unused workaround after Apple-Clang bump (MarcoFalke)
fadad7a494 Drop support for EOL macOS 13 (MarcoFalke)
Pull request description:
Now that macOS 13 is EOL (https://en.wikipedia.org/wiki/MacOS_Ventura), it seems odd to still support it.
(macOS Ventura 13.7.8 received its final security update on 20 Aug 2025: https://support.apple.com/en-us/100100)
This patch will only be released in version 31.x, another 6 months out from now.
So:
* Update the depends build and release note template to drop EOL macOS 13.
* As a result, update the earliest Xcode to version 16 in CI.
* Also, bump the macOS CI runner to version 15, to avoid issues when version 14 will be at its EOL in about 1 year.
This also allows to drop a small workaround in the fuzz tests and unlocks libcpp hardening (https://github.com/bitcoin/bitcoin/pull/33462)
ACKs for top commit:
stickies-v:
re-ACK 1aaaaa078b
l0rinc:
code review ACK 1aaaaa078b
hodlinator:
re-ACK 1aaaaa078b
hebasto:
ACK 1aaaaa078b.
Tree-SHA512: 6d247a8432ef8ea8c6ff2a221472b278f8344346b172980299507f9898bb9e8e16480c128b1f4ca692bcbcc393da2b2fd6895ac5f118bc09e0f30f910529d20c
c76de2eea1 net: support overriding the proxy selection in ConnectNode() (Vasil Dimov)
Pull request description:
Normally `ConnectNode()` would choose whether to use a proxy and which one. Make it possible to override this from the callers and same for `OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection succeeded or not. This can be used when the caller needs to keep track of how many (successful) connections were opened.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.
ACKs for top commit:
stratospher:
ACK c76de2e.
optout21:
ACK c76de2eea1
mzumsande:
Code Review ACK c76de2eea1
andrewtoth:
ACK c76de2eea1
Tree-SHA512: 1d266e4280cdb1d0599971fa8b5da58b1b7451635be46abb15c0b823a1e18cf6e7bcba4a365ad198e6fd1afee4097d81a54253fa680c8b386ca6b9d68d795ff0
These overloads were needed to allow passing `fs::path` objects directly to
libstdc++'s `fstream` constructors, but after the previous commit, there is no
longer any remaining code that does pass `fs::path` objects to `fstream`
constructors. Writing new code which does this is also discouraged because the
standard has been updated in https://wg21.link/lwg3430 to disallow it.
Dropping these also means its no longer possible to pass `fs::path` arguments
directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not
a big loss because it is already not possible to pass them to the constructor.
So this commit updates `fstream::open` calls.
Additionally, this change required updates to src/bitcoin.cpp since it was
relying on the overloaded filename() method.
As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545,
newer libc++ versions implementing https://wg21.link/lwg3430 will no longer
implicitly convert `fs::path` objects to `std::filesystem::path` objects when
constructing `std::ifstream` and `std::ofstream` types.
This is not a problem in Unix systems since `fs::path` objects use
`std::string` as their native string type, but it causes compile errors on
Windows which use `std::wstring` as their string type, since `fstream`s can't
be constructed from `wstring`s.
Fix the windows libc++ compile errors by adding a new `fs::path::std_path()`
method and using it construct `fstream`s more portably.
Additionally, delete `fs::path`'s implicit `native_string` conversion so these
errors will not go undetected in the future, even though there is not currently
a CI job testing Windows libc++ builds.
0f7d4ee4e8 p2p: Use different inbound inv timer per network (Martin Zumsande)
94db966a3b net: use generic network key for addrcache (Martin Zumsande)
Pull request description:
Currently, `NextInvToInbounds` schedules each round of `inv` at the same time for all inbound peers. It's being done this way because with a separate timer per peer (like it's done for outbounds), an attacker could do multiple connections to learn about the time a transaction arrived. (#13298).
However, having a single timer for inbounds of all networks is also an obvious fingerprinting vector: Connecting to a suspected pair of privacy-network and clearnet addresses and observing the `inv` pattern makes it trivial to confirm or refute that they are the same node.
This PR changes it such that a separate timer is used for each network.
It uses the existing method from `getaddr` caching and generalizes it to be saved in a new field `m_network_key` in `CNode` which will be used for both `getaddr` caching and `inv` scheduling, and can also be used for any future anti-fingerprinting measures.
ACKs for top commit:
sipa:
utACK 0f7d4ee4e8
stratospher:
reACK 0f7d4ee.
naiyoma:
Tested ACK 0f7d4ee4e8
danielabrozzoni:
reACK 0f7d4ee4e8
Tree-SHA512: e197c3005b2522051db432948874320b74c23e01e66988ee1ee11917dac0923f58c1252fa47da24e68b08d7a355d8e5e0a3ccdfa6e4324cb901f21dfa880cd9c
06df14ba75 test: add more TRUC reorg coverge (Greg Sanders)
26e71c237d Mempool: Do not enforce TRUC checks on reorg (Greg Sanders)
bbe8e9063c fuzz: don't bypass_limits for most mempool harnesses (Greg Sanders)
Pull request description:
This was the intended behavior but our tests didn't cover the scenario where in-block transactions themselves violate TRUC topological constraints.
The behavior in master will potentially lead to many erroneous evictions during a reorg, where evicted TRUC packages may be very high feerate and make sense to mine all together in the next block and are well within the normal anti-DoS chain limits.
This issue exists since the merge of https://github.com/bitcoin/bitcoin/pull/28948/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R956
ACKs for top commit:
sdaftuar:
ACK 06df14ba75
glozow:
ACK 06df14ba75
ismaelsadeeq:
Code review ACK 06df14ba75
Tree-SHA512: bdb6e4dd622ed8b0b11866263fff559fcca6e0ca1c56a884cca9ac4572f0026528a63a9f4c8a0660df2f5efe0766310a30e5df1d6c560f31e4324ea5d4b3c1a8
Modernizes interface by not forcing users to deal with raw pointers,
without adding copying overhead. Generalizes the logic of whether
we return by value or by optional/pointer.
In cases where functions take a `const std::string&` and it would
be too much work to update them, a string copy is made (which was
already happening anyway).
Normally `ConnectNode()` would choose whether to use a proxy and which
one. Make it possible to override this from the callers and same for
`OpenNetworkConnection()` - pass down the proxy to `ConnectNode()`.
Document both functions.
This is useful if we want to open connections to IPv4 or IPv6 peers
through the Tor SOCKS5 proxy.
Also have `OpenNetworkConnection()` return whether the connection
succeeded or not. This can be used when the caller needs to keep track
of how many (successful) connections were opened.
87e7f37918 doc: clarify peer address in getpeerinfo and addnode RPC help (Vasil Dimov)
2a4450ccbb net: change FindNode() to not return a node and rename it (Vasil Dimov)
4268abae1a net: avoid recursive m_nodes_mutex lock in DisconnectNode() (Vasil Dimov)
3a4d1a25cf net: merge AlreadyConnectedToAddress() and FindNode(CNetAddr) (Vasil Dimov)
Pull request description:
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the return value.
Remove a recursive lock on `m_nodes_mutex`.
Rename `FindNode()` to better describe what it does.
ACKs for top commit:
achow101:
ACK 87e7f37918
furszy:
Code review ACK 87e7f37918
hodlinator:
re-ACK 87e7f37918
Tree-SHA512: 44fb64cd1226eca124ed1f447b4a1ebc42cc5c9e8561fc91949bbeaeaa7fa16fcfd664e85ce142e5abe62cb64197c178ca4ca93b3b3217b913e3c498d0b7d1c9
0802398e74 fuzz: make it possible to mock (fuzz) CThreadInterrupt (Vasil Dimov)
6d9e5d130d fuzz: add CConnman::SocketHandler() to the tests (Vasil Dimov)
3265df63a4 fuzz: add CConnman::InitBinds() to the tests (Vasil Dimov)
91cbf4dbd8 fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests (Vasil Dimov)
50da7432ec fuzz: add CConnman::OpenNetworkConnection() to the tests (Vasil Dimov)
e6a917c8f8 fuzz: add Fuzzed NetEventsInterface and use it in connman tests (Vasil Dimov)
e883b37768 fuzz: set the output argument of FuzzedSock::Accept() (Vasil Dimov)
Pull request description:
Extend `CConnman` fuzz tests to also exercise the methods `OpenNetworkConnection()`, `CreateNodeFromAcceptedSocket()`, `InitBinds()` and `SocketHandler()`.
Previously fuzzing those methods would have resulted in real socket functions being called in the operating system which is undesirable during fuzzing. Now that https://github.com/bitcoin/bitcoin/pull/21878 is complete all those are mocked to a fuzzed socket and a fuzzed DNS resolver (see how `CreateSock` and `g_dns_lookup` are replaced in the first commit).
ACKs for top commit:
achow101:
ACK 0802398e74
jonatack:
Review re-ACK 0802398e74
dergoegge:
Code review ACK 0802398e74
Tree-SHA512: a717d4e79f42bacf2b029c821fdc265e10e4e5c41af77cd4cb452cc5720ec83c62789d5b3dfafd39a22cc8c0500b18169aa7864d497dded729a32ab863dd6c4d
Using bypass_limits=true is essentially fuzzing part of a
reorg only, and results in TRUC invariants unable to be
checked. Remove most instances of bypassing limits, leaving
one harness able to do so.
`CConnman::AlreadyConnectedToAddress()` is the only caller of
`CConnman::FindNode(CNetAddr)`, so merge the two in one function.
The unit test that checked whether `AlreadyConnectedToAddress()` ignores
the port is now unnecessary because now the function takes a `CNetAddr`
argument. It has no access to the port.
