💬 theuni commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231641407)
This was noticed while working on #30434, which requires us to install `boost::date_time`. Though vendoring `date.h` here may seem heavy, keep in mind that the status quo is a forward-incompatible mess of boost headers :)

💬 hebasto commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231641415)
> This allows us to get rid of our last use of `boost::date_time`

Concept ACK on that.

👍 maflcko approved a pull request: "logging: Replace LogError and LogWarning with LogAlert"
(https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2181072864)
For context: The majority of the changed log messages here stem from the `error()` removal in 31be1a47675e4449f856e61beb2b4bfc228ea219 which changed `ERROR:` to `[error]`. Many of them were not fatal error messages, but "warnings", or stuff that would not abort the program, so changing to to `[alert]`, seems clearer either way.


ACK 4e9ff3100ae79f3c3046a3358ff16daa7cd89627 🔗

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secre
...

💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1679920839)
nit in https://github.com/bitcoin/bitcoin/commit/fbdd8824d747c201eb01c8383d135ba08298c110: Forgot to add `Warning: ` and `Error: ` above? Can be achieved by just using `strCaption`. See also 824f47294a309ba8e58ba8d1da0af15d8d828f43

💬 maflcko commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#discussion_r1679956299)
Can you add more details, whether this is a crash/UB, or just wrong behavior? Is there a bug?

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2231676827)
I simplified PR as suggested so `bitcoin-mine` no longer spawns `bitcoin-node` if it can't connect to it. Instead it just prints an error message and suggests a command line to run `bitcoin-node` manually.

I also added a new commit to make the IPC code compatible with all the interfaces added in https://github.com/bitcoin/bitcoin/pull/30409, https://github.com/bitcoin/bitcoin/pull/30356, https://github.com/bitcoin/bitcoin/pull/30440, and https://github.com/bitcoin/bitcoin/pull/30443, so this
...

💬 maflcko commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231680026)
> Formatting in chrono is quite useful, so I don't think it makes sense to wait years until we require those compilers.

Can you give some examples where it would be useful? Right now it is only used in a single place in the wallet.

> This was noticed while working on https://github.com/bitcoin/bitcoin/pull/30434, which requires us to install boost::date_time. Though vendoring date.h here may seem heavy, keep in mind that the status quo is a forward-incompatible mess of boost headers :)


...

💬 maflcko commented on pull request "fuzz: add target for `CoinsResult`":
(https://github.com/bitcoin/bitcoin/pull/30461#issuecomment-2231685655)


Spend.cpp coverage is not too bad already: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/spend.cpp.gcov.html

It may be good to clarify the new coverage a bit.

✅ maflcko closed a pull request: "fuzz: wallet, add target for Spend"
(https://github.com/bitcoin/bitcoin/pull/28236)

💬 maflcko commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#issuecomment-2231686907)
Picked up in https://github.com/bitcoin/bitcoin/pull/30461

💬 theuni commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#discussion_r1679972352)
Specifically the comparison against the epoch (0) fails. So, if the timestamp is old enough, this should fail but doesn't:
```c++
assert tp < epoch
```

You can play around here: https://godbolt.org/z/G8r837oGf

I'll add more specific info. I should probably submit a bug report upstream as well.

🤔 instagibbs reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2178360431)
another pass, through https://github.com/bitcoin/bitcoin/pull/30126/commits/23496cb4d43327c3d27401a531586bcfc985b6ad

changes since last look make sense, I would like to spend more time validating the optimizations

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678216187)
```Suggestion
* been serialized here so far, and in what order. */
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678277812)
```Suggestion
for (const auto topo_idx : reordering) {
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678297762)
mind if we also `Assume(select.Any())`?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678263149)
This is the remaining part of logic that is hard to intuit now that the rest is a lot simpler to follow. I believe it's correct due to the harness itself but...

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678309127)
empircally

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678305787)
can we do same checks as https://github.com/bitcoin/bitcoin/pull/30126/commits/3b632d417764d79a51158aaea68586486d3b1cee#diff-1433c1fc4926a466291656ba67cf6b029523e4bd5da177ade812f25edf07343cR245 ?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1678345364)
lin_done doesn't exist

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1679851817)
```Suggestion
* is guaranteed not to make the linearization worse at any point.
```