🤔 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()`.
💬 mzumsande commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085053587)
nit: could also call just introduce and use `WriteBestBlock()` here since `SetLastBlockProcessedInMem` is already called above.
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085053587)
nit: could also call just introduce and use `WriteBestBlock()` here since `SetLastBlockProcessedInMem` is already called above.
🤔 l0rinc reviewed a pull request: "doc: Fix typo"
(https://github.com/bitcoin/bitcoin/pull/32472#pullrequestreview-2833955701)
If we're already tackling this, we could fix some of these as well (some are less important than others, but at least we'd be consistent):
* https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py#L128
* https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_reindex.py#L86
* https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L238
* https://github.com/bitcoin/bitcoin/blob/master/src/test/bip32_test
...
(https://github.com/bitcoin/bitcoin/pull/32472#pullrequestreview-2833955701)
If we're already tackling this, we could fix some of these as well (some are less important than others, but at least we'd be consistent):
* https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py#L128
* https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_reindex.py#L86
* https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L238
* https://github.com/bitcoin/bitcoin/blob/master/src/test/bip32_test
...
🤔 janb84 reviewed a pull request: "doc: Fix typo"
(https://github.com/bitcoin/bitcoin/pull/32472#pullrequestreview-2833964876)
good find, ACK https://github.com/bitcoin/bitcoin/pull/32472/commits/d847e17c9656020de9f378ae82ec89e0bb39ecbd
- "code" review ✅
- quick search for more, could not find others ✅
(https://github.com/bitcoin/bitcoin/pull/32472#pullrequestreview-2833964876)
good find, ACK https://github.com/bitcoin/bitcoin/pull/32472/commits/d847e17c9656020de9f378ae82ec89e0bb39ecbd
- "code" review ✅
- quick search for more, could not find others ✅
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873422307)
@donaldevinev1 (just in case this isn't an LLM answer) there are no quadratics introduced here, downstream parsers doing bizarre things is not in scope for this PR.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873422307)
@donaldevinev1 (just in case this isn't an LLM answer) there are no quadratics introduced here, downstream parsers doing bizarre things is not in scope for this PR.
📝 sipa opened a pull request: "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts"
(https://github.com/bitcoin/bitcoin/pull/32473)
This introduces a per-txin cache for sighash midstate computation to the script interpreter for legacy (bare), P2SH, P2WSH, and (as collateral effect, but not actually useful) P2WPKH. This reduces the impact of certain types of quadratic hashing attacks that use standard transactions. It is not known to improve the situation for attacks involving non-standard transaction attacks.
The cache works by remembering for each of the 6 sighash modes a `(scriptCode, midstate)` tuple, which gives a mid
...
(https://github.com/bitcoin/bitcoin/pull/32473)
This introduces a per-txin cache for sighash midstate computation to the script interpreter for legacy (bare), P2SH, P2WSH, and (as collateral effect, but not actually useful) P2WPKH. This reduces the impact of certain types of quadratic hashing attacks that use standard transactions. It is not known to improve the situation for attacks involving non-standard transaction attacks.
The cache works by remembering for each of the 6 sighash modes a `(scriptCode, midstate)` tuple, which gives a mid
...
💬 stonecutter-group commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2873462743)
At first glance, the proposal to remove the 80-byte `OP_RETURN` limit may seem like a harmless concession for convenience. But in protocol design, as in sovereign systems, limits are not flaws—they are the foundation of order. This change, therefore, warrants careful consideration as it directly impacts the foundational design principles and long-term integrity of the Bitcoin protocol.
The arguments against such a modification are rooted in these core tenets:
1. **Bitcoin's Defined Purpos
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2873462743)
At first glance, the proposal to remove the 80-byte `OP_RETURN` limit may seem like a harmless concession for convenience. But in protocol design, as in sovereign systems, limits are not flaws—they are the foundation of order. This change, therefore, warrants careful consideration as it directly impacts the foundational design principles and long-term integrity of the Bitcoin protocol.
The arguments against such a modification are rooted in these core tenets:
1. **Bitcoin's Defined Purpos
...
💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085182810)
Should it be named `default_scriptcode_wsh` to be explicit that it implements the Segwit v0 (and not legacy) logic with regard to `OP_CODESEPARATOR` serialization?
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085182810)
Should it be named `default_scriptcode_wsh` to be explicit that it implements the Segwit v0 (and not legacy) logic with regard to `OP_CODESEPARATOR` serialization?