💬 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).
🤔 theStack reviewed a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2833697540)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2833697540)
Concept ACK
💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2084963957)
for consistency, would use the same name here as in the C++ codebase, i.e. `SIGNET_HEADER`
https://github.com/bitcoin/bitcoin/blob/3edf400b1020d7b88402ebc0e758b1fad2e7a781/src/signet.cpp#L27
could also move this to a test_framework module (maye `blocktools.py`) so it can be reused in the signet miner script
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2084963957)
for consistency, would use the same name here as in the C++ codebase, i.e. `SIGNET_HEADER`
https://github.com/bitcoin/bitcoin/blob/3edf400b1020d7b88402ebc0e758b1fad2e7a781/src/signet.cpp#L27
could also move this to a test_framework module (maye `blocktools.py`) so it can be reused in the signet miner script
💬 katesalazar commented on issue "`used` balance should be shown on overview page":
(https://github.com/bitcoin-core/gui/issues/769#issuecomment-2873086913)
@luke-jr I don't remember which post to quote or link, the idea was:
> Algorithm to deal with forced address reuse attack utxos:
> * If the unlock script has not ever been revealed:
> * All attack utxos are to be spent together with the non-attack utxos in the spending event.
> * If the unlock script has already been revelead:
> * All attack utxos are to be ignored forever.
For this algorithm makes sense to have a configuration in order to allow the wallet operator to ignore reused address
...
(https://github.com/bitcoin-core/gui/issues/769#issuecomment-2873086913)
@luke-jr I don't remember which post to quote or link, the idea was:
> Algorithm to deal with forced address reuse attack utxos:
> * If the unlock script has not ever been revealed:
> * All attack utxos are to be spent together with the non-attack utxos in the spending event.
> * If the unlock script has already been revelead:
> * All attack utxos are to be ignored forever.
For this algorithm makes sense to have a configuration in order to allow the wallet operator to ignore reused address
...
📝 hebasto opened a pull request: "doc: Fix typo"
(https://github.com/bitcoin/bitcoin/pull/32472)
A translator on Transifex noticed:
> This is the only label which has two dots: ..
> Usually we see the elipsis (…)
This PR addresses this issue.
(https://github.com/bitcoin/bitcoin/pull/32472)
A translator on Transifex noticed:
> This is the only label which has two dots: ..
> Usually we see the elipsis (…)
This PR addresses this issue.
💬 hebasto commented on pull request "doc: Fix typo":
(https://github.com/bitcoin/bitcoin/pull/32472#issuecomment-2873177104)
> A translator on Transifex noticed:
>
> > This is the only label which has two dots: ..
> > Usually we see the elipsis (…)
Reported by @jesterhodl.
(https://github.com/bitcoin/bitcoin/pull/32472#issuecomment-2873177104)
> A translator on Transifex noticed:
>
> > This is the only label which has two dots: ..
> > Usually we see the elipsis (…)
Reported by @jesterhodl.
💬 supertestnet commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873215557)
Here is my motivation:
When mining a block on regtest, the command "generatetoaddress" is available if you want to send the entire coinbase to 1 address. Let's add generatetomany in case you want to split up the coinbase among multiple addresses, similar to how the "sendtoaddress" and "sendmany" commands both exist and let you send money to (1) one address or (2) multiple addresses. The generatetomany command would be particularly useful for simulating protocols that send money to multiple re
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873215557)
Here is my motivation:
When mining a block on regtest, the command "generatetoaddress" is available if you want to send the entire coinbase to 1 address. Let's add generatetomany in case you want to split up the coinbase among multiple addresses, similar to how the "sendtoaddress" and "sendmany" commands both exist and let you send money to (1) one address or (2) multiple addresses. The generatetomany command would be particularly useful for simulating protocols that send money to multiple re
...
💬 andrewtoth commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873256932)
> Still not sure about this, would like to know more opinions, for other things like `send` we have multiple RPCs
Since I don't seem to have convinced you, here are some more arguments for why this feature belongs in `generateblock` instead of a new RPC:
- New RPCs require adding more code to test, and once added must go through a deprecation cycle to be removed (if they can be removed at all without disrupting too many users).
- Consumers of the RPCs must write additional code for the ne
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873256932)
> Still not sure about this, would like to know more opinions, for other things like `send` we have multiple RPCs
Since I don't seem to have convinced you, here are some more arguments for why this feature belongs in `generateblock` instead of a new RPC:
- New RPCs require adding more code to test, and once added must go through a deprecation cycle to be removed (if they can be removed at all without disrupting too many users).
- Consumers of the RPCs must write additional code for the ne
...