👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1856695173)
Code review ACK 6752d218caeed1111f2520130c156b9ef42ba805. Just some cleanups and clarifications suggested by furszy since last review.

I think this PR is basically ready to merge, but there should be another ACK as it touches validation and some mempool code. The changes here should be very straightforward if any new reviewer wants to take a look.

Also @jamesob you may want to reack. The PR got a little smaller and simpler since your last review.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1474600461)
In commit "refactor: De-globalize g_signals" (09310dded4c063ae2a10d66082ba699a441a604d)

These changes seem to be part of wrong commit. The lines of code that these are referencing changed in earlier commit fe891bead4fabb945c21a7110459e3e18952a08c, not this commit, and now use `signals` instead of `m_signals`.

💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474641091)
You don't need a `Make*ByteSpan` when the underlying data is already bytes. Also, you don't need `Span` at all, when the object is an array. (arrays as well have their size not serialized)

💬 epiccurious commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1921562521)
Testing looks good so far:
```
TEST | STATUS | DURATION

feature_block.py | ✓ Passed | 67 s
feature_maxuploadtarget.py | ✓ Passed | 58 s
p2p_ibd_stalling.py | ✓ Passed | 7 s
p2p_ibd_stalling.py --v2transport | ✓ Passed | 8 s
p2p_invalid_messages.py | ✓ Passed | 15 s
p2p_timeouts.py | ✓ Passed | 1 s
p2p_timeouts.py --v2transport | ✓ Passed | 1 s
p2p_v2_earlykeyresponse.py
...

💬 jamesob commented on pull request "test: Assumeutxo with more than just coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/29354#issuecomment-1921591735)
Concept ACK

💬 epiccurious commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1921607926)
> I added a commit disabling a few select subtests

Rather than disabling, would it make sense to keep these subtests enabled not using v2transport?

📝 knst opened a pull request: "refactor: Remove excess reserve() call for SecureString"
(https://github.com/bitcoin/bitcoin/pull/29364)
This PR removes excess `reserve()` call for `SecureString`

Call `reverse` was introduced when `std::string` was used. For `std::string`, this makes sense as it prevents re-allocation when the string's size increases. However, with the use of a custom allocator, this call is no longer necessary.

👍 willcl-ark approved a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275#pullrequestreview-1856851136)
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547

Nice enhancements to safety, usability and standards.

🚀 fanquake merged a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275)

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474705042)
If I drop `MakeByteSpan()` the compiler complains:

```
./common/sv2_noise.h:68:17: error: no viable conversion from 'const std::array<unsigned char, SCHNORR_SIGNATURE_SIZE>' to 'Span<const value_type>' (aka 'Span<const std::byte>')
s.write(m_sig);
```

👍 fanquake approved a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1856895919)
ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183 - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do.

🚀 fanquake merged a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189)

✅ fanquake closed a pull request: "libconsensus: adapt API header to be compliant to ANSI C"
(https://github.com/bitcoin/bitcoin/pull/28661)

💬 fanquake commented on pull request "libconsensus: adapt API header to be compliant to ANSI C":
(https://github.com/bitcoin/bitcoin/pull/28661#issuecomment-1921685192)
Going to close this for now, given the deprecation / plan to remove (#29189).

💬 fanquake commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1921697022)
Could turn this PR into a full removal for 28.x? Otherwise I guess close, and we can followup with that in future?

✅ TheCharlatan closed a pull request: "build: Remove HAVE_CONSENSUS_LIB"
(https://github.com/bitcoin/bitcoin/pull/29123)

💬 TheCharlatan commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1921707277)
Seems cleaner to close and do the removal in a separate one. Might be good to open a PR for the full removal soon, so it can get enough review before branch-off.

💬 fanquake commented on issue "build warnings in outputtype.cpp: may be used uninitialized":
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1921734117)
> I wonder why g++ on other distros does not print those warnings.

I see the same if I use a similar GCC on a different distro, i.e Alpine 3.18 with GCC `g++ (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924)`:
```bash
CXX libbitcoin_common_a-protocol.o
In file included from ./hash.h:13,
from ./pubkey.h:10,
from ./addresstype.h:9,
from ./outputtype.h:9,
from outputtype.cpp:6:
In member function 'bool prevector<N
...

📝 starius opened a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365)
Inspired by https://github.com/bitcoin/bitcoin/pull/27446, this PR implements the proposal detailed in the comment https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1516600820.

## Rationale

Introduce the ability to configure a custom target time between blocks in a custom Bitcoin signet network. This enhancement enables users to create a signet that is more conducive to testing. The change enhances the flexibility of signet, rendering it more versatile for various testing scenario
...

💬 furszy commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1474722573)
In 3f0f43d60:

A topic about this change:

`GetBlockChecked()` internally checks the block proof of work and the signet signature after reading the data from disk. The new `GetRawBlockChecked()` does not perform this checks.

It isn't an issue for me because the block wouldn't have been stored if the PoW was invalid and I don't think that `getblock` is the appropriate place to check for corruption or tampered data.