💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885418)
Yeah the inv should match the peer's wtxid relay setting. The announcements are converted to GenTxids in `ProcessMessage` and that's what's passed into `AddTxAnnouncement` and used throughout `txdownloadman_impl` and `txrequest`.
I would prefer to keep it all in this PR, as I don't want both the old GenTxid class and the new variant in master. But I agree it's a bit iffy to have to manually ensure that we didn't miss a place where a previous `uint256` comparison is now suddenly a `GenTxid` co
...
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885418)
Yeah the inv should match the peer's wtxid relay setting. The announcements are converted to GenTxids in `ProcessMessage` and that's what's passed into `AddTxAnnouncement` and used throughout `txdownloadman_impl` and `txrequest`.
I would prefer to keep it all in this PR, as I don't want both the old GenTxid class and the new variant in master. But I agree it's a bit iffy to have to manually ensure that we didn't miss a place where a previous `uint256` comparison is now suddenly a `GenTxid` co
...
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885949)
We erase the iterator soon after but still use the hash, so making a copy here is correct.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142885949)
We erase the iterator soon after but still use the hash, so making a copy here is correct.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142887549)
I'll include this with my next push.
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2142887549)
I'll include this with my next push.
💬 l0rinc commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967059854)
So it does look like an incomplete virtualization in Docker of the underlying network adapter - or something similar I guess.
Is there anything we should do here (comment the code, open an issue anywhere)? If not, please close it.
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967059854)
So it does look like an incomplete virtualization in Docker of the underlying network adapter - or something similar I guess.
Is there anything we should do here (comment the code, open an issue anywhere)? If not, please close it.
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967109115)
@ryanofsky looks like Tidy got more strict again...
```
[10:33:51.654] /ci_container_base/src/ipc/capnp/protocol.cpp:20:10: error: inclusion of deprecated C++ header 'errno.h'; consider using 'cerrno' instead [modernize-deprecated-headers,-warnings-as-errors]
[10:33:51.654] 20 | #include <errno.h>
[10:33:51.654] | ^~~~~~~~~
[10:33:51.654] | <cerrno>
[10:33:51.654] 919 warnings generated.
```
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967109115)
@ryanofsky looks like Tidy got more strict again...
```
[10:33:51.654] /ci_container_base/src/ipc/capnp/protocol.cpp:20:10: error: inclusion of deprecated C++ header 'errno.h'; consider using 'cerrno' instead [modernize-deprecated-headers,-warnings-as-errors]
[10:33:51.654] 20 | #include <errno.h>
[10:33:51.654] | ^~~~~~~~~
[10:33:51.654] | <cerrno>
[10:33:51.654] 919 warnings generated.
```
💬 maflcko commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967112918)
It would be nice to exactly find the issue and fix it, but I won't be working on this for now 😅
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967112918)
It would be nice to exactly find the issue and fix it, but I won't be working on this for now 😅
💬 maflcko commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967130272)
Should be easy to fix by removing this line: https://github.com/bitcoin-core/libmultiprocess/blob/019839758085cb094f107e9ac354cbda79b388e2/.clang-tidy#L18
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967130272)
Should be easy to fix by removing this line: https://github.com/bitcoin-core/libmultiprocess/blob/019839758085cb094f107e9ac354cbda79b388e2/.clang-tidy#L18
📝 rkrux opened a pull request: "rpc, doc: clarify the response of listtransactions RPC"
(https://github.com/bitcoin/bitcoin/pull/32737)
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and
...
(https://github.com/bitcoin/bitcoin/pull/32737)
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and
...
💬 l0rinc commented on pull request "p2p: avoid traversing blocks (twice) during IBD for no reason":
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2143003197)
> The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).
I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param
(https://github.com/bitcoin/bitcoin/pull/32730#discussion_r2143003197)
> The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).
I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2967247693)
I have abstracted out the `std::next_permutation` logic into a separate `ExhaustiveLinearize()` function, and also added a comment that explains how the variable functions/classes/tests are related.
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2967247693)
I have abstracted out the `std::next_permutation` logic into a separate `ExhaustiveLinearize()` function, and also added a comment that explains how the variable functions/classes/tests are related.
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2143040953)
This shouldn't happen, but if it does, we don't want to accidentally approve the block. I added `NONFATAL_UNREACHABLE` to make this clear.
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2143040953)
This shouldn't happen, but if it does, we don't want to accidentally approve the block. I added `NONFATAL_UNREACHABLE` to make this clear.
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967285236)
> looks like Tidy got more strict again...
```diff
diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp
index 691bdf5f92..3326f70931 100644
--- a/src/ipc/capnp/protocol.cpp
+++ b/src/ipc/capnp/protocol.cpp
@@ -16,8 +16,8 @@
#include <mp/util.h>
#include <util/threadnames.h>
-#include <assert.h>
-#include <errno.h>
+#include <cassert>
+#include <cerrno>
#include <future>
#include <memory>
#include <mutex>
diff --git a/src/ipc/interfaces.cpp b/src/ipc/inte
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967285236)
> looks like Tidy got more strict again...
```diff
diff --git a/src/ipc/capnp/protocol.cpp b/src/ipc/capnp/protocol.cpp
index 691bdf5f92..3326f70931 100644
--- a/src/ipc/capnp/protocol.cpp
+++ b/src/ipc/capnp/protocol.cpp
@@ -16,8 +16,8 @@
#include <mp/util.h>
#include <util/threadnames.h>
-#include <assert.h>
-#include <errno.h>
+#include <cassert>
+#include <cerrno>
#include <future>
#include <memory>
#include <mutex>
diff --git a/src/ipc/interfaces.cpp b/src/ipc/inte
...
🤔 rkrux reviewed a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2921691287)
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
I'm in favour of this removal as it gets rid of duplication in the RPCs and leads to fewer sources of balance in the RPCs and to lesser code to maintain as well, which I find quite desirable.
I am not sure if the removal of the long deprecated items constitute to a breaking change as we would expect the users to _not_ have been using these items already.
Besides adding in the release notes, I like how the conventional commits suggest to mention
...
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2921691287)
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
I'm in favour of this removal as it gets rid of duplication in the RPCs and leads to fewer sources of balance in the RPCs and to lesser code to maintain as well, which I find quite desirable.
I am not sure if the removal of the long deprecated items constitute to a breaking change as we would expect the users to _not_ have been using these items already.
Besides adding in the release notes, I like how the conventional commits suggest to mention
...
📝 Bitchryankilledme opened a pull request: "Create msbuild.yml"
(https://github.com/bitcoin/bitcoin/pull/32738)
btc-isdnh.com
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new
...
(https://github.com/bitcoin/bitcoin/pull/32738)
btc-isdnh.com
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new
...
💬 ryanofsky commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2143115151)
re: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2143040953
> This shouldn't happen, but if it does, we don't want to accidentally approve the block. I added `NONFATAL_UNREACHABLE` to make this clear.
Is that change in the latest push? I don't see it and I'm not sure it would help as nonfatal macros are mostly intended for RPC code and nothing in the IPC code handles them right now. I think I would suggest just adding a comment to this line like:
```c++
if (state.IsValid()
...
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2143115151)
re: https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2143040953
> This shouldn't happen, but if it does, we don't want to accidentally approve the block. I added `NONFATAL_UNREACHABLE` to make this clear.
Is that change in the latest push? I don't see it and I'm not sure it would help as nonfatal macros are mostly intended for RPC code and nothing in the IPC code handles them right now. I think I would suggest just adding a comment to this line like:
```c++
if (state.IsValid()
...
💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967389155)
For reference, the failure in the test-each-commit CI (re-run) was (https://github.com/bitcoin/bitcoin/actions/runs/15610014963/job/43981198408#step:7:1986):
```bash
node0 2025-06-12T12:34:42.336592Z (mocktime: 2025-06-12T12:34:46Z) [net] [net.cpp:2041] [InactivityCheck] [net] version handshake timeout, disconnecting peer=0
test 2025-06-12T12:34:43.290505Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2967389155)
For reference, the failure in the test-each-commit CI (re-run) was (https://github.com/bitcoin/bitcoin/actions/runs/15610014963/job/43981198408#step:7:1986):
```bash
node0 2025-06-12T12:34:42.336592Z (mocktime: 2025-06-12T12:34:46Z) [net] [net.cpp:2041] [InactivityCheck] [net] version handshake timeout, disconnecting peer=0
test 2025-06-12T12:34:43.290505Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
def test_function():
...
📝 hodlinator converted_to_draft a pull request: "wallet: Warn upon failing to scan directory"
(https://github.com/bitcoin/bitcoin/pull/32736)
Make wallet DB detect and report failure to scan wallet directory.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
(https://github.com/bitcoin/bitcoin/pull/32736)
Make wallet DB detect and report failure to scan wallet directory.
Found while reviewing: https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753
💬 fanquake commented on issue "Networking tests fail on emulated big-endian systems":
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967418597)
> Running the exact same commands passes for me:
I ran the same commands, on a macOS machine, using podman, and it also worked fine.
(https://github.com/bitcoin/bitcoin/issues/31812#issuecomment-2967418597)
> Running the exact same commands passes for me:
I ran the same commands, on a macOS machine, using podman, and it also worked fine.
💬 vasild commented on pull request "doc: Update Qt 6 packages on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/32717#discussion_r2143191313)
`qt6-tools` suffices:
```suggestion
pkg install qt6-base qt6-tools
```
`qt6-translations` depends on `qt6-tools`. I tried installing just `qt6-base qt6-tools` (`qt6-tools` pulled a few more deps) and compilation succeeded.
(https://github.com/bitcoin/bitcoin/pull/32717#discussion_r2143191313)
`qt6-tools` suffices:
```suggestion
pkg install qt6-base qt6-tools
```
`qt6-translations` depends on `qt6-tools`. I tried installing just `qt6-base qt6-tools` (`qt6-tools` pulled a few more deps) and compilation succeeded.
🤔 BrandonOdiwuor reviewed a pull request: "wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance"
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2921936952)
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
<img width="759" alt="Screenshot 2025-06-12 at 19 31 15" src="https://github.com/user-attachments/assets/8a9dd961-ecb5-4ee6-b0a6-7b021083f00a" />
Master | c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
:-------------------------:|:-------------------
...
(https://github.com/bitcoin/bitcoin/pull/32721#pullrequestreview-2921936952)
ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated `balance, unconfirmed_balance, immature_balance` fields from `getwalletinfo` and `getunconfirmedbalance` RPCs, as this infomation can be found on the `getbalances` RPC
<img width="759" alt="Screenshot 2025-06-12 at 19 31 15" src="https://github.com/user-attachments/assets/8a9dd961-ecb5-4ee6-b0a6-7b021083f00a" />
Master | c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
:-------------------------:|:-------------------
...
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2967550739)
Turns out I got a little carried away by #32421; blocks do still need `rehash()`.
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2967550739)
Turns out I got a little carried away by #32421; blocks do still need `rehash()`.