👍 ryanofsky approved a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-2243634116)
Code review ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f

💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1720427422)
re: https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1719563783

> q in [f1c1b37](https://github.com/bitcoin/bitcoin/commit/f1c1b37d2247eed49156c0467daa68aa38497bb8): Can you explain why this change is correct?

Of course it is not correct. I only noticed that this function was updating `g_insecure_rand_ctx` and didn't notice it was updating global rng state. Thanks for fixing!

👍 ryanofsky approved a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2243660929)
Code review ACK 164732369fc5ecf4c7705136a2de884419023b5a. New commit replacing SeedRandomForTest is very nice and clarifies the code.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720368838)
Was removed it in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716588998 but then I went one step further and deferred the assert to the `base_blob` `Span`-ctor, making the comment more relevant again. Brought back now.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720392370)
Started going down the route of `std::span<const unsigned char, 32>` to enforce the constraint previously, got burnt and backed out a bit too far, `std::span` with dynamic extent and comment will do for this PR. :+1:

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720396199)
Leftover from half-way rename, will fix here and function below.

💬 whitslack commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294461104)
ACK 67b1e236334f38ec4e4d2251dbdfb790f20ed88b on Gentoo, modulo our usual [hacks](https://github.com/whitslack/bitcoin/commit/d028d85b4e9ae99067968fe480bd0eefe06c8be0) (now adapted to CMake) to allow linking against system-installed libsecp256k1 and LevelDB.

One small gripe: When we enter the install phase, CMake always rebuilds `src/clientversion.cpp`, which forces a relink of all target executables. It would be better if `ninja install` would not rebuild anything.

💬 Sjors commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2294705913)
re-ACK 221809b81cfcecb04050915eebacffda2599da42

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2294734148)
Addressed all code comments at c7bc444d02

——————————————

@gmaxwell

> That's like not-found, -- an idea that sounds attractive on it's face but on examination causes problems. For behavior that can't be prevented (like, say, inv-ing a transaction to you that already know), then it should obviously never disconnect for that. For behavior that can be prevented like sending a txn before an inv, sending an invalid block, etc.., peers are either compatible or they're not. If they're not you w
...

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1720699892)
fixed on latest push - good to have people not relying on github for code review.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#discussion_r1720702488)
The `ExpectedTx` have been updated to look by peer {peer, false, txhash} rather than iterating as we’re asking information for a given peer. I’ve not changed `ReceiveResponse` function call type, to avoid having unused value compiler errors, neither benchmarked yet to compare more the 2 approach.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2294752502)
I think the main open problem of this approach, and iiuc as pointed out by gmaxwell and aj, if it’s applied to upgraded peers only (`REJECT_UNSOLICITED_TX_VERSION`) is the occupation of inbound slots by buggy peers. Slots that could be rather used by more non-buggy peers to preserve robustness of the transaction-relay topology.

I think one way to alleviate is to introduce a new versioning of the transaction-relay protocol itself, which has not been really existent so far and that way have an
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256)
Rebased due to the conflicts. This PR branch is the current [staging branch](https://github.com/hebasto/bitcoin/commit/818a3c07fb48e4f7fdaf5640f3987d47280f54d0) with the top 3 commits dropped.

Some feedback has been addressed including:
- @fanquake's https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277722262
- @fanquake's https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2277886777
- @pablomartin4btc's https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2234
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737320)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737774)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737851)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737884)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1720737895)
Resolved in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294786256.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294789962)
@whitslack

> ACK [67b1e23](https://github.com/bitcoin/bitcoin/commit/67b1e236334f38ec4e4d2251dbdfb790f20ed88b) on Gentoo, modulo our usual [hacks](https://github.com/whitslack/bitcoin/commit/cmake-syslibs) (now adapted to CMake) to allow linking against system-installed libsecp256k1 and LevelDB.

Thanks for testing!

> One small gripe: When we enter the install phase, CMake always rebuilds `src/clientversion.cpp`, which forces a relink of all target executables. It would be better if `ni
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294793259)
During the recent CMake Working Group call, it was agreed to freeze this branch until it is merged, unless a severe bug is reported or a merge conflict arises. All other reported issues will be addressed in follow-up PRs.

Please continue testing this branch as thoroughly as possible.