💬 ryanofsky commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1851203430)
re: https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750

> Can't you skip the first conditional too then?

We do need the first condition because we want this to wait for m_tip_block to be set and for the value to be different than the current_tip value. Without the first condition this wouldn't wait at all for m_tip_block to be set.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2489948972)
Ready for a re-review when you're ready, @tdb3 @rkrux.

👍 ryanofsky approved a pull request: "Drop script_pub_key arg from createNewBlock"
(https://github.com/bitcoin/bitcoin/pull/31318#pullrequestreview-2450106183)
Code review ACK 1dfc54d87c7b35cc93cfb81cc2a46c9e8eae7194. Left a style suggestion, and a suggestion to split up the first commit, if we want to do that. But these can be ignored and this all looks good as-is. Makes a lot of sense to not make a parameter which should only be used for testing a required argument.

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

Would it make sense to change the `BlockAssembler::CreateNewBlock()` signature in a separate followup commit? The test changes in this commit vastly outnumber the non-test changes, which I think make it more likely a bug in the non-test code could be missed.

Maybe this there could be two overloads initially:

```c++
std::unique_ptr<CBlockTemplate> CreateNewBlock()
{
return CreateNewB
...

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