💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2389659427)
Even if the tx is already in the mempool `m_result_type` is never `ResultType::MEMPOOL_ENTRY` making it throw a `JSONTransactionError`.

This can be seen in the last test case in 5e4b8120af266026cfbc3ca29fc98ebb4fcf8f55.
This should not be the behavior, but I didn't find a solution yet.

💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2389659844)
This test case should be deleted as this is not the expected behavior, see https://github.com/bitcoin/bitcoin/pull/33507/files#r2389659427

💬 andrewtoth commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#issuecomment-3349669141)
Have you seen https://github.com/bitcoin/bitcoin/pull/29415? Is there a reason you would want this if we had private broadcast?

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389662377)
Precompiled regexes are optimized to NFAs, but this one doesn't even backtrack so it's likely really fast and very easy to read.

But looking at https://github.com/catchorg/Catch2/blob/devel/src/catch2/internal/catch_debugger.cpp#L95 it seems to me that we don't even need to parse it, just check if it starts with zero or not, maybe something like:
```C++
for (std::string line; !attached && std::getline(status, line);) {
if (line.starts_with("TracerPid:")) {
attached = line.find
...

💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2389746621)
nit: if you re-touch, same as `walletdb.cpp`, copyright dates needs to be updated...

🤔 pablomartin4btc reviewed a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3282443179)
Not sure about this, if there are no descriptor wallets during startup (no default, no one in settings), we would be logging the sqlite version but if the user had legacy wallets in settings, that would print a warning (`FAILED_LEGACY_DISABLED` -> `initWarning`) not showing the berkeley version of the DB instantiated (which code is not in `master` either but it doesn't look consistent). If we have a diff DB format in the future we'd need to change this LogDBInfo() (once someone finds it).

I'd
...

💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2389748193)
nit: if you re-touch... please update the copyright (also to match its header <code>walletdb.h</code>)
<code>// Copyright (c) 2009-present The Bitcoin Core developers</code>

💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2389936885)
Ah, I guess I could have written the comment clearer. I meant `Use the earliest Xcode requiring at least the version of macOS denoted in ...`. Otherwise, we'd unnecessarily limit our Xcode features to EOL versions of macOS.

Whether to pick the earliest minor version of Xcode or earliest major version of Xcode is not specified, but I think going with the earliest major version here seems fine, to be able to remove the fuzz.cpp workaround.

💬 maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3350089468)
> The minimum supported version in the release notes should only be changed with the along with changing the minimum supported version in depends.

Thx, done. However, I can't test the resulting depends change on macOS myself.

🤔 Eunovo reviewed a pull request: "test: Fix rpc_getblockstats for no-wallet environments"
(https://github.com/bitcoin/bitcoin/pull/33184#pullrequestreview-3282780757)
I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.

💬 Eunovo commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2389993284)
https://github.com/bitcoin/bitcoin/pull/33184/commits/12c9c7d927091f3be72f4acd99c166a41c789ae9:
IIUC, `MiniWallet` doesn't require wallet rpc, so what caused the "-disablewallet" error?

💬 Eunovo commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2389995639)
https://github.com/bitcoin/bitcoin/pull/33184/commits/316cf41c25438d61f4e9568e20878fa374476a5a:
I see that the CI fails because this argument conflicts with the argument defined in `rpc_blockstats`.

💬 maflcko commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2390009870)
You'll also have to check the fee? Otherwise, this just seems like a duplicate of `sendmsgtopeer 0 "tx" "$txhex"`?

💬 maflcko commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3350177873)
> I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.

I guess it depends on how long it takes to generate, but if it is just a few seconds, it seems easier not to cache.

💬 Eunovo commented on pull request "test: Fix rpc_getblockstats for no-wallet environments":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3350187225)
@maflcko This is from my 8-core laptop
```
☁ bitcoin [2e1836d744] build/test/functional/test_runner.py rpc_getblockstats --gen-test-data
Temporary test directory at /tmp/test_runner_₿_🏃_20250930_083649
Remaining jobs: [rpc_getblockstats.py]
1/1 - rpc_getblockstats.py passed, Duration: 1 s

TEST | STATUS | DURATION

rpc_getblockstats.py | ✓ Passed | 1 s

ALL | ✓ Passed | 1 s (accumulated)
Runtime: 1 s

```

💬 hebasto commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3350214088)
My Guix build:
```
aarch64
b286e83a69f60494b86be866ce3b4aaa7c58aeb2e9410a38b898c2afe6946f59 guix-build-980e4987db04/output/aarch64-linux-gnu/SHA256SUMS.part
c9bcfe3c18dd5a7130c3ccbb17f8f51f7486a9bbfdb57c9ee59164003013de39 guix-build-980e4987db04/output/aarch64-linux-gnu/bitcoin-980e4987db04-aarch64-linux-gnu-debug.tar.gz
f72be7de9bb5d037e9e27db645d8578f031e6a7b6b9096c573dfba56cbcb8a0c guix-build-980e4987db04/output/aarch64-linux-gnu/bitcoin-980e4987db04-aarch64-linux-gnu.tar.gz
ff9ff073
...

💬 maflcko commented on pull request "ci: use latest versions of lint deps":
(https://github.com/bitcoin/bitcoin/pull/33487#issuecomment-3350234120)
lgtm ACK d4f47f97715c7b6a2879e99c62f09ccead8cc4cd

🤔 janb84 reviewed a pull request: "ci: use latest versions of lint deps"
(https://github.com/bitcoin/bitcoin/pull/33487#pullrequestreview-3282863818)
lgtm ACK d4f47f97715c7b6a2879e99c62f09ccead8cc4cd

💬 hebasto commented on pull request "depends: static libxcb-cursor":
(https://github.com/bitcoin/bitcoin/pull/33434#issuecomment-3350313336)
@davidgumberg
> Trying to do a guix build on this branch I get the following error:

Is this consistently reproducible, or does it occur intermittently?

👍 hebasto approved a pull request: "ci: use latest versions of lint deps"
(https://github.com/bitcoin/bitcoin/pull/33487#pullrequestreview-3282958097)
ACK d4f47f97715c7b6a2879e99c62f09ccead8cc4cd, I have reviewed the code and it looks OK.