👍 ryanofsky approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2185954495)
Code review ACK f3632d53a5be7efe61be8181ff481d30f7313bd4. I left some suggestions, but feel free to ignore them. Overall I think this PR makes early logging behavior a lot better by making the log format consistent, and it also makes nice API improvements like avoiding runaway memory usage, and switching to std::string_view.

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682917751)
In commit "logging: Add DisableLogging()" (0b1960f1b29cfe5209ac68102c8643fc9553f247)

I think it's a good idea to add this `DisableLogging()` method for clarity, even though in practice it's equivalent to just calling `StartLogging()` which setting any logging options.

But I think in the future it would be nice to simplify the log interface and replace `StartLogging()` `DisableLogging()` and `DisconnectTestLogger()` methods with a single SetOptions() method:

```c++
struct BufferOptions
...

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682963306)
In commit "logging: Apply formatting to early log messages" (a67baea866719837865864a5eb56a47a9f132de1)

I think it might make sense to explicitly handle the case where `starts_new_line` is false and `m_msgs_before_open` is empty. Right now the code will cut off the first part of the log line but include the rest of the message, which could lead to confusing or misleading output. Better options might be to add a `... [truncated]` prefix to the line, or to discard the whole message.

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682981482)
In commit "logging: use std::string_view" (f3632d53a5be7efe61be8181ff481d30f7313bd4)

I wonder if using insert_many is actually more efficient than strprinf:

```c++
str.insert(0, strprintf("[%s:%d] [%s] ", RemovePrefix(source_file, "./"), source_line, logging_function);
```

Using strprintf seems more readable, and I suspect if we really wanted to optimize this code we probably would build up a new string rather than repeatedly inserting at the beginning of an existing string.

👍 brunoerg approved a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444#pullrequestreview-2186107158)
reACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

💬 luisschwab commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2236816129)
Yes, I took the example out of the best block at the time. Indeed, it may be best to add the extra field instead.

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1683029030)
Both benchmarks are doing LIMO with ancestor sort, right? The only difference is in cluster shape.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1683032146)
The "ANC" one is testing a scenario that is a worst-case for ancestor sort (but is trivial for the LIMO aspect), the "LIMO" one is testing a scenario that is a worst-case for LIMO (but is trivial for the ancestor sorting aspect).

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1683034976)
ah, parsing the name wrong basically, thanks

💬 maflcko commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2236846918)
> (and perhaps mark height aa deprecated?).

Not sure. It already exists and is convenient. If a user wants to shoot themselves in the foot, they can continue to do so by turning the block hash into a block height themselves. So I don't think there is a strong benefit in removing it?

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2236852405)
ACK https://github.com/bitcoin/bitcoin/pull/30126/commits/6160ccf9b4327649e9bb0293fba630a10b3befc3

reviewed via `git range-diff master 2003bb8a279c8891e55bab190ca36f0c6c8697ea 6160ccf9b4327649e9bb0293fba630a10b3befc3`

I didn't validate that the BFS optimisation for search improved results, but it at least doesn't seem to hurt benchmark performance.

💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2236871071)
concept ACK

could we return a new field that says the mode, then add a test for that to prevent regressions?

💬 theuni commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2236884983)
## Our plan for the next few weeks:

This PR has already pointed out some very helpful high-level issues.

Because we don't want to create too much noise (yet) in this PR, we're going to maintain the staging workflow for another ~ two weeks. This means discussing and merging into @hebasto's repo, then occasionally syncing with the upstream PR. More specifically:

### Goals for week 1:
- Upstream (and hopefully get merged) all outstanding outstanding issues/prerequisites pointed out by r
...

💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236887064)
> > Yes, but that should be harmless, as a confirmed transaction in the chain can be retrieved from the chain. The same problem would exist in a broadcast pool, because any transaction in it is not guaranteed to be confirmed, and an earlier (or later) replacement could be confirmed, or none at all.
>
> I don't agree - it could lead to a situation where a user is fee bumping a transaction unnecessarily. Certainly there are no guarantees on confirmation, but where a transaction is discarded loc
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683074856)
It was just easier to split it up into 2 commits this way.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683076133)
Hmm this was added by the cherry-picked commit from @sipa. Can we leave it for a follow-up?

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2236898769)
> Could we return a new field that indicates the mode?

Yes, I plan on addressing this https://github.com/bitcoin/bitcoin/commit/2ee3d774815f5e1840cb37d2a92007f19c217e4c, in a follow-up PR: as suggested here https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1682618237 along with some documentation updates proposed in #18217
I will indicate the mode as well in that follow-up

> then add a test for that to prevent regressions?

Could you please clarify about the test what specific
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683090988)
Fixed.

📝 fanquake converted_to_draft a pull request: "build: Introduce CMake-based buid system"
(https://github.com/bitcoin/bitcoin/pull/30454)
This PR introduces a new CMake-based build system, which is a drop-in replacement for the current Autotools-based build system.

As discussed during the recent CoreDev meetup in April, the switch from Autotools to CMake is intended to happen as soon as possible after branching 28.x off, which means that 29.0 will be built using CMake.

This PR branch is essentially the [staging branch](https://github.com/hebasto/bitcoin/tree/cmake-staging), with every change reviewed and tested by a group of
...

💬 glozow commented on pull request "test: Non-Shy version sender":
(https://github.com/bitcoin/bitcoin/pull/30453#issuecomment-2236923242)
ACK fa0e227fd0c