💬 dergoegge commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1921159121)
I'll re-open this once we switched to cmake.
I've rebased the commits on the current cmake staging branch and building individual binaries using cmake is quite easy (assuming there is one harness per file):
```cmake
file(GLOB fuzz_harness_file_list RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} "./*.cpp")
# Remove fuzz.cpp and util.cpp as they don't contain fuzz harnesses
list(REMOVE_ITEM fuzz_harness_file_list "fuzz.cpp")
list(REMOVE_ITEM fuzz_harness_file_list "util.cpp")
foreach(
...
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1921159121)
I'll re-open this once we switched to cmake.
I've rebased the commits on the current cmake staging branch and building individual binaries using cmake is quite easy (assuming there is one harness per file):
```cmake
file(GLOB fuzz_harness_file_list RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} "./*.cpp")
# Remove fuzz.cpp and util.cpp as they don't contain fuzz harnesses
list(REMOVE_ITEM fuzz_harness_file_list "fuzz.cpp")
list(REMOVE_ITEM fuzz_harness_file_list "util.cpp")
foreach(
...
🤔 stickies-v reviewed a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361#pullrequestreview-1856324522)
Concept ACK, but I think there are a couple more include changes that should be made as per my diff in https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1838562579
(https://github.com/bitcoin/bitcoin/pull/29361#pullrequestreview-1856324522)
Concept ACK, but I think there are a couple more include changes that should be made as per my diff in https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1838562579
📝 maflcko opened a pull request: "test: Fix CPartialMerkleTree.nTransactions signedness"
(https://github.com/bitcoin/bitcoin/pull/29363)
It is unsigned in Bitcoin Core, so the tests should match it:
https://github.com/bitcoin/bitcoin/blob/aa9231fafe45513134ec8953a217cda07446fae8/src/merkleblock.h#L59
Large positive values, or "negative" values, are rejected anyway, but it still seems fine to fix this.
The bug was introduced when the code was written in d280617bf569f84457eaea546541dc74c67cd1e4.
(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)
(https://github.com/bitcoin/bitcoin/pull/29363)
It is unsigned in Bitcoin Core, so the tests should match it:
https://github.com/bitcoin/bitcoin/blob/aa9231fafe45513134ec8953a217cda07446fae8/src/merkleblock.h#L59
Large positive values, or "negative" values, are rejected anyway, but it still seems fine to fix this.
The bug was introduced when the code was written in d280617bf569f84457eaea546541dc74c67cd1e4.
(Lowercase `i` means signed, see https://docs.python.org/3/library/struct.html#format-characters)
👍 maflcko approved a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1856328987)
ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f 🌵
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 6acfcbfe2487f683e8f62606d1
...
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1856328987)
ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f 🌵
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 6acfcbfe2487f683e8f62606d1
...
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474379876)
unrelated nit: Would be nice to use the python built-in `int.to_bytes` feature, because the struct interface is confusing and has lead to bugs in the past, see above.
(Can be done in a follow-up, as a good-first issue)
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474379876)
unrelated nit: Would be nice to use the python built-in `int.to_bytes` feature, because the struct interface is confusing and has lead to bugs in the past, see above.
(Can be done in a follow-up, as a good-first issue)
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474398219)
unrelated follow-up: (Same here: Remove `struct`)
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474398219)
unrelated follow-up: (Same here: Remove `struct`)
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474353379)
nit in ca7090d43fab2013bd396d683f4cf03062666b14: I think it would be consistent to omit the argument here, which is `False` by default. Otherwise, it could create the impression that other calls that have it omitted are different (signed) serializations. For example, the unsigned locktime below also has it omitted.
One could even argue that anything that has a `signed` value in this file here is a bug or at a minimum a red flag. See for example: fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa or htt
...
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474353379)
nit in ca7090d43fab2013bd396d683f4cf03062666b14: I think it would be consistent to omit the argument here, which is `False` by default. Otherwise, it could create the impression that other calls that have it omitted are different (signed) serializations. For example, the unsigned locktime below also has it omitted.
One could even argue that anything that has a `signed` value in this file here is a bug or at a minimum a red flag. See for example: fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa or htt
...
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474390803)
review note: This is fine for tinyformat, but if you re-touch it could make sense to change the "signed decimal" to `%s` or `%u`.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1474390803)
review note: This is fine for tinyformat, but if you re-touch it could make sense to change the "signed decimal" to `%s` or `%u`.
💬 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?