💬 davidgumberg commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2895646341)
> > This change only partially resolves #32391
>
> What is left unsolved after this PR?

This comment from above: https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2846972614

Points out that the deprecated Windows `Crypt*` functions are still [invoked](https://github.com/gcc-mirror/gcc/blob/fba34a0cc55488ad89becf81cf2c9ac517d244d4/libssp/ssp.c#L80-L92) by GCC's `libssp` (`-fstack-protector`).

So the bitcoin core binaries won't really be free of those functions until GCC merge
...

📝 maflcko opened a pull request: "ci: Avoid && dropping errors"
(https://github.com/bitcoin/bitcoin/pull/32573)
In bash, `&&` will ignore errexit. This can lead to silently ignoring errors. Compare the output of:

```
$ bash -c 'set -xe; false && false ; true; echo $?'
+ false
+ true
+ echo 0
0
```

In theory this could be fixed by using a subshell:

```
$ bash -c 'set -xe; ( false && false ) ; true; echo $?'
+ false
```

However, it is easier to just remove the `&&`.

This was introduced in commit faa807bdf8c3002a28005b4765604f518a6f2736

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