💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474470023)
Any suggested reading to wrap my head around this serialisation / bytes / span stuff?
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474470023)
Any suggested reading to wrap my head around this serialisation / bytes / span stuff?
💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1921373748)
> > With a little more work this could probably be it's own PR. Feel free to grab this branch, or I can open a PR if you're busy with other stuff!
>
> If you're feeling momentum, best just open a new PR. I'll then close this one. I can always take back the baton and reopen.
I was referring to opening a PR just for adding `start_height` to the indexes (the work that @fjahr had started), but I'm also happy to open a blockfilterindex PR! Although, curious if you had any thoughts regarding the
...
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1921373748)
> > With a little more work this could probably be it's own PR. Feel free to grab this branch, or I can open a PR if you're busy with other stuff!
>
> If you're feeling momentum, best just open a new PR. I'll then close this one. I can always take back the baton and reopen.
I was referring to opening a PR just for adding `start_height` to the indexes (the work that @fjahr had started), but I'm also happy to open a blockfilterindex PR! Although, curious if you had any thoughts regarding the
...
💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474513906)
A `std::byte` is serialized no different than an `uint8_t`. (https://en.cppreference.com/w/cpp/types/byte)
A span holds a pointer and a size. It points to memory outside of itself. For example, a string_view (special kind of span) creates a view on a pre-existing string (string literal or std::string).
When serializing a span in Bitcoin Core, the size is not written. Thus, the size of the pointed-to memory must be exactly equal every time at runtime. How you achieve that the memory is alwa
...
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474513906)
A `std::byte` is serialized no different than an `uint8_t`. (https://en.cppreference.com/w/cpp/types/byte)
A span holds a pointer and a size. It points to memory outside of itself. For example, a string_view (special kind of span) creates a view on a pre-existing string (string literal or std::string).
When serializing a span in Bitcoin Core, the size is not written. Thus, the size of the pointed-to memory must be exactly equal every time at runtime. How you achieve that the memory is alwa
...
💬 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
(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?
(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.
(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?
(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.
(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.
(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.
(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
(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
(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).
(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.
(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`.
(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)
(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
...
(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
(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?
(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.
(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.