💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1665607508)
Good suggestion thanks, implemented.

📝 maflcko opened a pull request: " net_processing: use existing RNG object in ProcessGetBlockData "
(https://github.com/bitcoin/bitcoin/pull/30393)
Two small rand fixups. The debug mode one is a follow-up to commit 7af95afa8bc3f36a37d082ef3475cb3e0bd3a0e4 and https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1658344049. The RNG object one to commit 8e31cf9c9b5e9fdd01e8b220c08a3ccde5cf584c

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665615924)
First half done in https://github.com/bitcoin/bitcoin/pull/30393

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665616205)
Done in https://github.com/bitcoin/bitcoin/pull/30393

💬 fjahr commented on pull request "validation: Check if mempool exists before size check in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/pull/30388#discussion_r1665621472)
nit: The type doesn't seem so complex that it has to be auto

```suggestions
CTxMemPool* mempool{m_active_chainstate->GetMempool()};
```

🤔 fjahr reviewed a pull request: "validation: Check if mempool exists before size check in ActivateSnapshot"
(https://github.com/bitcoin/bitcoin/pull/30388#pullrequestreview-2158787105)
ACK 33c48c106cf03ff62994ff5777a775982d90eab9

While this isn't something that can be hit by users I agree it makes sense conceptually. I am not sure this will help the performance of the tests much as I have laid out in my comment here: https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207510865

💬 willcl-ark commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2208839479)
My build on x86_64 (excluding SHA256SUMS.part):

```
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd bitcoin-f59e9057e2aa-aarch64-linux-gnu.tar.gz
99712894848ef7a0b90b3a8582d617809e986abda202e9d4941ee970ca0d9a3d bitcoin-f59e9057e2aa-arm-linux-gnueabihf-debug.tar.gz
aec34ff47cf315949f2ddbf4eb0ed69dd2ec278b20ab7909a16ce48abd41e7ee bitcoin-f59e9057e2aa-arm-
...

💬 fjahr commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2208845865)
utACK b9ba1a73094f4ad593b527e23d2681f41c225397

👍 willcl-ark approved a pull request: "depends: build libevent with CMake"
(https://github.com/bitcoin/bitcoin/pull/29835#pullrequestreview-2158819949)
ACK f59e9057e2aa596b54cf9e85bab35c3ead137547

Tested locally and with guix build. Changes all look good to me, with equivalent options being set where possible.

There are not direct equivalents of `--disable-dependency-tracking` and `--enable-option-checking` for cmake, but this doesn't seem like a problem for us here.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2208857200)
> **transaction_identifier.h** - Fixed dormant bug in `TxidFromString()` where the `string_view` length wasn't respected(!).

This is known, see https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1404437378. Thanks for picking it up!

Maybe submit the fix first?



> Realizing my GH history has minimum proof of work, I might have delayed creating a PR, but it felt timely as Testnet 4 is being worked on.

Not sure what this has to do with testnet 4. May be best to remove unrelate
...

💬 maflcko commented on pull request "validation: Check if mempool exists before size check in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/pull/30388#issuecomment-2208864426)
re-ACK 33c48c106cf03ff62994ff5777a775982d90eab9

🤔 fjahr reviewed a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2158854946)
tACK 29ab88dfe1c43af3620480c99cba844aa414023c

I reviewed the code and confirmed that the test fails as expected when the behavior change is not applied.

💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665663972)
Huh, either I am confused or something isn't right here: The `parent` above is at height `SNAPSHOT_BASE_HEIGHT - 1` and now the child of this `parent` has a coinbase also at height `SNAPSHOT_BASE_HEIGHT - 1`? It seems this isn't validated so it's not a huge issue though.

💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665666251)
nit: Comment could be a little more detailed

```suggestion
# On node 0, create an alternative chain containing 2 new blocks, forking off the main chain at the block before the snapshot block. This simulates a longer chain than the main chain when submitting these two block headers to node 1 because it is only aware of the main chain headers up to the snapshot height.
```

💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665674679)
nit: making the variables a bit more detailed could make the test a bit easier to parse

`parent` = > `parent_block_hash`
`fork1` => `fork_block1`
`main1` = `main_block1`

💬 hebasto commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#issuecomment-2208959206)
My Guix build:
```
x86_64
33e4e9bba968ed9ee96f67a8535b71bc1b1c5ede7038a2b636f406262474ddca guix-build-98ff3703b81f/output/aarch64-linux-gnu/SHA256SUMS.part
d670e9a8cc34c7b8e9567ee8654b5323ee102e18cbbd53d541deae779100f344 guix-build-98ff3703b81f/output/aarch64-linux-gnu/bitcoin-98ff3703b81f-aarch64-linux-gnu-debug.tar.gz
8a96d7fb1539524b8d29eb768cf3d4ed2428df203fba3bce0ff173b733cb8d51 guix-build-98ff3703b81f/output/aarch64-linux-gnu/bitcoin-98ff3703b81f-aarch64-linux-gnu.tar.gz
d0e0d9556
...

👍 dergoegge approved a pull request: "refactor: use existing RNG object in ProcessGetBlockData"
(https://github.com/bitcoin/bitcoin/pull/30393#pullrequestreview-2158916349)
utACK fa01c2af9eb91aebcdb87947145ba34b519bdfd8

There was some discussion on #29625 about reverting #28558 but this lgtm for now.

👍 dergoegge approved a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2158685410)
Code review ACK 17d41e7a547f16f0dd3802dac73a63772ca000b2

💬 dergoegge commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1665560023)
```suggestion
virtual void ActiveTipChange(const CBlockIndex* new_tip, bool is_ibd) {};
```

👍 hebasto approved a pull request: "contrib: use c++ compiler rather than c compiler for binary checks"
(https://github.com/bitcoin/bitcoin/pull/30387#pullrequestreview-2158920731)
ACK 98ff3703b81fbcece22eed55433cfe0fe101704f, I have reviewed the code and it looks OK. The Guix build script works as expected..