👍 theStack approved a pull request: "cmake: Allow `WITH_DBUS` on all Unix-like systems"
(https://github.com/bitcoin/bitcoin/pull/32469#pullrequestreview-2833096553)
tACK 5b7ed460c7c181f1fd34a927a09aed36284083cb
Tested with OpenBSD 7.7, xfce4.20.0 and the [dbus-1.16.2p0v0](https://openports.pl/path/x11/dbus) package installed that configuring the build with `-DBUILD_GUI=ON` enables "DBus (GUI)" and that DBus is indeed used for notifications (added a [debug message at the proper place](https://github.com/bitcoin/bitcoin/blob/59e09e0fb7b4cf8f8db97c2f81a8f6f69fe6cacf/src/qt/notificator.cpp#L225) just to be sure).
(https://github.com/bitcoin/bitcoin/pull/32469#pullrequestreview-2833096553)
tACK 5b7ed460c7c181f1fd34a927a09aed36284083cb
Tested with OpenBSD 7.7, xfce4.20.0 and the [dbus-1.16.2p0v0](https://openports.pl/path/x11/dbus) package installed that configuring the build with `-DBUILD_GUI=ON` enables "DBus (GUI)" and that DBus is indeed used for notifications (added a [debug message at the proper place](https://github.com/bitcoin/bitcoin/blob/59e09e0fb7b4cf8f8db97c2f81a8f6f69fe6cacf/src/qt/notificator.cpp#L225) just to be sure).
💬 vasild commented on pull request "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory":
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2084600466)
Thanks! Another option, if this gets frowned upon, with less changes, is to leave `GenerateAuthCookie()` alone and have two separate variables in `InitRPCAuthentication()`. In the `else` branch set them like `user = gArgs.GetArg("-rpcuser", "");` and `pass = gArgs.GetArg("-rpcpassword", "");` and in the `if` branch where `GenerateAuthCookie()` is called - use a temporary variable `user_colon_pass` and parse it immediately into the two `user` and `pass` variables.
Anyway, I think the current o
...
(https://github.com/bitcoin/bitcoin/pull/32423#discussion_r2084600466)
Thanks! Another option, if this gets frowned upon, with less changes, is to leave `GenerateAuthCookie()` alone and have two separate variables in `InitRPCAuthentication()`. In the `else` branch set them like `user = gArgs.GetArg("-rpcuser", "");` and `pass = gArgs.GetArg("-rpcpassword", "");` and in the `if` branch where `GenerateAuthCookie()` is called - use a temporary variable `user_colon_pass` and parse it immediately into the two `user` and `pass` variables.
Anyway, I think the current o
...
👍 vasild approved a pull request: "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory"
(https://github.com/bitcoin/bitcoin/pull/32423#pullrequestreview-2833098998)
ACK e49a7274a2141dcb9e188bc4b45c2d7b928ccecd
(https://github.com/bitcoin/bitcoin/pull/32423#pullrequestreview-2833098998)
ACK e49a7274a2141dcb9e188bc4b45c2d7b928ccecd
✅ fanquake closed an issue: "WITH_DBUS shouldn't be limited to Linux"
(https://github.com/bitcoin/bitcoin/issues/32464)
(https://github.com/bitcoin/bitcoin/issues/32464)
🚀 fanquake merged a pull request: "cmake: Allow `WITH_DBUS` on all Unix-like systems"
(https://github.com/bitcoin/bitcoin/pull/32469)
(https://github.com/bitcoin/bitcoin/pull/32469)
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872570256)
@k98kurz It's not generally true, that buffer is limited to 100 txns total, with no ratelimiting whatsoever. Please take the time to do some background reading: https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872570256)
@k98kurz It's not generally true, that buffer is limited to 100 txns total, with no ratelimiting whatsoever. Please take the time to do some background reading: https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052
📝 Eunovo opened a pull request: "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet"
(https://github.com/bitcoin/bitcoin/pull/32471)
This PR updates the `listdescriptors true` RPC to show public keys instead of private keys when the private keys aren't available.
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possibl
...
(https://github.com/bitcoin/bitcoin/pull/32471)
This PR updates the `listdescriptors true` RPC to show public keys instead of private keys when the private keys aren't available.
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possibl
...
💬 fanquake commented on pull request "cmake: Allow `WITH_DBUS` on all Unix-like systems":
(https://github.com/bitcoin/bitcoin/pull/32469#issuecomment-2872589894)
Backported to `29.x` in #32292.
(https://github.com/bitcoin/bitcoin/pull/32469#issuecomment-2872589894)
Backported to `29.x` in #32292.
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872602791)
re-ACK c164064c266c518588d9d9175f9b14140ee751b6
Since my last review it adds `MAX_SCRIPT_ELEMENT_SIZE` to the test and uses a larger payload for it.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872602791)
re-ACK c164064c266c518588d9d9175f9b14140ee751b6
Since my last review it adds `MAX_SCRIPT_ELEMENT_SIZE` to the test and uses a larger payload for it.
💬 fanquake commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2872611218)
```bash
/Users/runner/work/bitcoin/bitcoin/src/test/fuzz/miniscript.cpp:1040:42: error: no matching member function for call to 'ToString'
std::optional<std::string> str{node->ToString(parser_ctx)};
~~~~~~^~~~~~~~
/Users/runner/work/bitcoin/bitcoin/src/script/miniscript.h:830:17: note: candidate function template not viable: requires 2 arguments, but 1 was provided
std::string ToString(const CTx& ctx, bool& has_priv_key) const {
^
...
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2872611218)
```bash
/Users/runner/work/bitcoin/bitcoin/src/test/fuzz/miniscript.cpp:1040:42: error: no matching member function for call to 'ToString'
std::optional<std::string> str{node->ToString(parser_ctx)};
~~~~~~^~~~~~~~
/Users/runner/work/bitcoin/bitcoin/src/script/miniscript.h:830:17: note: candidate function template not viable: requires 2 arguments, but 1 was provided
std::string ToString(const CTx& ctx, bool& has_priv_key) const {
^
...
👍 theStack approved a pull request: "test: test MAX_SCRIPT_SIZE for block validity"
(https://github.com/bitcoin/bitcoin/pull/32304#pullrequestreview-2833275888)
ACK b1ea542ae651cec433910d8c727abc41f17a7678
nit: if you have to retouch, could add a log message (`self.log.info`)
(https://github.com/bitcoin/bitcoin/pull/32304#pullrequestreview-2833275888)
ACK b1ea542ae651cec433910d8c727abc41f17a7678
nit: if you have to retouch, could add a log message (`self.log.info`)
👍 vasild approved a pull request: "common: Close non-std fds before exec in RunCommandJSON"
(https://github.com/bitcoin/bitcoin/pull/32343#pullrequestreview-2833238669)
ACK a0eed55398f882d9390e50582b10272d18f2b836
(https://github.com/bitcoin/bitcoin/pull/32343#pullrequestreview-2833238669)
ACK a0eed55398f882d9390e50582b10272d18f2b836
💬 vasild commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084684177)
Can `close()` be called right away at line `1319` and avoid the `fds_to_close` variable and the second `for` loop?
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084684177)
Can `close()` be called right away at line `1319` and avoid the `fds_to_close` variable and the second `for` loop?
💬 vasild commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084710474)
Nothing prevents another thread from opening a new file after all these `close()` calls have completed and before the `execvp()` call below. So I guess this is best effort attempt?
A sure way would be to use `O_CLOEXEC` when opening a file: https://www.man7.org/linux/man-pages/man2/open.2.html.
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084710474)
Nothing prevents another thread from opening a new file after all these `close()` calls have completed and before the `execvp()` call below. So I guess this is best effort attempt?
A sure way would be to use `O_CLOEXEC` when opening a file: https://www.man7.org/linux/man-pages/man2/open.2.html.
📝 Eunovo converted_to_draft a pull request: "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet"
(https://github.com/bitcoin/bitcoin/pull/32471)
This PR updates the `listdescriptors true` RPC to show public keys instead of private keys when the private keys aren't available.
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possibl
...
(https://github.com/bitcoin/bitcoin/pull/32471)
This PR updates the `listdescriptors true` RPC to show public keys instead of private keys when the private keys aren't available.
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possibl
...
💬 Eunovo commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2872665572)
Putting draft while I fix some issues with the fuzz tests
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2872665572)
Putting draft while I fix some issues with the fuzz tests
💬 1ma commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872668421)
Concept NACK, for the same reasons stated in #32359 and #28130
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872668421)
Concept NACK, for the same reasons stated in #32359 and #28130
👍 ajtowns approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2832143503)
ACK c164064c266c518588d9d9175f9b14140ee751b6
I believe this PR makes the following changes:
* allows multiple data carrier outputs in a single transaction (removing `multi-op-return` standardness failures)
* puts multiple data carrier outputs under the same limit (giving a new `datacarrier` standardness failure when the limit is exceeded instead of `scriptpubkey`)
* increases the limit's default from 83 to 100,000, making the limit irrelevant
* deprecates the two datacarrier setting
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2832143503)
ACK c164064c266c518588d9d9175f9b14140ee751b6
I believe this PR makes the following changes:
* allows multiple data carrier outputs in a single transaction (removing `multi-op-return` standardness failures)
* puts multiple data carrier outputs under the same limit (giving a new `datacarrier` standardness failure when the limit is exceeded instead of `scriptpubkey`)
* increases the limit's default from 83 to 100,000, making the limit irrelevant
* deprecates the two datacarrier setting
...
💬 ajtowns commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2084189058)
I'd probably have phrased this more like:
> ### Updated Settings
>
> - `-datacarriersize` is increased to 100,000 which effectively uncaps the limit (as the maximum transaction size limit will be hit first). It can be overridden with `-datacarriersize=83` to revert to the limit enforced in previous versions. Both `-datacarrier` and `-datacarriersize` options have been marked as deprecated and are expected to be removed in a future release. (#32406)
> - Multiple data carrier (`OP_RETURN`) o
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2084189058)
I'd probably have phrased this more like:
> ### Updated Settings
>
> - `-datacarriersize` is increased to 100,000 which effectively uncaps the limit (as the maximum transaction size limit will be hit first). It can be overridden with `-datacarriersize=83` to revert to the limit enforced in previous versions. Both `-datacarrier` and `-datacarriersize` options have been marked as deprecated and are expected to be removed in a future release. (#32406)
> - Multiple data carrier (`OP_RETURN`) o
...
💬 ajtowns commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2084024874)
I'd suggest combining this commit with the "Init: warn if set" one.
I'm -0 on deprecating these; I'm not particularly bothered if they are marked as deprecated, but it doesn't seem necessary to me, and at least for now there seem to be a bunch of users who like the option.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2084024874)
I'd suggest combining this commit with the "Init: warn if set" one.
I'm -0 on deprecating these; I'm not particularly bothered if they are marked as deprecated, but it doesn't seem necessary to me, and at least for now there seem to be a bunch of users who like the option.