💬 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`.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955766889)
Does an external wallet here mean when a wallet is loaded from wallet file that was manually added by the user in the path?
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1955766889)
Does an external wallet here mean when a wallet is loaded from wallet file that was manually added by the user in the path?
🤔 hebasto reviewed a pull request: "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain"
(https://github.com/bitcoin/bitcoin/pull/31849#pullrequestreview-2617167523)
Concept ACK on using as many CMake's abstractions as possible.
We already use them in depends:https://github.com/bitcoin/bitcoin/blob/2549fc6fd1cc958a0e2d59838907c8808fd129b3/depends/funcs.mk#L194-L198
> ... we'd end up with a duplicated `--target=$ARCH-apple-darwin` on the compiler line
This will become a persistent source of confusion and complains, which could, of course, just be ignored. That was the only reason why I initially refrained from using `CMAKE_<LANG>_COMPILER_TARGET` whi
...
(https://github.com/bitcoin/bitcoin/pull/31849#pullrequestreview-2617167523)
Concept ACK on using as many CMake's abstractions as possible.
We already use them in depends:https://github.com/bitcoin/bitcoin/blob/2549fc6fd1cc958a0e2d59838907c8808fd129b3/depends/funcs.mk#L194-L198
> ... we'd end up with a duplicated `--target=$ARCH-apple-darwin` on the compiler line
This will become a persistent source of confusion and complains, which could, of course, just be ignored. That was the only reason why I initially refrained from using `CMAKE_<LANG>_COMPILER_TARGET` whi
...
💬 hebasto commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#discussion_r1955802997)
Add `set(CMAKE_OBJCXX_COMPILER_TARGET @host@)`?
(https://github.com/bitcoin/bitcoin/pull/31849#discussion_r1955802997)
Add `set(CMAKE_OBJCXX_COMPILER_TARGET @host@)`?
💬 TheCharlatan commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955803986)
Ah, I was irritated by the reference to the expiry, but it is only a couple of lines above, so I guess that is fine as is.
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955803986)
Ah, I was irritated by the reference to the expiry, but it is only a couple of lines above, so I guess that is fine as is.
💬 hebasto commented on pull request "cmake: Improve safety and robustness during building `crc32c` subtree":
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2658718992)
> Another thought, isn't this going to break git bisect, or, result in having to do unclean subtree pulls? As the commit where you update the subtree will no-longer compile unless it also simultaneously changes the hash hardcoded into our build system.
Fair enough.
(https://github.com/bitcoin/bitcoin/pull/31779#issuecomment-2658718992)
> Another thought, isn't this going to break git bisect, or, result in having to do unclean subtree pulls? As the commit where you update the subtree will no-longer compile unless it also simultaneously changes the hash hardcoded into our build system.
Fair enough.
✅ hebasto closed a pull request: "cmake: Improve safety and robustness during building `crc32c` subtree"
(https://github.com/bitcoin/bitcoin/pull/31779)
(https://github.com/bitcoin/bitcoin/pull/31779)
💬 Sjors commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2658745672)
@goodthebest this pull request is not about pruning blocks. A better place to ask is https://bitcoin.stackexchange.com
But yes, it doesn't matter if you prune your node for mining and any value is fine (the minimum is `-prune=550` MB).
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2658745672)
@goodthebest this pull request is not about pruning blocks. A better place to ask is https://bitcoin.stackexchange.com
But yes, it doesn't matter if you prune your node for mining and any value is fine (the minimum is `-prune=550` MB).
👍 TheCharlatan approved a pull request: "Cleanups to port mapping module post UPnP drop"
(https://github.com/bitcoin/bitcoin/pull/31157#pullrequestreview-2617235585)
ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6
Just a nit: Maybe cleanup the dangling includes?
(https://github.com/bitcoin/bitcoin/pull/31157#pullrequestreview-2617235585)
ACK 70398ae05bc36a2788e87f67ae06962f43fe35a6
Just a nit: Maybe cleanup the dangling includes?