💬 hebasto commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627403382)
> > The "clang-tidy" CI job shouldn't process sources in the `src/ipc/libmultiprocess/example/` [directory](https://github.com/hebasto/bitcoin/actions/runs/13072023868/job/36476232260)
>
> Maybe not, but I didn't see an easy way to exclude them, so I thought I would just fix the clang-tidy errors. I fixed the missing inputs problem by just adding necessary targets back to all:
>
> ```diff
> --- a/src/ipc/CMakeLists.txt
> +++ b/src/ipc/CMakeLists.txt
> @@ -9,6 +9,10 @@ if (NOT WITH_LIBMU
...
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2627403382)
> > The "clang-tidy" CI job shouldn't process sources in the `src/ipc/libmultiprocess/example/` [directory](https://github.com/hebasto/bitcoin/actions/runs/13072023868/job/36476232260)
>
> Maybe not, but I didn't see an easy way to exclude them, so I thought I would just fix the clang-tidy errors. I fixed the missing inputs problem by just adding necessary targets back to all:
>
> ```diff
> --- a/src/ipc/CMakeLists.txt
> +++ b/src/ipc/CMakeLists.txt
> @@ -9,6 +9,10 @@ if (NOT WITH_LIBMU
...
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937354583)
Just stumbled on this and posted a [comment to the PR elsewhere](https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937133582)... Having this only to make unit tests easier seems not enough justification to me.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937354583)
Just stumbled on this and posted a [comment to the PR elsewhere](https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937133582)... Having this only to make unit tests easier seems not enough justification to me.
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937357533)
Lets continue the discussion here https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1925533767 which is a previous comment about the same thing.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937357533)
Lets continue the discussion here https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1925533767 which is a previous comment about the same thing.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937363516)
If the loop uses `while (now < deadline)` then it will never generate a template, so all the `BOOST_REQUIRE(block_template)` checks will fail.
So it has to use `<=`, but without the `break` here the test will freeze, because time doesn't move.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937363516)
If the loop uses `while (now < deadline)` then it will never generate a template, so all the `BOOST_REQUIRE(block_template)` checks will fail.
So it has to use `<=`, but without the `break` here the test will freeze, because time doesn't move.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937372244)
That might work actually, with an extra minus. But it seems brittle, because the line `pblocktemplate->vTxFees[0] = -nFees;` doesn't do anything afaik and might one day get deleted, causes this code to thing fees are 0.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937372244)
That might work actually, with an extra minus. But it seems brittle, because the line `pblocktemplate->vTxFees[0] = -nFees;` doesn't do anything afaik and might one day get deleted, causes this code to thing fees are 0.
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937378509)
> unit tests can't easily move mock time during a blocking call
Consider the following pattern to do that. I think it would be better to have that in the tests instead of this `if (now == deadline)` in the main (production) code.
```cpp
std::atomic_int now = 0;
void waitNext(int timeout)
{
const int deadline = now + timeout;
while (now < deadline) {
std::cout << "waitNext() now=" << now << "\n";
std::this_thread::sleep_for(500ms);
}
}
int main(int,
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937378509)
> unit tests can't easily move mock time during a blocking call
Consider the following pattern to do that. I think it would be better to have that in the tests instead of this `if (now == deadline)` in the main (production) code.
```cpp
std::atomic_int now = 0;
void waitNext(int timeout)
{
const int deadline = now + timeout;
while (now < deadline) {
std::cout << "waitNext() now=" << now << "\n";
std::this_thread::sleep_for(500ms);
}
}
int main(int,
...
🤔 Sjors reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2586038876)
Reviewing in reverse order.
f7dd4373b06daf34033fa84fa99408a3938e4ad4 is a beast, but I'm not sure if it can be split further. There's some mixing of code modernisation with the move which make the diff a bit trickier to follow.
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2586038876)
Reviewing in reverse order.
f7dd4373b06daf34033fa84fa99408a3938e4ad4 is a beast, but I'm not sure if it can be split further. There's some mixing of code modernisation with the move which make the diff a bit trickier to follow.
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1936898358)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: would it make sense to call `MarkAsDisconnectAndCloseConnection` in the connman fuzzer?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1936898358)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: would it make sense to call `MarkAsDisconnectAndCloseConnection` in the connman fuzzer?
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937107332)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: why plural and not `NodeSocket`?
Does the I2P session `sess` also contain a socket? Or does it use `s`?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937107332)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: why plural and not `NodeSocket`?
Does the I2P session `sess` also contain a socket? Or does it use `s`?
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937394233)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: maybe add a comment here that both `-1` and `0` still warrant (require?) reaching `EventIOLoopCompletedForNode`, so we can't just have individual `if ... continue` guards for them.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937394233)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: maybe add a comment here that both `-1` and `0` still warrant (require?) reaching `EventIOLoopCompletedForNode`, so we can't just have individual `if ... continue` guards for them.
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937302384)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: it would be useful to document these two values.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1937302384)
f7dd4373b06daf34033fa84fa99408a3938e4ad4: it would be useful to document these two values.
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937405785)
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.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937405785)
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.
💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937410103)
> I think it would be better to have that in the tests instead of this `if (now == deadline)` in the main (production) code.
Do we actually think that behavior is bad to have in production code? It seems like it's perfectly reasonable for production code to return either at the deadline or immediately after, and the choice is basically arbitrary for normal code but important for tests, so we are choosing a behavior that makes it more convenient to write tests and documenting that fact. This d
...
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1937410103)
> I think it would be better to have that in the tests instead of this `if (now == deadline)` in the main (production) code.
Do we actually think that behavior is bad to have in production code? It seems like it's perfectly reasonable for production code to return either at the deadline or immediately after, and the choice is basically arbitrary for normal code but important for tests, so we are choosing a behavior that makes it more convenient to write tests and documenting that fact. This d
...
⚠️ 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.