💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598618650)
Going to leave this as is for now. The sage code in the comment is meant to provide background on where the constant comes from, so I'd prefer to leave it since it is more precise than a written out explanation. However, the code is not essential for checking the "correctness" of the value in that it would be sufficient for someone to simply check that this value matches BIP341. This value will never change, either, so I don't see any advantage to having a sage file in this repo for generating t
...

💬 maflcko commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#issuecomment-2107872102)
> Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.

IIRC this was done to start with a few outputs in the beginning, and increase the number of outputs as the size of the fuzz input increases. The hope is that the runtime is linear with the size of the fuzz input, and that minimizing a potential crash results in a transaction with less outputs. If you allow 256 in the
...

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595556422)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

What is the rationale for 64? That seems like a lot of connections.

🤔 sr-gi reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2041604763)
Code review up to 28adad8eefd18a62232b5c77cbc9ca739a53308d (no tests so far).

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.

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595748248)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd:

nit: if yes -> if so

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595819770)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

I don't think we should be giving `PRIVATE_BROADCAST` priority over every other type of connection. At the very least, they should go after `BLOCK_RELAY`.

Having connections that keep us up to date should be more important than sending out our own stuff

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595793107)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

I'm confused here. We choose not to use our permanent `m_i2p_sam_session` to avoid linking this to our permanent I2P ID, however, we use a pool of transient I2P sessions (`m_unused_i2p_sessions`) instead. I understand these transient sessions can be reused (or at least that's what I get from the docs of `m_unused_i2p_sessions`). So if the same `(peer, session_id)` pair is picked, the node will know that we created both transactions, wouldn't it?

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595750204)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd:

nit: sorry for bikeshedding `clearnet` doesn't seem the best name for this. Maybe `net` or `network`?

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595875212)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

This took me a while to grasp. `num_needed` is set to `m_private_broadcast_connections_to_open.load()`, which is only set to anything above 0 **if there are transactions pending to be broadcast**.

I think it'd be good to clarify this here, to make it clear that we won't try to open a connection of this type if we have no transactions pending

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595772136)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

The way you are defining `use_proxy` could lead to `proxy_opt` having no value, yet `use_proxy` being `true`

```suggestion
bool use_proxy;
if (conn_type == ConnectionType::PRIVATE_BROADCAST) {
const auto proxy_opt{ProxyForClearnetPrivateBroadcast(addrConnect.GetNetwork())};
if (proxy_opt.has_value()) {
use_proxy = true;
proxy = proxy_opt.value();
...

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1596842874)
In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: I don't think these belong to this commit yet

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1596831509)
In 434a20371e441660147da6d0c6c1832cb0d0073b

nit: per one -> for each

🚀 glozow merged a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994)

🚀 glozow merged a pull request: "fuzz: txorphan tests fixups"
(https://github.com/bitcoin/bitcoin/pull/29974)

📝 vasild opened a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095)
Store the thread name in a `thread_local` variable of type `char[]` instead of `std::string`. This avoids calling the destructor when the thread exits. This is a workaround for
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

For type-safety, return `std::string_view` from
`util::ThreadGetInternalName()` instead of `char[]`.

As a side effect of this change, we no longer store a reference to a `thread_local` variable in `CLockLocation`. This was dangerous because if the thread qui
...

💬 vasild commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2107921564)
> Is anybody interested in reviewing the patch I posted above if I PR it: [#29952 (comment)](https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079776241)

PRed at https://github.com/bitcoin/bitcoin/pull/30095

💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2107925627)
This came from the discussion in https://github.com/bitcoin/bitcoin/issues/29952

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2107935583)
> I don't think so: only the Conflict and Replace reasons need extra data, and from what I understand it can only be one reason at any given time. Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, `std::variant<CTransactionRef, BlockData>`, where block data holds the block hash and block number.

@josibake Makes sense but I'm skeptical about how this affects the removal reason usage. For example, you can just d
...

💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1598646404)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd "net: implement opening PRIVATE_BROADCAST connections": `m_max_private_broadcast` is not used.

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1598649148)
Done. Thanks