💬 mzumsande commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1815521913)
This log isn't accurate if `keep_alive` is set, because then we'll log that we close the connection but actually stay connected.

💬 achow101 commented on pull request "doc: replace `-?` with `-h` and `-help`":
(https://github.com/bitcoin/bitcoin/pull/31118#issuecomment-2436408742)
ACK 33a28e252a7349c0aa284005aee97873b965fcfe

🤔 hodlinator reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2393651994)
Concept ACK a1c9d23ff587b84d37175b2993ea6760f9762177

Good to fix weird negation corner cases.

(Very tempting to slap `DISALLOW_NEGATION` on non-list args for some reason, but I guess someone could be using `norpcwallet` to reset earlier values instead of `rpcwallet=`).

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815689215)
Better to explain *why* than *what* (log message already explains *what*). To me it's not immediately obvious that we should avoid using DNS seeds when having a Tor SOCKS5 proxy?

Furthermore, as I understand it we don't really ignore `-dnsseed` when having a proxy; we use them differently, making the log message not correct to begin with?:
https://github.com/bitcoin/bitcoin/blob/a1c9d23ff587b84d37175b2993ea6760f9762177/src/net.cpp#L2284-L2288

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815722428)
More useful comment IMO:
```suggestion
// Populate outgoing connections unless negated or disabled.
if (connect.size() != 1 || connect[0] != "0") {
connOptions.m_specified_outgoing = connect;
}
```

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815672325)
nit: `LogPrintf`-string provides enough info that I find this added comment redundant.

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815740964)
nit: Would move this line resetting the config after the `for`-loop to clarify that it does not affect it (though I understand the urge to keep it close to the original replace).

(Verified locally that test still succeeds with the move just to be sure).
```suggestion
for msg in [dnsseed_disabled, listen_disabled]:
if msg not in info.log:
raise AssertionError(f"Expected {msg!r} in -noconnect log message")
self.nodes[0].replace_in
...

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1815702002)
Use local variable.
```suggestion
onlynets.empty() ||
```

💬 achow101 commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2436410526)
ACK 9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5

🚀 achow101 merged a pull request: "doc: replace `-?` with `-h` and `-help`"
(https://github.com/bitcoin/bitcoin/pull/31118)

✅ achow101 closed an issue: "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it"
(https://github.com/bitcoin/bitcoin/issues/30390)

🚀 achow101 merged a pull request: "util: Remove RandAddSeedPerfmon"
(https://github.com/bitcoin/bitcoin/pull/31124)

💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2436439210)
Turned off hourly CI for now, since fix removing the offending function was merged into master.

Intend to experiment with more surgical solutions and document that here in the future.

💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1815773346)
nit: perhaps the comment on line 144 can be more descriptive for this part. (or the comments in general can be improved). It's the strangest test in the group.

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2436507075)
> > > Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
> > > https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
> > > Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test
...

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815804685)
I think the @cleanup function when moving the test to `p2p_orphan_handling.py` in [3bc3bcc](https://github.com/bitcoin/bitcoin/pull/31040/commits/3bc3bcce546581f450748b8106dc36dfc3e71b26) should be handling this now

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815804921)
This is now removed after moving to `p2p_orphan_handling.py` in [3bc3bcc](https://github.com/bitcoin/bitcoin/pull/31040/commits/3bc3bcce546581f450748b8106dc36dfc3e71b26)

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815805677)
I've just defined it at the top of the test file should that be fine?

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815805874)
updated in [f2b920c](https://github.com/bitcoin/bitcoin/pull/31040/commits/f2b920c31cc1254c510dcb9cdc14b73d50f800a2)

💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815813480)
Thanks! I moved it more towards the start of the function in [34c6643](https://github.com/bitcoin/bitcoin/pull/31139/commits/34c6643c2fe0e73d73edb2ddddf0900e4256de57)