💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2457548171)
I could remove another `sleep` import line after https://github.com/bitcoin/bitcoin/pull/30942 was merged which was what the linter complained about. Should be trivial to re-ACK: https://github.com/bitcoin/bitcoin/compare/352b8209aa5327f7d369e2acc4d87f9767389a6b..f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96

💬 fanquake commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#issuecomment-2457600736)
Guix build:
```bash
67b7e95e0f1d45debad8a8cac4c2d349e3e01efcf62e0b109345896f296758d9 guix-build-788c1324f3d8/output/aarch64-linux-gnu/SHA256SUMS.part
cd3c67eaa5f6fadf0a73cdb1aa03563d720fc7fdae1b59aa495a0eda9d2695af guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu-debug.tar.gz
5c1eb4ff827336a9d113497459981e213844b87e232c33fc00317ce20302f761 guix-build-788c1324f3d8/output/aarch64-linux-gnu/bitcoin-788c1324f3d8-aarch64-linux-gnu.tar.gz
dd78923f8fb7410a
...

👍 fanquake approved a pull request: "depends, doc: List packages required to build `qt` package separately"
(https://github.com/bitcoin/bitcoin/pull/31192#pullrequestreview-2416094490)
ACK 4747f030956d6876ec7cef4e2500a8579bc82146

🚀 fanquake merged a pull request: "depends, doc: List packages required to build `qt` package separately"
(https://github.com/bitcoin/bitcoin/pull/31192)

💬 fanquake commented on pull request "doc: Fix word order in developer-notes.md":
(https://github.com/bitcoin/bitcoin/pull/31220#issuecomment-2457604363)
ACK 44939e5de1b37ccd85ef31268219c5866bd3ee3f

🚀 fanquake merged a pull request: "doc: Fix word order in developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/31220)

🚀 fanquake merged a pull request: "doc: Use relative hyperlinks in release-process.md"
(https://github.com/bitcoin/bitcoin/pull/31206)

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1829641670)
Sure, I will do it.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2457621199)
Rebased due to the conflict with the merged bitcoin/bitcoin#31192.

💬 Abdulkbk commented on pull request "test: cover edge case for lockunspent rpc":
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2457625789)
> Not sure about this. This seems to be adding coverage for code that shouldn't exist in the first place.
>
> The integer is passed to `COutPoint`, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.

I did a quick test by commenting the check and you're right an error was thrown. However, the error is not as descriptive as the current implementation. The error without the check: "Invalid parame
...

👍 fanquake approved a pull request: "msvc: Update vcpkg manifest"
(https://github.com/bitcoin/bitcoin/pull/31186#pullrequestreview-2416130023)
ACK f6577b717416191e65ba8221be57f1a48d74e5d7

🚀 fanquake merged a pull request: "msvc: Update vcpkg manifest"
(https://github.com/bitcoin/bitcoin/pull/31186)

💬 fanquake commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#discussion_r1829664613)
> Overall, I think this is a reasonable approach because it pretty much just works with no fiddling. The main drawback is that over time the headers list will grow stale, so it'll require some attention now and then.

Not sure if here, but it'd seem useful to document some (minified) version of the PR description in regards to maintaining this going forward. If anything to prevent if from being too arbtrary.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2457652154)
Rebased due to the conflict with the merged bitcoin/bitcoin#31186.

💬 instagibbs commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2457652583)
is this ready for undraft?

👍 hodlinator approved a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2416152348)
re-ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440

(Hoping Drahtbot will react properly to this comment.. already ACKed the same PR version above).

💬 fanquake commented on pull request "depends: Specify CMake generator explicitly":
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2457662041)
> in environments where the CMAKE_GENERATOR variable is set.

Which environment/distro did you see this in?

💬 hebasto commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2457665391)
Rebased due to the conflict with the merged bitcoin/bitcoin#31186.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1829677516)
Good catch. I'll make that modification if I need to retouch otherwise, but I think it'd be fine as a followup.

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1829677704)
I think so, it just needs to check if the script was built with a loaded key.