💬 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
...
💬 aviv57 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873261060)
Concept ACK, if this won't be merged, actors will mine their non-standard transactions by contacting the miners directly and not via the p2p network.
in this way anyone willing to change their own mempool policy is still able to do it
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873261060)
Concept ACK, if this won't be merged, actors will mine their non-standard transactions by contacting the miners directly and not via the p2p network.
in this way anyone willing to change their own mempool policy is still able to do it
💬 chrisguida commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873262276)
Concept NACK, same rationale as https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2833932824
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873262276)
Concept NACK, same rationale as https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2833932824
💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2085079203)
Couldn't you use `ConsumeDeserializable` to create a `CBlock`? I think it might simplify the way you're creating the block and the transactions.
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2085079203)
Couldn't you use `ConsumeDeserializable` to create a `CBlock`? I think it might simplify the way you're creating the block and the transactions.
💬 donaldevinev1 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873306021)
Concept NACK, by removing the per-output cap and switching to a cumulative budget model, you're increasing the degrees of freedom in how data can be embedded in transactions. Nodes will have to sum all OP_RETURN output sizes to determine policy compliance, rather than applying a simple size check per output. This adds a non-trivial computational cost during mempool admission. Even if the cumulative size is bounded multiple small OP_RETURN outputs cause higher indexing and management overhead in
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873306021)
Concept NACK, by removing the per-output cap and switching to a cumulative budget model, you're increasing the degrees of freedom in how data can be embedded in transactions. Nodes will have to sum all OP_RETURN output sizes to determine policy compliance, rather than applying a simple size check per output. This adds a non-trivial computational cost during mempool admission. Even if the cumulative size is bounded multiple small OP_RETURN outputs cause higher indexing and management overhead in
...
🤔 mzumsande reviewed a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2833838561)
Code Review ACK f1f254f6c9d3cead617300367442c1bbb449af7c
(nits are not important)
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2833838561)
Code Review ACK f1f254f6c9d3cead617300367442c1bbb449af7c
(nits are not important)
💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085051375)
nit: could save a little bit of duplicated code by having `SetLastBlockProcessed()` call `WriteBestBlock()` (which means that the locking would have to happen in `RemoveWallet()`.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085051375)
nit: could save a little bit of duplicated code by having `SetLastBlockProcessed()` call `WriteBestBlock()` (which means that the locking would have to happen in `RemoveWallet()`.