💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128424406)
The previous docstring is misleading, `LogPrintLevel()` can also be used for unconditional logging when a `Level::Info` is passed as level, as is e.g. done in `mapport.cpp` and in a few other places. I don't think this is a robust way of deciding on rate limiting.

I think I would prefer either:
1. using `LogPrintLevel_` directly in `UpdateTipLog()`, with a comment on why we're using a private function and not enabling rate limiting here. A `LogInfoAlways` macro could be an alternative.
2. r
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128425813)
I've left some comments for further encapsulation, but yeah I think this a step in the right direction, thanks!

💬 maflcko commented on issue "ci: libFuzzer disabled leak detection after every mutation":
(https://github.com/bitcoin/bitcoin/issues/32681#issuecomment-2943527766)
See https://github.com/bitcoin/bitcoin/pull/31481

💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128468936)
Taken, thank you.

📝 Sjors opened a pull request: "wallet: have external signer use PSBT error code EXTERNAL_SIGNER_NOT_FOUND"
(https://github.com/bitcoin/bitcoin/pull/32682)
When attempting to sign a transaction involving an external signer, if the device is connected we throw an `std::runtime_error`. This prevents the (mainly GUI) code that's actually supposed to handle this case from running.

This PR returns an `PSBTError::EXTERNAL_SIGNER_NOT_FOUND` instead of throwing.

The first commit is a refactor to have `GetExternalSigner()` return a `util::Result<ExternalSigner>` so the caller can decide how to handle the error. There are two other places where call `G
...

💬 Sjors commented on issue "external signer: PSBT error code `EXTERNAL_SIGNER_NOT_FOUND` is never returned":
(https://github.com/bitcoin/bitcoin/issues/32426#issuecomment-2943569685)
Fixed in #32682. It has the nice benefit of actually using the existing translations for "External signer not found".

💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128483548)
@hodlinator agreed and taken

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128488386)
This logic should be encapsulated in `ResetWindows` imo, so probably this statement could just be:

```suggestion
m_limiter.MaybeResetWindow();
```

💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128518144)
It was renamed to prevent a clash with the now imported platform package. I have reworked the variables to be more clear.

💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128520456)
Taken and applied everywhere.

🤔 BrandonOdiwuor reviewed a pull request: "test: wallet: cover wallet passphrase with a null char"
(https://github.com/bitcoin/bitcoin/pull/32675#pullrequestreview-2899781298)
Code Review ACK 7cfbb8575e1ffbad5c48e2c461b45dd6ac63d064

🤔 janb84 reviewed a pull request: "ci: Rewrite test-each-commit as rust script"
(https://github.com/bitcoin/bitcoin/pull/32680#pullrequestreview-2899822942)
Concept ACK

What do you think about the idea to use cargo for this (maybe not now because it's still in 'unstable) ?

💬 janb84 commented on pull request "ci: Rewrite test-each-commit as rust script":
(https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2128559420)
```suggestion
git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && cargo -Zscript ./.github/ci-test-each-commit-exec.rs && git reset --hard" ${{ env.TEST_BASE }}
```

This removes the extra execution step if cargo is used. Do note that this is stil in "unstable features" .

💬 janb84 commented on pull request "ci: Rewrite test-each-commit as rust script":
(https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2128554692)
```suggestion
---cargo
[package]
edition = "2024"
---
// Copyright (c) The Bitcoin Core developers
```

nit / idea, add embedded manifest for direct cargo execution

💬 marcofleon commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2943783645)
Concept ACK

I've differentially [fuzzed](https://github.com/marcofleon/bitcoin/blob/084d430dcb87e708e0fc9a65dc3d0c4d1f468f85/src/test/fuzz/block_index_diff.cpp) `BlockTreeDB` and `BlockTreeStore` for ~5000 cpu hours so far and no issues. Happy to continue testing (differentially fuzzing or otherwise) once the final approach is implemented.

💬 maflcko commented on pull request "ci: Rewrite test-each-commit as rust script":
(https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2128644660)
Sure, happy to review a pull doing that once it is stable

💬 maflcko commented on pull request "ci: Rewrite test-each-commit as rust script":
(https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2128646681)
I think you disabled warnings. Otherwise: https://github.com/bitcoin/bitcoin/pull/32680#discussion_r2128644660

🤔 maflcko reviewed a pull request: "[WIP] rpc: add `clearmempool` command for regtest mode"
(https://github.com/bitcoin/bitcoin/pull/32418#pullrequestreview-2900025631)
I am mostly thinking that this is a test-only regtest-only feature, and the mentioned alternatives are ways to achieve the same already today on vanilla Bitcoin Core

💬 maflcko commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#discussion_r2128677281)
this is the wrong order

💬 maflcko commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#discussion_r2128677493)
node0 stderr Assertion failed: detected inconsistent lock order for '::cs_main' in rpc/mempool.cpp:1150 (in thread 'httpworker.3'), details in debug log.