💬 glozow commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1608272072)
Hm, in the previous setup, we can change any of txdownload_impl.cpp without net_processing.cpp noticing, but yeah maybe less clean to depend on txdownload_impl.h.

I've updated with a new setup where I've added a txdownloadman.cpp depending on txdownload_impl.h, and txdownloadman.h is included by txdownload_impl.h instead of the other way around. And so net_processing.cpp no longer depends on txdownload_impl.h, which makes sense to me. Had to update `lint-circular-dependencies.py` but I do lik
...

🤔 fjahr reviewed a pull request: "net: add ASMap info in `getrawaddrman` RPC"
(https://github.com/bitcoin/bitcoin/pull/30062#pullrequestreview-2068538362)
Concept ACK

💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608272490)
nit, I think this wording would be more clear: "The ASN mapped to the IP of this peer per our current ASMap." (and something corresponding to this below).

💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608267929)
I would prefer these to be optional results rather than a magic 0 value.

💬 fjahr commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1608281308)
> I would prefer these to be optional results rather than a magic 0 value.

(if there is no mapping of course)

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608284734)
Yes, if there is only one transaction submitted for broadcast. But if there is more than one, then `m_private_broadcast_connections_to_open` should not be set to `0`.

For example:
* 3 transactions are submitted at approximately the same time: tx1, tx2, tx3.
* NUM_PRIVATE_BROADCAST_PER_TX=5 for each one, so `m_private_broadcast_connections_to_open` is set to 15.
* tx1 is broadcast 2 times, tx2 once and tx3 once, `m_private_broadcast_connections_to_open` is now 15-2-1-1=11.
* tx1 is receive
...

💬 theuni commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608289449)
I think not using `thread_local` vars with destructors is a reasonable policy anyway, regardless of where it's supported.

💬 maflcko commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2122586712)
utACK 17fe948cce2eb75f0f3f4b0db9d0d90648c7d4af

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608293544)
Right! Removed.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608306032)
Changed to `"priv"`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608308448)
Done

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608310055)
I removed the argument to this function and renamed it to `ProxyForIPv4or6PrivateBroadcast()`. It is a bit more simpler now.

💬 maflcko commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608310558)
I see, so the comment should say: "While this particular bug has been fixed, `thread_local` and especially `thread_local` with non-POD objects should be avoided."?

👍 TheCharlatan approved a pull request: "rpc: Optimize serialization and enhance metadata of dumptxoutset output"
(https://github.com/bitcoin/bitcoin/pull/29612#pullrequestreview-2068620308)
Re-ACK 542e13b2937356810bda2c41be83c3b1675e2f2f

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2122621311)
`65ba8d0203...e85cc59bad`: rebase, address suggestions and remove the argument of `ProxyForClearnetPrivateBroadcast()` because it is always IPv4 or IPv6 and we treat both in the same way.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1608327017)
_from the main thread at https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2041604763_

> The main thing I'm wondering currently is why are we tacking `m_private_broadcast_connections_to_open` loosely? It feels harder to reason about, but I don't see what the benefit of it is.

The reason for this is that it is simpler to implement that way (I think, somebody has a simpler proposal?). This is because the connman thread is creating the connections, they are consumed/used by the
...

💬 hebasto commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2122679965)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/203.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608374619)
> I guess this would mean slightly cleaner code, but would make it harder to see which part of the single function returned False when a failure happens. Though, debug logs could be added to help with that, if needed.
>
> What do you think?

I don't think that the slightly cleaner code worth the extra debug logs noise we will get. These will be outputted even when peers connect successfully, so we would see some "connect_nodes(): veracity msg hasn't arrived yet" or ".. pong msg hasn't arrive
...

💬 fanquake commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2122701550)
Guix Build (aarch64):
```bash
77a67096a2225cafa44637d69e9618cb275b26eeda00585dfa246dfbec7fc818 guix-build-17fe948cce2e/output/aarch64-linux-gnu/SHA256SUMS.part
57867960d6c8597648cc49efe4165e543b1181ad0a3a97a16f66bd41b93afe8b guix-build-17fe948cce2e/output/aarch64-linux-gnu/bitcoin-17fe948cce2e-aarch64-linux-gnu-debug.tar.gz
76c872bcb98e8fc46a5cb2ebd13c489caa6b7d3b0b6db92c467aae27b30308fc guix-build-17fe948cce2e/output/aarch64-linux-gnu/bitcoin-17fe948cce2e-aarch64-linux-gnu.tar.gz
5093f7
...

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2122705861)
> Looks like there is a mismatch on the arm64 build.

Disabled adhoc-codesigning for now, as the non-determinism is coming from the identifier field (also why it only happens for arm64). also cc @theuni