💬 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

📝 davidgumberg opened a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606)
Processing unsolicited CMPCTBLOCK's from a peer that has not been marked high bandwidth is not well-specified behavior in BIP-0152, in fact the BIP seems to imply that it is not permitted:

> "[...] method is not useful for compact blocks because `cmpctblock` blocks can be sent unsolicitedly in high-bandwidth mode"

See https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#separate-version-for-segregated-witness

This partially mitigates a mempool leak described in [#28272](#28272
...

💬 davidgumberg commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2905911412)
Opened #32606 to drop unsolicited CMPCTBLOCK messages.

💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2105479437)
I'll delete this on next rebase.

💬 jonatack commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-2905928927)
Concept ACK. BIP152 is marked as Final but perhaps could be clarified on this point if this moves forward.

💬 davidgumberg commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#issuecomment-2905941970)
ACK https://github.com/bitcoin/bitcoin/pull/32404/commits/53e9b71b2fd59c18b75e45e3a24c39182c20a59e

💬 davidgumberg commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105490756)
non-blocking nit: `uint8_t` here seems unnecessary

📝 benthecarman opened a pull request: "rpc: Note in fundrawtransaction doc, fee rate is for package"
(https://github.com/bitcoin/bitcoin/pull/32607)
Accidentally made some transactions with a much higher fee rate than I wanted because I did not know this would do it for the package rather than the individual tx.