💬 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
...
🤔 fanquake requested changes to a pull request: "doc: Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3492766868)
See https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641.
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3492766868)
See https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641.
✅ diegoviola closed a pull request: "Fix bitcoin-qt visual glitches on Wayland"
(https://github.com/bitcoin-core/gui/pull/904)
(https://github.com/bitcoin-core/gui/pull/904)
💬 diegoviola commented on pull request "Fix bitcoin-qt visual glitches on Wayland":
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3563050872)
Closing in favor of [#904.](https://github.com/bitcoin-core/gui/pull/914).
(https://github.com/bitcoin-core/gui/pull/904#issuecomment-3563050872)
Closing in favor of [#904.](https://github.com/bitcoin-core/gui/pull/914).
💬 apogio commented on pull request "Add coins (UTXOs) tab and makes it view-only":
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3563051500)
I'm not sure what the comment about motivation means, but I appreciate your time.
(https://github.com/bitcoin-core/gui/pull/898#issuecomment-3563051500)
I'm not sure what the comment about motivation means, but I appreciate your time.
💬 diegoviola commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3563080669)
@hebasto ACK. Thanks for looking into this and for the revert. :+1:
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3563080669)
@hebasto ACK. Thanks for looking into this and for the revert. :+1:
💬 frankomosh commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549829849)
```suggestion
bool mutated = fuzzed_data_provider.ConsumeBool(); // output param, initial value ignored
```
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2549829849)
```suggestion
bool mutated = fuzzed_data_provider.ConsumeBool(); // output param, initial value ignored
```
🤔 l0rinc reviewed a pull request: "coins: use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/33512#pullrequestreview-3492784897)
Thanks for the comments @optout21, since I wasn't invalidating any ACKs, I have applied your suggestions (with rebase) - let me know if this is what you meant.
> keeping a count like the dirty count is very efficient, but prone to going out of sync
yeah, we have a few of these - but the sanitizers and sanity checks and fuzzers usually reveal the problems (sanitizers break on underflow and the sanity checks recalculate everything during testing).
(https://github.com/bitcoin/bitcoin/pull/33512#pullrequestreview-3492784897)
Thanks for the comments @optout21, since I wasn't invalidating any ACKs, I have applied your suggestions (with rebase) - let me know if this is what you meant.
> keeping a count like the dirty count is very efficient, but prone to going out of sync
yeah, we have a few of these - but the sanitizers and sanity checks and fuzzers usually reveal the problems (sanitizers break on underflow and the sanity checks recalculate everything during testing).