📝 jirijakes opened a pull request: "doc: Fix description of byte order of hashes in ZMQ documentation"
(https://github.com/bitcoin/bitcoin/pull/31862)
ZMQ produces transaction and block hashes in the same format as RPC, that is with bytes in a reversed order from the output of hashing function. Previously, the ZMQ documentation incorrectly informed that the two are in different formats, and this changes fixes this.
Additionally, as the terms 'Big Endian' and 'Little Endian' are misleading in terms of order of bytes in an arbitrary sequence of bytes, this change also removes this notion and uses the term 'reversed byte order' instead.
Fix
...
(https://github.com/bitcoin/bitcoin/pull/31862)
ZMQ produces transaction and block hashes in the same format as RPC, that is with bytes in a reversed order from the output of hashing function. Previously, the ZMQ documentation incorrectly informed that the two are in different formats, and this changes fixes this.
Additionally, as the terms 'Big Endian' and 'Little Endian' are misleading in terms of order of bytes in an arbitrary sequence of bytes, this change also removes this notion and uses the term 'reversed byte order' instead.
Fix
...
💬 davidgumberg commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2658490999)
> @davidgumberg Looks like you reviewed a stale commit?
Oops.
utreACK https://github.com/bitcoin/bitcoin/commit/b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1
Recent rebase makes a comment change, and removes an unnecessary loop, addressing reviewer feedback.
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2658490999)
> @davidgumberg Looks like you reviewed a stale commit?
Oops.
utreACK https://github.com/bitcoin/bitcoin/commit/b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1
Recent rebase makes a comment change, and removes an unnecessary loop, addressing reviewer feedback.
💬 jirijakes commented on issue "doc/zmq: Note about endianness does not match reality":
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2658493762)
> current ZMQ documentation is mostly wrong describe RPC interface as using big endian format
I don't know where the notion of Big/Little Endian of hashes comes from but I have seen the RPC-style (reversed by order) to be mostly described as Big Endian, for example [learnmebitcoin](https://learnmeabitcoin.com/technical/general/byte-order/#reverse-byte-order) uses it. Anyway, it is apparently very misleading.
> Best way to fix this would be to avoid using terms little endian and big endian at a
...
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2658493762)
> current ZMQ documentation is mostly wrong describe RPC interface as using big endian format
I don't know where the notion of Big/Little Endian of hashes comes from but I have seen the RPC-style (reversed by order) to be mostly described as Big Endian, for example [learnmebitcoin](https://learnmeabitcoin.com/technical/general/byte-order/#reverse-byte-order) uses it. Anyway, it is apparently very misleading.
> Best way to fix this would be to avoid using terms little endian and big endian at a
...
👍 hebasto approved a pull request: "doc: build: Fix instructions for msvc gui builds"
(https://github.com/bitcoin/bitcoin/pull/31851#pullrequestreview-2616969253)
ACK c3fa043ae560289759b6f836ac62736d97ba91bf.
(https://github.com/bitcoin/bitcoin/pull/31851#pullrequestreview-2616969253)
ACK c3fa043ae560289759b6f836ac62736d97ba91bf.
💬 maflcko commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2658504053)
> so much is different with the `std::chrono` implementation that anything similar is unlikely to re-occur.
To give some more context, if this exact issue were to re-appear, it would now be caught by other tests (maybe).
For testing, I took the constant offset from https://github.com/bitcoin/bitcoin/issues/3494#issuecomment-31849134: `std::chrono::seconds{0x0000ffc4'00000000}` and applied it to the system clock as offset. Given that the system clock is normally in nanosecond precision, thi
...
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2658504053)
> so much is different with the `std::chrono` implementation that anything similar is unlikely to re-occur.
To give some more context, if this exact issue were to re-appear, it would now be caught by other tests (maybe).
For testing, I took the constant offset from https://github.com/bitcoin/bitcoin/issues/3494#issuecomment-31849134: `std::chrono::seconds{0x0000ffc4'00000000}` and applied it to the system clock as offset. Given that the system clock is normally in nanosecond precision, thi
...
💬 maflcko commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2658549377)
> I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.
I don't see why this should be a 29.0 blocker at all, or why GCC is unusable, given that it is used, for example, see the previous comment above: https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2540826789
Let's continue discussion in the thread https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658475173?
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2658549377)
> I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.
I don't see why this should be a 29.0 blocker at all, or why GCC is unusable, given that it is used, for example, see the previous comment above: https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2540826789
Let's continue discussion in the thread https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658475173?
💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1955758861)
AFAIU the second flush is the original one, while the second flush was introduced in https://github.com/bitcoin/bitcoin/commit/3192975f1d177aa9f0bbd823c6387cfbfa943610 to help cleanly process all remaining callbacks. Maybe it could have removed the second flush in that commit?
That said I think it is good practice to be defensive here and keep both.
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1955758861)
AFAIU the second flush is the original one, while the second flush was introduced in https://github.com/bitcoin/bitcoin/commit/3192975f1d177aa9f0bbd823c6387cfbfa943610 to help cleanly process all remaining callbacks. Maybe it could have removed the second flush in that commit?
That said I think it is good practice to be defensive here and keep both.
👍 TheCharlatan approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2617102083)
Re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2617102083)
Re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d
💬 hebasto commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2658646546)
> iirc one of the issues with `install-name-tool` was that it was only available version-suffixed on certain distros (even when the main llvm install was not suffixed), making it hard to find.
If it's not too much trouble, could you recall which distros those were?
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2658646546)
> iirc one of the issues with `install-name-tool` was that it was only available version-suffixed on certain distros (even when the main llvm install was not suffixed), making it hard to find.
If it's not too much trouble, could you recall which distros those were?
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2658649883)
> Can you update the PR description to say that this is based on #31854? The convention then is to mark this PR draft until that one is merged.
Added a note at the bottom of the description of this PR. This PR was not a draft before #31854. The creation of #31854 did not change anything about this PR, so I think the existence of a chop off PR shouldn't render this one as a draft. There is no dependency between the two - either one can be merged first.
> Additionally, it's 25% easier to rev
...
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2658649883)
> Can you update the PR description to say that this is based on #31854? The convention then is to mark this PR draft until that one is merged.
Added a note at the bottom of the description of this PR. This PR was not a draft before #31854. The creation of #31854 did not change anything about this PR, so I think the existence of a chop off PR shouldn't render this one as a draft. There is no dependency between the two - either one can be merged first.
> Additionally, it's 25% easier to rev
...
💬 laanwj commented on issue "doc/zmq: Note about endianness does not match reality":
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2658680925)
> The most straightforward way to think about hashtx and hashblock values is as byte arrays, not as numbers.
Yes, we should never use the word 'endian' in the context of hashes in our documentation. They're just byte blobs. Which can be reversed or in original order.
It's mistaken terminology that Satoshi introduced back in the day and unfortunately still sticks around.
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2658680925)
> The most straightforward way to think about hashtx and hashblock values is as byte arrays, not as numbers.
Yes, we should never use the word 'endian' in the context of hashes in our documentation. They're just byte blobs. Which can be reversed or in original order.
It's mistaken terminology that Satoshi introduced back in the day and unfortunately still sticks around.
👍 TheCharlatan approved a pull request: "doc: build: Fix instructions for msvc gui builds"
(https://github.com/bitcoin/bitcoin/pull/31851#pullrequestreview-2617147568)
ACK c3fa043ae560289759b6f836ac62736d97ba91bf
Am I misremembering or was this, or a similar workaround, part of the previous build instructions at some point?
(https://github.com/bitcoin/bitcoin/pull/31851#pullrequestreview-2617147568)
ACK c3fa043ae560289759b6f836ac62736d97ba91bf
Am I misremembering or was this, or a similar workaround, part of the previous build instructions at some point?
📝 eval-exec opened a pull request: "random: Initialize variables in hardware RNG functions"
(https://github.com/bitcoin/bitcoin/pull/31863)
See: https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955045279 , So this PR want to prevent potential uninitialized value issues and improve code clarity.
(https://github.com/bitcoin/bitcoin/pull/31863)
See: https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955045279 , So this PR want to prevent potential uninitialized value issues and improve code clarity.
👍 rkrux approved a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2617045748)
ACK 409241db5dca0b23f5c7714f99be52411fc5541e
Build and tests successful.
Seems to be an edge case that's fixed but I'm in favour because it ensures data correctness.
Re: this comment https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936
I agree with @furszy that this should be in a separate PR because of the separate context.
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2617045748)
ACK 409241db5dca0b23f5c7714f99be52411fc5541e
Build and tests successful.
Seems to be an edge case that's fixed but I'm in favour because it ensures data correctness.
Re: this comment https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936
I agree with @furszy that this should be in a separate PR because of the separate context.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955732785)
Nit: The third node isn't really used in the test. I ran the test without it and it worked.
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955732785)
Nit: The third node isn't really used in the test. I ran the test without it and it worked.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955746499)
Would it be helpful to assert for `'category': 'orphan'` as well?
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955746499)
Would it be helpful to assert for `'category': 'orphan'` as well?
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955742226)
These comments are very helpful!
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955742226)
These comments are very helpful!
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955765173)
Trying to understand why the `AbandonTransaction` was overloaded. IIUC, the older one `AbandonTransaction(const uint256& hashTx)` can be used as well here with the `id` passed as the argument.
Am I missing something? I do notice `EXCLUSIVE_LOCKS_REQUIRED` in the new function signature but I also see it being used already in the older function before this change.
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955765173)
Trying to understand why the `AbandonTransaction` was overloaded. IIUC, the older one `AbandonTransaction(const uint256& hashTx)` can be used as well here with the `id` passed as the argument.
Am I missing something? I do notice `EXCLUSIVE_LOCKS_REQUIRED` in the new function signature but I also see it being used already in the older function before this change.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955795282)
Is the following scenario handled by the usual handling or via this new one?
> User has their node running and receive a coinbase transaction, and then the node is stopped. User restarts the node after a long time and meanwhile that transaction was invalidated (block invalidated), and at the time of loading the wallet the correct balance is shown.
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955795282)
Is the following scenario handled by the usual handling or via this new one?
> User has their node running and receive a coinbase transaction, and then the node is stopped. User restarts the node after a long time and meanwhile that transaction was invalidated (block invalidated), and at the time of loading the wallet the correct balance is shown.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955733317)
Was this statement supposed to be inside the `for` loop? Otherwise only `blocks` seem to be copied to `node0` while the `chainstate` as well is deleted in `node0`.
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955733317)
Was this statement supposed to be inside the `for` loop? Otherwise only `blocks` seem to be copied to `node0` while the `chainstate` as well is deleted in `node0`.