💬 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.
🤔 mzumsande reviewed a pull request: "rpc: generatetomany"
(https://github.com/bitcoin/bitcoin/pull/32468#pullrequestreview-2833513348)
Could the motivation be added here? Doesn't seem ok having to resort to twitter (where people without an account can't even see most of the thread) to know the reason for a PR.
(https://github.com/bitcoin/bitcoin/pull/32468#pullrequestreview-2833513348)
Could the motivation be added here? Doesn't seem ok having to resort to twitter (where people without an account can't even see most of the thread) to know the reason for a PR.
👍 theStack approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2833529331)
re-ACK c164064c266c518588d9d9175f9b14140ee751b6
(test-only change since my previous ACK; as per `git diff 8c360d78 c164064c`, the review comments https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077900934 ff. were addressed)
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2833529331)
re-ACK c164064c266c518588d9d9175f9b14140ee751b6
(test-only change since my previous ACK; as per `git diff 8c360d78 c164064c`, the review comments https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077900934 ff. were addressed)
👍 Sjors approved a pull request: "test: Remove legacy wallet RPC overloads"
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2833548358)
Much better.
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2833548358)
Much better.
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
💬 ajtowns commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2872909611)
> It doesn't belong in a block template imo.
Right, but we currently don't include it in a block template either, so that's (currently) fine.
> > Would it work for the stratumv2 interface ... to just recognise we supply a dummy extra nonce, and drop it
>
> Yes but this would be a foot gun if a future soft fork requires an additional commitment. It's also up to every consumer of our Mining interface to implement that (correctly), not just the one I wrote.
I think it's more likely that
...
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2872909611)
> It doesn't belong in a block template imo.
Right, but we currently don't include it in a block template either, so that's (currently) fine.
> > Would it work for the stratumv2 interface ... to just recognise we supply a dummy extra nonce, and drop it
>
> Yes but this would be a foot gun if a future soft fork requires an additional commitment. It's also up to every consumer of our Mining interface to implement that (correctly), not just the one I wrote.
I think it's more likely that
...
💬 1440000bytes commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872963909)
reACK https://github.com/bitcoin/bitcoin/commit/c164064c266c518588d9d9175f9b14140ee751b6
Last review: https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2857253356
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2872963909)
reACK https://github.com/bitcoin/bitcoin/commit/c164064c266c518588d9d9175f9b14140ee751b6
Last review: https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2857253356
🤔 rkrux reviewed a pull request: "test: refactor: overhaul (w)txid determination for `CTransaction` objects"
(https://github.com/bitcoin/bitcoin/pull/32421#pullrequestreview-2833630198)
Preliminary ACK efc79f2c30f0029bc7ebc5574e37a59d822da06a
I feel it's a good cleanup. It became quite evident to me that the previous function names were not expressive enough to gauge their intention just by their name as I had to look into their implementations while verifying the mappings with the new property attributes introduced in the PR description table.
As mentioned in the description, it seems that the onus of fetching the updated transaction hashes was on the test writer previou
...
(https://github.com/bitcoin/bitcoin/pull/32421#pullrequestreview-2833630198)
Preliminary ACK efc79f2c30f0029bc7ebc5574e37a59d822da06a
I feel it's a good cleanup. It became quite evident to me that the previous function names were not expressive enough to gauge their intention just by their name as I had to look into their implementations while verifying the mappings with the new property attributes introduced in the PR description table.
As mentioned in the description, it seems that the onus of fetching the updated transaction hashes was on the test writer previou
...
✅ glozow closed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359)
(https://github.com/bitcoin/bitcoin/pull/32359)
💬 glozow commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2872995148)
Based on the discussion, there seems to be consensus around leaving the config options in place. That approach is implemented in #32406, so closing this PR. (Also: please do not jump to conclusions about whether this means the other PR will be merged/closed)
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2872995148)
Based on the discussion, there seems to be consensus around leaving the config options in place. That approach is implemented in #32406, so closing this PR. (Also: please do not jump to conclusions about whether this means the other PR will be merged/closed)
💬 ajtowns commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2873037419)
> Current Bitcoin behavior is `OP_CAT = False` and `DISCOURAGE_OP_CAT = False`.
DISCOURAGE_OP_SUCCESS in current bitcoin covers the behaviour of DISCOURAGE_OP_CAT.
OP_CAT=false and DISCOURAGE_OP_CAT=false are used for validating blocks prior to activation of OP_CAT, so this isn't an edge case as far as I can see? OP_CAT=true and DISCOURAGE_OP_CAT=true should be an edge case, but it doesn't make sense of course: "disallow OP_CAT, but if it were allowed, which it's not, enforce its rules".
...
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2873037419)
> Current Bitcoin behavior is `OP_CAT = False` and `DISCOURAGE_OP_CAT = False`.
DISCOURAGE_OP_SUCCESS in current bitcoin covers the behaviour of DISCOURAGE_OP_CAT.
OP_CAT=false and DISCOURAGE_OP_CAT=false are used for validating blocks prior to activation of OP_CAT, so this isn't an edge case as far as I can see? OP_CAT=true and DISCOURAGE_OP_CAT=true should be an edge case, but it doesn't make sense of course: "disallow OP_CAT, but if it were allowed, which it's not, enforce its rules".
...
💬 Sjors commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2873043121)
I also don't think `CBlockTemplate` should have an extraNonce. Instead it could be added by code that actually does the mining, such as the `GenerateBlock` block method in `rpc/mining.cpp`.
Although CheckBlock() also needs it to be present for these early blocks, unless we pass an argument in to skip the `bad-cb-length` check.
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2873043121)
I also don't think `CBlockTemplate` should have an extraNonce. Instead it could be added by code that actually does the mining, such as the `GenerateBlock` block method in `rpc/mining.cpp`.
Although CheckBlock() also needs it to be present for these early blocks, unless we pass an argument in to skip the `bad-cb-length` check.
💬 JeremyRubin commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2873050360)
good alternative approach -- I can change to that, after there's a clear preference from a couple devs who look. I don't know if people like altering execdata if there isn't a clear API reason to want to propagate that data (I think of it as only relevant when we really need to pass the information across, whereas the change as written is fully local to script exec).
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2873050360)
good alternative approach -- I can change to that, after there's a clear preference from a couple devs who look. I don't know if people like altering execdata if there isn't a clear API reason to want to propagate that data (I think of it as only relevant when we really need to pass the information across, whereas the change as written is fully local to script exec).