💬 fanquake commented on pull request "build: replace custom `MAC_OSX` macro with existing `__APPLE__`":
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2434855072)
@l0rinc can you rebase this?
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2434855072)
@l0rinc can you rebase this?
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814692827)
@0xB10C Any thoughts on changing the way replacement logging works via tracepoints, in the package RBF case? One issue I can think of with what I'm proposing here is that the "package hash" we'd be sending is not one that would come through any other tracing messages -- would that be a problem?
If that is a problem, would it make sense to add a tracepoint that lines up with the existing `txpackages` debug log message, which (as of this PR) would now report the package hash along with the tran
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814692827)
@0xB10C Any thoughts on changing the way replacement logging works via tracepoints, in the package RBF case? One issue I can think of with what I'm proposing here is that the "package hash" we'd be sending is not one that would come through any other tracing messages -- would that be a problem?
If that is a problem, would it make sense to add a tracepoint that lines up with the existing `txpackages` debug log message, which (as of this PR) would now report the package hash along with the tran
...
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814694147)
Tracking this issue above (https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814692827), so marking this conversation as resolved.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814694147)
Tracking this issue above (https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814692827), so marking this conversation as resolved.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814698298)
I think this is done?
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814698298)
I think this is done?
💬 laanwj commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2434870262)
> Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection.
Right. Would it maybe make sense to have an option to restrict transaction broadcast an relay to v2-only, for IPv4 and IPv6 connections? Instead of not connect at all? Eg consider v1 connections blocks-only (except those over Tor/I2P). This alleviates the risk of network
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2434870262)
> Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection.
Right. Would it maybe make sense to have an option to restrict transaction broadcast an relay to v2-only, for IPv4 and IPv6 connections? Instead of not connect at all? Eg consider v1 connections blocks-only (except those over Tor/I2P). This alleviates the risk of network
...
💬 maflcko commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1814701175)
I don't see it. I still see `else if (g_fuzzing && !val) abort();`. Thus, I don't see any `||g_fuzzing`. Also, the `std::abort` wasn't removed nor the "duplicate" val check.
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1814701175)
I don't see it. I still see `else if (g_fuzzing && !val) abort();`. Thus, I don't see any `||g_fuzzing`. Also, the `std::abort` wasn't removed nor the "duplicate" val check.
💬 maflcko commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1814702785)
If you want to keep them for some reason, it would be good to explain, and then also add the missing includes.
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1814702785)
If you want to keep them for some reason, it would be good to explain, and then also add the missing includes.
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814706398)
Agreed, I have restored this argument where `LimitMempoolSize()` is invoked in `AcceptSingleTransaction()`, and I pulled this change out into its own commit so that it's easier to review and less tangled with the other changes.
More thoughts on this [here](https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-2433501732). It would be nice to have a test that packages which involve a parent transaction that would be just at the bottom of the mempool without their children are able to mak
...
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1814706398)
Agreed, I have restored this argument where `LimitMempoolSize()` is invoked in `AcceptSingleTransaction()`, and I pulled this change out into its own commit so that it's easier to review and less tangled with the other changes.
More thoughts on this [here](https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-2433501732). It would be nice to have a test that packages which involve a parent transaction that would be just at the bottom of the mempool without their children are able to mak
...
📝 maflcko opened a pull request: "ci: Temporary workaround for old CCACHE_DIR cirrus env"
(https://github.com/bitcoin/bitcoin/pull/31146)
On a CI re-run, the historic env vars and CI config is used from Cirrus. However, the most recent CI config and CI scripts from this repo are used. This may lead to issues.
For example, `CCACHE_DIR` in the old location may be missing on new CI workers and lead to errors.
Fix it, by falling back to the old logic when the old `CCACHE_DIR` was detected.
(https://github.com/bitcoin/bitcoin/pull/31146)
On a CI re-run, the historic env vars and CI config is used from Cirrus. However, the most recent CI config and CI scripts from this repo are used. This may lead to issues.
For example, `CCACHE_DIR` in the old location may be missing on new CI workers and lead to errors.
Fix it, by falling back to the old logic when the old `CCACHE_DIR` was detected.
💬 dergoegge commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1814722191)
I misunderstood what you meant, slow start today😴 should be fixed now!
(https://github.com/bitcoin/bitcoin/pull/31093#discussion_r1814722191)
I misunderstood what you meant, slow start today😴 should be fixed now!
📝 hebasto opened a pull request: "cmake, qt, test: Remove problematic code"
(https://github.com/bitcoin/bitcoin/pull/31147)
The removed code aimed to make Qt's minimal integration plugin DLL available for `test_bitcoin-qt.exe` on Windows.
However, there are two issues:
1. The code is broken because the destination directory must end with a trailing slash (`/`).
2. It is unnecessary because Qt's minimal integration plugin is not used on Windows. For more details, please refer to the following code:https://github.com/bitcoin/bitcoin/blob/fb46d57d4e7263495c007f4a499a349bff2b21e0/src/qt/test/CMakeLists.txt#L38-L44
...
(https://github.com/bitcoin/bitcoin/pull/31147)
The removed code aimed to make Qt's minimal integration plugin DLL available for `test_bitcoin-qt.exe` on Windows.
However, there are two issues:
1. The code is broken because the destination directory must end with a trailing slash (`/`).
2. It is unnecessary because Qt's minimal integration plugin is not used on Windows. For more details, please refer to the following code:https://github.com/bitcoin/bitcoin/blob/fb46d57d4e7263495c007f4a499a349bff2b21e0/src/qt/test/CMakeLists.txt#L38-L44
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1814722910)
Please see https://github.com/bitcoin/bitcoin/pull/31147.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1814722910)
Please see https://github.com/bitcoin/bitcoin/pull/31147.
💬 l0rinc commented on pull request "build: replace custom `MAC_OSX` macro with existing `__APPLE__`":
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2434907274)
> @l0rinc can you rebase this?
Rebased, checked for leftover `MAC_OSX` (found none)
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2434907274)
> @l0rinc can you rebase this?
Rebased, checked for leftover `MAC_OSX` (found none)
✅ fanquake closed an issue: "Support transaction broadcast in REST interface"
(https://github.com/bitcoin/bitcoin/issues/31017)
(https://github.com/bitcoin/bitcoin/issues/31017)
💬 fanquake commented on issue "Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/issues/31017#issuecomment-2434913714)
Closing for now, given the PR was closed. Feel free to discuss further. cc @laanwj @0xB10C
(https://github.com/bitcoin/bitcoin/issues/31017#issuecomment-2434913714)
Closing for now, given the PR was closed. Feel free to discuss further. cc @laanwj @0xB10C
💬 l0rinc commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#issuecomment-2434915226)
Rebased to avoid unrelated build failure
(https://github.com/bitcoin/bitcoin/pull/30670#issuecomment-2434915226)
Rebased to avoid unrelated build failure
💬 maflcko commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#issuecomment-2434920707)
It is also possible to simply re-run the CI, which wouldn't invalidate review.
(https://github.com/bitcoin/bitcoin/pull/30670#issuecomment-2434920707)
It is also possible to simply re-run the CI, which wouldn't invalidate review.
💬 laanwj commented on pull request "optimization: pack util::Xor into 64 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814734979)
i have a hard time believing this will make a large difference, especially with the two memcpys involved
On modern CPUs, ALU operations (especially bitwise ones) are so fast compared to any kind of memory access.
And this isn't some advanced crypto math, it's *one* Xor operation per word with a fixed key.
Could avoid the memcpys if the code takes memory alignment into account, but that makes it even more complex. Not sure the pros/cons work out here.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814734979)
i have a hard time believing this will make a large difference, especially with the two memcpys involved
On modern CPUs, ALU operations (especially bitwise ones) are so fast compared to any kind of memory access.
And this isn't some advanced crypto math, it's *one* Xor operation per word with a fixed key.
Could avoid the memcpys if the code takes memory alignment into account, but that makes it even more complex. Not sure the pros/cons work out here.
💬 laanwj commented on pull request "optimization: pack util::Xor into 64 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814736038)
Please avoid % (especially with a dynamic value) in an inner loop. It's essentially a division operation and those are not cheap.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814736038)
Please avoid % (especially with a dynamic value) in an inner loop. It's essentially a division operation and those are not cheap.
💬 l0rinc commented on pull request "optimization: pack util::Xor into 64 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814739738)
Thanks for the hint, I deliberately removed that optimization (please check the commit messages for details), since these are optimized away.
Also, this is just the leftover part, so for key of length 8 (the version used in most places) this will have 7 iterations at most.
Can you see any difference with any of the benchmarks?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814739738)
Thanks for the hint, I deliberately removed that optimization (please check the commit messages for details), since these are optimized away.
Also, this is just the leftover part, so for key of length 8 (the version used in most places) this will have 7 iterations at most.
Can you see any difference with any of the benchmarks?
💬 l0rinc commented on pull request "optimization: pack util::Xor into 64 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814741753)
The speedup comes from the vectorized operations, i.e. doing 64 bit xor instead of byte-by-byte xor (memcpy seems to be eliminated on 64 bit architectures successfully), see https://godbolt.org/z/Koscjconz
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814741753)
The speedup comes from the vectorized operations, i.e. doing 64 bit xor instead of byte-by-byte xor (memcpy seems to be eliminated on 64 bit architectures successfully), see https://godbolt.org/z/Koscjconz