💬 polespinasa commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873672475)
> * `generateblock` is much more flexible than `generatetoaddress` (or `generatetodescriptor`), since it takes the txs to include as the second argument. For advanced test cases, it is likely that users would want this functionality to test multiple output coinbases with blocks that mine non-standard txs or otherwise don't mine the entire mempool. They would not have this capability with `generatetomany` as implemented here, but would have that for free with `generateblock`.
This is a fair po
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873672475)
> * `generateblock` is much more flexible than `generatetoaddress` (or `generatetodescriptor`), since it takes the txs to include as the second argument. For advanced test cases, it is likely that users would want this functionality to test multiple output coinbases with blocks that mine non-standard txs or otherwise don't mine the entire mempool. They would not have this capability with `generatetomany` as implemented here, but would have that for free with `generateblock`.
This is a fair po
...
💬 moth-oss commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873678445)
> @moth-oss config options are not removed in this pull request.
It is deprecating the option though. I don't think deprecating it now is the right move. Increase the default (ideally not disabled/max) and see how things go before making the decision to deprecate it.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873678445)
> @moth-oss config options are not removed in this pull request.
It is deprecating the option though. I don't think deprecating it now is the right move. Increase the default (ideally not disabled/max) and see how things go before making the decision to deprecate it.
🤔 glozow reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2834208934)
ACK 7208ec2f1f0
Light code review + tested by [implementing](https://github.com/glozow/bitcoin/commits/2025-04-package-validation/) a package linearizer + chunker on top of `TxGraph` and running #26711 unit tests. It is linearizing and chunking the transactions as I expect, which is great (it still has trouble with cases like RBF, chunks not fitting, and skipping non-descendants, but definitely out of scope for this PR).
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2834208934)
ACK 7208ec2f1f0
Light code review + tested by [implementing](https://github.com/glozow/bitcoin/commits/2025-04-package-validation/) a package linearizer + chunker on top of `TxGraph` and running #26711 unit tests. It is linearizing and chunking the transactions as I expect, which is great (it still has trouble with cases like RBF, chunks not fitting, and skipping non-descendants, but definitely out of scope for this PR).
💬 darosior commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085282675)
It did, legacy skips `OP_CODESEPARATOR`s in the suffix (i.e. "upcoming `CODESEP`s") whereas Segwit v0 doesn't.
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085282675)
It did, legacy skips `OP_CODESEPARATOR`s in the suffix (i.e. "upcoming `CODESEP`s") whereas Segwit v0 doesn't.
💬 willcl-ark commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873707039)
Reminder that this project's current [moderation guidelines](https://github.com/bitcoin-core/meta/blob/main/MODERATION-GUIDELINES.md) describe what we expect of contributors to this repository.
To keep it productive, especially on threads like these please:
- Stay on-topic: discuss the patch, not Bitcoin in general.
- Critique ideas, not motives.
- Contribute new signal: avoid repeating arguments already made earlier (these will start being marked as duplicate, to make the thread easier
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873707039)
Reminder that this project's current [moderation guidelines](https://github.com/bitcoin-core/meta/blob/main/MODERATION-GUIDELINES.md) describe what we expect of contributors to this repository.
To keep it productive, especially on threads like these please:
- Stay on-topic: discuss the patch, not Bitcoin in general.
- Critique ideas, not motives.
- Contribute new signal: avoid repeating arguments already made earlier (these will start being marked as duplicate, to make the thread easier
...
💬 sipa commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085298526)
Ah, yes, but this all happens before the call to `LegacySignatureMsg()`, which filters out the `OP_CODESEPARATOR`s that remain. In either case, this function `default_scriptcode_suffix` is exercised for both p2wsh and p2sh, so the fact that the test works should mean something...
(https://github.com/bitcoin/bitcoin/pull/32473#discussion_r2085298526)
Ah, yes, but this all happens before the call to `LegacySignatureMsg()`, which filters out the `OP_CODESEPARATOR`s that remain. In either case, this function `default_scriptcode_suffix` is exercised for both p2wsh and p2sh, so the fact that the test works should mean something...
💬 1440000bytes commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873734612)
> > @moth-oss config options are not removed in this pull request.
>
> It is deprecating the option though. I don't think deprecating it now is the right move. Increase the default (ideally not disabled/max) and see how things go before making the decision to deprecate it.
Deprecation notice can last for years: https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2861778194
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873734612)
> > @moth-oss config options are not removed in this pull request.
>
> It is deprecating the option though. I don't think deprecating it now is the right move. Increase the default (ideally not disabled/max) and see how things go before making the decision to deprecate it.
Deprecation notice can last for years: https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2861778194
🤔 BootsStribling reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2834259644)
Concept NACK.
While appearing to be a compromise, this PR uniquely does not achieve the desired outcomes of either faction as stated in [#32359]( https://github.com/bitcoin/bitcoin/pull/32359) and as such is ineffective.
You might say this is the nature of a compromise, but I find this "splitting the difference" strategy to be uniquely harmful in its result.
This PR achieves none of the stated benefits of #32359 which were:
* better fee estimation/ unification of mempool state
* redu
...
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2834259644)
Concept NACK.
While appearing to be a compromise, this PR uniquely does not achieve the desired outcomes of either faction as stated in [#32359]( https://github.com/bitcoin/bitcoin/pull/32359) and as such is ineffective.
You might say this is the nature of a compromise, but I find this "splitting the difference" strategy to be uniquely harmful in its result.
This PR achieves none of the stated benefits of #32359 which were:
* better fee estimation/ unification of mempool state
* redu
...
💬 bigshiny90 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873751241)
> > > @moth-oss config options are not removed in this pull request.
> >
> >
> > It is deprecating the option though. I don't think deprecating it now is the right move. Increase the default (ideally not disabled/max) and see how things go before making the decision to deprecate it.
>
> Deprecation notice can last for years: [#32406 (comment)](https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2861778194)
Deprecation is a clear signal that a feature will be removed in the futu
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873751241)
> > > @moth-oss config options are not removed in this pull request.
> >
> >
> > It is deprecating the option though. I don't think deprecating it now is the right move. Increase the default (ideally not disabled/max) and see how things go before making the decision to deprecate it.
>
> Deprecation notice can last for years: [#32406 (comment)](https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2861778194)
Deprecation is a clear signal that a feature will be removed in the futu
...
💬 n4HeVQSGqDeEu6 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873760187)
Concept NACK.
There clearly is no consensus, moving forward now will completely destroy the core reputation.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873760187)
Concept NACK.
There clearly is no consensus, moving forward now will completely destroy the core reputation.
💬 andrewtoth commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873767514)
Consider if we already had the functionality for this in `generateblock` - what would be the motivation to add a new RPC `generatetomany`?
Note that `sendtoaddress` and `sendmany` have both been superseded by `send`, but the older RPCs can't easily be removed. `sendall` is an interesting case, which we might want to mimic instead of adding a `remainder` field.
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2873767514)
Consider if we already had the functionality for this in `generateblock` - what would be the motivation to add a new RPC `generatetomany`?
Note that `sendtoaddress` and `sendmany` have both been superseded by `send`, but the older RPCs can't easily be removed. `sendall` is an interesting case, which we might want to mimic instead of adding a `remainder` field.
💬 0106003 commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2873770410)
-------- البريد الإلكتروني الأصلي --------
من: Pieter Wuille ***@***.***>
مرسل: ١٢ مايو ٢٠٢٥ ١٠:١٦:٢٢ م GMT+03:00
ل: bitcoin/bitcoin ***@***.***>
نسخة: Subscribed ***@***.***>
موضوع: Re: [bitcoin/bitcoin] Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts (PR #32473)
@sipa commented on this pull request.
> @@ -218,6 +222,20 @@ def default_controlblock(ctx):
"""Default expression for "controlblock": combine leafversion, negflag, pubkey_internal,
...
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-2873770410)
-------- البريد الإلكتروني الأصلي --------
من: Pieter Wuille ***@***.***>
مرسل: ١٢ مايو ٢٠٢٥ ١٠:١٦:٢٢ م GMT+03:00
ل: bitcoin/bitcoin ***@***.***>
نسخة: Subscribed ***@***.***>
موضوع: Re: [bitcoin/bitcoin] Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts (PR #32473)
@sipa commented on this pull request.
> @@ -218,6 +222,20 @@ def default_controlblock(ctx):
"""Default expression for "controlblock": combine leafversion, negflag, pubkey_internal,
...
💬 k98kurz commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873839263)
@BootsStribling
> This PR achieves none of the stated benefits of #32359 which were:
>
> * reduction of harm through the use of OP_RETURN rather than witness storage
>
> The ultimate result of this particular PR will be:
>
> * continued use of witness data field for arbitrary data incentivized by lower cost from witness discount
The harm reduction argument is that OP_RETURN use allows L2 users an alternative to stuffing arbitrary data into non-provably unspendable outputs that bloa
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2873839263)
@BootsStribling
> This PR achieves none of the stated benefits of #32359 which were:
>
> * reduction of harm through the use of OP_RETURN rather than witness storage
>
> The ultimate result of this particular PR will be:
>
> * continued use of witness data field for arbitrary data incentivized by lower cost from witness discount
The harm reduction argument is that OP_RETURN use allows L2 users an alternative to stuffing arbitrary data into non-provably unspendable outputs that bloa
...
💬 fjahr commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2085377969)
double-nit: maybe `m_queue`
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2085377969)
double-nit: maybe `m_queue`
💬 fjahr commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2873868970)
Code review ACK 5aca850c205a20a0f198827f9797fb8053f2b3dd
(https://github.com/bitcoin/bitcoin/pull/32467#issuecomment-2873868970)
Code review ACK 5aca850c205a20a0f198827f9797fb8053f2b3dd
👍 pinheadmz approved a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2834257645)
ACK 2059eaa692ff614c41dee50f4bc2d2946e5d42af
Built and ran all tests on macos/arm64. Reviewed both commits and left a few nits.
Played with the feature on mainnet, and connected to all networks:
```
ipv4 ipv6 onion cjdns total block manual
in 0 0 0 0 0
out 1 1 2 7 11 0 7
total 1 1 2 7 11
```
Even used tor's ipv4 and unix sockets for separate proxies ;-)
...
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2834257645)
ACK 2059eaa692ff614c41dee50f4bc2d2946e5d42af
Built and ran all tests on macos/arm64. Reviewed both commits and left a few nits.
Played with the feature on mainnet, and connected to all networks:
```
ipv4 ipv6 onion cjdns total block manual
in 0 0 0 0 0
out 1 1 2 7 11 0 7
total 1 1 2 7 11
```
Even used tor's ipv4 and unix sockets for separate proxies ;-)
...
💬 pinheadmz commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2085337724)
4af5b4d4298b4c69cd9131e87fe50e9386def13a
The default port here is a little funny since a user could set `-proxy=127.0.0.1=cjdns` right?
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2085337724)
4af5b4d4298b4c69cd9131e87fe50e9386def13a
The default port here is a little funny since a user could set `-proxy=127.0.0.1=cjdns` right?
💬 pinheadmz commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2085310297)
https://github.com/bitcoin/bitcoin/pull/32425/commits/4af5b4d4298b4c69cd9131e87fe50e9386def13a
nit: could move `Proxy onion_proxy;` from L1590 into this group
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2085310297)
https://github.com/bitcoin/bitcoin/pull/32425/commits/4af5b4d4298b4c69cd9131e87fe50e9386def13a
nit: could move `Proxy onion_proxy;` from L1590 into this group
🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2834221930)
Rebased 81c0b9edfe533afbb2f4dda56142afdedffdb347 -> 27874b9c6e10ac5b6e71bb5e17f44c58691684fe ([`pr/wrap.29`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.29) -> [`pr/wrap.30`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.30), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.29-rebase..pr/wrap.30)) due to conflict with #28710, also made many suggested improvements, particularly dropping windows escaping code and replacing with call to subprocess library
---
re:
...
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2834221930)
Rebased 81c0b9edfe533afbb2f4dda56142afdedffdb347 -> 27874b9c6e10ac5b6e71bb5e17f44c58691684fe ([`pr/wrap.29`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.29) -> [`pr/wrap.30`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.30), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.29-rebase..pr/wrap.30)) due to conflict with #28710, also made many suggested improvements, particularly dropping windows escaping code and replacing with call to subprocess library
---
re:
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085359836)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077503435
> nit: Could use util/string.h:
That's slightly simpler, but doesn't seem enough to want to construct and unneeded vector and pull in an extra dependency here. Worth keeping in mind if this code needs to be extended though.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085359836)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077503435
> nit: Could use util/string.h:
That's slightly simpler, but doesn't seem enough to want to construct and unneeded vector and pull in an extra dependency here. Worth keeping in mind if this code needs to be extended though.