💬 hebasto commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562645421)
> Should #29977 be closed, if we add these docs?
No need to wait for these docs. Static linking of `xcb` and friends should be sufficient.
> Is 20.04 considered unsupported?
I think so.
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562645421)
> Should #29977 be closed, if we add these docs?
No need to wait for these docs. Static linking of `xcb` and friends should be sufficient.
> Is 20.04 considered unsupported?
I think so.
💬 hebasto commented on issue "depends: Freetype and xcbproto version in depends are too new":
(https://github.com/bitcoin/bitcoin/issues/29977#issuecomment-3562659799)
> At least if we're considering Ubuntu 20.04LTS as the reference.
Let’s update the baseline to Ubuntu 22.04 LTS instead.
In that case, this issue can be closed in light of [the static linking of XCB](https://github.com/bitcoin/bitcoin/pull/33537).
(https://github.com/bitcoin/bitcoin/issues/29977#issuecomment-3562659799)
> At least if we're considering Ubuntu 20.04LTS as the reference.
Let’s update the baseline to Ubuntu 22.04 LTS instead.
In that case, this issue can be closed in light of [the static linking of XCB](https://github.com/bitcoin/bitcoin/pull/33537).
💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2549545939)
Couldn't stop thinking about this. It felt weird to implement reverse byte order in `GetBitBE` but then also start `bits` from the end of `ip` data and count backwards. Found a way that I think simplifies things further, making the order we consume bytes between LE&BE implementations the same, but having different bit-order. Diff against b41f5a29f7d56da8fd157770ad29b5776c3684c6:
```diff
--- a/src/util/asmap.cpp
+++ b/src/util/asmap.cpp
@@ -48,21 +48,25 @@ namespace {
constexpr uint32_t INV
...
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2549545939)
Couldn't stop thinking about this. It felt weird to implement reverse byte order in `GetBitBE` but then also start `bits` from the end of `ip` data and count backwards. Found a way that I think simplifies things further, making the order we consume bytes between LE&BE implementations the same, but having different bit-order. Diff against b41f5a29f7d56da8fd157770ad29b5776c3684c6:
```diff
--- a/src/util/asmap.cpp
+++ b/src/util/asmap.cpp
@@ -48,21 +48,25 @@ namespace {
constexpr uint32_t INV
...
💬 l0rinc commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3562729119)
Fixed the remaining linter warnings after the rebase.,
<details>
<summary>Details</summary>
```patch
diff --git a/src/crypto/siphash.h b/src/crypto/siphash.h
index 0027a6850e..10d2713257 100644
--- a/src/crypto/siphash.h
+++ b/src/crypto/siphash.h
@@ -19,15 +19,15 @@ class SipSalt
public:
explicit SipSalt(uint64_t k0, uint64_t k1) noexcept : v{C0 ^ k0, C1 ^ k1, C2 ^ k0, C3 ^ k1} {}
- std::array<uint64_t, 4> v{};
+ std::array<uint64_t, 4> v;
};
// General Sip
...
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3562729119)
Fixed the remaining linter warnings after the rebase.,
<details>
<summary>Details</summary>
```patch
diff --git a/src/crypto/siphash.h b/src/crypto/siphash.h
index 0027a6850e..10d2713257 100644
--- a/src/crypto/siphash.h
+++ b/src/crypto/siphash.h
@@ -19,15 +19,15 @@ class SipSalt
public:
explicit SipSalt(uint64_t k0, uint64_t k1) noexcept : v{C0 ^ k0, C1 ^ k1, C2 ^ k0, C3 ^ k1} {}
- std::array<uint64_t, 4> v{};
+ std::array<uint64_t, 4> v;
};
// General Sip
...
💬 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.