💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2246139045)

Generally, I am not sure if the metadata is the right place to add the block height. It is just another field that may be corrupt (intentionally, or accidentally) and may lead to issues down the line, when used in a context that hasn't fully checked it.

My recommendation would be to just remove it from the metadata. The block hash should be sufficient to get the height (for logging and other purposes). And if the block headers are insufficient to get the height, `ActivateSnapshot` will erro
...

💬 maflcko commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2246141298)
Found https://github.com/bitcoin/bitcoin/issues/30514

🤔 stickies-v reviewed a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2194917738)
post-merge ACK 09ce3501fa2ea2885a857e380eddb74605f7038c

💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688621638)
Is the `ArithToUint256` conversion necessary?

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2246153002)
Updated 4e67086e4c0babb97be30bd9fe3c3b96e258a502 -> 79cfc0d198faecec598f923e6bf83ccf8dc09bbb ([`pr/mine-types.1`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.1) -> [`pr/mine-types.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.1..pr/mine-types.2)) to fix link error in https://github.com/bitcoin/bitcoin/pull/30510/checks?check_run_id=27810447228

💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1688628700)
Looks like it worked.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688630912)
Good point, I missed the `base_uint::ToString()` method. Not as familiar with `arith_uint256` as I am with `uint256` yet.

Should probably also document endianness of `arith_uint256` strings to mirror the added comments to `uint256` in this PR.

💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1688654352)
Yes, that would be along the lines of `uint256::FromHex()` that you have in the works in #30482.

One thing that comes to mind now though is that `uint256::FromHex()` can return a failure state. Maybe it would be better to have a constructor for this `consteval` thing - rename `FixedVec` into `ByteVec` with an added `consteval` constructor.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688658743)
FWIW, this is what the clusters should look like:

Even (mentally swap T4 and T6):
```mermaid
graph BT
T0["T0: 1"];T1["T1: 3"];T2["T2: 1"];T3["T3: 4"];T4["T4: 1"];T5["T5: 4"];T6["T6: 1"];T7["T7: 4"];T8["T8: 1"];T9["T9: 4"];T10["T10: 1"];T1-->T0;T1-->T2;T4-->T3-->T2;T4-->T5;T6-->T5;T4-->T7;T8-->T7;T4-->T9;T10-->T9;
```

Odd (mentally swap T3 and T5):

```mermaid
graph BT
T0["T0: 1/2"];T1["T1: 14/2"];T2["T2: 6/1"];T3["T3: 5/1"];T4["T4: 7/1"];T5["T5: 5/1"];T6["T6: 7/1"];T7["T7: 5/1"];T8
...

🤔 theuni reviewed a pull request: "depends: Fix CMake-generated `libzmq.pc` file"
(https://github.com/bitcoin/bitcoin/pull/30508#pullrequestreview-2194988671)
Concept ACK and utACK. I'd rather wait until upstream weighs in though, since we don't need this at least until we switch to CMake.

👍 theuni approved a pull request: "depends: Bump `libmultiprocess` for CMake fixes"
(https://github.com/bitcoin/bitcoin/pull/30513#pullrequestreview-2195010512)
Sorry for jumping the gun and causing extra churn with #30490. Hopefully this is the last bump for CMake :)

utACK ec0e805d11d6a73c542032fc49a58a1d05b62d24.

💬 ryanofsky commented on pull request "depends: Bump `libmultiprocess` for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30513#issuecomment-2246270392)
> Sorry for jumping the gun and causing extra churn with #30490. Hopefully this is the last bump for CMake :)

I probably should have asked about this when I reviewed #30490. I knew the cmake changes it included caused problems earlier, but I assumed the problems were fixed since then.

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2246282737)
Rebased 38c93e6a8e573dbdf3fc4d14be7483961e18a576 -> cf6d99bed842361fb06986780097fb80c9e879e4 ([`pr/indexy.51`](https://github.com/ryanofsky/bitcoin/commits/pr/indexy.51) -> [`pr/indexy.52`](https://github.com/ryanofsky/bitcoin/commits/pr/indexy.52), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/indexy.51-rebase..pr/indexy.52)) due to conflicts with #29671, #29776, #29867, #30058

> There's discussion in #29770 about whether to use some or all the things in this PR. Can you give it
...

🤔 Sjors reviewed a pull request: "index: Check all necessary block data is available before starting to sync"
(https://github.com/bitcoin/bitcoin/pull/29770#pullrequestreview-2194366605)
As a quick sanity check, I rebuilt `-coinstatsindex` and checked `gettxoutsetinfo` still gives me the same stats at block 840,000.

💬 Sjors commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r1688294883)
adabfbc23700056e5bd22b673c3078aa49ad83ea: it would be useful to have an example where `getblockfrompeer` _does_ work, i.e. an index that doesn't need undo data.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1688752764)
Bumped even further in https://github.com/bitcoin/bitcoin/pull/30513 :)

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1688758712)
Cross-posted in https://github.com/hebasto/bitcoin/issues/279 to make tracking easier.

📝 luisschwab opened a pull request: "rpc: add utxo's blockhash and number of confirmations to scantxoutset output"
(https://github.com/bitcoin/bitcoin/pull/30515)
This PR resolves #30478 by adding two fields to the `scantxoutset` RPC:
- blockhash: the blockhash that an UTXO was created
- confirmations: the number of confirmations an UTXO has relative to the chaintip.

The rationale for the first field is that a blockhash is a much more reliable identifier than the height:
> When using the scantxoutset RPC, the current behaviour is to show the block height of the UTXO. This is not optimal, as block height is ambiguous, especially in the case of a bloc
...

💬 luisschwab commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2246364773)
PR is up, reviews are appreciated.

🤔 hodlinator reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2194180912)
Concept ACK 9196cfe26da4784029500da482c6f7d55fca5ac2

### Concept

In https://github.com/bitcoin/bitcoin/pull/16545#discussion_r311440561 you say:

> Also this case really can and should be a compile error, not a runtime error.

That's along the lines of what I was going to bring up before reading the comments too. Hopefully this PR will take us further in that direction as you say.

### Commit messages

Both commits mention not changing application behavior 2 times each, seems a bit
...