💬 fanquake commented on pull request "build: create Depends build type for depends and use it by default for depends builds":
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-3139342071)
40a01e9d30b0c7deabd5996867f248637f3d1a08 has been merged via #32584.
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-3139342071)
40a01e9d30b0c7deabd5996867f248637f3d1a08 has been merged via #32584.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244981257)
It is just 50 lines, so it should be manageable. Anyone is free to review it file-by-file, if they want to split it up, but I am not sure about splitting it up too much?
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244981257)
It is just 50 lines, so it should be manageable. Anyone is free to review it file-by-file, if they want to split it up, but I am not sure about splitting it up too much?
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244981786)
thx, done
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244981786)
thx, done
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244982186)
seems unrelated? But i am happy to review a pull request changing it
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244982186)
seems unrelated? But i am happy to review a pull request changing it
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244982876)
Just to make review easier, because you agree that the commit is already large in https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233735207. Will leave as-is for now.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244982876)
Just to make review easier, because you agree that the commit is already large in https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233735207. Will leave as-is for now.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244984778)
> Looking at the code I understand it of course, but maybe something like:
>
> ```c++
> ElapseTime elapse_time{};
> elapse_time.Advance(777s);
> ```
Not sure. This is doing something else, as explained above by yourself? So this replacement is not correct to apply here.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244984778)
> Looking at the code I understand it of course, but maybe something like:
>
> ```c++
> ElapseTime elapse_time{};
> elapse_time.Advance(777s);
> ```
Not sure. This is doing something else, as explained above by yourself? So this replacement is not correct to apply here.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244985411)
> are we planning on using the new declared object?
Yes, it can trivially be used, if there is need to. Possibly it isn't used in this specific instance any further, but it seems beneficial to allow it and also beneficial to be consistent in the naming and usage.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244985411)
> are we planning on using the new declared object?
Yes, it can trivially be used, if there is need to. Possibly it isn't used in this specific instance any further, but it seems beneficial to allow it and also beneficial to be consistent in the naming and usage.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244987390)
I think it should be allowed to have it set previously, as there may be valid use-cases. Using it in multiple threads is not a thing right now and I can't imagine a use-case in the future. However, if there is one, it seems fine to support it and allow it as well.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244987390)
I think it should be allowed to have it set previously, as there may be valid use-cases. Using it in multiple threads is not a thing right now and I can't imagine a use-case in the future. However, if there is one, it seems fine to support it and allow it as well.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244987874)
thx, done
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244987874)
thx, done
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988027)
thx, mentioned global in a new struct-level-doc
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988027)
thx, mentioned global in a new struct-level-doc
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988253)
thx, mentioned global in a new struct-level-doc
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988253)
thx, mentioned global in a new struct-level-doc
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988435)
thx, done
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988435)
thx, done
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988724)
thx, done
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244988724)
thx, done
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244991616)
> nit: I don't find the operator to be intuitive here, maybe we could call it `Advance` instead?
I think `elapse_time(4h)` or `elapse_steady(4h)` is self-explanatory. So leaving as-is for now.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244991616)
> nit: I don't find the operator to be intuitive here, maybe we could call it `Advance` instead?
I think `elapse_time(4h)` or `elapse_steady(4h)` is self-explanatory. So leaving as-is for now.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994040)
thx, reworded comment
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994040)
thx, reworded comment
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994154)
thx, reworded comment a bit
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994154)
thx, reworded comment a bit
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994320)
thx, fixed
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994320)
thx, fixed
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994428)
thx, split up into a new commit
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994428)
thx, split up into a new commit
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994901)
It is split up, because you agree that the commit is already large in https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233735207. Will leave as-is for now.
(https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2244994901)
It is split up, because you agree that the commit is already large in https://github.com/bitcoin/bitcoin/pull/32430#discussion_r2233735207. Will leave as-is for now.
💬 maflcko commented on pull request "test: Add and use ElapseTime helper":
(https://github.com/bitcoin/bitcoin/pull/32430#issuecomment-3139428832)
Force pushed with some minor doc-changes and small refactoring in `src/test/util/time.h`
(https://github.com/bitcoin/bitcoin/pull/32430#issuecomment-3139428832)
Force pushed with some minor doc-changes and small refactoring in `src/test/util/time.h`