💬 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?
💬 copy2018 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873505859)
Concept NACK
Relaxing the datacarriersize limit may seem appealing for certain niche use cases, but it comes with significant trade-offs that contradict Bitcoin’s fundamental goals.
First, there is no **pressing** indication that this relaxation is necessary at the current time. The current limit strikes a balance between enabling modest data-carrier use cases and protecting the network's efficiency, decentralization, and sustainability. Introducing unnecessary changes risks opening the do
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873505859)
Concept NACK
Relaxing the datacarriersize limit may seem appealing for certain niche use cases, but it comes with significant trade-offs that contradict Bitcoin’s fundamental goals.
First, there is no **pressing** indication that this relaxation is necessary at the current time. The current limit strikes a balance between enabling modest data-carrier use cases and protecting the network's efficiency, decentralization, and sustainability. Introducing unnecessary changes risks opening the do
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2873513948)
re: Sjors https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771
> @ryanofsky CI complains about `std::move` in `libmultiprocess`: https://cirrus-ci.com/task/6187773452877824, e.g.:
Thanks for catching this. I implemented a fix for that error and the other errors in https://github.com/bitcoin-core/libmultiprocess/pull/172
If you wanted to test the fix here you could do:
```shell
git fetch https://github.com/bitcoin-core/libmultiprocess 8d8e9f65f1d9faaafe91ca90910c1f96
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2873513948)
re: Sjors https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771
> @ryanofsky CI complains about `std::move` in `libmultiprocess`: https://cirrus-ci.com/task/6187773452877824, e.g.:
Thanks for catching this. I implemented a fix for that error and the other errors in https://github.com/bitcoin-core/libmultiprocess/pull/172
If you wanted to test the fix here you could do:
```shell
git fetch https://github.com/bitcoin-core/libmultiprocess 8d8e9f65f1d9faaafe91ca90910c1f96
...
💬 xstoicunicornx commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873550444)
>Bitcoin’s success hinges on its decentralization and accessibility.
>Moreover, out-of-band services that cater to data-carrier transactions are not inherently harmful to the network. They exist because Bitcoin was intentionally not designed as a general-purpose data storage layer, but rather as a secure payments system. Private markets offering these services reflect a willingness to pay for non-standard transactions, and this market demand does not justify altering Bitcoin’s core design to
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873550444)
>Bitcoin’s success hinges on its decentralization and accessibility.
>Moreover, out-of-band services that cater to data-carrier transactions are not inherently harmful to the network. They exist because Bitcoin was intentionally not designed as a general-purpose data storage layer, but rather as a secure payments system. Private markets offering these services reflect a willingness to pay for non-standard transactions, and this market demand does not justify altering Bitcoin’s core design to
...
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873582873)
@xstoicunicornx It's LLM spam
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873582873)
@xstoicunicornx It's LLM spam
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085229102)
Hmm, this didn't change between legacy and witv0, I think?
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085229102)
Hmm, this didn't change between legacy and witv0, I think?
💬 joshdoman commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2873627043)
@JeremyRubin I think this is a great idea. I made a PR into Peter Todd's repo doing something similar, which may or may not be useful (https://github.com/petertodd/bitcoin/pull/10).
Primary difference is I modified execdata like @instagibbs described and add a single line `execdata.m_annex_committed = execdata.m_annex_present;` to the end of CheckSchnorrSignature. This eliminates ~6 lines of code.
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2873627043)
@JeremyRubin I think this is a great idea. I made a PR into Peter Todd's repo doing something similar, which may or may not be useful (https://github.com/petertodd/bitcoin/pull/10).
Primary difference is I modified execdata like @instagibbs described and add a single line `execdata.m_annex_committed = execdata.m_annex_present;` to the end of CheckSchnorrSignature. This eliminates ~6 lines of code.
👍 pinheadmz approved a pull request: "doc: Fix typo"
(https://github.com/bitcoin/bitcoin/pull/32472#pullrequestreview-2834167326)
utACK trivial typo fix in error string
(https://github.com/bitcoin/bitcoin/pull/32472#pullrequestreview-2834167326)
utACK trivial typo fix in error string
💬 pinheadmz commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2085261557)
Since you're touching this doc, you could also add a note about UNIX sockets which I should've added in #27375 ;-)
e.g.
```
UNIX domain sockets may be used for proxy connections. Set `-onion` or `-proxy`
to the local socket path with the prefix `unix:` (e.g. `-onion=unix:/home/me/torsocket`).
```
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2085261557)
Since you're touching this doc, you could also add a note about UNIX sockets which I should've added in #27375 ;-)
e.g.
```
UNIX domain sockets may be used for proxy connections. Set `-onion` or `-proxy`
to the local socket path with the prefix `unix:` (e.g. `-onion=unix:/home/me/torsocket`).
```
🚀 fanquake merged a pull request: "doc: Fix typo"
(https://github.com/bitcoin/bitcoin/pull/32472)
(https://github.com/bitcoin/bitcoin/pull/32472)