💬 furszy commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987318572)
No need to log the error message. It will be retrieved to the user via the RPC response.

💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#discussion_r1987418788)
Looked around for `wait_for_`-functions, but a bunch of those are used to check for `last_message` in `P2PInterface`. Staring a bit at `sync_txindex` I realized the wait condition could be simplified, decided to do that to pre-empt other reviewers. Excuse me for invalidating your ACK.

📝 Chand-ra opened 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`

💬 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-2710893296)
Addressed the feedback from @hodlinator. Thank you!

👍 laanwj approved a pull request: "wallet: Replace "non-0" with "non-zero" in translatable error message"
(https://github.com/bitcoin/bitcoin/pull/31987#pullrequestreview-2671328596)
Code review ACK 18e83534ace7aa2d26bc7dfa521b1d591b66edfa
Aside from the issue raised, this makes the message more readable.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987507317)
Removed on Transifex.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987507697)
Fixed on Transifex.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987508337)
Removed on Transifex.

💬 Sjors 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-2710949958)
Tested on macOS 15.3.1 that `cmake --build build --target deploy` still works. It's impossible (?) to test the missing condition, because /usr/bin/zip comes with the OS.

📝 marcofleon opened a pull request: "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`"
(https://github.com/bitcoin/bitcoin/pull/32025)
This PR addresses a small bug in [`AcceptMultipleTransactions`](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/validation.cpp#L1598) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.

The second commit in this PR is a refactor of the `PackageMempoolAcc
...

💬 yancyribbens commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2710957626)
Is there any reason the `TODO` was not removed on L159 with this commit?

💬 marcofleon commented on pull request "validation, fix: Use wtxid instead of txid in `CheckEphemeralSpends`":
(https://github.com/bitcoin/bitcoin/pull/32025#issuecomment-2710962639)
note: I found this while working on the uint256 to txid/wtxid full refactor. I figured I would include a tiny part of that in this PR because it relates to the bug. If it's preferred to only have the fix be merged for branch off, then I can remove the second commit and include it as part of the txid type safety project later on.

💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2710976385)
I've started reviewing this PR and will leave a review soon in the coming days.

💬 Sjors 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-2710977444)
Oh this code path isn't used at all when building natively? E.g. on macOS I can do `ZIP_EXECUTABLE=$HOME/temp/zip2 cmake --build build --target deploy` which doesn't fail.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#discussion_r1987530875)
No, it is not expected. Removed on Transifex.

💬 Sjors commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2711000319)
I'm not too worried about the size increase. A typical guix build eats 30GB on my Ubuntu VM (3 GB for the outputs).

> it makes it possible to check if mistakes have been made in the process at earlier steps.

Agreed.

It seems simple enough to backport, especially because we're not bumping the version.

I'll see if I can reproduce the new hashes.

💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2711014150)
Feedback from @achow101 has been addressed.

💬 Sjors commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2711029947)
One slight problem is that Apple no longer makes XCode 15.0 available for download. They do have Xcode 15.0.1: https://download.developer.apple.com/Developer_Tools/Xcode_15.0.1/Xcode_15.0.1.xip

I still have the original xip on one of my machines, I'll upload it somewhere for testing. But we should probably bump it.

⚠️ janb84 opened an issue: "29.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/32026)
This issue is to discuss the [29.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.

Note: This is for feedback on the document, not on Bitcoin Core or on the 29.0 changes.

Thank you for taking a look at the guide and leaving your feedback.

💬 pablomartin4btc commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1987563061)
But `CWallet::AddWalletDescriptor` could be called from other places (at the moment only tests and bench)? Doesn't matter?