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

💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851686045)
Nit: Is there one `coinbaseaux` too many here?

💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851693999)
In commit b35b23d823ac0fc43e4d137525167a7788ad672a:

Nit: I think we just directly call the new function here?

💬 fanquake commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490602573)
Sure, but it'd be good to get a better explanation as to how Clang builds in oss-fuzz are broken, if Clang builds outside of oss-fuzz are working.

💬 maflcko commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#discussion_r1851712070)
Looks like https://github.com/bitcoin/bitcoin/pull/31337/checks passed CI recently, so maybe this was just a temporary issue?

💬 Sjors commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#discussion_r1851712986)
This change was premature since the interface is changed in a later commit. It also doesn't do anything, so I'm dropping it.

💬 dergoegge commented on pull request "build: Use `-ffile-prefix-map` in release builds only":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)
Here is a recent example of clang coverage working for me: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin/harnesses/rpc/coverage_report/coverage/workdir/bitcoin/index.html

I'm slightly confused why oss-fuzz broke while my setup keeps working. Afaict I'm using the same flags and `llvm-*` tools as them...