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

💬 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_r1474760569)
In 3d7ae60f21:

Same discussion as with the previous commit. `ReadBlockFromDisk()` checks PoW and the signet signature after reading the block from disk while `ReadRawBlockFromDisk()` doesn't.

I agree on not performing these checks but would be nice to describe the changes in the commit description.

💬 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_r1474716665)
In 3f0f43d605b44:

could inline it:
```c++
if (verbosity <= 0) {
return HexStr(block_data);
}
```

💬 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_r1474711807)
As we only care about the data existence here, what about doing a more general check:
```c++
if (!(blockindex.nStatus & BLOCK_HAVE_DATA)) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
}
```

(Unless you want to keep the same error message as before)