💬 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.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553254778)
Changed to `_strong()` even though the [docs read](https://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange):

> compare_exchange_weak is allowed to fail spuriously, that is, acts as if *this != expected even if they are equal. When a compare-and-exchange is in a loop, compare_exchange_weak will yield better performance on some platforms.
>
> When compare_exchange_weak would require a loop and compare_exchange_strong would not, compare_exchange_strong is preferable ...

I find the
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3566902150)
`7987000d36...73ea6405da`: take some suggestions and avoid the `<=>` operator since it does not compile on Xcode 15.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553258829)
In other cases like https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550252802 we were going with the simplest one having the strongest guarantees - but if you think I'm wrong here, please leave the old value, no strong feelings here.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553306050)
Discussed this out of band, the tx has a clear pattern already, let's keep the current value - please resolve the comment

📝 151henry151 opened a pull request: "Align legacy script policy with P2SH policy in AreInputsStandard"
(https://github.com/bitcoin/bitcoin/pull/33926)
Legacy scripts currently require solvability, while P2SH scripts only check sigop count. This asymmetry causes some non-solvable legacy scripts to be rejected even when they have acceptable sigop counts, while equivalent P2SH scripts would be accepted.

This change updates AreInputsStandard to check sigop count for legacy scripts instead of requiring solvability, matching P2SH behavior. Non-solvable legacy scripts with sigop counts within the limit are now accepted, consistent with P2SH.

This i
...

💬 hebasto commented on pull request "qt: Defer transaction signing until user clicks Send":
(https://github.com/bitcoin/bitcoin/pull/33925#issuecomment-3566981166)
Please move this PR to the GUI repository: https://github.com/bitcoin-core/gui/pulls.