💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549354887)
This is a continuation of https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214671917. I think it is better to continue the discussion there to have it in one place. I asked at IRC for it to be unlocked, I guess it was locked due to inactivity.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2549354887)
This is a continuation of https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214671917. I think it is better to continue the discussion there to have it in one place. I asked at IRC for it to be unlocked, I guess it was locked due to inactivity.
✅ maflcko closed a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455)
(https://github.com/bitcoin/bitcoin/pull/30455)
📝 maflcko reopened a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455)
Adding tests in `./test/functional/wallet_assumeutxo.py` to cover the following scenario:
- test loading a wallet (backup) on a pruned node
(https://github.com/bitcoin/bitcoin/pull/30455)
Adding tests in `./test/functional/wallet_assumeutxo.py` to cover the following scenario:
- test loading a wallet (backup) on a pruned node
💬 hebasto commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562633993)
The recent feedback from @fanquake has been addressed.
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562633993)
The recent feedback from @fanquake has been addressed.
💬 hebasto commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2549474353)
Thanks! [Dropped](https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562633993).
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2549474353)
Thanks! [Dropped](https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562633993).
💬 hebasto commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2549475436)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562633993).
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2549475436)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3562633993).
💬 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
...