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

💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-2895815514)
Can someone with permissions turn this into draft?

✅ fjahr closed a pull request: "RFC: Accept non-std transactions in Testnet4 by default again"
(https://github.com/bitcoin/bitcoin/pull/32133)

💬 fjahr commented on pull request "RFC: Accept non-std transactions in Testnet4 by default again":
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2895823345)
> Only after you've signed the transaction in your test environment with a real mainnet key that's protecting real mainnet funds.

I think it's reasonable for a bitcoin company to have a few sats in a mainnet hot wallet to use for testing, they don't need to touch their cold storage for this.

Anyway, closing for now, my opinion on this hasn't changed much but I think it's better to focus on something else.