💬 hebasto commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#issuecomment-2326652638)
Concept ACK.

⚠️ maflcko opened an issue: "intermittent issue in wallet_keypool.py: assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0) AssertionError: not(1725366101 == 0)"
(https://github.com/bitcoin/bitcoin/issues/30797)
https://cirrus-ci.com/task/5492198311985152?logs=ci#L4100


```
node0 2024-09-03T12:21:40.411672Z [httpworker.0] [src/rpc/server.cpp:586] [RPCRunLater] [rpc] queue run of timer lockwallet(default_wallet) in 1 seconds (using HTTP)
node0 2024-09-03T12:21:40.416379Z [http] [src/httpserver.cpp:304] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:34622
node0 2024-09-03T12:21:40.416429Z [httpworker.1] [src/rpc/request.cpp:232] [parse] [rpc] ThreadRPCServer method=keypo
...

💬 maflcko commented on pull request "kernel: Use spans instead of vectors for passing block headers to validation functions":
(https://github.com/bitcoin/bitcoin/pull/30742#issuecomment-2326683029)
CI failure looks unrelated and can be ignored: https://github.com/bitcoin/bitcoin/issues/30797

💬 maflcko commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326696273)
> call `procdump -ma <PID>` on the `bitcoind.exe` process and hopefully figure out where the process is stalling?

Is it clear that the issue is Bitcoin Core? IIRC it may also be an issue in the Python subprocess module, no?

💬 maflcko commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326700782)
Seems fine to integrate in a pull request and then keep the pull request unmerged, and only merge the pull request in the GHA CI, as that is the only place where it reproduces. Adding temporary test test-only code for everyone else may be a bit too much.

💬 maflcko commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#issuecomment-2326707221)
On my system the test_bitcoin and bench_bitcoin binaries are smaller by 30kB, and 4kB respectively. But that isn't the goal of this pull, just a nice side-effect.

💬 fjahr commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#issuecomment-2326717439)
Concept ACK

💬 hodlinator commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326719252)
Alright, I can live with having it a pending PR until it has proven itself useful on CI. :)

Will see if I can get a Windows/Wine environment going locally and study how to integrate it into GHA.

🤔 glozow reviewed a pull request: "test: add check that too large txs aren't put into orphanage"
(https://github.com/bitcoin/bitcoin/pull/30784#pullrequestreview-2277680444)
Concept ACK, but this should be a unit test, not a functional test looking for logs. We could accidentally delete the `return false;` line but keep the `LogPrint` line, and this test would pass.

> so this is only relevant if tx standardness rules are disabled via -acceptnonstdtxns=1.

Similarly, it's a bit icky to have to disable standardness checks to test this.

A unit test in orphanage_tests.cpp could directly create a large tx and call `AddTx`. With #30110, we can also test by callin
...

✅ tdb3 closed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793)

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2326728575)
Thought more about this and other than increased visibility and monitoring of the orphanage over time (e.g. for statistical purposes), I'm not seeing a ton of compelling reasons to add. Will close for now.

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1738970588)
```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be string of %d hex digits (not '%s' with len %s)", name, uint256::size() * 2, v.get_str(), len));
```

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1742206994)
Any reason to omit the length from the error message when it was previously present?

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1742221623)
we're already displaying the value itself, isn't that enough?

🤔 stickies-v reviewed a pull request: "test: increase FromUserHex FUZZ and unit testing coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2277400199)
Approach ACK 50defb82c5c52f9cca00de53f76064c376220329

Being able to compare `std::optional` directly in `BOOST_CHECK_*` macros is very helpful, thank you for picking this up. The added fuzz coverage seems helpful too but I'm lacking fuzzing experience to comment in detail here.

A couple of suggested improvements besides the comments left:
1. The PR description would benefit from a bit more context/rationale on what this PR does and why it is helpful (`TrySanitizeHexNumber` is also no long
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742051446)
nit: can be onelined to `return os << v ? *v : "std::nullopt";`:

<details>
<summary>git diff on e0a96acc82</summary>

```diff
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index cea1481c88..22597f16c7 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -253,11 +253,7 @@ inline std::ostream& operator<<(typename std::enable_if<std::is_enum<T>::value,
template <typename T>
inline std::ostream& operator<<(std::ostream& os, const s
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742038234)
This can be cleaned up in C++20:

<details>
<summary>git diff on e0a96acc82</summary>

```diff
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index cea1481c88..0a65726395 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -243,11 +243,10 @@ CBlock getBlock13b8a();

// Make types usable in BOOST_CHECK_* {
namespace std {
-// Enum class types
-template <typename T>
-inline std::ostream& operator<<(typename std::enable_if<std
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742058922)
nit: I think the `@{` notation is more common here?

<details>
<summary>git diff on e0a96acc82</summary>

```diff
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index cea1481c88..96c3b5caac 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -241,7 +241,8 @@ std::unique_ptr<T> MakeNoLogFileContext(const ChainType chain_type = ChainType::

CBlock getBlock13b8a();

-// Make types usable in BOOST_CHECK_* {
+// Make types usable
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742151887)
I think it's best to keep the `const` qualifier? (and less importantly, also [here](https://github.com/bitcoin/bitcoin/pull/30618/files#diff-dbada1fe3a3d0af884304dd28be8c9df74b592401dec2c6400f6b491aefe6c9bR811) and [here](https://github.com/bitcoin/bitcoin/pull/30618/files#diff-dbada1fe3a3d0af884304dd28be8c9df74b592401dec2c6400f6b491aefe6c9bR822))

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742119463)
What's the rationale behind these include updates? According to `iwyu` this include is still necessary. More generally, I think it's most productive to either fix all the includes, or the ones that are affected by the PR, but not a (seemingly random) subset?