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

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742071054)
nit: missing `optional` and `ostream` includes

<details>
<summary>iwyu</summary>

```
/ci_container_base/src/test/util/setup_common.h should add these lines:
#include <stddef.h> // for size_t
#include <stdint.h> // for uint32_t
#include <exception> // for exception
#include <memory> // for make_unique, unique_ptr
#include <optional> // for optional
#include <ostream> // for ostrea
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742093199)
e0a96acc82c2c918572a2c9b90431db5bca19174: I don't really see the purpose of this ~move-only change and would rather this file can be dropped from the diff entirely (the include updates feel rather arbitrary so I'm okay with them being dropped too)

I don't think moving these functions is particularly helpful in the header either but can somewhat see the rationale since you're adding new code to it (still prefer it wouldn't move, though), but here it just feels like unnecessary churn?

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742149892)
nit: could use the consteval `uint256` hex string ctor here (+ below)

```suggestion
BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x1234"}).assumed_valid_block, uint256{"0000000000000000000000000000000000000000000000000000000000001234"});
```



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

```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 3914deb926..e9d9fee1fe 100644
--- a/src/test/validation
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742175853)
I'm not sure these 3 lines are worth adding, since they're already covered [higher up](https://github.com/bitcoin/bitcoin/pull/30618/files#diff-12dd31b274739b6a132d9c57e5ab14b403436991e4201cf8d3f3cc4fc7995066R29-R34), so this might unnecessarily slow down the test?

🤔 glozow reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2277710418)
Concept ACK

I once talked with @0xB10C about adding this, but never got around to it. I think it would be helpful for visualizing / trying out different orphan protection methods.

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742222919)
The orphanage can contain duplicates by txid, so adding wtxid as well could be helpful. This could be a `UniValue::VOBJ` instead, with both fields? That would allow adding more fields in the future (perhaps with different verbosity levels), e.g. telling us the size or originator.

💬 glozow commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742224059)
What's wrong with a `std::vector<CTransactionRef>`? Doesn't this need to make copies of the `CTransaction` objects?

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1742232560)
Previously we disambiguated between length errors and other errors. Since that's not longer true, I think not including the length in the error is the most elegant approach, especially since we're already showing the wrong value as @l0rinc says. Just feels like a useless addition to me.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742240161)
Yeah, I tried that before, but getting:
> error: incompatible operand types ('const value_type' (aka 'const uint256') and 'const char[13]')
return os << v ? *v : "std::nullopt";

⚠️ maflcko opened an issue: "intermittent issue in wallet_upgradewallet.py AssertionError: [node 2] Node returned unexpected exit code (1) vs (0) when stopping"
(https://github.com/bitcoin/bitcoin/issues/30798)
https://cirrus-ci.com/task/4629002713825280?logs=ci#L3296


```
test 2024-09-03T07:00:27.797000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-g
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742246864)
Sorry you're right, `return v ? os << *v : os << "std::nullopt";` should work though.