β
fanquake closed an issue: "Compilation failure with Clang SNAPSHOT"
(https://github.com/bitcoin/bitcoin/issues/33539)
(https://github.com/bitcoin/bitcoin/issues/33539)
π¬ Christewart commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3368275934)
It seems like the RPC interface is working as expected with bitcoin-s: https://github.com/bitcoin-s/bitcoin-s/pull/6095
The only other nit I would have is technically the `watchonly` json fields are deprecated this release, not removed. They will be removed in a future release.
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3368275934)
It seems like the RPC interface is working as expected with bitcoin-s: https://github.com/bitcoin-s/bitcoin-s/pull/6095
The only other nit I would have is technically the `watchonly` json fields are deprecated this release, not removed. They will be removed in a future release.
π andrewtoth approved a pull request: "net: support overriding the proxy selection in ConnectNode()"
(https://github.com/bitcoin/bitcoin/pull/33454#pullrequestreview-3301899692)
ACK c76de2eea18076f91dd80b52f66ba790f071a2b1
(https://github.com/bitcoin/bitcoin/pull/33454#pullrequestreview-3301899692)
ACK c76de2eea18076f91dd80b52f66ba790f071a2b1
π¬ fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3368298124)
tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
I might just address some of the left-over comments in a follow-up together with my additional tests, if this PR doesn't get retouched anymore.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3368298124)
tACK ac599c4a9cb3b2d424932d3fd91f9eed17426827
I might just address some of the left-over comments in a follow-up together with my additional tests, if this PR doesn't get retouched anymore.
π¬ andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2404014654)
I see, so if this gets merged the test here can be modified in #31132 to test for insertion if already exists?
I don't see where we verify the accounting though. You are referring to the `cachedCoinsUsage` accounting? This test doesn't seem to check that. Maybe that can be added?
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2404014654)
I see, so if this gets merged the test here can be modified in #31132 to test for insertion if already exists?
I don't see where we verify the accounting though. You are referring to the `cachedCoinsUsage` accounting? This test doesn't seem to check that. Maybe that can be added?
π¬ andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2404015081)
Could we add a unit test to trigger this accounting error?
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2404015081)
Could we add a unit test to trigger this accounting error?
π maflcko approved a pull request: "Clear out space on centos job"
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3301923979)
lgtm. left a comment to avoid churn in the future and one of the merge conflicts.
(https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3301923979)
lgtm. left a comment to avoid churn in the future and one of the merge conflicts.
π¬ maflcko commented on pull request "Clear out space on centos job":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2404015061)
```suggestion
if: ${{ needs.runners.outputs.provider == 'gha' }}
```
nit: Could run it for all tasks, to avoid having to select them individually?
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2404015061)
```suggestion
if: ${{ needs.runners.outputs.provider == 'gha' }}
```
nit: Could run it for all tasks, to avoid having to select them individually?
π¬ andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2404065695)
> Can we add a single-threaded input fetcher first and add multithreading only as the very last step?
Done.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2404065695)
> Can we add a single-threaded input fetcher first and add multithreading only as the very last step?
Done.
π¬ andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2404066620)
Done.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2404066620)
Done.
π¬ TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3368378954)
Thank you for the review @alexanderwiederin
f3ca1d65b0444097fd0e2f059a01eb5b29c950c1 -> 45ca0bb904d308671635d233b737047f740d487b ([kernelApi_68](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_68) -> [kernelApi_69](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_69), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_68..kernelApi_69))
* Addressed @alexanderwiederin's various nits; cleaning up commits, improving commit messages, smaller doc fixes.
* Added a
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3368378954)
Thank you for the review @alexanderwiederin
f3ca1d65b0444097fd0e2f059a01eb5b29c950c1 -> 45ca0bb904d308671635d233b737047f740d487b ([kernelApi_68](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_68) -> [kernelApi_69](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_69), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_68..kernelApi_69))
* Addressed @alexanderwiederin's various nits; cleaning up commits, improving commit messages, smaller doc fixes.
* Added a
...
β
benthecarman closed a pull request: "Fix dark mode detection on Linux"
(https://github.com/bitcoin-core/gui/pull/895)
(https://github.com/bitcoin-core/gui/pull/895)
π¬ hebasto commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2404081843)
Removed.
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2404081843)
Removed.
π¬ hebasto commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3368402179)
[Feedback](https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2402371308) from @fanquake has been addressed.
(https://github.com/bitcoin/bitcoin/pull/33451#issuecomment-3368402179)
[Feedback](https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2402371308) from @fanquake has been addressed.
π andrewtoth approved a pull request: "log: print every script verification state change"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3302015530)
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3302015530)
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
π pablomartin4btc opened a pull request: "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)"
(https://github.com/bitcoin/bitcoin/pull/33540)
This PR simplifies the argument parsing logic and resolves practical issues for `bitcoin-cli` and similar binaries (e.g. `bitcoin-wallet`) making their usage more consistent and predictable, enabling valid patterns such as mixing commands with wallet- or RPC-specific optionsβwithout relying on argument ordering quirks.
<details>
<summary>What's changed in ArgsManager that allows the above.</summary>
- Previously, `ArgsManager` stopped interpreting options once a non-option argument was en
...
(https://github.com/bitcoin/bitcoin/pull/33540)
This PR simplifies the argument parsing logic and resolves practical issues for `bitcoin-cli` and similar binaries (e.g. `bitcoin-wallet`) making their usage more consistent and predictable, enabling valid patterns such as mixing commands with wallet- or RPC-specific optionsβwithout relying on argument ordering quirks.
<details>
<summary>What's changed in ArgsManager that allows the above.</summary>
- Previously, `ArgsManager` stopped interpreting options once a non-option argument was en
...
π¬ Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2404232251)
Thanks for pointing this out.
> 1. `out` doesn't need to be cleared if no priv keys found. For non watch-only wallets, a descriptor without any private keys can't be imported anyway so there can't be a case wherein we need to propagate this check from `MuSigPubkeyProvider` to `TrDescriptor`.
Yes, there must be a private key somewhere in the descriptor, so the `MusigPubkeyProvider` should not clear the string.
I used your patch and added you as co-author.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2404232251)
Thanks for pointing this out.
> 1. `out` doesn't need to be cleared if no priv keys found. For non watch-only wallets, a descriptor without any private keys can't be imported anyway so there can't be a case wherein we need to propagate this check from `MuSigPubkeyProvider` to `TrDescriptor`.
Yes, there must be a private key somewhere in the descriptor, so the `MusigPubkeyProvider` should not clear the string.
I used your patch and added you as co-author.
π¬ Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2404232564)
I have updated the comment.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2404232564)
I have updated the comment.
π¬ Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2404232718)
I don't believe the suggested message is an improvement, so I will leave it as is.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2404232718)
I don't believe the suggested message is an improvement, so I will leave it as is.
π¬ katesalazar commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3368884598)
Such good news. Hard to believe this PR doesn't reference an existing user request.
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3368884598)
Such good news. Hard to believe this PR doesn't reference an existing user request.