💬 l0rinc commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2483381183)
Thanks @hebasto, addressed the bench and fuzz ones as well (in separate commits, explaining the decisions)

📝 Sjors opened a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318)
Providing a script for the coinbase transaction is only done in test code and for CPU solo mining.

Production miners use the getblocktemplate RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.

This commit removes the script_pub_key argument from createNewBlock() in the Mining interface.

A coinbase script can still be passed via BlockCreateOptions instead. Tests are modified to do so.

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1846795785)
#31318 drops this argument. It's only slightly easier to merge that first, so I'll leave this PR open.

💬 furszy commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1846797832)
What about checking `TryCreateDirectories` too? And doing this in a loop until it creates a new dir?
Probably we might also need to lock the directory like we do for the custom datadir path.

💬 hebasto commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2483394112)
> TODO:
>
> * [ ] fix `arm-linux-gnueabihf-g++: error: unrecognized command-line option ‘-mpclmul’` [failure](https://cirrus-ci.com/task/4621127983562752) and re-enable multiprocess for arm job

I don't think the problem description is correct.

In my opinion, the root of the issue is:
```
$ make --no-print-directory -C depends HOST=arm-linux-gnueabihf print-multiprocess_packages
multiprocess_packages=
```

💬 fanquake commented on pull request "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC":
(https://github.com/bitcoin/bitcoin/pull/31307#issuecomment-2483395447)
Can you link to your upstream bug report.

💬 l0rinc commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#issuecomment-2483397450)
ACK 3b24c24889c09f6c8f33408b57c2042c4e565388

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2483402160)
The PR was prompted because I noticed in #31283 I had to needlessly pass `script_pub_key` around.

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1846807424)
tidy doesn't like this
```suggestion
ThreadPool() = default;
```

💬 hebasto commented on pull request "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC":
(https://github.com/bitcoin/bitcoin/pull/31307#issuecomment-2483402528)
> Can you link to your upstream bug report.

I haven’t reported this upstream yet, as I’ve been struggling to create minimal reproducible code.

💬 maflcko commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846813317)
not sure about making this optional. Even if this is a dummy, it would be good to spell it out.

Using `std::optional` here is just making code more complex, adding two lines of code in one place to save one line of code in another place.

Also, it may be better to call mention *output* script in the name.

💬 ryanofsky commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2483435630)
re: https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2481190404

> What are the steps to test this with sqlite? If it isn't needed after the bdb removal, it can probably be skipped?

Not sure if this will always be the case, but it is currently possible to create an sqlite wallet with no name with `bitcoin-cli -regtest createwallet ""`.

Regardless of the details of the bug though, I think the real problem is that the `GetDisplayName()` method name suggests it should be used to g
...

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1846841935)
In production this is always a dummy. `CreateNewBlock()` sets the script to `OP_TRUE` and then the `getblocktemplate` RPC omits the whole coinbase transaction.

It's not safe for the caller to set an _arbitrary_ dummy, because IIUC if it's larger than `coinbase_max_additional_weight` the `TestBlockValidity` internal check for `getblocktemplate` will spuriously fail. Which is why I think it's better for `CreateNewBlock` to decide what the dummy should be.

💬 maflcko commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1846842029)
> loop

I am not sure that running tests in a loop until they pass is a good approach. Generally, when there is an issue, it should be addressed, instead of being silently hidden.

> Probably we might also need to lock the directory like we do for the custom datadir path.

Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests.

Al
...

🤔 hebasto reviewed a pull request: "refactor: Fix remaining clang-tidy performance-inefficient-vector errors"
(https://github.com/bitcoin/bitcoin/pull/31305#pullrequestreview-2443044464)
Tested 01e54b7d82a053238b62121eeba2ac2c45b88859 on Ubuntu 24.04. The `clang-tidy-19` with the enabled "performance-inefficient-vector-operation" check is happy.

🤔 furszy reviewed a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2443054669)
Code ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3

💬 maflcko commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#discussion_r1846855789)
This seems backwards? Fuzz engines will generally prefer smaller fuzz inputs over larger ones, given identical coverage metrics. Optimizing for the worst case, which will likely never happen, and possibly pessimising the happy case reads odd.

Either this has no effect at all on the fuzz target anyway, in which case the code reads odd, or this is actually pessimising.

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2483491305)
Renamed to `coinbase_output_script`, added warning and discouragement, fixed fuzz binary build.

💬 flack commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846892284)
Nit: double semicolon

💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846901519)
resolved both!