💬 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?

💬 maflcko commented on issue "Require thread_local":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2080410522)
> We only use `thread_local` for storing the thread name in `std::string`. Can use dumb `char` array for this:

If this works, then string_view may also work? Yet another alternative may be to just completely nuke thread_local with a map from id to $obj (See https://github.com/bitcoin/bitcoin/issues/18678#issuecomment-621717592)

💬 laanwj commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2080412606)
i think the main problem with the map approach is that it doesn't clean up data when threads disappear, this is something that TLS handles automatically, and also why it's so hard for platforms to get right

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581758233)
Same question as https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566378431

I like that this doesn't add extra burden to `m_recent_rejects` (which is probably the busier of the 2 filters even though they have the same size). It makes more sense if you think of each tx within the package as reconsiderable, even though the package isn't?

💬 laanwj commented on issue "qa: Support git worktrees when running the linters locally via Docker":
(https://github.com/bitcoin/bitcoin/issues/29972#issuecomment-2080415116)
i think the root problem here is that docker only provides access to the source directory to the container, and a worktree isn't self-contained, it contains a path to the actual repository the worktree comes from, which won't be available there. The linters need access to git metadata to do their checks (for example, to make sure that they only check files in git, not generated ones).

💬 maflcko commented on issue "qa: Support git worktrees when running the linters locally via Docker":
(https://github.com/bitcoin/bitcoin/issues/29972#issuecomment-2080420581)
> Yes, this happens if you try to run any of the CI tasks locally in a worktree.

At least for the "real" CI tasks, we should consider making a fallback, as I don't think much from git is needed? So far I could only find `git log -1` in `ci/test/`, but that should work in a worktree, no?

⚠️ laanwj opened an issue: "depends: Freetype and xcbproto version in depends are too new"
(https://github.com/bitcoin/bitcoin/issues/29977)
While investigating interface versions for [qtsowrap](https://github.com/laanwj/qtsowrap/blob/main/scripts/collector.py), i noticed that currently, in depends, the version of freetype and xproto are too new. As of 8cd9475321737a69454f3b54a588b7bfe9f32847.

At least if we're considering Ubuntu 20.04LTS as the reference. xcb-proto is even too new for 22.04LTS:

| library | 20.04LTS | 22.04LTS | depends |
|-------|--------|---------|---------|
| libfontconfig | 2.13.1 | 2.13.1 |
...