💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285457363)
If we wanted to annotate it, could write as `extra_init: list[dict[str, Any]] = ...` (with `from typing import Any`) but would also seem reasonable to suppress this with `# type: ignore[var-annotated]`

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3201001743)
Some Major Updates:
- The `SilentPaymentDescriptorScriptPubKeyMan` now obtains through a `GetScanKey` method, only available on Silent Payment descriptors
- The spend key is no longer derived during descriptor parsing; it now has to be "expanded" the same way other descriptors are expanded
- `LoadKeysAndChangeLabel` has been replaced with an overridden `TopUpWithDB` method. The wallet calls `TopUp` when it wants the SPKMans to derive and load new keys and scripts.

💬 sipa commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3201020041)
With #33190 closed, I'll comment further here.

@achow101 @ryanofsky In my [comment](https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3196869655) in the other PR regarding seeing this PR as a successor to that one, I failed the consider the possibility of enabling it in release builds without making it default in from-source builds, like Qt, ZMQ, USDT, so I saw it as two separate sequential decisions to be made, rather than seeing them as orthogonal.

That said, I think there is a
...

💬 janb84 commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285491902)

>
> Maybe this line should be changed to use stdout instead of stderr?
>
> https://github.com/bitcoin/bitcoin/blob/530ca0bc57477b77f60af142695d9bf200ef6e3c/test/functional/test_framework/test_node.py#L232
>
> (Sorry if this is the bug)

I Could reproduce the bug locally and this change solves it for me.

<details>

Without change :

```shell
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py failed, Duration: 4 s

stdout:
2025-08-19T14:35:47.614991Z TestFramework (I
...

💬 hodlinator commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3201147329)
### Concept

Pretty sure this not being enabled has wasted my time (even though I mostly worked on PRs which didn't include the #33101 change). I assumed it was something broken with my LSP plugin, and was meaning to look into it eventually, getting by through grepping files.

Having this on by default saves many developers from spending time on figuring this aspect out. As shown by the author, changing the default is commonplace among other projects: https://github.com/bitcoin/bitcoin/pull/
...

💬 marcofleon commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3201214771)
Re ACK 53341ea10dc2f7df371b416060863bbc094b8773

Breaking up the three scenarios into their own test cases makes sense. I checked to make sure the test logic is unchanged.

💬 hebasto commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3201231535)
> Having this on by default...

Why doesn’t `export CMAKE_EXPORT_COMPILE_COMMANDS=ON` in ~/.profile (or something similar) work for you?

💬 theuni commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3201253439)
Concept ACK.

But note that this may require some care when we start shipping the kernel, as it likely needs to be built against a shared libgcc (and maybe libstdc++, depending on what our api/abi looks like at that point).

Maybe add a note so we don't forget?

💬 sipa commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3201255500)
We generally don't include editor-specific configuration, as it gets unclear what the bar for relevance is, and easily gets unmaintainable. For example, `.gitignore` also doesn't include such things either (see #32417).

That doesn't mean that we can't have some documentation about integration in specific systems.

💬 purpleKarrot commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3201268524)
> A different approach that might be more palatable would be to only enable `CMAKE_EXPORT_COMPILE_COMMANDS` in the dev-mode preset in _CMakePresets.json_.

I would object much less than setting this in `CMakeLists.txt`.

💬 edwargix commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2285678340)
please forgive me if I'm misunderstanding this, but can't this logic be simplified to:
```suggestion
#if defined(_WIN32)
#define BITCOINKERNEL_API __declspec(dllexport)
#elif defined(__GNUC__)
#define BITCOINKERNEL_API __attribute__((visibility("default")))
```

💬 hodlinator commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3201326420)
> Why doesn’t `export CMAKE_EXPORT_COMPILE_COMMANDS=ON` in `~/.profile` (or something similar) work for you?

It does for me personally, now that I know about this PR/issue.

I think the main concern here for me & PR author is not to fix it for ourselves, but to minimize time spent by other developers onboarding to the project, or existing developers shifting to look at newer commits post #33101.

> We generally don't include editor-specific configuration, as it gets unclear what the bar f
...

💬 mzumsande commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3201340456)
> Yeah, that's messy. Calculating the MuHash for the utxo set from scratch is essentially the same as recomputing the index.

I think @fjahr meant to calculate the MuHash object directly from the current utxo set (as in `bitcoin-cli gettxoutsetinfo 'muhash'` without an coinstatsindex), which takes ~8 minutes on my laptop and doesn't involve reading blocks from disk - still a bit messy though.

💬 cedwies commented on pull request "Avoid file overwriting in fallback `AllocateFileRange` implementation":
(https://github.com/bitcoin/bitcoin/pull/33164#issuecomment-3201401890)
Clarification: The doc for `AllocateFileRange` says: “the range specified … will never contain live data.” But in reality some call paths (during reindex on a few OSes) did pass ranges that overlapped live bytes? The old fallback then zero‑wrote inside the file and corrupted data. This PR makes the fallback defensive by appending after EOF only, so even if a caller violates the promise, we still don’t overwrite (?).

But what about the guarantee from the doc of `AllocateFileRange`? Should we d
...

💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3201416196)
> I think this is an undesirable situation - shipping an entire new replacement set of binaries that only get eyes/practice from a tiny subset of developers and users.

Thanks for being specific about the negative outcome you see here.

I think I might not understand the significance of the situation you're describing though. The only difference between the binary that supports the `-ipcbind` option and the binary that does not support it is that one binary is linked against [`init/bitcoind.
...

💬 Crypt-iQ commented on pull request "test: modify logging_filesize_rate_limit params":
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3201508089)
Latest push ab5c3be -> 5dda364 adds the diff referenced in [this comment](https://github.com/bitcoin/bitcoin/pull/33211#pullrequestreview-3131772733) so that test failures are more informative. No other changes.

💬 ryanofsky commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3201512895)
Note: achow101 commented against enabling ENABLE_IPC by default in https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3129705803 and sipa commented in favor in https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3201020041, so more discussion about this is happening in the other PR.

I think the difference comes down to short vs. long term perspective. In the short term it doesn't make a huge amount of sense to turn this one feature on by default while comparable features a
...

💬 zaidmstrr commented on pull request "test: p2p block malleability":
(https://github.com/bitcoin/bitcoin/pull/33172#discussion_r2285844408)
nit: you can change this comment to `Confirm the block is still relayable by weight`, As this is not checking whether a block is relayable its just checking weight.

👍 Sjors approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3133140212)
ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f (after base PR lands)

💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285845245)
088dc2c486af0ad6803d919d8114ab29d3cd652f: nit if you have to retouch: these comments are useful as `self.log.debug()`

💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285836684)
088dc2c486af0ad6803d919d8114ab29d3cd652f: eventually I'd like to move this and `load_capnp_modules` into the test framework so that it's easier to expand e.g. `mining_template_verification.py` to call both `getblocktemplate proposal` and `checkBlock()`.