💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921081600)
Not sure if related, but I'm still running into issues. I've tried a lot by now, from changing antivirus program, disabling antivirus and anti-malware, checking CPU temperature, checking SSD integrity, updating firmware, updating BIOS, trying older versions of Bitcoin Core and more.
This specific data corruption error hasn't come up the last few attempts to get to IBD. But often after stopping Core and starting it up again, I get the message that I should reindex. This seems to be a problem
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921081600)
Not sure if related, but I'm still running into issues. I've tried a lot by now, from changing antivirus program, disabling antivirus and anti-malware, checking CPU temperature, checking SSD integrity, updating firmware, updating BIOS, trying older versions of Bitcoin Core and more.
This specific data corruption error hasn't come up the last few attempts to get to IBD. But often after stopping Core and starting it up again, I get the message that I should reindex. This seems to be a problem
...
💬 maflcko commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921094517)
> It's may still be antivirus software that's persistent in interfering with Core.
It would be good to check this. If you have a completely separate machine, you could try to install the "anti"-virus software there and see if the issue happens?
Other than that, I believe this is a caused by overheating. It may be possible that the CPU itself happens to work under high heat as part of a benchmark or stress test. Also, other hardware components are fine by themselves as part of a benchmark o
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1921094517)
> It's may still be antivirus software that's persistent in interfering with Core.
It would be good to check this. If you have a completely separate machine, you could try to install the "anti"-virus software there and see if the issue happens?
Other than that, I believe this is a caused by overheating. It may be possible that the CPU itself happens to work under high heat as part of a benchmark or stress test. Also, other hardware components are fine by themselves as part of a benchmark o
...
📝 maflcko opened a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361)
Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it.
(https://github.com/bitcoin/bitcoin/pull/29361)
Remove unused includes. Also, fixup comments, see https://github.com/bitcoin/bitcoin/pull/28956/files#r1464827885. Also, add missing includes to `chain.h` while touching it.
📝 hebasto opened a pull request: "build: Add missed definition for `AM_OBJCXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/29362)
This PR adds the missed definition for `AM_OBJCXXFLAGS` which has the same value as `AM_CXXFLAGS`.
Otherwise, the compiling flags used by Objective C++ (for `.mm` source files) differ from C++ ones including hardening, debug, warning etc options.
(https://github.com/bitcoin/bitcoin/pull/29362)
This PR adds the missed definition for `AM_OBJCXXFLAGS` which has the same value as `AM_CXXFLAGS`.
Otherwise, the compiling flags used by Objective C++ (for `.mm` source files) differ from C++ ones including hardening, debug, warning etc options.
✅ dergoegge closed a pull request: "wip: Split fuzz binary"
(https://github.com/bitcoin/bitcoin/pull/29010)
(https://github.com/bitcoin/bitcoin/pull/29010)
💬 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.