💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090840593)
This will also pass if no exception is thrown. Either add `assert False` after line 214 or have a boolean variable to false before the `try` and set it to true inside `except` and assert that it is true afterwards.
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090840593)
This will also pass if no exception is thrown. Either add `assert False` after line 214 or have a boolean variable to false before the `try` and set it to true inside `except` and assert that it is true afterwards.
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090912262)
Is the intention here to send 8MB of data?
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090912262)
Is the intention here to send 8MB of data?
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090917556)
Here and elsewhere in the added tests, `assert_equal()` produces a better error message:
`assert` (value of `out1` is not printed):
```
assert out1 == b'{"result":"high-hash","error":null}\n'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```
vs
`assert_equal()`:
```
AssertionError: not(b'{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}\n' == b'{"result":"high-hash","error":null}\n')
```
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090917556)
Here and elsewhere in the added tests, `assert_equal()` produces a better error message:
`assert` (value of `out1` is not printed):
```
assert out1 == b'{"result":"high-hash","error":null}\n'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```
vs
`assert_equal()`:
```
AssertionError: not(b'{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}\n' == b'{"result":"high-hash","error":null}\n')
```
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2091093913)
This would not recognize numbers larger than `FFFFFFFF` (4 bytes, 8 chars). That should be `if (str.size() > 16) return false;` instead of `8`.
But better not roll our own and use `std::from_chars()` like the others:
<details>
<summary>[patch] Use ParseIntegral() which uses std::from_chars(), like the other Parse*() functions</summary>
```diff
diff --git i/src/util/strencodings.cpp w/src/util/strencodings.cpp
index d923bdd121..05a5dced62 100644
--- i/src/util/strencodings.cpp
+++ w
...
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2091093913)
This would not recognize numbers larger than `FFFFFFFF` (4 bytes, 8 chars). That should be `if (str.size() > 16) return false;` instead of `8`.
But better not roll our own and use `std::from_chars()` like the others:
<details>
<summary>[patch] Use ParseIntegral() which uses std::from_chars(), like the other Parse*() functions</summary>
```diff
diff --git i/src/util/strencodings.cpp w/src/util/strencodings.cpp
index d923bdd121..05a5dced62 100644
--- i/src/util/strencodings.cpp
+++ w
...
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2091083332)
Add some more test cases and avoid `BOOST_CHECK(A && B)` because when that fails it is unclear which one of `A` or `B` was false. Prefer `BOOST_CHECK(A); BOOST_CHECK(B);`. Also, because `B` is `n == 123`, do `BOOST_CHECK_EQUAL(n, 123)`:
```cpp
BOOST_AUTO_TEST_CASE(test_ParseUInt64Hex)
{
uint64_t n;
// Valid values
BOOST_CHECK(ParseUInt64Hex("1234", nullptr));
BOOST_CHECK(ParseUInt64Hex("1234", &n));
BOOST_CHECK_EQUAL(n, 0x1234);
BOOST_CHECK(ParseUInt64Hex("a",
...
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2091083332)
Add some more test cases and avoid `BOOST_CHECK(A && B)` because when that fails it is unclear which one of `A` or `B` was false. Prefer `BOOST_CHECK(A); BOOST_CHECK(B);`. Also, because `B` is `n == 123`, do `BOOST_CHECK_EQUAL(n, 123)`:
```cpp
BOOST_AUTO_TEST_CASE(test_ParseUInt64Hex)
{
uint64_t n;
// Valid values
BOOST_CHECK(ParseUInt64Hex("1234", nullptr));
BOOST_CHECK(ParseUInt64Hex("1234", &n));
BOOST_CHECK_EQUAL(n, 0x1234);
BOOST_CHECK(ParseUInt64Hex("a",
...
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090504818)
1 second timeout to send or receive seems more than enough for local testing on a dev machine. However, CI virtual machines sometimes are surprisingly slow. To avoid unnecessary test failures maybe it would be better to have this be 5 or 10 seconds for the `sendall()` calls and then set to 1 for the `recv()` call which we expect to timeout.
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090504818)
1 second timeout to send or receive seems more than enough for local testing on a dev machine. However, CI virtual machines sometimes are surprisingly slow. To avoid unnecessary test failures maybe it would be better to have this be 5 or 10 seconds for the `sendall()` calls and then set to 1 for the `recv()` call which we expect to timeout.
🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2842608359)
Posting review midway. Reviewed up to and including 144a777f86 `string: add CaseInsensitiveComparator`. I like that the PR starts with some new tests to enforce correct behavior.
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2842608359)
Posting review midway. Reviewed up to and including 144a777f86 `string: add CaseInsensitiveComparator`. I like that the PR starts with some new tests to enforce correct behavior.
💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2883956445)
(fixed typo in commit message)
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2883956445)
(fixed typo in commit message)
🤔 furszy reviewed a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843919937)
What if instead of excluding the entire test file, we only exclude the specific failing test case?
```diff
diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py
--- a/test/functional/wallet_reorgsrestore.py (revision 8d5a11f34157934d9aecf8d5535ec3c17b13fcf3)
+++ b/test/functional/wallet_reorgsrestore.py (date 1747319065527)
@@ -14,6 +14,7 @@
"""
from decimal import Decimal
+import os
import shutil
from test_framework.test_framework i
...
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843919937)
What if instead of excluding the entire test file, we only exclude the specific failing test case?
```diff
diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py
--- a/test/functional/wallet_reorgsrestore.py (revision 8d5a11f34157934d9aecf8d5535ec3c17b13fcf3)
+++ b/test/functional/wallet_reorgsrestore.py (date 1747319065527)
@@ -14,6 +14,7 @@
"""
from decimal import Decimal
+import os
import shutil
from test_framework.test_framework i
...
🤔 mzumsande reviewed a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843925049)
utACK fa981b90f53101bff2eda606d9479233e71736b5
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843925049)
utACK fa981b90f53101bff2eda606d9479233e71736b5
🤔 shahsb reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-2843940627)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-2843940627)
Concept ACK
⚠️ fanquake opened an issue: "test: failure in `mining_basic.py` AssertionError: not(4.656542373906924E-10 == 4.656542373906925E-10)"
(https://github.com/bitcoin/bitcoin/issues/32515)
x86_64 Alpine Linux
master @ c779ee3a4044df3a263394bbb8b104aeeaa7c727
```bash
/bitcoin # build/test/functional/test_runner.py mining_basic.py --valgrind --timeout-factor=0
Temporary test directory at /tmp/test_runner_₿_🏃_20250515_142647
Remaining jobs: [mining_basic.py]
1/1 - mining_basic.py failed, Duration: 58 s
stdout:
2025-05-15T14:26:48.007000Z TestFramework (INFO): PRNG seed is: 1679234749093477821
2025-05-15T14:26:4
...
(https://github.com/bitcoin/bitcoin/issues/32515)
x86_64 Alpine Linux
master @ c779ee3a4044df3a263394bbb8b104aeeaa7c727
```bash
/bitcoin # build/test/functional/test_runner.py mining_basic.py --valgrind --timeout-factor=0
Temporary test directory at /tmp/test_runner_₿_🏃_20250515_142647
Remaining jobs: [mining_basic.py]
1/1 - mining_basic.py failed, Duration: 58 s
stdout:
2025-05-15T14:26:48.007000Z TestFramework (INFO): PRNG seed is: 1679234749093477821
2025-05-15T14:26:4
...
💬 fanquake commented on pull request "scripted-diff: Remove unused leading newline in RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32514#issuecomment-2884064294)
```bash
Run rpc with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/bin/fuzz', PosixPath('/
Assertion failed: (error_msg.find("trigger_internal_bug") != std::string::npos), function rpc_fuzz_target, file rpc.cpp, line 389.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/rpc/9cfc6f75197c087f55eb7ea406ef812047989f2b"
⚠️ Failure generated from target with exit code 1: ['/Users/runner/work/bitcoin/bitcoin/ci
...
(https://github.com/bitcoin/bitcoin/pull/32514#issuecomment-2884064294)
```bash
Run rpc with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/bin/fuzz', PosixPath('/
Assertion failed: (error_msg.find("trigger_internal_bug") != std::string::npos), function rpc_fuzz_target, file rpc.cpp, line 389.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/rpc/9cfc6f75197c087f55eb7ea406ef812047989f2b"
⚠️ Failure generated from target with exit code 1: ['/Users/runner/work/bitcoin/bitcoin/ci
...
💬 fanquake commented on pull request "depends: bump to latest config.guess and config.sub":
(https://github.com/bitcoin/bitcoin/pull/32505#issuecomment-2884067533)
Guix Build
```bash
c44d180153f6844a636a0693f588fa20c45c33efd31194a9f4d02858f5eaaaf7 guix-build-486bc9179072/output/aarch64-linux-gnu/SHA256SUMS.part
1beb0041c2d22378094d38e53878a2806d5d76557ef4f9dd2f40287e66388647 guix-build-486bc9179072/output/aarch64-linux-gnu/bitcoin-486bc9179072-aarch64-linux-gnu-debug.tar.gz
d824bbf3f03bf7679f0721173dbe2387b9168ea0f0cc24f9f676af85e5527c9b guix-build-486bc9179072/output/aarch64-linux-gnu/bitcoin-486bc9179072-aarch64-linux-gnu.tar.gz
b0ddcb7739e44760b
...
(https://github.com/bitcoin/bitcoin/pull/32505#issuecomment-2884067533)
Guix Build
```bash
c44d180153f6844a636a0693f588fa20c45c33efd31194a9f4d02858f5eaaaf7 guix-build-486bc9179072/output/aarch64-linux-gnu/SHA256SUMS.part
1beb0041c2d22378094d38e53878a2806d5d76557ef4f9dd2f40287e66388647 guix-build-486bc9179072/output/aarch64-linux-gnu/bitcoin-486bc9179072-aarch64-linux-gnu-debug.tar.gz
d824bbf3f03bf7679f0721173dbe2387b9168ea0f0cc24f9f676af85e5527c9b guix-build-486bc9179072/output/aarch64-linux-gnu/bitcoin-486bc9179072-aarch64-linux-gnu.tar.gz
b0ddcb7739e44760b
...
💬 maflcko commented on pull request "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now":
(https://github.com/bitcoin/bitcoin/pull/32507#issuecomment-2884068477)
> What if instead of excluding the entire test file, we only exclude the specific failing test case?
Your diff would exclude the test case for all CI runs (even CI runs not using valgrind). I think longer term it would be better to just fix it.
(https://github.com/bitcoin/bitcoin/pull/32507#issuecomment-2884068477)
> What if instead of excluding the entire test file, we only exclude the specific failing test case?
Your diff would exclude the test case for all CI runs (even CI runs not using valgrind). I think longer term it would be better to just fix it.
🤔 stickies-v reviewed a pull request: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2844053598)
Concept ACK for reducing suppressions.
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2844053598)
Concept ACK for reducing suppressions.
💬 stickies-v commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091385844)
Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2091385844)
Doesn't look like a regression in this PR because the conversion was happening implicitly anyway, but, isn't this a rather dangerous thing to do? How do we know `a` is never going to be negative?
👍 hebasto approved a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2844036012)
ACK 2549a1075521202ee0d9fe069ba1062563ee6387, tested locally on Windows 11. Additionally tested in my personal repo with invalidated vcpkg binary cache.
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-2844036012)
ACK 2549a1075521202ee0d9fe069ba1062563ee6387, tested locally on Windows 11. Additionally tested in my personal repo with invalidated vcpkg binary cache.
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091377191)
Why use an exception? Wouldn’t exiting with an error code suffice?
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091377191)
Why use an exception? Wouldn’t exiting with an error code suffice?
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091379846)
GHA [docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#example-of-adding-a-system-path) do not suggest `-Encoding utf8`. Is it necessary?
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091379846)
GHA [docs](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#example-of-adding-a-system-path) do not suggest `-Encoding utf8`. Is it necessary?