👍 TheCharlatan approved a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423#pullrequestreview-2194838525)
ACK 1bc9f64bee919bc46eb061ef8c66f936eb6a8918

💬 hebasto commented on pull request "depends: bump libmultiprocess for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30490#issuecomment-2246089575)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/277.

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688610458)
Agree the diff would have been simpler, in this instance I was attempting to revert back to what already had been discussed and ACK:ed before you joined the review. Thanks for helping improve this PR!

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688616750)
Let me try a different angle: if a method is inline, I would expect to be able to copy-paste its content to the call site directly (at least conceptually, to understand its effect).
If the method is pure in a functional sense (including that it doesn't even mutate its parameters), it's a lot easier to reason about its overall behavior.
Does this resonate more?

⚠️ maflcko opened an issue: "implicit-integer-sign-change in ActivateSnapshot"
(https://github.com/bitcoin/bitcoin/issues/30514)
```
$ echo 'dXR4b/8BAPq/tdph' | base64 --decode > ./crash

$ UBSAN_OPTIONS="suppressions=$PWD/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=utxo_snapshot ./src/test/fuzz/fuzz ./crash
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2946336184
INFO: Loaded 1 modules (625551 inline 8-bit counters): 625551 [0x572ffe28af78, 0x572ffe323b07),
INFO: Loaded 1 PC tables (625551 PCs): 625551 [0x572ffe323b08,0x572ffecaf3f8),
./
...

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