💬 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.
💬 ryanofsky commented on issue "RFC: Should node Wallet Startup Options Apply to Individual Wallets?":
(https://github.com/bitcoin/bitcoin/issues/32462#issuecomment-2872709780)
Is idea with the "set on each wallet, potentially different for each" suggestion that the existing `-maxtxfee` option should remain global, but `-maxfeerate` should be per-wallet?
If so, that seems confusing and it would be good to know what rationale would be.
It seems like just making both global would let them be easier to understand and more convenient to set. Then if there is a use-case for wanting different wallets to have different values, support for that could be added in another PR.
...
(https://github.com/bitcoin/bitcoin/issues/32462#issuecomment-2872709780)
Is idea with the "set on each wallet, potentially different for each" suggestion that the existing `-maxtxfee` option should remain global, but `-maxfeerate` should be per-wallet?
If so, that seems confusing and it would be good to know what rationale would be.
It seems like just making both global would let them be easier to understand and more convenient to set. Then if there is a use-case for wanting different wallets to have different values, support for that could be added in another PR.
...
💬 Sjors commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2872711781)
Rebased after #32155.
> So I think it's more fair to say that our miner code adds OP_0 as a dummy extraNonce
Possibly, but `extraNonce` seems like an implementation detail that should be left to miners. E.g. the Stratum v2 spec defines how to use it here: https://github.com/stratum-mining/sv2-spec/blob/main/05-Mining-Protocol.md
It doesn't belong in a block template imo.
> I wonder if there isn't a way to have this work for stratumv2 without changing the way the existing code works?
...
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2872711781)
Rebased after #32155.
> So I think it's more fair to say that our miner code adds OP_0 as a dummy extraNonce
Possibly, but `extraNonce` seems like an implementation detail that should be left to miners. E.g. the Stratum v2 spec defines how to use it here: https://github.com/stratum-mining/sv2-spec/blob/main/05-Mining-Protocol.md
It doesn't belong in a block template imo.
> I wonder if there isn't a way to have this work for stratumv2 without changing the way the existing code works?
...
💬 instagibbs commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2872712480)
I don't love the script-time check, but this level of granularity may make sense.
I've been mulling over future interactions with things like CTV, where enabling CTV (which doesn't commit to annex) is vulnerable to witness inflation in the taproot context. It's not immediately clear to me if that means the annex should be committed or not. This patchset would at least be a minimal relaxation of annex policy, if paired with another annex relay PR such as https://github.com/petertodd/bitcoin/co
...
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2872712480)
I don't love the script-time check, but this level of granularity may make sense.
I've been mulling over future interactions with things like CTV, where enabling CTV (which doesn't commit to annex) is vulnerable to witness inflation in the taproot context. It's not immediately clear to me if that means the annex should be committed or not. This patchset would at least be a minimal relaxation of annex policy, if paired with another annex relay PR such as https://github.com/petertodd/bitcoin/co
...
💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084757939)
There are no threads at this point, mind this is after the `fork()`, in the child process.
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084757939)
There are no threads at this point, mind this is after the `fork()`, in the child process.
💬 laanwj commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084762598)
Not sure if changing the contents during iteration would mess with the directory iteration. This feels safer.
(https://github.com/bitcoin/bitcoin/pull/32343#discussion_r2084762598)
Not sure if changing the contents during iteration would mess with the directory iteration. This feels safer.
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2084808247)
Hi @achow101 could you please check this. Merge it if everything is okay.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2084808247)
Hi @achow101 could you please check this. Merge it if everything is okay.
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872802091)
@iicuriosity I could not find your comment with justification in the other PR. Please edit and add it to your concept NACK
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872802091)
@iicuriosity I could not find your comment with justification in the other PR. Please edit and add it to your concept NACK
💬 Hackzero00 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2872843179)
> As per [recent bitcoindev mailing list discussion](https://groups.google.com/g/bitcoindev/c/d6ZO7gXGYbQ).
>
> Also removes the code to enforce those limits, including the `-datacarrier` and `-datacarriersize` config options.
>
> These limits are easily bypassed by both direct submission to miner mempools (e.g. MARA Slipstream), and forks of Bitcoin Core that do not enforce them (e.g. Libre Relay). Secondly, protocols are bypassing them by simply publishing data in other ways, such as uns
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2872843179)
> As per [recent bitcoindev mailing list discussion](https://groups.google.com/g/bitcoindev/c/d6ZO7gXGYbQ).
>
> Also removes the code to enforce those limits, including the `-datacarrier` and `-datacarriersize` config options.
>
> These limits are easily bypassed by both direct submission to miner mempools (e.g. MARA Slipstream), and forks of Bitcoin Core that do not enforce them (e.g. Libre Relay). Secondly, protocols are bypassing them by simply publishing data in other ways, such as uns
...
💬 Hackzero00 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2872863916)
Concept NACK. Removing these limits formalizes surrender to protocol abuse. The fact that data spam circumvents config options doesn't justify abandoning enforcement. Bitcoin is a monetary protocol, not a general-purpose data store. Normalizing arbitrary payloads weakens decentralization, bloats the chain, and betrays the principle of node sovereignty. The base layer must remain focused, lean, and resistant to parasitic use. This change undermines that.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2872863916)
Concept NACK. Removing these limits formalizes surrender to protocol abuse. The fact that data spam circumvents config options doesn't justify abandoning enforcement. Bitcoin is a monetary protocol, not a general-purpose data store. Normalizing arbitrary payloads weakens decentralization, bloats the chain, and betrays the principle of node sovereignty. The base layer must remain focused, lean, and resistant to parasitic use. This change undermines that.