💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549613286)
No, but the `switch` warrants each case to be handled.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549613286)
No, but the `switch` warrants each case to be handled.
🤔 hebasto reviewed a pull request: "Fix bitcoin-qt visual glitches on Wayland"
(https://github.com/bitcoin-core/gui/pull/904#pullrequestreview-3492565990)
Tested 095f920629207b5ec4c50de7b454dfced0eafefb, it breaks UX.
For example, on Fedora 43 with Gnome 49 and Wayland, follow these steps:
1. Run `bitcoin-qt`.
2. Hide the main window using the "Hide" command in context menu.
3. Click on "Receive" in the system tray icon menu.
On the master branch, the main window reappears.
With this PR, however, the main window remains hidden.
(https://github.com/bitcoin-core/gui/pull/904#pullrequestreview-3492565990)
Tested 095f920629207b5ec4c50de7b454dfced0eafefb, it breaks UX.
For example, on Fedora 43 with Gnome 49 and Wayland, follow these steps:
1. Run `bitcoin-qt`.
2. Hide the main window using the "Hide" command in context menu.
3. Click on "Receive" in the system tray icon menu.
On the master branch, the main window reappears.
With this PR, however, the main window remains hidden.
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549618397)
can we throw in that case with a TODO maybe?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549618397)
can we throw in that case with a TODO maybe?
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549645597)
I do not like treating booleans as integers like that. I find it confusing. For example then it is not immediately clear whether `IsPrivateBroadcastConn()` returns an integer that can be greater than `1`.
The code coverage would be more greener but that would be misleading because we would still never increment `active_connections` and it will remain `0` in either case. There is no test right now that retries a send while there are active private broadcast connections and this is rightfully i
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549645597)
I do not like treating booleans as integers like that. I find it confusing. For example then it is not immediately clear whether `IsPrivateBroadcastConn()` returns an integer that can be greater than `1`.
The code coverage would be more greener but that would be misleading because we would still never increment `active_connections` and it will remain `0` in either case. There is no test right now that retries a send while there are active private broadcast connections and this is rightfully i
...
📝 hebasto opened a pull request: "Revert "gui, qt: brintToFront workaround for Wayland""
(https://github.com/bitcoin-core/gui/pull/914)
This PR reverts a Wayland-specific workaround introduced in https://github.com/bitcoin-core/gui/pull/831, which appears to break the UI, as reported in:
- https://github.com/bitcoin/bitcoin/issues/33432
- https://github.com/bitcoin-core/gui/pull/831
An alternative to https://github.com/bitcoin-core/gui/pull/904, as suggested in https://github.com/bitcoin-core/gui/pull/904#discussion_r2435752838.
(https://github.com/bitcoin-core/gui/pull/914)
This PR reverts a Wayland-specific workaround introduced in https://github.com/bitcoin-core/gui/pull/831, which appears to break the UI, as reported in:
- https://github.com/bitcoin/bitcoin/issues/33432
- https://github.com/bitcoin-core/gui/pull/831
An alternative to https://github.com/bitcoin-core/gui/pull/904, as suggested in https://github.com/bitcoin-core/gui/pull/904#discussion_r2435752838.
💬 hebasto commented on pull request "Fix bitcoin-qt visual glitches on Wayland":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3562890479)
From https://github.com/bitcoin-core/gui/pull/904#discussion_r2435752838:
> Would it be more clear to simply revert [15aa7d0](https://github.com/bitcoin-core/gui/commit/15aa7d023688700a47997b92108de95f2d864f5a)?
Done in https://github.com/bitcoin-core/gui/pull/914.
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3562890479)
From https://github.com/bitcoin-core/gui/pull/904#discussion_r2435752838:
> Would it be more clear to simply revert [15aa7d0](https://github.com/bitcoin-core/gui/commit/15aa7d023688700a47997b92108de95f2d864f5a)?
Done in https://github.com/bitcoin-core/gui/pull/914.
💬 hebasto commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3562896629)
cc @benthecarman @diegoviola @pablomartin4btc
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3562896629)
cc @benthecarman @diegoviola @pablomartin4btc
💬 hebasto commented on issue "v30rc1 Weird GUI windowing behavior":
(https://github.com/bitcoin/bitcoin/issues/33432#issuecomment-3562903769)
@benthecarman
> Changing tabs in the GUI will close and re-open the app, it also will reposition the app
Could you please test https://github.com/bitcoin-core/gui/pull/914?
(https://github.com/bitcoin/bitcoin/issues/33432#issuecomment-3562903769)
@benthecarman
> Changing tabs in the GUI will close and re-open the app, it also will reposition the app
Could you please test https://github.com/bitcoin-core/gui/pull/914?
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549681649)
Yes, until the malleated version is mined (if it is valid) or until we receive the original transaction back (same txid, same wtxid). But no strong opinion, maybe we can do better here?
(fixed `wxtid` in the commit message)
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549681649)
Yes, until the malleated version is mined (if it is valid) or until we receive the original transaction back (same txid, same wtxid). But no strong opinion, maybe we can do better here?
(fixed `wxtid` in the commit message)
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549698974)
I also found it confusing at first, but it's a branchless structure that we're using in a [few](https://github.com/bitcoin/bitcoin/blob/1fe851a4781a6af6264f1ba9d93c3281b59a6cac/src/net_processing.cpp#L1632) [other](https://github.com/bitcoin/bitcoin/blob/fa9ca13f35be0a023aeed78775ad66f95717b28b/src/util/feefrac.h#L75) [places](https://github.com/bitcoin/bitcoin/pull/33512/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R684) [already](https://github.com/bitcoin/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549698974)
I also found it confusing at first, but it's a branchless structure that we're using in a [few](https://github.com/bitcoin/bitcoin/blob/1fe851a4781a6af6264f1ba9d93c3281b59a6cac/src/net_processing.cpp#L1632) [other](https://github.com/bitcoin/bitcoin/blob/fa9ca13f35be0a023aeed78775ad66f95717b28b/src/util/feefrac.h#L75) [places](https://github.com/bitcoin/bitcoin/pull/33512/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R684) [already](https://github.com/bitcoin/bitcoin
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549705856)
> there's no retry count limit or final timeout for retries?
Correct.
> restarting the node won't continue the retries?
Correct. The list of transactions to be broadcast is not persisted on disk. There is an item about that in the "Further extensions:" section in the PR description.
> Can we add some unit tests for the code so that it's not only exercisably by python? Would help with debugging to understand the feature in isolation.
Should be doable. I will look into it.
> coul
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549705856)
> there's no retry count limit or final timeout for retries?
Correct.
> restarting the node won't continue the retries?
Correct. The list of transactions to be broadcast is not persisted on disk. There is an item about that in the "Further extensions:" section in the PR description.
> Can we add some unit tests for the code so that it's not only exercisably by python? Would help with debugging to understand the feature in isolation.
Should be doable. I will look into it.
> coul
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549707293)
What would happen if we would just remove it regardless of malleation? We could still log a warning or something, but removing seems slightly safer.
Wouldn't it slightly simplify the code, while I don't see what the disadvantage (i.e. attack surface) would be. Please prove me wrong if I'm misunderstanding the problem. No strong opinion.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549707293)
What would happen if we would just remove it regardless of malleation? We could still log a warning or something, but removing seems slightly safer.
Wouldn't it slightly simplify the code, while I don't see what the disadvantage (i.e. attack surface) would be. Please prove me wrong if I'm misunderstanding the problem. No strong opinion.
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549718424)
> If the input was double spent and the other transaction mined?
yes, if I release a double spend non-privately while I'm also waiting for the private double-spend to come back from others (which might never arrive, since the other one will likely propagate faster).
It seems to me this can only backfire on the author of the transaction, but wanted to run it by you. If it's nonsensical, it doesn't need a test of course (was just wondering what would happen when the tx never arrives back to us
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549718424)
> If the input was double spent and the other transaction mined?
yes, if I release a double spend non-privately while I'm also waiting for the private double-spend to come back from others (which might never arrive, since the other one will likely propagate faster).
It seems to me this can only backfire on the author of the transaction, but wanted to run it by you. If it's nonsensical, it doesn't need a test of course (was just wondering what would happen when the tx never arrives back to us
...
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2549729288)
There is a small mismatch to the error message returned from the RPC, might be a silent merge conflict.
```suggestion
error_message = "Wallet loading failed. Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of a pruned node)"
```
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2549729288)
There is a small mismatch to the error message returned from the RPC, might be a silent merge conflict.
```suggestion
error_message = "Wallet loading failed. Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of a pruned node)"
```
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2549731769)
Not so silent with the re-run CI now I guess...
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r2549731769)
Not so silent with the re-run CI now I guess...
💬 hebasto commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3562991916)
The PR description lacks motivation for the new feature.
This change significantly affects the UX, and input from designers would be welcome. However, it is worth noting that that designers are currently focused on the QML GUI (see https://github.com/bitcoin-core/gui-qml/tree/qt6 and https://bitcoincore.app/). Feel free to join https://bitcoindesign.slack.com and engage designers in discussion.
Concept NACK for this new feature in the Qt Widget-based GUI.
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3562991916)
The PR description lacks motivation for the new feature.
This change significantly affects the UX, and input from designers would be welcome. However, it is worth noting that that designers are currently focused on the QML GUI (see https://github.com/bitcoin-core/gui-qml/tree/qt6 and https://bitcoincore.app/). Feel free to join https://bitcoindesign.slack.com and engage designers in discussion.
Concept NACK for this new feature in the Qt Widget-based GUI.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549734317)
In general I prefer to have less nesting. It was in some book and I agree that more nesting requires more cognitive load to process. Maybe not necessary in this case because here the nested code is very short.
I find `foo->first` and `foo->second` very unreadable and prefer to always give them a name by assigning them to a named variable. `Txid` in this case.
I prefer:
```cpp
int x = expression;
if (x == 5) {
```
instead of
```cpp
if (int x = expression; x == 5) {
```
it is subjec
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549734317)
In general I prefer to have less nesting. It was in some book and I agree that more nesting requires more cognitive load to process. Maybe not necessary in this case because here the nested code is very short.
I find `foo->first` and `foo->second` very unreadable and prefer to always give them a name by assigning them to a named variable. `Txid` in this case.
I prefer:
```cpp
int x = expression;
if (x == 5) {
```
instead of
```cpp
if (int x = expression; x == 5) {
```
it is subjec
...
💬 fanquake commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549737017)
> Only in rare cases
I don't think wanting to build without having a GCC installation should be considered rare? Other than the clang-only Nix example linked too above, we are actively trying to make that "normal" for some of our builds, i.e #30206.
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549737017)
> Only in rare cases
I don't think wanting to build without having a GCC installation should be considered rare? Other than the clang-only Nix example linked too above, we are actively trying to make that "normal" for some of our builds, i.e #30206.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549737708)
Same: https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549734317
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549737708)
Same: https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549734317
💬 fanquake commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641)
> They should be exactly equivalent.
Unfortunately this isn't the case. `make build_CC=clang build_CXX=clang++ host_CC=clang host_CXX=clang++` doesn't work properly on a machine with no `g++`, because Boosts CMake will still try and find `g++`, and fail. Arguably this is also a CMake issue that we should fix/workaround in some way, because there's no reason for Boost to try and find a compiler, when we aren't compiling anything, and only need to copy headers. In this case, you do need to use
...
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641)
> They should be exactly equivalent.
Unfortunately this isn't the case. `make build_CC=clang build_CXX=clang++ host_CC=clang host_CXX=clang++` doesn't work properly on a machine with no `g++`, because Boosts CMake will still try and find `g++`, and fail. Arguably this is also a CMake issue that we should fix/workaround in some way, because there's no reason for Boost to try and find a compiler, when we aren't compiling anything, and only need to copy headers. In this case, you do need to use
...