📝 m3dwards opened a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513)
We only need to find MSBuild.exe for the job to function and can do that ourselves using vswhere.exe and so can remove the dependency on ilammy/msvc-dev-cmd.
Fixes: #32508
(https://github.com/bitcoin/bitcoin/pull/32513)
We only need to find MSBuild.exe for the job to function and can do that ourselves using vswhere.exe and so can remove the dependency on ilammy/msvc-dev-cmd.
Fixes: #32508
💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2091196979)
Dropped it again, but I also changed things slightly to get the block hash from the block index now.
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2091196979)
Dropped it again, but I also changed things slightly to get the block hash from the block index now.
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2883865539)
Concept ACK.
Thank you for taking this!
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2883865539)
Concept ACK.
Thank you for taking this!
💬 nervana21 commented on pull request "doc: Add missing top-level description to pruneblockchain RPC":
(https://github.com/bitcoin/bitcoin/pull/32333#discussion_r2091213444)
Updated, thanks!
(https://github.com/bitcoin/bitcoin/pull/32333#discussion_r2091213444)
Updated, thanks!
💬 maflcko commented on pull request "doc: Add missing top-level description to pruneblockchain RPC":
(https://github.com/bitcoin/bitcoin/pull/32333#issuecomment-2883881160)
lgtm ACK 135a0f0aa711b95c50aa4cbe0c38d82d647f1c8b
(https://github.com/bitcoin/bitcoin/pull/32333#issuecomment-2883881160)
lgtm ACK 135a0f0aa711b95c50aa4cbe0c38d82d647f1c8b
📝 maflcko opened a pull request: " scripted-diff: Remove unused leading newline in RPC docs "
(https://github.com/bitcoin/bitcoin/pull/32514)
It is harmless, but newlines in the beginning read a bit odd ("nReturns"). So just require them to not be present.
The diff is large, but a trivial scripted-diff.
(https://github.com/bitcoin/bitcoin/pull/32514)
It is harmless, but newlines in the beginning read a bit odd ("nReturns"). So just require them to not be present.
The diff is large, but a trivial scripted-diff.
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090527952)
Shouldn't this be `assert False`? Here the expectation is that the `recv()` will throw an exception due to timeout.
https://docs.python.org/3/library/socket.html#socket.socket.recv
> A returned empty bytes object indicates that the client has disconnected.
An "empty bytes object" will not trigger the assert `assert not res` but if that happens (= disconnect) then the test should fail.
suggestion:
```diff
- assert not res
+ assert False
`
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090527952)
Shouldn't this be `assert False`? Here the expectation is that the `recv()` will throw an exception due to timeout.
https://docs.python.org/3/library/socket.html#socket.socket.recv
> A returned empty bytes object indicates that the client has disconnected.
An "empty bytes object" will not trigger the assert `assert not res` but if that happens (= disconnect) then the test should fail.
suggestion:
```diff
- assert not res
+ assert False
`
💬 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
...