💬 gmaxwell commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2905537657)
I don't recall if the vulnerability related to requesting the transaction was closed (relay pool limited its scope at least to recently announced txn) but there were complications related to orphan txn (e.g. that relaying a transaction ought to imply permission to fetch its parents if they are still in your memory pool). But if that vulnerablity hasn't been closed it ought to be closed.

Ignoring the unrequested CB is desirable for non-privacy reasons, specifically because if it doesn't someone
...

💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2105273746)
Yes, there is no index available, only the desired hash, wherever it comes from.
Would it help if we included the two different hashes in the error?

💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105277605)
Good call, changed, rebased, added you as co-author.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105280293)
Ah. I've added a test which exercises this code.

💬 achow101 commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#issuecomment-2905565617)
ACK e5cbea416b2f63e5d99819052f3e69a6383336d6

💬 ryanofsky commented on issue "subprocess: `sh -c 'echo err 1>&2 && false'` must return `1`":
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2905570392)
> The underlying issue is not specific to illumos.

I couldn't find an explanation of the underlying issue, but looking at this more, I think I maybe understand the problem. The test code that is failing looks like:

```c++
49 // Return non-zero exit code, with error message for stderr
50 const std::string command{"sh -c 'echo err 1>&2 && false'"};
51 const std::string expected{"err"};
52 BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std:
...

🚀 achow101 merged a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596)

💬 ryanofsky commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2105315243)
> [#32577 (comment)](https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2902382891):
>
> > * I don't think the added quotes in external_signer.cpp are doing anything, or it isn't clear what they are trying to do.
>
> Descriptors contain the `(` and `)` characters, which may cause an issue with the shell:
>
> ```
> $ echo pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798)
> bash: syntax error near unexpected token `('
> ```

I see. I think this will proba
...

💬 ryanofsky commented on pull request "subprocess: Let shell parse command on non-Windows systems":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2105352322)
> it would be best to change signature of RunCommandParseJSON to take a vector<string> argument instead of a std::string to avoid the need for any splitting

Another advantage of this approach is that we should be able to avoid needing to change subprocess library at all, all the changes could be internal

💬 waketraindev commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2905749892)
Concept ACK. Tested on top of current master, no issues observed. Looks good to me.

🤔 TheCharlatan reviewed a pull request: "index: Fix coinstats overflow and introduce index versioning"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-2865638569)
Approach ACK

This all looks good to me, but I am not sure if embedding the database in our functional test suite is a good idea. I don't think it is too much data, but I am not sure how stable it is in terms of file formats. Is this really compatible with all posix systems? Then again it could just be removed once/if it starts becoming unreliable.

💬 TheCharlatan commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2105373992)
Nit: I think this message should be a little clearer. How about: "Migrating coinstatsindex, current block height %s".

💬 TheBlueMatt commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2905778748)
Yea, I'm not really sure if twas entirely deliberate or not, but they'd presumably have to be the first node to relay you a block (so at least its not entirely trivial, as IIRC it has to build on the tip, or if it doesn't it should!), and, indeed, I believe FIBRE took advantage of it (tho not very effectively due to lack of parallel compact block reconstruction in Bitcoin Core at the time). Now that we do have parallel compact block reconstruction, I wonder if we shouldn't disable this while at
...

📝 Crypt-iQ opened a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604)
This revives the work done by @dergoegge in https://github.com/bitcoin/bitcoin/pull/21603. The approach is largely the same except that `std::source_location` is used under-the-hood now that we can use c++20 features. The logging functions have also changed slightly since that PR was opened, so work has been done to preserve the intent of the original rate limiting change. I have tried to give commit attribution where possible (unfortunately I was not able to figure out how to type ö in vim).

...

💬 Cyber-Lord commented on issue "intermittent issue in feature_init.py (bitcoind should have exited with expected error LevelDB error: Corruption)":
(https://github.com/bitcoin/bitcoin/issues/32600#issuecomment-2905784667)
I've attempted reproducing this issue locally using "test/functional/feature_init.py". After deleting some LevelDB .ldb files manually, I was able to trigger the failure. Instead of the expected "Error opening coins database.", the node exited with "Error opening block database.", confirming that the error message varies depending on which part of the LevelDB data is corrupted.

I am thinking of submitting a patch to make the test more robust by allowing multiple expected error messages (e.g., b
...

👍 TheCharlatan approved a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2865691734)
ACK 8117c16fd7a0a5334fe63efaff94ea4e1d3cf851

I guess checking the hash could also protect against some weird file corruption, where a different block occupies the same position? Could this happen if e.g. the files are renamed?

💬 davidgumberg commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2905800955)
Concept ACK

🤔 furszy reviewed a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2865753098)
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0

💬 gmaxwell commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-2905866738)
Any thoughts about adding some kind of dying gasp so that if a node crashes or hits some fatal error the unfiltered log can be saved?

🤔 janb84 reviewed a pull request: "wallet, rpc: Return normalized descriptor in parent_descs"
(https://github.com/bitcoin/bitcoin/pull/32594#pullrequestreview-2865782128)
re ACK a759a22d59e805834d077a28c95695e4834983a9

Changes since last ACK:
- small typo change in commit