💬 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

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1598651033)
Not sure. Maybe you can try opening an issue to discuss it

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2107965861)
Rebased. I also addressed some feedback from rkrux's comments: changed hardcoded value of `nblocks` from 99 to a dynamic value, i.e `SNAPSHOT_BASE_HEIGHT-START_HEIGHT-1`

🤔 glozow reviewed a pull request: "26.x: backport #29853 ("sign: don't assume we are parsing a sane Miniscript")"
(https://github.com/bitcoin/bitcoin/pull/29854#pullrequestreview-2053051963)
Commit needs to be updated. I can add this to #29899 (to include the release notes changes as well) and close this?

⚠️ hebasto opened an issue: "windows: Newer libevent causes `http_request` fuzz target failure"
(https://github.com/bitcoin/bitcoin/issues/30096)
When building with MSVC, the `libevent` dependency package is provided by the vcpkg package manager.

The https://github.com/bitcoin/bitcoin/pull/27335 pinned the `libevent` version to [`2.1.12#7`](https://github.com/libevent/libevent/releases/tag/release-2.1.12-stable) to avoid issues with the changed signature of the `evhttp_connection_get_peer` function.

Then, https://github.com/bitcoin/bitcoin/pull/29774 introduced the `fuzz.exe` binary.

It turned out that the newer `libevent` versio
...