💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851080067)
You mean
```
LogInfo("Using random cookie authentication.\n");
```
was run independently of whether `GenerateAuthCookie` returned `true` or `false`, but now it may not be? It seemed wrong to me so I made it only happen when the cookie was actually generated.

If you mean that
```
LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth
...

💬 luke-jr commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2489692408)
>It currently doesn't give me any results for x9, IIRC the most common filter:

Same here

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851090795)
Yeah, was itching to do that too, thanks for a second vote. :)

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093183)
I want it to be easy for reviewers to drop the impl commits or re-order and run what is in the test commits to verify that I'm actually fixing issues.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093620)
https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851093183

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1851098868)
Good point about double negation. Changed to:
```C++
// If the file is explicitly specified, it must be readable
```
since the distinction is important.

👍 hodlinator approved a pull request: "macOS: swap docs & CI from pkg-config to pkgconf"
(https://github.com/bitcoin/bitcoin/pull/31335#pullrequestreview-2449903968)
utACK fa9e7fb167d7a1c476af931610ccbae39e29b964

Don't have a valid Mac to test on but change looks very straightforward and is in a similar vane to the linked upstream change.

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1851112761)
I was able to contrive a test that requires this loop, which also revealed a bug.

The situation is if a `sh(wsh(pkh()))` is imported with `importmulti`, the `wsh(pkh())` will only be in `mapScripts`. However, this `wsh(pkh())` is still `ISMINE_SPENDABLE` and coins sent to it will be seen by the wallet. This loop is where that would be detected.

👍 tdb3 approved a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333#pullrequestreview-2449906384)
ACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49

Lightly tested by running with `FUZZ=utxo_snapshot`.
Verified the observation in the description `<name>/<random>`.

💬 hodlinator commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2489745328)
> Add a workaround for #31334.

Sounds like a TODO? Why not "Fixes #31334"?

💬 luke-jr commented on pull request "rpc: print P2WSH witScript in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#discussion_r1851133029)
This isn't an indicator of p2wsh... Maybe use d1f0ff0e1fed2c4124d0d81f18daf7b76a770dda or similar?

💬 lucasbalieiro commented on pull request "contrib: fix BUILDDIR in gen-bitcoin-conf script":
(https://github.com/bitcoin/bitcoin/pull/31332#issuecomment-2489778682)
**Hi @MarnixCroes!**

**tACK** [b3a96b3](https://github.com/bitcoin/bitcoin/pull/31332/commits/b3a96b35691f3fbf958958056cd905e119fb48fe)

### **What I Understand**
The incoming changes to the build process introduce `cmake`, which builds in the `./build` directory. This PR updates the `BUILDDIR` path to correctly point to the new `./build` directory generated by `cmake`. This update is proposed to ensure compatibility with in-tree builds, as described in `contrib/devtools/README.md`:

> W
...

👍 lucasbalieiro approved a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(https://github.com/bitcoin/bitcoin/pull/31332#pullrequestreview-2449947660)
**Hi @MarnixCroes!**

**tACK** [b3a96b3](https://github.com/bitcoin/bitcoin/pull/31332/commits/b3a96b35691f3fbf958958056cd905e119fb48fe)

### **What I Understand**
The incoming changes to the build process introduce `cmake`, which builds in the `./build` directory. This PR updates the `BUILDDIR` path to correctly point to the new `./build` directory generated by `cmake`. This update is proposed to ensure compatibility with in-tree builds, as described in `contrib/devtools/README.md`:

> W
...

👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2450023917)
Code review ACK c305ad5b06545497752b2f12d8d9f72e8a6c2941. This is a nice change that should make the RPC (and test) more reliable, and the implementation seems to be pretty simple now.

💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851184397)
In commit "rpc: add optional blockhash to waitfornewblock" (c305ad5b06545497752b2f12d8d9f72e8a6c2941)

Might be useful if comment explained the different cases here. Maybe: // If caller provided a current_tip value, pass it to waitTipChanged. If the caller did not provide a current tip hash, call getTip() to get one and wait for the tip to be different from this value. (This mode is less reliable because if the tip changed between waitfornewblock calls, it will need to change a second time bef
...

💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1851191687)
In commit "rpc: add optional blockhash to waitfornewblock" (c305ad5b06545497752b2f12d8d9f72e8a6c2941)

Note: dropping IsRPCRunning call is a minor change in behavior, but probably a good one, as it should let the waitfornewblock function in the GUI console even if the RPC server is not enabled. In any case it is safe to drop this because waitTipChanged already will return if the node is shutting down.

💬 kevkevinpal commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#issuecomment-2489923051)
Concept ACK

👍 ryanofsky approved a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2450052356)
Code review ACK b283bc8dae0e0b714bdf1828579c67ab94da2cc7.

Can ignore this, but I want to express a slightly dissenting opinion on this change. I don't object to it but wouldn't have recommended it, because checking for `hash.IsNull()` is so widespread in the code, and I think writing interoperable code becomes more error prone when there are multiple ways of describing an unset hash instead of standardizing on one. But on the other hand I understand using `std::optional` can be more self-docu
...

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