📝 hebasto opened a pull request: "build, msvc: Drop duplicated `common\url.cpp` source file"
(https://github.com/bitcoin/bitcoin/pull/29976)
After https://github.com/bitcoin/bitcoin/pull/29904, the `common\url.cpp` source file is included into the `SOURCE_FILES` by the `msvc-autogen.py` script.

Removes a compiler [warning](https://github.com/bitcoin/bitcoin/actions/runs/8853698173/job/24315127236#step:20:1776):
```
url.obj : warning LNK4006: "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl UrlDecode(class std::basic_string_view<char,struct std::char_traits<char> >)" (?UrlDecode@@YA
...

💬 TheCharlatan commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#issuecomment-2080168836)
Concept ACK

💬 laanwj commented on pull request "test: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2080191475)
Yes, that's usually how it'd be done.

But gah, `requests` is an external dependency? That's unfortunate, i don't think we should be adding dependencies unless absolutely necessary. Probably better to do this with python's built in urllib or http client functionality, or keep it like this.

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2025935950)
Updated e1be443315be6ba6c80267e0e6be59deee77de50 -> 9552619961049d7673f84c40917b0385be70b782 ([`pr/saferesult.7`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.7) -> [`pr/saferesult.8`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.7..pr/saferesult.8)) with small suggested cleanups

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581579143)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580997920

> nit: unnecessary extra newline

Thanks, removed

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581580768)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581039091

> nit: Is this line needed?

Nope, dropped this

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581579195)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581409638

> Can you briefly explain why these `operator=` are made private as opposed to deleting them?

Changed these to `delete`. The default operators were needed in an earlier version of the PR which had an `Update()` method which called them privately.

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581580725)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581128440

> But, in any case, it's not a big deal if you don't think it's worth it. I also don't think this is a problem anyway.

I'm happy to go with any suggestion. I don't see shadowing as a problem in all cases, so this is just the best way to write the code that I could think of.

💬 fjahr commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2080224670)
Alright, ready for review again. I hope I have addressed all the ideas that were mentioned as intended. Sorry but the enhancement of the metadata is now one big commit for now, I can try to split it up if it makes review easier.

💬 davidgumberg commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2080236283)
reACK https://github.com/bitcoin/bitcoin/commit/d53d84834747c37f4060a9ef379e0a6b50155246

> I don't think I follow you here. `HEADERS_RESPONSE_TIME` and `CHAIN_SYNC_TIMEOUT` are constants hardcoded in `net_processing.cpp` (and mapped here). I would expect unit test to fail if those are updated.

I didn't mean to say that these tests should be responsible for enforcing the particular values of `HEADERS_RESPONSE_TIME` and `CHAIN_SYNC_TIMEOUT`, just that they would not catch a regression where
...

💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2080239525)
I think I will leave it in here since the motivation for https://github.com/bitcoin/bitcoin/commit/54b2115bb848e1aad8a86aae858de63a851235ce is mostly the increased number of lint checks only accessible through the lint test runner. But I will split it out if anyone has an objection, or if this PR stalls.

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2080372645)
I replaced the `requests` with `urllib`, tested the script for downloading .tar.gz file and works fine for me.

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#issuecomment-2080373067)
Hello @fjahr , @alfonsoromanz , @kevkevinpal ,
Could you please confirm if it is ready for merging, or does it require further reviews?

💬 Karimazam commented on issue "Prune mode is incompatible with -txindex.":
(https://github.com/bitcoin/bitcoin/issues/28684#issuecomment-2080384383)
i installed ltc core wallet when i run it for the very fast time it shows same issue litecoin core wallet prune mode is incompatible with blockfilterindex does anybody know why i am facing this f**king issue/?

👍 fanquake approved a pull request: "build, msvc: Drop duplicated `common\url.cpp` source file"
(https://github.com/bitcoin/bitcoin/pull/29976#pullrequestreview-2026447201)
ACK 97a4ad5713853f51c729cced73f133fafa735ba2

🚀 fanquake merged a pull request: "build, msvc: Drop duplicated `common\url.cpp` source file"
(https://github.com/bitcoin/bitcoin/pull/29976)

📝 fanquake converted_to_draft a pull request: "depends: swap some cctools tools for LLVM tools"
(https://github.com/bitcoin/bitcoin/pull/29739)
These tools are used in GUI packaging on macOS, and also somewhat of a blocker for #21778. The main issue is that some distros don't really ship these tools in a standard ways, i.e Ubuntu only ships `llvm-otool` with a version suffix, i.e `llvm-otool-17`, which makes it hard to find and use. Rather than trying to deal with that mess, switch to using the equivalent LLVM tools.

💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581749184)
Is it required to check for 404 in a separate, additional request?

💬 laanwj commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581750310)
That's how the current code works too - it first does a HEAD request to check that the file is there, then goes to downloading. i don't know the original reasoning behind this, though.

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581750345)
No, I could write more pythonically and with one request, but the reason I wrote it this way was to commit to the previous code. If you agree with one request, I can change it.
Should this change be in a new commit or should I force it on the previous commit?