💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830996993)
I think "other topology" is a little under-specified here, could be helpful to name what tests you think could be added

🤔 hodlinator reviewed a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2418274192)
Post-merge re-ACK 9e5089dbb02ef8a32f484aab682ad06748e1a532

Only resolving against formatting changes made to *vcpkg.json* in #31186 since my previous ACK.

💬 maflcko commented on pull request "ci: `add second_deadlock_stack=1` to TSAN options":
(https://github.com/bitcoin/bitcoin/pull/31232#issuecomment-2459790214)
lgtm ACK 5161c2618cd36cd2d64c64d42d6f6d3b1929cb28

📝 hebasto opened a pull request: "cmake: Improve robustness and usability"
(https://github.com/bitcoin/bitcoin/pull/31233)
This PR:

1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.

2. Disables tests instead of ignoring them.

For example:
- building without Python available:
```
$ cmake -B build
$ cmake --build build -j 16
$ # ctest --test-dir build -j 16 -R "^util_"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 113: util_tests
Start 114: util_threadnames_tests

...

👍 vasild approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2418332973)
ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1

👋 dergoegge's pull request is ready for review: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)

💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic":
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2459823076)
Thanks, tested that https://github.com/bitcoin/bitcoin/pull/31222 (74ff8467d107b609d699729399b83a1219add78a) or #30125 (d45eb3964f693eddcf96f1e4083cf19d327be989) fix the issue.

I reproduced in a fresh install of Ubuntu 24.10 VM with zfs (experimental) selected, then ran the test in podman.

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2459823341)
Now it works, ready for review!

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831070572)
maybe not the most important check, just makes sure that being an ancestor in mempool alone somehow isn't enough to get checked for dust

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831073747)
I think it's correct, spending dust from first dusty tx but not second

💬 hebasto commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2459852408)
My Guix build:
```
aarch64
c448a9c8d6d436350846ccb998e6a5e2bb66ac519e3539188c4792c6a88f4a1c guix-build-a6a3cab235cf/output/aarch64-linux-gnu/SHA256SUMS.part
613ef63f92fdf0422179fdedad230a7b8c5eb1530c892745bd2d5e76e8fb580c guix-build-a6a3cab235cf/output/aarch64-linux-gnu/bitcoin-a6a3cab235cf-aarch64-linux-gnu-debug.tar.gz
2d77932ce9f8808f9673420320faaf9b20d910595f3d3d65687b5cbc55cb84dd guix-build-a6a3cab235cf/output/aarch64-linux-gnu/bitcoin-a6a3cab235cf-aarch64-linux-gnu.tar.gz
73019d4f
...

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1831084185)
In fact, I tried to track watchonly and spendable scripts, but I don't think it's simple since we load keys, hd chain, diverse scripts, etc. I ended up writing similar or copying a lot of things from `wallet/scriptpubkey.cpp` which I don't think would be a valuable approach here.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831084282)
could be worth a comment

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2459863507)
Rebased due to the conflicts with the merged bitcoin/bitcoin#30634 and bitcoin/bitcoin#31173.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831091020)
alternatively could just check `mempool.m_opts.require_standard` as well?

💬 hebasto commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1831098179)
You might want to use `${{ matrix.conf.job_name }}`.

Same for the Windows job.

💬 maflcko commented on pull request "test: cover edge case for lockunspent rpc":
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2459879971)
> Removing the check for negative would technically change RPC behavior for users ([#30212 (comment)](https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2347371722)), but do we think anyone's downstream use case is actually relying on this odd edge case (negative `vout` arg)?

Matching errors based on the error string is fragile and considered bad practise, because error strings are considered less stable than error codes or error types. Also, error strings may be verbose and (if trunc
...

💬 hebasto commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1831107121)
Instead of saving another cache, we can simply skip the cache saving step for the "fuzz" job type.

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831108819)
leaving as-is

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831114106)
From an implementation perspective it was cleaner to enforce it in `IsStandardTx`, and I'm not sure I see the value in doubling up enforcement of it vs a single location.

If we already had fee information by the time we would call `IsStandardTx` it might be even cleaner, but I expect it would be a very difficult to evaluate diff.