💬 maflcko commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1745224882)
> Maybe, in that case we can use shell wildcard patterns with `ls-files`:

I think you meant "git pathspec" instead of "shell wildcard patterns", but I like the suggestion, because it let's `git` do everything without requiring any additional code or external dependency.

💬 maflcko commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1745225587)
Added *two* linebreaks

💬 maflcko commented on pull request "build: Unify `-logsourcelocations` format":
(https://github.com/bitcoin/bitcoin/pull/30811#discussion_r1745232776)
Ah, thanks for testing. I guess this is a possible cmake regression, but I haven't ever used different dirs with autotools, so I can't say for sure.

Seems fine to fix here, or in a follow-up, as this pull request is an improvement already and this bug already exists in current `master`.

👍 theStack approved a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2278788723)
Light review ACK 1d4e33e7f88162cf6fcd6aee0b63973015853591

Reviewed the refactoring changes and the unit tests thouroughly, but didn't look ran or look in-depth at the fuzz tests.

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1742884245)
in commit c40cb8565952f62732fa64875c99ef9082b9a6f4: now that the if-body above only reads and writes from local variables (`vInv` and `tx_invs`), acquiring the `m_tx_download_mutex` lock before the for-loop (line 5140 at this commit) is not needed anymore

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