💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2430081120)
Suggestion for bitcoind code maintainers-- As I've mentioned, I believe this data corruption happens but is not detected until a few days later when bitcoind attempts to read an older block (for whatever reason, idk). It would be very handy if there were some utility or bitcoind RPC command or command line option that read and checked every file on the disk, or all the files containing data from the prior X days or X blocks. That would help users ensure that they have a known good copy/backup of
...

💬 sipa commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2430085267)
`gettxoutsetinfo` should read through all the chainstate LevelDB files.

💬 syaifulnizamiphone7 commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2430102161)
> The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
>
> ### Code Coverage
> For detailed information about the code coverage, see the [test coverage report](https://corecheck.dev/bitcoin/bitcoin/pulls/31053).
>
> ### Reviews
> See [the guideline](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review) for information on the review process.
>
> Type Reviewers
> Concept ACK [TheCharlatan](https://github.com/bi
...

⚠️ mzumsande opened an issue: "Remove BIP94 from regtest"
(https://github.com/bitcoin/bitcoin/issues/31137)
[BIP94](https://github.com/bitcoin/bips/blob/master/bip-0094.mediawiki) which, amongst others, fixes the timewarp attack on testnet4, has been activated on regtest by #30681 (commit https://github.com/bitcoin/bitcoin/pull/30681/commits/e85f386c4b157b7d1ac16aface9bd2c614e62b46), in order to allow having a functional test for the new testnet4 behavior.

As I argued in https://github.com/bitcoin/bitcoin/pull/30681#pullrequestreview-2275751672 I believe that regtest's main task is to test mainnet
...

💬 mzumsande commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2430181845)
Forgot about this until last week - I opened issue #31137 to continue discussion.

👋 jonatack's pull request is ready for review: "rpc, cli: return "verificationprogress" of 1 when up to date"
(https://github.com/bitcoin/bitcoin/pull/31135)

👍 hodlinator approved a pull request: "fees: Remove CLIENT_VERSION serialization"
(https://github.com/bitcoin/bitcoin/pull/29702#pullrequestreview-2386255699)
ACK fa8bd0be8432fda3c7312050433a6deb6722a073

Have a slight preference for the commit removing CLIENT_VERSION and the commit introducing CURRENT_FEES_FILE_VERSION being one and the same in order to give a more complete picture of the change.

Used hexdump to inspect old *~/.bitcoin/regtest/fee_estimates.dat* and *~/.bitcoin/fee_estimates.dat* files, confirming that the first two little-endian integers correspond to the *minimum required version* and *CLIENT_VERSION* (version that wrote).


...

💬 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