💬 pablomartin4btc commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987567116)
Shouldn't return null as other validations like the previous one (`IsWalletFlagSet`)?
```suggestion
return nullptr;
```
Moreover, `DescriptorScriptPubKeyMan::UpdateWalletDescriptor` also calls `DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor`... could this call be removed?

💬 dergoegge commented on pull request "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`":
(https://github.com/bitcoin/bitcoin/pull/32025#issuecomment-2711052308)
Concept ACK

Awesome to see the txid types paying off!

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1987579951)
https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1986316731

> I've always been a bit puzzled of this check. Is it really required? If so, why don't we need to check it on every iteration of the do loop further down? What if it is actually reached, will validation on that chain just always be stuck in an unrecoverable state? Wouldn't a fatal error be more appropriate then?

This is labeled as "Belt-and-suspenders check" so it should be safe to assume that it is not required. Sometim
...

💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711068992)
> Oh this code path isn't used at all when building natively?

Correct.

💬 dergoegge commented on pull request "test: get rid of redundant TODO tag in fuzz tests":
(https://github.com/bitcoin/bitcoin/pull/32024#issuecomment-2711071334)
Thanks for your contribution but `FuzzedDataProvider.h` is not maintained by us. This should be opened against llvm: https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h.

💬 laanwj commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2711075412)
Tested windows path without NSIS
```
$ ninja -j4
[224/224] Linking CXX executable src/test/test_bitcoin.exe (successful build)
$ ninja deploy
[1/1] cd /home/user/src/bitcoin-windows/build && /usr/bin/cmake -E echo && /usr/bin/cm... -E echo "Then re-run cmake to regenerate the build system." && /usr/bin/cmake -E echo

Error: NSIS not found.
Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
for example, by setting the MAKENSIS_EXECUTABLE va
...

✅ hebasto closed a pull request: "test: get rid of redundant TODO tag in fuzz tests"
(https://github.com/bitcoin/bitcoin/pull/32024)

📝 hebasto locked a pull request: "test: get rid of redundant TODO tag in fuzz tests"
(https://github.com/bitcoin/bitcoin/pull/32024)
`list.size()` is determined at runtime, so using `static_assert` on it as suggested by the TODO comment is not feasible and produces the following error when done:

`error: static assertion expression is not an integral constant expression`

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1987604281)
Indeed, done, thanks!

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711102281)
`36d932cebb...af622d00ba`: address https://github.com/bitcoin/bitcoin/pull/32015#discussion_r1985793960

💬 fjahr commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2711162276)
re-ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b

Only change was to make the helper internals a bit nicer.

📝 hebasto opened a pull request: "cmake: Add `NO_CACHE_IF_FAILED` option for checking linker flags"
(https://github.com/bitcoin/bitcoin/pull/32027)
Use it for checking `-fsanitize`.

This change improves the user experience when the configuration step fails due to a missing library. Now, there is no need to manually clean the CMake cache after installing the required library.

Addresses [this](https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2703801270) comment from https://github.com/bitcoin/bitcoin/issues/31942.

💬 hebasto commented on issue "[rfc] build: Reject unclean configure?":
(https://github.com/bitcoin/bitcoin/issues/31942#issuecomment-2711249969)
> > reliable steps to reproduce those issues.
>
> For example, configuring with GCC + thread sanitizer (where libtsan is missing). After installing the missing dependency, and re-running CMake, even though the check is being re-run, CMake doesn't "fix" itself, and requires removing the build directory entirely.

Fixed in https://github.com/bitcoin/bitcoin/pull/32027.

📝 hebasto opened a pull request: "Update `secp256k1` subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/32028)
This PR updates the `secp256k1` subtree to https://github.com/bitcoin-core/secp256k1/commit/4ba1ba2af953b7d124db9b80b34568e5c4a2d48a, which includes the following changes:
- https://github.com/bitcoin-core/secp256k1/pull/1633
- https://github.com/bitcoin-core/secp256k1/pull/1634
- https://github.com/bitcoin-core/secp256k1/pull/1641
- https://github.com/bitcoin-core/secp256k1/pull/1650
- https://github.com/bitcoin-core/secp256k1/pull/1646
- https://github.com/bitcoin-core/secp256k1/pull/165
...

💬 hebasto commented on pull request "[POC] build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#issuecomment-2711376386)
Rebased on https://github.com/bitcoin/bitcoin/pull/32028.

🤔 mzumsande reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2671833519)
Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b

💬 purpleKarrot commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711463453)
Instead of passing around shared pointers to mutable data (aka global variables in disguise), would it be possible to increase the level of abstraction and pass around values with regular semantics? Every C++ programmer should rewatch https://youtu.be/W2tWOdzgXHA from time to time.

💬 yancyribbens commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1987820088)
Is this documented anywhere? This is a good question and I would imagine others will have the same in the future.

💬 yancyribbens commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r1987822504)
This method name looks funky. SingleTxTXO is short for single transaction transaction output?

💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1987847360)
nit - this should be indented. Right now it's mis-aligned.