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

> I would like if it will emit the help text of `rpc` command instead.

Makes sense! I expanded the "Integrating help" followup note in the description and linked to this comment.

💬 willcl-ark commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2895686124)
Mismatching hashes on my build:

```
src/core/bitcoin on  alpine_MKDIRPROG:refs/pull/32568/head [$] via △ v3.31.6 via 🐍 v3.12.10 via ❄️ impure (nix-shell-env) took 1h13m42s
❯ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum

cb7dbe6112cdd38db9757af64cd4e617cdced8a6525db8a1469f3f35d29de9f6 guix-build-df9ebbf659d5/output/aarch64-linux-gnu/SHA256SUMS.part
a0b2b6c3fd30ae9aa7667b99f0ac2fe7d59725fff6fef547bd73f2cc5b50b1b3
...

⚠️ hebasto opened an issue: "`system_tests/run_command` test fails on illumos OS"
(https://github.com/bitcoin/bitcoin/issues/32574)
On OpenIndiana:
```
./test/system_tests.cpp(54): error: in "system_tests/run_command": check what.find(tfm::format("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos has failed
./test/system_tests.cpp(55): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
```

Same on [OmniOS](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15121791773/job/42505618501):
```
./test/system_tests.cpp(54): error: in "system_tes
...

💬 maflcko commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-2895744563)
Looks like CI failed

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2895764479)
> users can use `createrawtransaction` to create a v3 transaction, fund it with `fundrawtransaction`, and then wind up with a v3 transaction that they cannot broadcast.

`createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.

Though, CI is failing on this pull.

💬 maflcko commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r2098827990)
```suggestion
"Returns the messages count and total number of bytes for network traffic.\n"
```

the ci failure is a bit vague, but I guess this would fix it

💬 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_r2098836650)
This whole conversation reminded me that I really wanted to split up `CheckInputScripts` after interacting with it for the batch validation stuff. I will open a draft of that for conceptual feedback.

💬 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-2895776416)
re-ACK fd290730f530a8b76a9607392f49830697cdd7c5

💬 maflcko commented on pull request "Prune locks":
(https://github.com/bitcoin/bitcoin/pull/19463#discussion_r2098837042)
```suggestion
"Returns a list of pruning locks.\n",
```

the ci failure is a bit vague, but this should fix it

💬 maflcko commented on pull request "Prune locks":
(https://github.com/bitcoin/bitcoin/pull/19463#discussion_r2098838883)
nit: the error log message looks already unique enough and the `__func__` can be removed

💬 maflcko commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2895783055)
for reference, the CI failure is:

```
[16:36:22.725] /ci_container_base/src/test/transaction_tests.cpp:1089:20: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
[16:36:22.725] 1089 | tx_create.vout.push_back(CTxOut(424242, max_sigops_p2sh));
[16:36:22.725] | ^~~~~~~~~~~~~~~~~ ~
[16:36:22.725] | emplace_back(
[16:36:22.725] /ci_container_base/src/test/transaction_tests.cpp:
...

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2895784312)
Yeah i'm changing for `emplace_back` and addressing @Sjors' nits right now.

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2098849030)
Fine, but went for a more compressed version:
```suggestion
* We also check the total number of legacy sigops across the whole transaction, as per BIP54.
```

Looks good?

💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2098855504)
Fine, done.

🤔 ryanofsky reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-2714456634)
Rebased fae300f159cd25a12abf4d5fbb93135cececd38d -> 6645095e6ae0d9c208a47fc7322d859c4fe6afb0 ([`pr/mine.22`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.22) -> [`pr/mine.23`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.23), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.22-rebase..pr/mine.23)) due to conflicts with #28710 and also pulled in MakeBasicInit function to share code with #32297

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2012509342)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2012199249

> nit if you re-touch: Probably a typo in the program name?

Thanks, fixed

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2098846198)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2096479030

Yes I think depending on what uses there may for this in the future I think it could be renamed to something more generic. For example I could see this potentially being used to test more IPC functionality from functional tests, and if that would happen a name like `bitcoin-ipc` or `bitcoin-test-ipc` could make more sense. I think for now the name `bitcoin-mine` is reasonable since it's only connecting and using the mini
...

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2098837784)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2096468681

Good suggestion, this is now moved to an earlier commit, and it is now sharing code with #32297 (MakeMineInit is now replaced by MakeBasicInit originally introduced in that PR)

📝 fjahr opened a pull request: "RFC: refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)
This topic has been motivated by my work on batch validation and a related conversation just happened here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199

`CheckInputScripts` currently only does what its name implies if there is no multithreading usage for validation. If there are worker threads available the function instead only creates the checks and puts them into a vector. This dual use is awkward and hard to grasp. Splitting up the single thread case from the mult
...

💬 theuni commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895813848)
Concept ACK. Anything to clean this up :)

See also #32317 which moves some of this functionality around similarly.