💬 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

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1608389058)
sure. Pushed.

🤔 glozow reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2068674969)
lgtm ACK 6fcd3cc016eaf07e47c5c448482ff844855a247b

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1608351999)
07bb2736c17dd50b6ec3d4770001f8e6f0c8710a: would be good to put the sanity checks here

```
// If we are using package feerates, we must be doing package submission.
// It also means carveouts and sibling eviction are not permitted.
if (m_package_feerates) {
Assume(m_package_submission);
Assume(!m_allow_carveouts);
Assume(!m_allow_sibling_eviction);
}
if (m_allow_sibling_evction)
...

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1608401645)
done