💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2331277841)
Paging some people who touched this code in recent years... well, decades: @sipa, @theuni

👍 TheCharlatan approved a pull request: "lint: Check for release note snippets in the wrong folder"
(https://github.com/bitcoin/bitcoin/pull/30812#pullrequestreview-2282652541)
Re-ACK fa3a7ebe5b5db63e4cb4fb6974c028cc7af0b898

Just updated docstring.

💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1745300563)
> think you meant "git pathspec" instead of "shell wildcard patterns"

I did indeed confuse the two in this case, thanks for clarifying.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2331303834)
> the change in logic is a bit tricky to follow.

Any particular commit that's problematic? I could try splitting it.

💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#issuecomment-2331311275)
ACK fa3a7ebe5b5db63e4cb4fb6974c028cc7af0b898

Verified with two files, I like this new version more.

💬 naiyoma commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#issuecomment-2331326030)
Tested ACK [https://github.com/bitcoin/bitcoin/pull/30529/commits/0e98e5b908dfc7cc8f6b4d2dca4d67b7fd8b5f7c](https://github.com/bitcoin/bitcoin/pull/30529/commits/0e98e5b908dfc7cc8f6b4d2dca4d67b7fd8b5f7c)
Ran the functional test locally and test passes successfully, checks -noconnect and -connect=0 disables `-listen` and` -dnsseed`.

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