💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474518715)
Possible solutions can be:

* Remove the `Sv2SignatureNoiseMessage` default constructor and/or check that the size of m_sig is properly initialized
* Use a `std::array` instead
* Serialize as `std::vector` instead

💬 theuni commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921424791)
This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

💬 hebasto commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921431646)
> This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?

It is Qt 5 code base. I did not check it out, but I hope Qt 6 dropped using of a deprecated API.

💬 theuni commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921444156)
> > This warning seems legitimate. Looks to me like we should modernize this code rather than ignoring it, no?
>
> It is Qt 5 code base. I did not check it out, but I hope Qt 6 dropped using of a deprecated API.

I don't understand what you mean. From what @fanquake pasted above, It's in our `macnotificationhandler.mm`. A quick google says that we should switch to the `UserNotifications.frameworks API` instead. What part of this is qt specific?

Or are there warnings other than those 3?

💬 fanquake commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921447607)
> I don't understand what you mean.

Me either. The code that is producing warnings is definitely in our source tree. I don't see how this is related to the Qt version.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1921448658)
@josibake I'll get back to you on those later.

💬 fanquake commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921456217)
I just had a look, and anyone compiling on macOS, with Qt 6.6.1, will still see these exact same deprecation warnings. So this isn't solved by Qt 6.

💬 epiccurious commented on pull request "test: Fix CPartialMerkleTree.nTransactions signedness":
(https://github.com/bitcoin/bitcoin/pull/29363#issuecomment-1921458982)
utACK

👍 TheCharlatan approved a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1856657731)
Re-ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474609984)
Since `m_sig` is always 64 bytes, I changed it to `std::array` (and using `std::byte` for consistency).

👍 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);
```