💬 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.
```

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1679854169)
nitty renaming
```Suggestion
ClusterIndex NumChunksLeft() const noexcept { return m_chunks.size(); }
```

💬 maflcko commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#discussion_r1679976789)
Ah, so it is UB/crash, according to ubsan:

```
/opt/compiler-explorer/gcc-14.1.0/include/c++/14.1.0/bits/chrono.h:227:38: runtime error: signed integer overflow: -9223372037 * 1000000000 cannot be represented in type 'long int'

💬 achow101 commented on pull request "rpc: Use CHECK_NONFATAL over Assert":
(https://github.com/bitcoin/bitcoin/pull/30429#issuecomment-2231701658)
ACK fa6270737eb9655bfb4e29b7070ecb6cd2087b7f

💬 maflcko commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#discussion_r1679982342)
Would be good to add a fuzz target as well to https://github.com/google/oss-fuzz/tree/master/projects/libstdcpp

💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2231709067)
ACK 66d35a5c1384bf579e2da8e7054be7536e7a7101 🐛

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

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 66d35a5c1384bf579e2da8e705
...

🤔 ryanofsky reviewed a pull request: "logging: Replace LogError and LogWarning with LogAlert"
(https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2181173092)
Updated 4e9ff3100ae79f3c3046a3358ff16daa7cd89627 -> 66d35a5c1384bf579e2da8e7054be7536e7a7101 ([`pr/alert.3`](https://github.com/ryanofsky/bitcoin/commits/pr/alert.3) -> [`pr/alert.4`](https://github.com/ryanofsky/bitcoin/commits/pr/alert.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/alert.3..pr/alert.4)) with suggested change adding noui.cpp warning and error prefixes

@maflcko do you think you could review #30361 as well? #30361 is a documentation bugfix that I think would mak
...

💬 ryanofsky commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1679983749)
re: https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1679920839

> nit in [fbdd882](https://github.com/bitcoin/bitcoin/commit/fbdd8824d747c201eb01c8383d135ba08298c110): Forgot to add `Warning: ` and `Error: ` above? Can be achieved by just using `strCaption`. See also [824f472](https://github.com/bitcoin/bitcoin/commit/824f47294a309ba8e58ba8d1da0af15d8d828f43)

Good catch. Done in new commit

💬 theuni commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231717016)
> > 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.

I was assuming that it'd be useful in other places where it's simply cumbersome to deal with these conversions like logging. Or as a simplification for `FormatISO8601DateTime` in `util/time.cpp`. But sure, if we're not going to end up using any more of thi
...

🚀 achow101 merged a pull request: "rpc: Use CHECK_NONFATAL over Assert"
(https://github.com/bitcoin/bitcoin/pull/30429)

💬 achow101 commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-2231730709)
ACK fe92c15f0c44d1405b9048306736bd0eae868506

💬 maflcko commented on pull request "utils: replace boost::date_time usage with c++20 std::chrono":
(https://github.com/bitcoin/bitcoin/pull/30462#issuecomment-2231739397)
> I started down this rabbit hole intending to simply rewrite this function, but ended up afraid of introducing bugs due to the locale footguns. Maybe I'm over-complicating it?

Happy to try myself, but https://en.cppreference.com/w/cpp/chrono/year_month_day (which can convert to system clock) is available, so using `Split` + `ToIntegral` + `year_month_day`, then adding the hours, minutes and seconds should do everything in about 20 lines of code?

💬 maflcko commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2231748835)
> * It is not how `LogError` and `Level::Error` are used currently. Of 129 current uses only 58 cases are fatal according to [#30348](https://github.com/bitcoin/bitcoin/issues/30348)

The reason is that many stem from `error()`, see also https://github.com/bitcoin/bitcoin/pull/30364#pullrequestreview-2181072864.

> * "[T]here's not much benefit in a log that says "hey this error is about to cause the node to stop" -- you already get that information by seeing "Shutdown: in progress..." imme
...