💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1867640087)
912377ac4999467be7dfd51481c38972fb1475dd: maybe call it `-maintainers` to reduce confusion with `-unsigned`?
A more generic term might also be handy in the future if e.g. we want to include an OTS timestamp that commits to the (pre codesigning, pgp signed?) guix hashes (with all architectures).
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1867640087)
912377ac4999467be7dfd51481c38972fb1475dd: maybe call it `-maintainers` to reduce confusion with `-unsigned`?
A more generic term might also be handy in the future if e.g. we want to include an OTS timestamp that commits to the (pre codesigning, pgp signed?) guix hashes (with all architectures).
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1867682422)
> I'm not sure I understand the reason for the suppression
The reason of the suppression is the correctness of our code.
> please see my original suggestion in [#31306 (comment)](https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700)
https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1861679662
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1867682422)
> I'm not sure I understand the reason for the suppression
The reason of the suppression is the correctness of our code.
> please see my original suggestion in [#31306 (comment)](https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700)
https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1861679662
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867697664)
`GetNewNodeId()` is private. Not worth weakening the encapsulation for this test.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867697664)
`GetNewNodeId()` is private. Not worth weakening the encapsulation for this test.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867701118)
Done
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867701118)
Done
💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1867703633)
Thanks!
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1867703633)
Thanks!
💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867710276)
I agree that empty string is not the best way to signal negation, and `nullopt` may not be either. Looking forward to `Disabled`-functionality mentioned in https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866406451, but keeping as-is for now.
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1867710276)
I agree that empty string is not the best way to signal negation, and `nullopt` may not be either. Looking forward to `Disabled`-functionality mentioned in https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866406451, but keeping as-is for now.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867731551)
Good observation. I guess this is the nature of things: accepting new connections is driven (initiated) by low level socket event when a new connection arrives, while disconnecting can be low level driven or high level driven - e.g. misbehaving peer at the application layer.
I extended the comment of `EventIOLoopCompletedForAllPeers()` a little bit:
```diff
-+ * Can be used to execute periodic tasks for all nodes.
++ * Can be used to execute periodic ta
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867731551)
Good observation. I guess this is the nature of things: accepting new connections is driven (initiated) by low level socket event when a new connection arrives, while disconnecting can be low level driven or high level driven - e.g. misbehaving peer at the application layer.
I extended the comment of `EventIOLoopCompletedForAllPeers()` a little bit:
```diff
-+ * Can be used to execute periodic tasks for all nodes.
++ * Can be used to execute periodic ta
...
📝 hebasto opened a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410)
This PR aims to address https://github.com/bitcoin/bitcoin/issues/31409.
The first commit prevents possible false positives by ensuring that each condition potentially causing the "Error scanning" log message is tested separately.
The second commit (to be added shortly) disables checks on Windows that do not work or make no sense.
(https://github.com/bitcoin/bitcoin/pull/31410)
This PR aims to address https://github.com/bitcoin/bitcoin/issues/31409.
The first commit prevents possible false positives by ensuring that each condition potentially causing the "Error scanning" log message is tested separately.
The second commit (to be added shortly) disables checks on Windows that do not work or make no sense.
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1867741954)
> (Functional tests could be handled in a follow-up, if you want)
The problematic tests have been disabled and documented.
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1867741954)
> (Functional tests could be handled in a follow-up, if you want)
The problematic tests have been disabled and documented.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867742015)
I don't think we have an official recommendation. My personal preference is to not treat integers as booleans: use `if (x == 0)` instead of `if (!x)` when `x` is an integer. But I am leaving this as it is because it is like that in `master` and this PR only moves it from `net.cpp` to `sockman.cpp` verbatim. I hope it is easier to reviewers to just check that a blob of code is moved without having to validate its correctness since that same code is already in `master`. This helps with such moves:
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867742015)
I don't think we have an official recommendation. My personal preference is to not treat integers as booleans: use `if (x == 0)` instead of `if (!x)` when `x` is an integer. But I am leaving this as it is because it is like that in `master` and this PR only moves it from `net.cpp` to `sockman.cpp` verbatim. I hope it is easier to reviewers to just check that a blob of code is moved without having to validate its correctness since that same code is already in `master`. This helps with such moves:
...
💬 hebasto commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2514595827)
> Is this test-only change with two reviews rfm?
>
> This is the most common false positive right now.
And the [replacement](https://github.com/bitcoin/bitcoin/pull/31176) is ready.
> Fixes #31071
Maybe drop it from the PR description?
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2514595827)
> Is this test-only change with two reviews rfm?
>
> This is the most common false positive right now.
And the [replacement](https://github.com/bitcoin/bitcoin/pull/31176) is ready.
> Fixes #31071
Maybe drop it from the PR description?
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867517086)
nit: Could use `""sv` or simply `""` to avoid string allocations here and in `base64_padding`.
```suggestion
BOOST_CHECK(!DecodeBase32("========"));
```
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867517086)
nit: Could use `""sv` or simply `""` to avoid string allocations here and in `base64_padding`.
```suggestion
BOOST_CHECK(!DecodeBase32("========"));
```
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867745966)
They are invalid, but still worth testing regardless of why precisely the current implementation errors out?
```C++
BOOST_CHECK(!DecodeBase64("nQB/pZw=\0invalid"); // padding and NUL followed by invalid data
BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid\0"); // padding followed by invalid data and NUL
```
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867745966)
They are invalid, but still worth testing regardless of why precisely the current implementation errors out?
```C++
BOOST_CHECK(!DecodeBase64("nQB/pZw=\0invalid"); // padding and NUL followed by invalid data
BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid\0"); // padding followed by invalid data and NUL
```
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867734893)
Last line no longer has an embedded NUL character as specified in the comment above this code block (`// Decoding strings with embedded NUL characters should fail`). Might belong more in `base64_padding` or at least in its own newline-separated block here:
```suggestion
BOOST_CHECK(!DecodeBase64("invalid\0"s)); // correct size, invalid due to \0
BOOST_CHECK( DecodeBase64("nQB/pZw="s)); // valid
BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid"s)); // invalid, padding only allowed at
...
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1867734893)
Last line no longer has an embedded NUL character as specified in the comment above this code block (`// Decoding strings with embedded NUL characters should fail`). Might belong more in `base64_padding` or at least in its own newline-separated block here:
```suggestion
BOOST_CHECK(!DecodeBase64("invalid\0"s)); // correct size, invalid due to \0
BOOST_CHECK( DecodeBase64("nQB/pZw="s)); // valid
BOOST_CHECK(!DecodeBase64("nQB/pZw=invalid"s)); // invalid, padding only allowed at
...
📝 willcl-ark opened a pull request: "cmake: use python from venv if available"
(https://github.com/bitcoin/bitcoin/pull/31411)
When running targets that depend on a python, such as:
```bash
cmake --build . --target test-security-check
cmake --build . --target check-symbols
cmake --build . --target check-security
```
... it can be tricky to configure cmake to use a python venv. This change checks for the `$VIRTUAL_ENV` env var and uses python from there using the following logic:
- First check if $VIRTUAL_ENV is set. If it is, use the python binary from this venv.
- Fall back to system python otherwise.
(https://github.com/bitcoin/bitcoin/pull/31411)
When running targets that depend on a python, such as:
```bash
cmake --build . --target test-security-check
cmake --build . --target check-symbols
cmake --build . --target check-security
```
... it can be tricky to configure cmake to use a python venv. This change checks for the `$VIRTUAL_ENV` env var and uses python from there using the following logic:
- First check if $VIRTUAL_ENV is set. If it is, use the python binary from this venv.
- Fall back to system python otherwise.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867749410)
Maybe yes, but `sock.{h,cpp}` does not currently include `netaddress.h` and I guess including it may cause circular dependency. It would also increase the scope of this PR. I am leaving it as it is.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1867749410)
Maybe yes, but `sock.{h,cpp}` does not currently include `netaddress.h` and I guess including it may cause circular dependency. It would also increase the scope of this PR. I am leaving it as it is.
💬 willcl-ark commented on pull request "cmake: use python from venv if available":
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514601829)
`cmake` >= 3.17 does support a [`Python_FIND_VIRTUALENV`](https://cmake.org/cmake/help/latest/module/FindPython.html#hints) variable, but this doesn't seem to support all venv providers (I use `uv`).
As far as I know, all venv providers set `VIRTUAL_ENV`, so I think this solution should be more universal.
(https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2514601829)
`cmake` >= 3.17 does support a [`Python_FIND_VIRTUALENV`](https://cmake.org/cmake/help/latest/module/FindPython.html#hints) variable, but this doesn't seem to support all venv providers (I use `uv`).
As far as I know, all venv providers set `VIRTUAL_ENV`, so I think this solution should be more universal.
✅ maflcko closed an issue: "ci: Replace wine tests with real tests on Windows?"
(https://github.com/bitcoin/bitcoin/issues/31071)
(https://github.com/bitcoin/bitcoin/issues/31071)
💬 maflcko commented on issue "ci: Replace wine tests with real tests on Windows?":
(https://github.com/bitcoin/bitcoin/issues/31071#issuecomment-2514611461)
Let's continue discussion in #31176
(https://github.com/bitcoin/bitcoin/issues/31071#issuecomment-2514611461)
Let's continue discussion in #31176
💬 brunoerg commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1867758148)
In bdd42ed8de8a92ae3e69e7b2dfceae7330f32676: I think it can be simplified. See:
```diff
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 928ab4acb4..06bc4e1fbb 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1070,12 +1070,7 @@ class WalletMigrationTest(BitcoinTestFramework):
wpkh_desc = descsum_create(f'wpkh({pubkey_hex})')
expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_d
...
(https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1867758148)
In bdd42ed8de8a92ae3e69e7b2dfceae7330f32676: I think it can be simplified. See:
```diff
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 928ab4acb4..06bc4e1fbb 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1070,12 +1070,7 @@ class WalletMigrationTest(BitcoinTestFramework):
wpkh_desc = descsum_create(f'wpkh({pubkey_hex})')
expected_descs = [pk_desc, pkh_desc, sh_wpkh_desc, wpkh_d
...