💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024571708)
@glozow

I agree this needs a ML post. Will take care of this tomorrow.

However the motivation for this PR is not to reduce validation times, as it only marginally reduces the worst case for standard transactions. I don't think it's fair to qualify the motivation given in OP of "presumptuous" and it's certainly not a secondary motivation: if a soft fork is activated and such transactions are still standard it would be trivial to DoS any unupgraded miner into producing a block invalid accord
...

💬 hebasto commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3024595350)
> I was surprised that `make -C depends -j$(nproc) CC=clang CXX=clang++ MULTIPROCESS=1` didn't work.

FWIW, on some systems, such as [NetBSD](https://github.com/hebasto/bitcoin-core-nightly/blob/a1f9ee0d675cf74bf5caaec1131800c5d2b66893/.github/workflows/netbsd.yml#L127C21-L127C153), it may be necessary to be more specific about the compilers used to build native packages:
```
gmake -C depends MULTIPROCESS=1 build_CC=/usr/pkg/gcc14/bin/gcc build_CXX=/usr/pkg/gcc14/bin/g++ CC=/usr/pkg/gcc14/bi
...

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3024600108)
re-ACK 65b7deda12338b17f342c1d37f44e7c6da850771

([more future](https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177518858), [less signing](https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2175236097))

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2177952812)
Good to know you are verifying my findings. I hadn't seen your message before leaving my comment, looks like you already confirmed the first one, good.

Regarding how expensive transaction processing can be, yes this is interesting and something i have spent a fair bit of time looking into. However it's fairly tangential to this PR so let's discuss it separately. I DM'd you about this elsewhere.

💬 maflcko commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2178007144)
KeyError: 'immature_balance'

💬 pablomartin4btc commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2178024826)
It was removed in #32721.

👍 theStack approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976158406)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d

Makes sense to clarify, have tripped over this as well in the past.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2178048277)
Done! Also added the empty array case to the test and updated the description in before & after sections with it.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2178057885)
I think the benchmark is fine as it stands, for the purpose it serves (which is not to measure transaction processing time). I don't think more needs to be done here and will resolve this thread.

💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#issuecomment-3024771408)
<ins>_Updates_</ins>:
- Addressed @stickies-v's feedback correcting the error condition for `getdescriptoractivity`, adding also the empty array case in its corresponding test and updating the PR description in the before & after sections accordingly.

💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3024775910)
Apologies for the force push. However, it seems that the fuzz target actually produces valid block headers, so the chainman state is dirty and needs to be reset as well for those fuzz inputs. I've pushed a new commit doing this (after a rebase).

There is still non-determinism in `operator==<transaction_identifier>`, however I fail to see the source. I may take a look in the future.

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2178059683)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2176634257

> Maybe comment that the `!` is for gcc

Makes sense, added comment

🤔 furszy reviewed a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-2976212991)
I think this worth a general RPC util function like (haven't tested it):

```c++
template <typename... Value>
bool AreParamsNullOrEmpty(const Value&... params) {
return ((params.isNull() || params.empty()) && ...);
}
```

Which, for example should work fine for `getdescriptoractivity `:
```c++
if (AreParamsNullOrEmpty(request.params[0], request.params[1])) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "At least 1 blockhash or 1 descriptor must be specified.");
}
```

💬 maflcko commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024849863)
> It think it works on bitcoin testnet & mainnet network with version v22.0.0.
>
> I am wondering why it's not working with signet.

What are the exact steps to reproduce the working case and the failing case. What is the error message?

Without any details there is nothing that can be done here.

👍 brunoerg approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976319365)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d

🤔 w0xlt reviewed a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976354289)
ACK https://github.com/bitcoin/bitcoin/pull/32846/commits/0e9f409db3b7b08aef75ce39765b018b69cc8e9d

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3024909015)
@instagibbs @ismaelsadeeq See https://github.com/sipa/bitcoin/commit/0a8e1d6e39bd1ba6d3df149945b8dfd78e1fb7e6

👍 theStack approved a pull request: "test: check P2SH sigop count for coinbase tx"
(https://github.com/bitcoin/bitcoin/pull/32850#pullrequestreview-2976370364)
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

Good catch. Verified that on master, indeed all unit and functional tests pass if the tested code part in `GetP2SHSigOpCount` is changed (or removed), whereas unit tests fail with this PR.

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2178187277)
seems easier and less brittle to modify `GetWalletNameFromJSONRPCRequest` to:

```
const std::string wallet_name{GetWalletNameFromJSONRPCRequest(request, self.MaybeArg<std::string>("wallet_name")};
```

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3024957772)
Concept ACK. @SomberNight it's still useful if you open an issue to track the reason you need this, just in case this implementation PR doesn't make.