💬 Realrobwoodx commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1636690172)
I want out of this contract I don't even know what it is or how it linked to me but someone needs to send back my funds in the amount of 35.588 OP tokens or, ultimately I will involve the authorities. This isn't a joke I consider this theft/fraud return to me ASAP 

💬 hebasto commented on pull request "bugfix: Make `CCheckQueue` RAII-styled (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1636693701)
Rebased 6e079015707188ac15405662ce396dd837da569a -> 2235765f7fd946d3b08fd9e109584967407e46d1 ([pr26762.13](https://github.com/hebasto/bitcoin/commits/pr26762.13) -> [pr26762.14](https://github.com/hebasto/bitcoin/commits/pr26762.14)) due to the conflict with #28048.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264388335)
> But I'm not able to see another reason why we should add the txid to the `inventory_known_filter`, so why not delete this whole block?

That seems correct to me.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264391225)
It's okay to discard it though -- you just might as well not have called the function in the first place if you don't care about the return value? (And given it's inline, presumably the compiler will treat it as if it wasn't called anyway).

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1636756516)
> Why not just use C++ then? (I'm not a Rust expert, maybe there are good reasons)

I tried that, but vanilla C++ doesn't have a nice interface for processes. There's only libc, or third party libs. So it seemed easier to just use rust with the added bonus that the written code closely resembles the look and feel of python. If you write something elegant in C++, I am happy to push it here, tough.

💬 brunoerg commented on issue "fuzz: connman, `m_nodes` is always empty":
(https://github.com/bitcoin/bitcoin/issues/27980#issuecomment-1636762785)
cc: @MarcoFalke

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264440771)
Right, nodiscard is "use it or lose it" (remove it) -- applicable to getter/pure functions and any interfaces where the return value must be checked. Unlike the latter, if the result of a getter is not used it would not be incorrect, but in that case the call is useless and should be removed; nodiscard just enforces it.

💬 furszy commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1636792749)
Failing CI task isn't related. Failure is on `feature_fee_estimation.py`, in the periodical flush case.

💬 furszy commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264456268)
In df850c25:
Removed the only usage of the `nodes_wallet_dir` function but not the, now unused, function.

💬 furszy commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264460963)
In df850c25:

Could simplify this:

```suggestion
# Copy wallets to previous version
for wallet in os.listdir(node_master_wallets_dir):
source = node_master_wallets_dir / wallet
if self.major_version_equals(node, 16):
# 0.16 node expect the wallet to be in the wallet dir but as a plain file rather than in directories
source / "wallet.dat"
shutil.copytree(source, node.wallets
...

💬 furszy commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264468473)
s/sinced/since

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1636857565)
Initial testing with the Java I2P Router shows that this pull fixes the issue (nice work!)

On master without this PR, thousands of lines of error messages are logged (`Error accepting: Cannot decode Base64: "STREAM STATUS RESULT=I2P_ERROR"`) and bitcoind I2P session re-creation fails until the I2P router is manually stopped and restarted.

With this pull, the error is logged only once and there is no need to restart the I2P Router. A new persistent I2P session is created on the second try.
...

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264577309)
I can see the point of adding `nodiscard` in that case for confusing APIs (eg, where someone not familiar with the API might write `foo.empty()` when they meant `foo.clear()`) but for a `foo.GetBar()` function, it just seems like busy work, that I don't think makes sense to do.

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1264577392)
Updated to not add txids to the known filter for wtxid peers, and to set the entry sequence to 0 when adding txs that were in a block that was reorged out.

💬 MarcoFalke commented on issue "fuzz: connman, `m_nodes` is always empty":
(https://github.com/bitcoin/bitcoin/issues/27980#issuecomment-1637013376)
Jup. Sgtm. If there are no downsides, I don't see a reason not to do it.

📝 MarcoFalke opened a pull request: "ci: Use DOCKER_BUILDKIT for lint image"
(https://github.com/bitcoin/bitcoin/pull/28083)
Currently the lint docker/podman image has many issues:

* It relies on an EOL debian version.
* It relies on a debian version different from the one used in the CI lint task.
* It relies on the legacy docker build command, which requires the user to make `cd ./ci/lint/` before the build step.
* It doesn't use the `.python-version` file, but a hardcoded version.

Fix all issues by using the recommended `DOCKER_BUILDKIT=1` to generate the image.

Also:
* Rename `/tmp/python` to `/python
...

📝 fanquake opened a pull request: "doc: update windows `-fstack-clash-protection` doc"
(https://github.com/bitcoin/bitcoin/pull/28084)
Now that changes have been made in GCC, to fix the build failures.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90458.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1637129953)
@vasild OK I got this branch passing all CI. ✅ I ran tests locally on Ubuntu and MacOS and also tested the feature in production on both platforms with `tor --SocksPort unix:/...` Everything is working! yay.

The real fix was here actually (diff below) Embarrassingly simple. This was a bug on Linux but NOT on macOS.

I did not need the `SendComplete` changes or `Socks5` changes which is great because that keeps review simple and the branch focused. I will be happy to review those changes if
...

👍 TheCharlatan approved a pull request: "doc: update windows `-fstack-clash-protection` doc"
(https://github.com/bitcoin/bitcoin/pull/28084#pullrequestreview-1531767457)
ACK 05ef059a333479e553382c2ae6ef6fde668ce3cb

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1637131364)
Verified that the issue doesn't occur with the i2pd router, only with the Java I2P router.