💬 hodlinator commented on pull request "fees: Remove CLIENT_VERSION serialization":
(https://github.com/bitcoin/bitcoin/pull/29702#discussion_r1811355454)
Yes, in case it somehow gets corrupted or modified by an entirely different software, that makes sense.

Appreciated if you re-touch.

💬 hodlinator commented on issue "Windows Python: Intermittent issue in p2p_unrequested_blocks.py", line 98, in run_test test_node.send_and_ping(msg_block(blocks_h2[0])) assert self.is_connected AssertionError / AcceptBlock FAILED (time-too-old, block's timestamp is too early) ":
(https://github.com/bitcoin/bitcoin/issues/29897#issuecomment-2430401806)
Encountered here: https://github.com/hodlinator/bitcoin/actions/runs/11460619753/job/31887736863#step:12:590

(Still running on a base of 84cd6478c422bc296589ab031f5c76e7bab2704d from 16 September though).

💬 theStack commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2430509479)
Concept ACK

👍 tdb3 approved a pull request: "test: extend the SOCKS5 Python proxy to actually connect to a destination"
(https://github.com/bitcoin/bitcoin/pull/29420#pullrequestreview-2380545052)
CR and test ACK 57529ac4dbb2721c1ad0a3566f0299dbdb5ca5c0

Checked with a hackie little sanity test where a connection was made to the proxy on 127.0.0.1:10000 (set up to detour to 127.0.0.1:10001). A small echo server listened on 10001 (echoing back "testack"). The new detouring functionality of the proxy seemed to work as expected.

<details>
<summary>hackie test</summary>

```diff
diff --git a/test/functional/feature_framework_unit_tests.py b/test/functional/feature_framework_unit_te
...

💬 tdb3 commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1807873344)
non-blocking femto nit: If this file is touched again, could add a trailing comma to prevent the line from being changed if another import from netutil is needed.

👍 rkrux approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2387132611)
tACK ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9

Successful make and functional tests. Pretty straightforward PR; I agree with @instagibbs's suggestion to keep the variable name in functional test same as in the CPP code.

👍 rkrux approved a 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#pullrequestreview-2387408321)
tACK bf46723537c37cb1ee7f84ffe7b75c253beadb89

Successful make and functional tests.

💬 rkrux 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_r1811959750)
Let's keep the same name in the functional test for uniformity and easy reference?
https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.h#L27

💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2431257072)
Worth bringing back the `try`/`catch` in *src/test/fuzz/strprintf.cpp* for Debug builds?
```
₿ cmake --preset=libfuzzer -DCMAKE_BUILD_TYPE=Debug
...
₿ cmake --build build_fuzz -j<X>
...
₿ FUZZ=str_printf build_fuzz/src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 734467281
INFO: Loaded 1 modules (1292656 inline 8-bit counters): 1292656 [0x5d5503ee60a0, 0x5d5504021a10),
INFO: Loaded 1 PC tables (1292656 P
...

💬 fanquake commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2431330789)
@real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.

> (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).

I would think the code would not be worse performance wise, as it's implemented using `__stosb`.

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2431441971)
Rebased.

💬 vasild commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2431475513)
I am in favor of 2. (port+1) or 5. (do nothing) in the short term, for 28.1.

For long term, maybe deprecate and remove `port=` (yes, I think it is a subset of `bind=`, somebody correct me if I am wrong) in favor of `bind=...:port`. I agree that `port=` is "helpful, especially for users less knowledgeable about networking", maybe to resolve that extend the syntax of `bind=` to allow `bind=*:8333`. That is used in at least Apache configs: `<VirtualHost *:80>`.

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2431486918)
Inputs for this are available at: https://github.com/brunoerg/qa-assets/pull/2

💬 brunoerg commented on issue "TestFramework: TestShell.reset() will always fail":
(https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2431516183)
Thanks for it! I think this is similar to #30714 (which fixed the initialization).

💬 naiyoma commented on issue "rpc: show P2(W)SH redeemScript in getrawtransaction (and friends)":
(https://github.com/bitcoin/bitcoin/issues/27391#issuecomment-2431610508)
Picking this up since all previous attempts are now closed.

:lock: fanquake locked an issue: "COIN_VALUE no longer accessible in const contexts"
(https://github.com/bitcoin/bitcoin/issues/31113)

💬 brunoerg commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2431705621)
Concept ACK

🤔 stickies-v reviewed a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2388250835)
Concept ACK

👍 laanwj approved a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2388406290)
Code review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0
Not the easiest refactor to review, carefully checked that there are no behavior differences, but i think the change absolutely worth it for the more readable API, and the removal of ambiguity.

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1812617677)
Yeah will break existing grepping scripts, but I think we will need to do this anyway to improve the logging for package replacements.

I think we should log the new package or new transaction replacement only once after logging all the replaced transactions:

1. First, log all replaced transactions with their ID, witness, fee, and size.
2. Then:
- For a single transaction replacement, log its ID, witness, fee, and size.
- For a package replacement, log that a new package has been
...