💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2553198152)
In commit "Rewrite GatherClusters to use the txgraph implementation"

Nit for a follow-up: this 500 limit doesn't really help with anything if it's only enforced after doing all the work. I would think it can either be dropped, or moved into the loop above after every `GetCluster` call.

📝 hebasto opened a pull request: "qa: Remove no longer needed `feature_dirsymlinks.py`"
(https://github.com/bitcoin/bitcoin/pull/33924)
This PR follows up on https://github.com/bitcoin/bitcoin/pull/33842, which removed (1) the [unused `create_directories` workaround](https://github.com/bitcoin/bitcoin/pull/33842/commits/fa9dacdbde7dc18d134019bdad24f47e4dea1dda) and (2) the corresponding [unit test](https://github.com/bitcoin/bitcoin/pull/33842/commits/fa3854e43295f71f5dad8557dd621f0f799b0ee0), but left the related functional test in place. The latter is now being removed.

For historical context, see:
- https://github.com/bit
...

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

```cpp
case node::TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST:
what = "for private broadcast without adding to the mempool";
```
normally "yes" but the thing is that the `switch` without the case will fail to compile. Would you prefer something like:

```cpp
case node::TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST:
what = "for private broadcast without adding to the mempool";
throw std::runtime_error("Remove this throw when callers exist
...

💬 plebhash commented on issue "should concurrent IPC requests directed to the same thread cause a crash?":
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3566852691)
I guess the main thing I wanted to report here is a duplicate of https://github.com/bitcoin-core/libmultiprocess/pull/214, so it's good to know these crashes won't happen on `v30.x`!

> how many requests should be allowed? Should it allow 5, 10, or 100 requests before returning "thread busy"? Or an arbitrary amount? Or a configurable amount?

it would be up to the client to know what are the reasonable bounds for backpressure so that it can avoid the need to handle errors, so I'd say that a conf
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553224623)
Changed to
```
if ...
if ...
return ok
return notok
```
while avoiding `if (int x = expression; x == 5)`.

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

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

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553230493)
Inverted the `if (... == end())` to `if (... != end())`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553233889)
Reverted the logic but kept the `num_broadcasted` variable and the two erases, it is more debugger friendly.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553234773)
don't have strong feelings about this, was just surprised to find out this new code is unused

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

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

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553244084)
Yes, but I elaborated and expanded the commit message a little bit.

💬 maflcko commented on pull request "qa: Remove no longer needed `feature_dirsymlinks.py`":
(https://github.com/bitcoin/bitcoin/pull/33924#issuecomment-3566883556)
Being able to symlink the dir seems like a Bitcoin Core feature that should be supported and tested?

📝 151henry151 opened a pull request: "qt: Defer transaction signing until user clicks Send"
(https://github.com/bitcoin/bitcoin/pull/33925)
Fixes #30070

When creating an unsigned PSBT from the GUI ("Create Unsigned"), the transaction was already signed during preparation, causing legacy inputs to have non-empty scriptSig fields. This made the PSBT parser reject the transaction with the error "Unsigned tx does not have empty scriptSigs and scriptWitnesses."

The fix defers signing until the user explicitly clicks "Send" instead of signing during transaction preparation. This ensures unsigned PSBTs are truly unsigned while still allo
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553247329)
It does not matter. This number will barely go more than 10. For such cases I think the most appropriate type is "word size" integer.

Related to https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2539016880

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553247864)
> Reverted the logic

👍

> but kept the num_broadcasted variable and the two erases, it is more debugger friendly.

Isn't this the exact scenario for why `extract` was introduced in C++20? Or was I mis-suggesting it?

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

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553249953)
It *is* indeed related to that - I haven't seen the discussion yet, my mistake.
Though after reading what you wrote I came to the opposite conclusion: it doesn't matter therefore let's use the safest, an atomic 64 or atomic 32, instead of a value that changes between these to based on architecture and is therefore harder to reason about.

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