💬 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.

💬 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.

💬 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,
...

💬 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
...

💬 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`

💬 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

👍 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 ;-)


...

💬 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?

💬 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

🤔 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:
...

💬 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.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085292210)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942

Thanks, fixed both

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085306496)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474

Agree it would be nice to add a friendlier error message that would suggest how to fix the problem. Added this to the list of followups in the PR description that could make the command nicer & easier to use. My goal is to start off with the simplest working implementation and more niceties from there.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085373623)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077548018

> nit: Could verify we succeeded:

Good point. An assert seems extreme here since there could be some OS variation, but this case wasn't being handled well in the calling function and could do things like try to execute the same path twice. Add better handling now in caller.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085361665)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077515690

> nit: Could keep style consistent with function body rather than wrapped function.

Thanks, I can never remember where the spaces go.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085351896)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077299356

Good catch. If the vector was reallocated and string objects were using a small string optimization, the previous code could easily result in passing pointers to already freed memory. Fixed by adding a separate loop to construct new_argv. Also removed the c cast as that was unnecessary.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085362931)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077517153

Good catch, removed

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085288505)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2048440135

> if you retouch

Thanks, updated

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2085384043)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405

> nit: Child executable names leak through

Good suggestion, added it to the list in the PR description.

🤔 pablomartin4btc reviewed a pull request: "test: Remove legacy wallet RPC overloads"
(https://github.com/bitcoin/bitcoin/pull/32452#pullrequestreview-2834574797)
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7

Commit reorg proposed by @Sjors made sense.

On first commit, [test: Remove unnecessary importprivkey from wallet_createwallet](https://github.com/bitcoin/bitcoin/pull/32452/commits/94c87bbbd06eb9a57930b9f59315533cfbe8f460), the reasoning is more that `importprivkey` was only compatible with legacy wallets.

On the second commit, [test: Replace importprivkey with wallet_importprivkey](https://github.com/bitcoin/bitcoin/pull/32452/commits/fcc
...