💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1679915895)
> because CBlock inherits from CBlockHeader

Ah ok

> that method is useful for avoiding round trips

The choice is between having the caller be careful to avoid round trips, vs not needing round trips at all because we already got the header at construction time.

That's likely a tiny difference, so whatever is better code is probably best.

📝 theuni opened a pull request: "utils: replace boost::date_time usage with c++20 std::chrono"
(https://github.com/bitcoin/bitcoin/pull/30462)
This would be a very straightforward and uncontroversial change if not for the sad state of std lib implementations which implement this functionality.

As of now, `std::chrono::parse` and friends (c++20 additions) are only available as of gcc 14 and (unreleased) clang 19.

Formatting in chrono is quite useful, so I don't think it makes sense to wait years until we require those compilers.

Instead of waiting around, this PR takes the sledgehammer approach of adding a fully-conformant impl
...

💬 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 ?