💬 maflcko commented on issue "Test Framework - test_framework.test_node.FailedToStartError: No RPC credentials":
(https://github.com/bitcoin/bitcoin/issues/30818#issuecomment-2331206166)
I think it would be fine to add support for functional tests in WSL (or document how to do it), but I am not sure how easy it is.

I presume adding support for tests directly on Windows is easier (or adding documentation on how to do it). However, I don't use WSL, nor Windows, so I can't tell for sure.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1745243016)
4ef4eee3b7806ad3710c12cbf4e305a814ba8b40: this commit still uses `GetRandMillis`

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1745244511)
Oh nvm, the fixup! commit isn't squashed yet.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2331219145)
@laanwj is there anything you still want to change here, or should we review it as is first?

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

🤔 mzumsande reviewed a pull request: "Halt processing of unrequested transactions v2"
(https://github.com/bitcoin/bitcoin/pull/30572#pullrequestreview-2282602457)
> Beyond, I maintain that you're naively adding AlreadyHaveTx() on this current branch, you would add a bandwidth free relay vector, as peer1 and peer2 can collude to send a unique child towards the same node (the parent not existing at all for any other node), and have the GETDATA for the parent duplicated by the peer.

I don't get this at all.
The suggestion above was, if you receive an unrequested TX, and you already have it, ignore it but don't disconnect the peer. Which is the same as c
...

💬 hebasto commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2331243915)
Apparently, this PR made this test dependent on the Python executable, which should be reflected in the build system, I guess.

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2331255069)
The tests already depend on Python and `ctest` can already not be run without it. See also `doc/dependencies.md`:

```
| [Python](https://www.python.org) (scripts, tests) | [3.9](https://github.com/bitcoin/bitcoin/pull/28211) |
```

If you find an alternative command, maybe based on GNU Coreutils that achieves the same, feel free to replace it.

💬 gruve-p commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2331262329)
Needs backport to 28.x branch

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