💬 ryanofsky commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851238482)
In commit "Drop script_pub_key arg from createNewBlock" (42f880aeb080070a42229d0bccd517100a3fe7fb)

Would suggest using '*' instead of `.value()` since this code isn't intending to trigger the `std::bad_optional_access` exception, and to make it more obvious a dereference is happening.

👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177)
Code review and test ACK 878b6c85466366c4ae5f454ec49b5a5f561e0ed2

Really liking how this turned out.
Left a couple of minor things. None blocking.

<details>
<summary>quick retest with tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg</summary>

```
$ build/src/bitcoin-cli scanblocks start '["addr(tb1qr6u5sukpxq7pzh54gvmtgk0cg47u7dxnp7hzdg)"]'
{
"from_height": 0,
"to_height": 222935,
"relevant_blocks": [
"000000ddbaa40d82c7ae3e3188347dd77858f37e57aae0e51d004
...

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851274367)
non-blocking nit

If needing to touch this file again:

> If more than one name from a module is needed, use lexicographically sorted multi-line imports in order to reduce the possibility of potential merge conflicts

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

💬 tdb3 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1851301558)
For a follow-up (or if touching this file again before merge):

Could explicitly use `getnewdestination(address_type='bech32m')` instead of relying on the default.

Later in the function, the `output_spk` of the returned activity assumes p2tr. This currently works because bech32m is the default `address_type` argument of `getnewdestination()`. Being explicit prevents future changes from causing this test to fail.

💬 tdb3 commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2490065953)
Thanks. Planning to touch this up soon.

💬 naumenkogs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2490182594)
ACK 466e4df3fb83ef82b6add22e202e3a70dbf83a12

a trivial refactoring since the last ack

💬 1440000bytes commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2490302102)
> Later, the hashes are compared. That's exactly what this change is doing too but the syntax is different.

```c++
bool PartiallySignedTransaction::Merge(const PartiallySignedTransaction& psbt)
{
// Prohibited to merge two PSBTs over different transactions
if (tx->GetHash() != psbt.tx->GetHash()) {
return false;
}
```

Right. Only the error message was different which looks same now,

👍 1440000bytes approved a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298#pullrequestreview-2450523630)
utACK https://github.com/bitcoin/bitcoin/pull/31298/commits/85c216871a01aab003e1b1aba6246b4178afc929

🤔 maflcko reviewed a pull request: "build: Use `-ffile-prefix-map` in release builds only"
(https://github.com/bitcoin/bitcoin/pull/31337#pullrequestreview-2450548902)
Good catch, but I think the upstream docs were wrong in this case. For me, they don't mention coverage is affected as well: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffile-prefix-map

💬 maflcko commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1851542525)
Why is this needed? guix never compiles with `--coverage`, no? See https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fprofile-prefix-map

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2490530847)
Rebased after #31122.

I changed `coinbase_output_script` from `std::optional` to having a default of `OP_TRUE`.

I split the main commit into three parts:
1. Remove `scriptPubKeyIn` arg from `CreateNewBlock`, using a temporary overload to avoid test changes
2. Dropping the temp overload and change the tests
3. Change the interface

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851675109)
I changed `coinbase_output_script` to no longer be `std::optional`.

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851676023)
I made an overload, though slightly differently.

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851678502)
Hmmm, I though `-noversion` was meant to be exactly the same in each scenario as `-version=false`?

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851684788)
I usually soft reset and revert the non-test changes to test - even simpler usually than an interactive rebase.
I still think the change and test should be in the same commit to give proper context.

💬 fanquake commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490585705)
As far as I'm aware, Clang based coverage builds are working outside of oss-fuzz (cc @dergoegge), so it's not clear why this would be the right solution. If it's an oss-fuzz only problem, shouldn't the fix happen there?

> With this PR, only the -fdebug-prefix-map and -fmacro-prefix-map options are applied only for non-release builds.

Not sure about introducing these kinds of (undocumented) distinctions. By release do you just mean Guix build? Why should these options only apply to one buil
...

💬 fanquake commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1851692947)
If this was going to be done, this would need some sort of documentation, otherwise someone would likely assume this can be consolidated back to `-ffile-prefix-map`.

💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2490590565)
> Sounds like a TODO? Why not "Fixes https://github.com/bitcoin/bitcoin/issues/31334"?

Sure, changed to fixes. However it is a TODO in the sense that we shouldn't need to do any of this for a working CI, and ideally, these changes should be removed at some point, as they are all workaround for other problems.

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2490592436)
I also split out the renames in 55f2bb7a96806d33e475122f4d8f54b117838afe. They were an accidental find & replace, but useful to be consistent with the new `BlockCreateOptions` field.

💬 maflcko commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490598011)
> As far as I'm aware, Clang based coverage builds are working outside of oss-fuzz (cc @dergoegge), so it's not clear why this would be the right solution. If it's an oss-fuzz only problem, shouldn't the fix happen there?

This also affects GCC coverage, see https://cirrus-ci.com/github/maflcko/b-c-cov/ci, so the fix here is needed and likely correct (modulo my question/nit about the guix build).