👍 stickies-v approved a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796#pullrequestreview-2282671101)
ACK faecca9a85c1c1917d024f55cca34d19cc94f3b9

Commit messages were very helpful, thank you.

💬 stickies-v commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1745310574)
nit: I think the code is more clear without this helper variable, avoids the first multiplying and then dividing by 8 again:

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

```diff
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index da6ff77924..fd7a1f2ba2 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -49,11 +49,10 @@ static CService ResolveService(const std::string& ip, uint16_t port = 0)

static std::vector<bool> FromBytes
...

👍 hebasto approved a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796#pullrequestreview-2282716760)
ACK faecca9a85c1c1917d024f55cca34d19cc94f3b9, I have reviewed the code and the generated headers.

💬 naiyoma commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1745339409)
nit : not sure about this but I think it would be useful to check that when -noconnect/connect=0 is set no outbound connections are made.

💬 maflcko commented on pull request "test: Use std::span and std::string_view for raw data":
(https://github.com/bitcoin/bitcoin/pull/30796#discussion_r1745354177)
Will push if I have to re-touch.

💬 TheCharlatan commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2331380598)
> Any particular commit that's problematic? I could try splitting it.

I don't think so. The logic in the changed lines is relatively easy to understand, but it takes a bit of time to check where the notifications are issued and what the differences between the two might be. I don't think dividing anything here might help with that.

🚀 fanquake merged a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796)

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1745457490)
good point :+1:

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745462272)
Maybe in a follow-up, given that the existing linter doesn't warn either?

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745464467)
> NACK

Ok, moved the tests to a unit test file. I hope this is better.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745472652)
> BOOST_CHECK_THROW

I am not using `BOOST_CHECK_THROW`, so I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745475010)
> We could change this to a `constexpr`, leaving the constructor as `consteval`, keeping the usage failures during runtime and be able to properly test it via positive and negative cases.

Ok, done

📝 glozow opened a pull request: "TxDownloadManager refactor followups"
(https://github.com/bitcoin/bitcoin/pull/30820)
Followups to #30110
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1742665719 + a few more doc fixes
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1742884245

💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1745491541)
Could rebase and use the existing `generate_header_from_raw`, which should do the same thing, basically?

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2331616132)
Thanks for the reviews 🙏

I've created a followups PR #30820

After that will be the second part of #28031 ("try multiple peers for orphan resolution"). I'll open a separate PR, since it has 140 comments. After that, I plan to open a PR that moves `m_tx_download_mutex` inside `TxDownloadManagerImpl` (i.e. make it internally thread-safe), which I mention in the PR description here. I had considered making it the next step, but I think waiting a little more is better because
(1) It gives th
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745515725)
> and the tests run so fast that in the case where they are redundant, no one should notice.

Absolutely, the cleanup is to avoid having tests that have unclear purpose, making them easier to maintain going forward. Performance is not the goal here.

I think it's impossible for this PR to not have significant changes to the unit tests, so I feel like since the review is happening anyway the test improvements I've introduced here make sense and add little overhead?

That said, I'm unsure if
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745505000)
the errors are really ugly this way:
> util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~

Would there be any disadvantage in inlining them?
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
{
ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("");
ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%");
...
ConstevalFormatString<1>::Detail_CheckNumForm
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745513285)
is this a leftover?

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745500013)
How come? Wouldn't that be a simpler way to check that we're throwing in this case (instead of the constant err and err_types which cannot even change)
```C++
BOOST_CHECK_THROW(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), const char*);
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745509491)
As mentioned in another comment, wouldn't this be simpler:
```C++
BOOST_CHECK_THROW(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), const char*);
BOOST_CHECK_THROW(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%_"), const char*);
BOOST_CHECK_THROW(ConstevalFormatString<2>::Detail_CheckNumFormatSpecifiers("%_"), const char*);
```
?