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

💬 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_r1815815759)
Ya that makes sense!

What I did in [34c6643](https://github.com/bitcoin/bitcoin/pull/31139/commits/34c6643c2fe0e73d73edb2ddddf0900e4256de57)

Is I used a proper hex value and then had the second value malformed so that we can cover more bases

let me know if that looks good to you I can modify it other ways aswell!

👍 sdaftuar approved a pull request: "functional test: Additional package evaluation coverage"
(https://github.com/bitcoin/bitcoin/pull/31152#pullrequestreview-2393878103)
Looks good, just some comment nits.

💬 sdaftuar commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815810879)
"belo"?

💬 sdaftuar commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815817176)
This comment isn't right, is it? We don't expect the package to be evicted?

💬 instagibbs commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815820994)
> it only plays with non-hex values and do not exercise the hex parsing.

an empty string is valid hex, it's just an invalid tx, no? If you want to test invalid hex that's another thing.

> if the first tx is invalid, the second one has no effect.

You need two transactions currently for it to get far enough to try and deserialize. I don't really care which one(s) are valid except for ease of reading.

> let me know if that looks good to you I can modify it other ways aswell!

imo havi
...