💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823243237)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)

Not important, but usually test code and output is more readable with `CHECK(a); CHECK(b);` instead of `CHECK(a && b)`

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823185560)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)

Maybe wait for other opinions to see if this suggestion is worthwhile, but IMO this commit provides a good opportunity to remove the `Assume(&self.second == this);` footgun and make calling these functions more straightforward:

<details><summary>diff</summary>
<p>

```diff
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -49,7 +49,7 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(cons
...

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823264089)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)

It seems like these methods are not called in this commit, but they could be called in `InsertCoinsMapEntry` below. Maybe call them there or drop them here?

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823433791)
In commit "test: Group values and states in tests into CoinEntry wrappers" (553db5dec1334ed5e8e07e34306f16de9afc22bb)

Review note: It's not really easy to verify by looking at code that new and old test cases are equivalent, but an approach that worked for me was to add extra prints showing the test cases, and then diff the output from `test_bitcoin -t coins_tests` before and after this commit, and confirm there are were no changes. My change used to test this was:

<details><summary>diff</
...

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823241356)
In commit "coins: Remove direct GetFlags access" (8df16f23a22b2e0b72dd6338ed37c09485f5ede3)

May should check `!IsFresh()` here so new check is equivalent to previous check. Alternately, could just mention in commit message that this commit reduces test coverage a little bit by not checking some flags that are not relevant to the tests.

Same comment applies to other tests below like `linked_list_random_deletion`

💬 m3dwards commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448635473)
The slowdown appears to be introduced with https://github.com/bitcoin/bitcoin/commit/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001

Not sure why yet.

💬 davidgumberg commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2448647985)
> I've built this branch on Fedora 41, linking against UCRT (see [this](https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2446405138) workflow), but the issue persists.

+1: built this branch on Fedora 40 doing a similar workflow and the same crash occurred:

```bash
ucrt64-make -C depends/ HOST=x86_64-w64-mingw32 -j $(nproc) # rpm macro: /usr/lib/rpm/macros.d/macros.ucrt64
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
cmake --build build -j $(nproc)
``
...

💬 davidgumberg commented on pull request "[POC] ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2448653950)
Concept ACK

#31172 @ https://github.com/bitcoin/bitcoin/pull/31172/commits/7dd0ee89a092c6ec4e305fbdb0cf3afa9e41cab6 passes CI but trivially fails to start on any Windows system because Wine lacks some undocumented Windows behavior.

🤔 hodlinator reviewed a pull request: "msvc: Update vcpkg manifest"
(https://github.com/bitcoin/bitcoin/pull/31186#pullrequestreview-2406377117)
Tested on Windows 11 Home:
```
> rmdir /S build
> cmake -B build --preset vs2022-static
> cmake --build build --config Release -j<X>
> build\src\qt\Release\bitcoin-qt.exe (started successfully)
```

💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1823427758)
<details>
<summary>

`cat vcpkg.json | jq -S` results in a different order, seems more lexicographical to me?

</summary>

```json
{
"$schema": "https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json",
"builtin-baseline": "9edb1b8e590cc086563301d735cae4b6e732d2d2",
"default-features": [
"wallet",
"zeromq",
"tests",
"qt5"
],
"dependencies": [
"boost-date-time",
"boost-multi-index",
"boost-signals2",
"libeve
...

💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/31186#discussion_r1823436767)
`$comment` should appear ordered before `$schema`, unless the latter *must* appear first?

🤔 tdb3 reviewed a pull request: "Policy: Report reason inputs are non standard"
(https://github.com/bitcoin/bitcoin/pull/29060#pullrequestreview-2406688998)
I'm a fan of the more descriptive errors.

However, would some of these more descriptive errors flow down to RPC responses? (e.g. `testmempoolaccept`).

https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2347371722
> Does bitcoind consider this to be a breaking change? The new error message may be "more accurate", but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.

💬 tdb3 commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1823630805)
Could also clarify that invalid TxValidationState returned describes the first nonstandard input encountered.

For example:
```diff
- * invalid TxValidationState which states why an input is not standard
+ * invalid TxValidationState which states why the first invalid input is not standard
```

💬 tdb3 commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1823661556)
Instantiating the CScript here could allow re-use later (in `txToNonStd4.vin[0].scriptSig = CScript(op_return, op_return + sizeof(op_return));`)

💬 sipa commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2448864279)
This means that no `Assume` can be compiled out in production builds anymore, as they all involve a `g_fuzzing` check?

💬 sipa commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448867990)
@m3dwards Thanks for digging! I believe that explains it: since #31093, `Assume()` calls are no longer optimized out in production builds.

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2448891405)
I'm still confused why Ninja is needed and why nothing complains when it's missing.

> Ninja is required to build the qt package in depends. It is mentioned in depends/README.md.

It only mentions it in the context of an Ubuntu & Debian install.

I did a depends build for af05dd9a12b89224dc7ad229698eeceb3e560ed4 again on Intel macOS 15.0.1 with Xcode 16 and no Ninja. It seems to work fine, at first glance.

I didn't try without Xcode since my laptop seems to be in a weird state: https:
...

💬 davidgumberg commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448905871)
> @m3dwards Thanks for digging! I believe that explains it: since #31093, `Assume()` calls are no longer optimized out in production builds.

I've reproduced this performance issue locally, and when you remove `g_fuzzing` from `inline_assertion_check` the regression goes away:

`./build/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11 -min-time=30000`

|branch | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op
...

💬 Khanwafa232426 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/6e21dedbf2b3029c729108f225469b321a1b3d39#r148579495)
https://m.facebook.com/help/396528481579093/?ref=sharebnb165q9dz39mqh789zuuuqwkv22plut6f4nzy9jc9https://www.binance.com/en/fan-token/BWS/FCShakhtarFanverse/vote/c021757d3a3b47f2b6beaffff9c745e1?utm_campaign=ft-sd-fanverse&utm_medium=ft-faq-body&utm_source=fan-tokenhttps://www.binance.com/fan-token/BWS/FCShakhtarFanverse?ref=CG5V70EE&registerChannel=fcshakhtar&utm_source=fan-token&utm_campaign=fcshakhtar-fanverse-invitehttps://images.app.goo.gl/zwU8BxrdydT9nmsv8

💬 Khanwafa232426 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/6e21dedbf2b3029c729108f225469b321a1b3d39#r148579517)
https://m.facebook.com/help/396528481579093/?ref=sharehttps://www.binance.com/en/fan-token/BWS/FCShakhtarFanverse/vote/c021757d3a3b47f2b6beaffff9c745e1?utm_campaign=ft-sd-fanverse&utm_medium=ft-faq-body&utm_source=fan-tokenbnb165q9dz39mqh789zuuuqwkv22plut6f4nzy9jc9https://www.binance.com/fan-token/BWS/FCShakhtarFanverse?ref=CG5V70EE&registerChannel=fcshakhtar&utm_source=fan-token&utm_campaign=fcshakhtar-fanverse-invitehttps://images.app.goo.gl/zwU8BxrdydT9nmsv8go.opencensus.iohttps://safu.im/Ddn
...