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 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).
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
bc706955d7 ci: expose all ACTIONS_* vars (willcl-ark)
Pull request description:
When using `docker buildx build` in conjunction with the `gha` backend cache type (as we do in our CI) it's important to specify the URL and TOKEN needed to authenticate.
On Cirrus runners this is working with only `ACTIONS_CACHE_URL` and `ACTIONS_RUNTIME_TOKEN`, but this is not enough for the GitHub backend.
Fix this by exporting all `ACTIONS_*` variables.
This fixes docker build layer cache restore/save on forks or where GH-hosted runners are being used, and addresses https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3324707093
ACKs for top commit:
m3dwards:
ACK bc706955d7
maflcko:
lgtm ACK bc706955d7
Tree-SHA512: 13e973bb1c1ca5448dd6c3c176fb5ce39c725886ba2012d3253158205309a7038a1430135b37400e1f2f69408a9d0f4e2b3c5f0515154a593ec382ab7db10266
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>
fa6fd16f36 ci: Use native platform for win-cross task (MarcoFalke)
Pull request description:
Forcing the architecture to amd64 is no longer required. Dropping it should have some benefits:
* Faster CI speed on other arches (riscv64, arm, ...)
* Unlock the CI task to run on riscv64 at all
ACKs for top commit:
hebasto:
ACK fa6fd16f36, tested on Ubuntu 24.04, RISC-V.
Tree-SHA512: 68a3fc90cc22ab085d6946deb106e50b22e06eebc61523a9dcb53b38a50021a19da26cc29e2cd20f4673ffc5cc10f441dacca7cc799782258351609d9fa04969
671b774d1b depends: Use $(package)_file_name when downloading from the fallback (Ava Chow)
Pull request description:
The server hosting the fallbacks uses `make download` so the files are only available with their overridden names rather than the original name on the upstream source. We should therefore also use the overridden name when downloading from the fallback.
Fixes https://github.com/bitcoin-core/bitcoincore.org/issues/1168
ACKs for top commit:
theuni:
utACK 671b774d1b. I was going to PR the same change.
janb84:
ut ACK 671b774d1b
hebasto:
ACK 671b774d1b, tested with the following patch:
Tree-SHA512: ba010adb64900d8d748487cc1a658e2b163872354f4e7b38c4dfc37a14fcb22fec4379a635d2c6788c64dd46bef0d94aa3eb6f522ec700680e886d5468678031
`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.
During `BatchWrite`, the parent entry is created under a guard that guarantees insertion, so the new `Coin` is default-constructed and empty.
Assert this invariant to document why there is no `cachedCoinsUsage` decrement before the assignment at this site.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
This adds a specialized Cluster implementation for singleton clusters, saving
a significant amount of memory by avoiding the need for m_depgraph, m_mapping,
and m_linearization, and their overheads.
This adds 4 functions to Cluster to help implement Merge() and Split() without
needing access to the internals of the other Cluster. This is a preparation for
a follow-up that will make Clusters a virtual class whose internals are abstracted
away.
This reduces per-Cluster memory usage by making Clusters not aware of their
own level. Instead, track it either in calling code, or infer it based on
the transactions in them.
Without this change, logging (even if unused) may account for a
substantial portion of bitcoin-node's and/or client's runtime cpu usage, due
to libmultiprocess's expensive message serialization.
This (along with some recent upstream changes) avoids the overhead by opting
out of log handling for messages that we're not interested in.
Info, Warning, and Error are logged unconditionally to match our behavior
elsewhere. See BCLog::Logger::GetCategoryLogLevel .
f6567527d8 doc: bump the template macOS version (kevkevinpal)
Pull request description:
Motivated by https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3361601497
The minimum version of MacOS for this repo is now 14 and above so it makes sense to update the issue template to reflect that.
We are now using a higher version but since it is just a bug template, there is no need to put the lowest version we support.
ACKs for top commit:
maflcko:
lgtm ACK f6567527d8
l0rinc:
ACK f6567527d8
janb84:
ACK f6567527d8
Tree-SHA512: 701b161bda25245996c94b6d2119b5cc85a34917551dcf8c92ffacf3aa80fa7fe84bb3497edc7e600c5b2443de13a6f6107fc7289721e585b16c4972d07a796c
e4335a3192 Revert "depends: Update URL for `qrencode` package source tarball" (Ava Chow)
a89a822e6e Revert "depends: Use hash instead of file name for package download stamp" (Ava Chow)
Pull request description:
The new URL breaks CI on the current release branches, see https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380802351.
The old URL also no longer exists so the tarball is fetched from the depends sources cache that we host, but the original tarball has already been overwritten on there. We will need to manually reinstate the original tarball.
ACKs for top commit:
m3dwards:
utACK e4335a3192
maflcko:
review ACK e4335a3192💤
glozow:
ACK e4335a3192
Tree-SHA512: a5028342d77b4768daaec8688acd364795d683aed2bea0407c7827d44f814a97d50cc3b30c2de2a8296a2b212115fe1e76c57685a74e93387fc57afdabb93bd2
a1226bc760 doc: how to update a subtree (Sjors Provoost)
Pull request description:
We have instructions on how to verify a subtree update, but not on how to perform one.
ACKs for top commit:
yuvicc:
ACK a1226bc760
achow101:
ACK a1226bc760
janb84:
ACK a1226bc760
furszy:
ACK a1226bc760
Tree-SHA512: ba3ccc56a9f1c7f461e0db9699612e1fd64b7c72bfd1dae63d4cb830db416871a493820d3a7924c19b6ce353fc20c5fe07578b053dec6ea68273a007cbebc512
ceeb53adcd ci: Properly include $FILE_ENV in DEPENDS_HASH (Ava Chow)
Pull request description:
$FILE_ENV has a full relative path already, prepending with ci/test/ results in a non-existent path which means that DEPENDS_HASH was not actually committing to the test's environment file.
ACKs for top commit:
maflcko:
lgtm ACK ceeb53adcd
Tree-SHA512: 80a7a23676ff8bf2f48a7d3c5897217f11d7d4d4f8a54897d2b7c42689585d2d63e45fad2b8f4c442111f128a87eeb6edeac2b25c79862e6bc035eeb1ebc7f4e
b35341b9ba Update ci.yml (Coder)
Pull request description:
Release notes:https://github.com/actions/download-artifact/releases/tag/v5.0.0
Change:
uses: actions/download-artifact@v4 -> uses: actions/download-artifact@v5
ACKs for top commit:
maflcko:
lgtm ACK b35341b9ba
willcl-ark:
ACK b35341b9ba
hebasto:
ACK b35341b9ba, I have reviewed the code and it looks OK.
Tree-SHA512: f82dd0fe3ca8d431b9ff6ef9f23a4f2e92a1463c6f55fbe9b46b9e13750d311bd2aa915a8570f76600363b3a1ccbf394c95216cfac0f6db30846d9be7ec7c4cf
The `createwallet` RPC doesn't return the empty passphrase
warning anymore if no passphrase was passed explicitly.
The `noshutdown` parameter key was removed in commit
fa0dc09b90, so remove it from
the table.
$FILE_ENV has a full relative path already, prepending with ci/test/
results in a non-existent path which means that DEPENDS_HASH was not
actually committing to the test's environment file.
