💬 fanquake commented on pull request "build: Add missed definition for `AM_OBJCXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921239864)
```bash
CXX qt/libbitcoinqt_a-moc_bitcoin.o
qt/macnotificationhandler.mm:27:9: error: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Werror,-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Fram
...
(https://github.com/bitcoin/bitcoin/pull/29362#issuecomment-1921239864)
```bash
CXX qt/libbitcoinqt_a-moc_bitcoin.o
qt/macnotificationhandler.mm:27:9: error: 'NSUserNotification' is deprecated: first deprecated in macOS 11.0 - All NSUserNotifications API should be replaced with UserNotifications.frameworks API [-Werror,-Wdeprecated-declarations]
NSUserNotification* userNotification = [[NSUserNotification alloc] init];
^
/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Fram
...
💬 fanquake commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1921246012)
Backported to 26 in #29209.
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1921246012)
Backported to 26 in #29209.
💬 theStack commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1921271673)
Brought back the commit "test: p2p: process post-v2-handshake data immediately" again and put it in _before_ the one changing the version message flow. This leads to a cleaner history compared to the squashed commit (containing two long descriptions), while still passing the checks at each commit. Kudos to @stratospher for the idea.
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1921271673)
Brought back the commit "test: p2p: process post-v2-handshake data immediately" again and put it in _before_ the one changing the version message flow. This leads to a cleaner history compared to the squashed commit (containing two long descriptions), while still passing the checks at each commit. Kudos to @stratospher for the idea.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1474434487)
That said, your comment indicates that this could be phrased better, since it appears to be difficult to follow. Will improve.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1474434487)
That said, your comment indicates that this could be phrased better, since it appears to be difficult to follow. Will improve.
👍 furszy approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1856447879)
ACK 6752d218
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1856447879)
ACK 6752d218
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1921316998)
I looked into the possibility of subclassing `CKey` or making a similar class from scratch, but that got too complicated too quickly.
So I'm keeping the approach as-is, and will work around it in some more verbose way if this PR doesn't make it.
I did however bring back the `compressed` boolean. That way the serialisation maps 1-to-1 to `CKey`.
I think that if the wallet was designed from scratch it would use 32 byte encoding for private keys. The DER encoding adds no value, we don't ev
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1921316998)
I looked into the possibility of subclassing `CKey` or making a similar class from scratch, but that got too complicated too quickly.
So I'm keeping the approach as-is, and will work around it in some more verbose way if this PR doesn't make it.
I did however bring back the `compressed` boolean. That way the serialisation maps 1-to-1 to `CKey`.
I think that if the wallet was designed from scratch it would use 32 byte encoding for private keys. The DER encoding adds no value, we don't ev
...
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1921321547)
> 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.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1921321547)
> 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.
💬 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).