💬 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
...
💬 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.