π¬ sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285335214)
See IRC:
```
09:15:59 < sipa> anyone have any clue what's going on here? https://github.com/bitcoin/bitcoin/actions/runs/17053347607/job/48345965497?pr=33201
09:16:13 < sipa> 240/272 - interface_ipc.py failed, Duration: 6 s
09:16:18 < sipa> 2025-08-18T22:28:30.854262Z TestFramework (INFO): Tests successful
09:28:06 < instagibbs> TIL SystemExit and GeneratorExit don't inherit Exception, so maybe SystemExit is triggering, causing the failure to not be set
09:46:23 < sipa> instagibbs: let's
...
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285335214)
See IRC:
```
09:15:59 < sipa> anyone have any clue what's going on here? https://github.com/bitcoin/bitcoin/actions/runs/17053347607/job/48345965497?pr=33201
09:16:13 < sipa> 240/272 - interface_ipc.py failed, Duration: 6 s
09:16:18 < sipa> 2025-08-18T22:28:30.854262Z TestFramework (INFO): Tests successful
09:28:06 < instagibbs> TIL SystemExit and GeneratorExit don't inherit Exception, so maybe SystemExit is triggering, causing the failure to not be set
09:46:23 < sipa> instagibbs: let's
...
π¬ ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3200849464)
I've summarized current state of this PR below, and think this maybe ready for merge.
### Review
- ryanofsky: current ack, low level reviews
- ismaelsadeeq: current ack, testing, and comments on documentation and related prs
- josibase: current ack, high level review, no code comments
- fanquake: low level reviews, most thorough build & CI testing, reporting and fixing openbsd & lint/tidy issues previously, and current [gcc 11 / capnproto 0.8](https://github.com/bitcoin/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3200849464)
I've summarized current state of this PR below, and think this maybe ready for merge.
### Review
- ryanofsky: current ack, low level reviews
- ismaelsadeeq: current ack, testing, and comments on documentation and related prs
- josibase: current ack, high level review, no code comments
- fanquake: low level reviews, most thorough build & CI testing, reporting and fixing openbsd & lint/tidy issues previously, and current [gcc 11 / capnproto 0.8](https://github.com/bitcoin/bitcoi
...
π€ janb84 reviewed a pull request: "Release: Prepare "Translation string freeze" step"
(https://github.com/bitcoin/bitcoin/pull/33193#pullrequestreview-3132438847)
re ACK 0df2c3c42e8a3ee3da4ca6c4b9b9c28060bb1fe3
changes since last ACK:
- rebase (churn is inline with changes from #33209 )
(https://github.com/bitcoin/bitcoin/pull/33193#pullrequestreview-3132438847)
re ACK 0df2c3c42e8a3ee3da4ca6c4b9b9c28060bb1fe3
changes since last ACK:
- rebase (churn is inline with changes from #33209 )
π¬ BrandonOdiwuor commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2285345277)
I have updated the code to only do the second `IsMine` check if the address is not in `mapTally`
(https://github.com/bitcoin/bitcoin/pull/30972#discussion_r2285345277)
I have updated the code to only do the second `IsMine` check if the address is not in `mapTally`
π¬ BrandonOdiwuor commented on pull request "Wallet: "listreceivedby*" fix":
(https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-3200864286)
Rebased and updated to address https://github.com/bitcoin/bitcoin/pull/30972#discussion_r1989789588
(https://github.com/bitcoin/bitcoin/pull/30972#issuecomment-3200864286)
Rebased and updated to address https://github.com/bitcoin/bitcoin/pull/30972#discussion_r1989789588
β
ryanofsky closed a pull request: "build: Enable ENABLE_IPC option by default"
(https://github.com/bitcoin/bitcoin/pull/33190)
(https://github.com/bitcoin/bitcoin/pull/33190)
π¬ ryanofsky commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3200889700)
Thanks everybody who commented! Will close this in favor of #31802. If anybody thinks this should be reopened, I'd be curious and can reopen it. But balance of opinion seems to be in favor of not doing this.
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3200889700)
Thanks everybody who commented! Will close this in favor of #31802. If anybody thinks this should be reopened, I'd be curious and can reopen it. But balance of opinion seems to be in favor of not doing this.
π¬ ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285441508)
IIUC it's failing because stderr is nonempty:
https://github.com/bitcoin/bitcoin/blob/22e689587a7ae02f82ad2464017731f3b23b5363/test/functional/test_runner.py#L760
because the [log](https://github.com/bitcoin/bitcoin/actions/runs/1753347607/job/48345965497) ([raw](https://productionresultssa13.blob.core.windows.net/actions-results/bfb1b205-2f78-46f1-95d9-f32ca7112d38/workflow-job-run-45b4c18f-0ff3-5fd3-8716-ea6a91129b24/logs/job/job-logs.txt?rsct=text%2Fplain&se=2025-08-19T14%3A22%3A57Z&sig
...
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2285441508)
IIUC it's failing because stderr is nonempty:
https://github.com/bitcoin/bitcoin/blob/22e689587a7ae02f82ad2464017731f3b23b5363/test/functional/test_runner.py#L760
because the [log](https://github.com/bitcoin/bitcoin/actions/runs/1753347607/job/48345965497) ([raw](https://productionresultssa13.blob.core.windows.net/actions-results/bfb1b205-2f78-46f1-95d9-f32ca7112d38/workflow-job-run-45b4c18f-0ff3-5fd3-8716-ea6a91129b24/logs/job/job-logs.txt?rsct=text%2Fplain&se=2025-08-19T14%3A22%3A57Z&sig
...
π¬ 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]`
(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.
(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
...
(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
...
(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/
...
(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.
(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?
(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?
(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.
(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`.
(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")))
```
(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
...
(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.
(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.