⚠️ fanquake opened an issue: "cmake: incorrectly reporting MSVC as using ccache"
(https://github.com/bitcoin/bitcoin/issues/31771)
Mentioned by @stickies-v. The CMake configure currently outputs that MSVC builds are using ccache, i.e (https://github.com/bitcoin/bitcoin/actions/runs/13063954360/job/36452895056#step:8:2374):
```bash
Treat compiler warnings as errors ..... ON
Use ccache for compiling .............. ON
```
However that's a bug, as MSVC is currently excluded entirely from using ccache:
https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/cmake/ccache.cmake#L4-L6
(https://github.com/bitcoin/bitcoin/issues/31771)
Mentioned by @stickies-v. The CMake configure currently outputs that MSVC builds are using ccache, i.e (https://github.com/bitcoin/bitcoin/actions/runs/13063954360/job/36452895056#step:8:2374):
```bash
Treat compiler warnings as errors ..... ON
Use ccache for compiling .............. ON
```
However that's a bug, as MSVC is currently excluded entirely from using ccache:
https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/cmake/ccache.cmake#L4-L6
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937423612)
> IIRC I used magic values back when these were passed in as arguments, but in a struct using an `std::optional` is not much friction. I'll look into that.
IMO, MAX_MONEY is a magic value that would be good to replace with nullopt, but MillisecondsDouble::max() is not a magic value and the code would be simpler if it were kept.
A magic value (imo) is a value that is handled specially and adds a special case, so replacing `CAmount fee_threshold` with std::optional` is good because the code
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937423612)
> IIRC I used magic values back when these were passed in as arguments, but in a struct using an `std::optional` is not much friction. I'll look into that.
IMO, MAX_MONEY is a magic value that would be good to replace with nullopt, but MillisecondsDouble::max() is not a magic value and the code would be simpler if it were kept.
A magic value (imo) is a value that is handled specially and adds a special case, so replacing `CAmount fee_threshold` with std::optional` is good because the code
...
💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2627535033)
Rebased.
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2627535033)
Rebased.
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937441681)
> But if that is changed to an integer type this will break (overflow).
I don't think there is anything unusual about that. If you change a floating point variable to an integer, you should expect code using that variable to need to be updated to handle overflows correctly, because integer and floating point numbers overflow differently. This code could work correctly if it were switched to an integer. It would just need to use saturating integer operations like the ones in util/overflow.h.
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937441681)
> But if that is changed to an integer type this will break (overflow).
I don't think there is anything unusual about that. If you change a floating point variable to an integer, you should expect code using that variable to need to be updated to handle overflows correctly, because integer and floating point numbers overflow differently. This code could work correctly if it were switched to an integer. It would just need to use saturating integer operations like the ones in util/overflow.h.
...
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937451223)
This is not the case. It's just that block template building will be much faster, but operationally nothing changes: you have to build a template to know what the fees will be. Of course, it'd be possible to create a way to run the template-building code without actually remembering the template, and only keeping track of the fees, which would be even faster, but that could be done right now too.
I think eventually, the correct approach is to have a background thread that continuously creates
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937451223)
This is not the case. It's just that block template building will be much faster, but operationally nothing changes: you have to build a template to know what the fees will be. Of course, it'd be possible to create a way to run the template-building code without actually remembering the template, and only keeping track of the fees, which would be even faster, but that could be done right now too.
I think eventually, the correct approach is to have a background thread that continuously creates
...
✅ achow101 closed an issue: "incrementalrelayfee configuration option should be present in bitcoin.conf"
(https://github.com/bitcoin/bitcoin/issues/31769)
(https://github.com/bitcoin/bitcoin/issues/31769)
💬 achow101 commented on issue "incrementalrelayfee configuration option should be present in bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/31769#issuecomment-2627557208)
All command line options, hidden or not, can be set in the bitcoin.conf.
The help for incrementalrelayfee can be seen if `-help-debug` is additionally passed.
(https://github.com/bitcoin/bitcoin/issues/31769#issuecomment-2627557208)
All command line options, hidden or not, can be set in the bitcoin.conf.
The help for incrementalrelayfee can be seen if `-help-debug` is additionally passed.
💬 theuni commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1937464559)
Good point. Move it down below language selection, and also now it's only set if not specified by the user.
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1937464559)
Good point. Move it down below language selection, and also now it's only set if not specified by the user.
💬 theuni commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2627582147)
Updated to address hebasto's comment, and removed comments about it being a speedup. Should be good to go now.
This speeds things up significantly for me in some cases where I'm only making specific targets. I think it should be a general speedup in other cases too, but sure, it's more about the dependency reduction.
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2627582147)
Updated to address hebasto's comment, and removed comments about it being a speedup. Should be good to go now.
This speeds things up significantly for me in some cases where I'm only making specific targets. I think it should be a general speedup in other cases too, but sure, it's more about the dependency reduction.
⚠️ fanquake opened an issue: "test: 32-bit Clang `ipc_test` failure at `-O0`"
(https://github.com/bitcoin/bitcoin/issues/31772)
Noticed as part of this branch #29796, however it can also be reproduced with master (8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf) by reproducing the equivalent CI & setting C(XX)FLAGS to -O0:
```bash
make -C depends/ MULTIPROCESS=1 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 CFLAGS="-O0" CXXFLAGS="-O0" DEBUG=1 -j19 HOST=i686-pc-linux-gnu
cmake -B build --toolchain /root/ci_scratch/depends/i686-pc-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang;-m32' -DCMAKE_CXX_COMPILER='cl
...
(https://github.com/bitcoin/bitcoin/issues/31772)
Noticed as part of this branch #29796, however it can also be reproduced with master (8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf) by reproducing the equivalent CI & setting C(XX)FLAGS to -O0:
```bash
make -C depends/ MULTIPROCESS=1 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 CFLAGS="-O0" CXXFLAGS="-O0" DEBUG=1 -j19 HOST=i686-pc-linux-gnu
cmake -B build --toolchain /root/ci_scratch/depends/i686-pc-linux-gnu/toolchain.cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang;-m32' -DCMAKE_CXX_COMPILER='cl
...
💬 fanquake commented on pull request "[RFC] Align debugging flags to `-O0`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2627584482)
I've pushed up a patch that should work around the broken qt code (although it's unclear that 32-bit Clang builds are something we need to support for the gui).
Note that this branch is likely going to fail a different way now, in the `ipc_tests`, see #31772.
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2627584482)
I've pushed up a patch that should work around the broken qt code (although it's unclear that 32-bit Clang builds are something we need to support for the gui).
Note that this branch is likely going to fail a different way now, in the `ipc_tests`, see #31772.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937491375)
I was hoping we could simply have a variable that tracks the total fees for the top 1 vMB, which is constantly updated as things are inserted and removed from the mempool.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937491375)
I was hoping we could simply have a variable that tracks the total fees for the top 1 vMB, which is constantly updated as things are inserted and removed from the mempool.
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937500659)
It's probably fast enough that that can be done as an add-on, updated after every mempool change, but limited to every N milliseconds or so.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937500659)
It's probably fast enough that that can be done as an add-on, updated after every mempool change, but limited to every N milliseconds or so.
💬 ismaelsadeeq commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937503273)
@sipa, I imagine that an RBF over any transaction in the last built block template be an indicator that the previously built template improve.
And since we would be using fee rate diagram for RBF's after cluster mempool, we could use use RBF's as determinant?
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937503273)
@sipa, I imagine that an RBF over any transaction in the last built block template be an indicator that the previously built template improve.
And since we would be using fee rate diagram for RBF's after cluster mempool, we could use use RBF's as determinant?
💬 sr-gi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937515621)
I don't have a strong opinion on this.
I've checked and looks like no other test uses this, or re-defines it. I'd lean towards leaving it locally to avoid bloating `p2p.py`, but I'm open to move it if there is support for it
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937515621)
I don't have a strong opinion on this.
I've checked and looks like no other test uses this, or re-defines it. I'd lean towards leaving it locally to avoid bloating `p2p.py`, but I'm open to move it if there is support for it
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937516539)
You mean "never generate a template" when run from the tests, right? Why is that? The default timeout is `inf` so `now < deadline` should be `true`?
> So it has to use <=, but without the break here the test will freeze, because time doesn't move.
Right, but it is not too difficult to advance the time in unit tests.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937516539)
You mean "never generate a template" when run from the tests, right? Why is that? The default timeout is `inf` so `now < deadline` should be `true`?
> So it has to use <=, but without the break here the test will freeze, because time doesn't move.
Right, but it is not too difficult to advance the time in unit tests.
💬 tdb3 commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937519743)
Works for me. If/when the file is touched again, it could be moved then.
(https://github.com/bitcoin/bitcoin/pull/31758#discussion_r1937519743)
Works for me. If/when the file is touched again, it could be moved then.
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937522651)
Functionality-wise I think the behavior of the production code is ok. I am concerned about its readability and maintainability. It just looks confusing (to me).
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937522651)
Functionality-wise I think the behavior of the production code is ok. I am concerned about its readability and maintainability. It just looks confusing (to me).
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937527468)
Agreed about `MillisecondsDouble::max()`. Note here that we rely to have such a max value that when we add to it the result is the same max value (not overflow or cause unexpected things).
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937527468)
Agreed about `MillisecondsDouble::max()`. Note here that we rely to have such a max value that when we add to it the result is the same max value (not overflow or cause unexpected things).
💬 sipa commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937550966)
Just discussed this with @ismaelsadeeq a bit more.
The hard part is not tracking improvements to the mempool (that is ultimately what all of cluster mempool is, making this explicit), but know which improvements affect the top 1 MvB of the mempool and to what extent. My thinking was that we can't really maintain an index for that, as doing so would be less efficient than just recomputing when it's needed.
But maybe that is not the case. We could keep track at all times of (a) the sum of th
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937550966)
Just discussed this with @ismaelsadeeq a bit more.
The hard part is not tracking improvements to the mempool (that is ultimately what all of cluster mempool is, making this explicit), but know which improvements affect the top 1 MvB of the mempool and to what extent. My thinking was that we can't really maintain an index for that, as doing so would be less efficient than just recomputing when it's needed.
But maybe that is not the case. We could keep track at all times of (a) the sum of th
...