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

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2098877081)
Not really, seems like it was changed in that specific file and resolved without being changed more broadly.

💬 achow101 commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2895845004)
> `createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.

`fundrawtransaction` does though, and that was my point. This PR doesn't touch `fundrawtransaction` (or rather the wallet's coin selection code) and hence this is incomplete.

🤔 mzumsande reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2855643647)
Code Review ACK 2b202e9db56487e658fc41089178f31ef4259a0d

👋 fjahr's pull request is ready for review: "RFC: refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)

💬 fjahr commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895912036)
Cleaned up the comments so this should be ready for more detailed feedback, but expect a rebase once https://github.com/bitcoin/bitcoin/pull/32467 is merged.

💬 fjahr commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2098936599)
I have been contemplating kicking this out (here and in `CheckInputScripts`) or even turning this into an assert because the callers already handle this everywhere. On the other hand it conceptually makes sense to return early if we know there is nothing to check because we don't expect any inputs. So I am still undecided on this one.

💬 TheCharlatan commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895950866)
Concept ACK

💬 davidgumberg commented on pull request "qt: drop unused watch-only functionality":
(https://github.com/bitcoin/bitcoin/pull/32459#issuecomment-2895951950)
Review ACK https://github.com/bitcoin/bitcoin/commit/e8661aac752eb08fee318eb8f56e599578d78f9f.

Quickly tested with GUI built on master and GUI built on this branch. Confirmed the watch-only behavior removed here has nothing to do with no-private-keys descriptor wallets, this is dead code AFAICT.