💬 fanquake commented on pull request "depends: build FreeType with CMake":
(https://github.com/bitcoin/bitcoin/pull/29880#issuecomment-2236654832)
No-longer a version bump. Just a switch from Autotools -> CMake.

👋 fanquake's pull request is ready for review: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880)

💬 maflcko commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2236660706)
> This is not optimal, as block height is ambiguous, especially in the case of a block reorganization happening at the same instant of the query.

Sounds like a bug. The code:


```cpp
{
ChainstateManager& chainman = EnsureChainman(node);
LOCK(cs_main);
Chainstate& active_chainstate = chainman.ActiveChainstate();
active_chainstate.ForceFlushStateToDisk();
pcursor = CHECK_NONFATAL(active_chainstate.CoinsDB().Cursor());
...

🤔 danielabrozzoni reviewed a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444#pullrequestreview-2185988177)
ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

I checked manually that `+` and `-` in `getutxos` calls were once accepted and are now rejected.

💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#issuecomment-2236683680)
Yes, makes sense.

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1682962565)
Not sure about these. It should fail compilation instead if non-hex is detected.

💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2236753415)
Concept NACK

There's no reason to limit this behavior to TRUC/V3 transactions. The implementation should not ship with this restriction.

💬 maflcko commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2236758192)
Sorry, no. The code is fine, because the cursor and the tip are taken in the same lock.

And the height and hash are already returned. Not sure what is left to be done here.

I don't see the value of removing an RPC field and breaking clients for no clear reason.

I understand that the current docs for the fields may not be ideal. So pull requests with improvements are welcome :)

💬 sipa commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2236762902)
@luisschwab Just to be clear, I believe that the block hash you're asking for is already returned, under the name `"bestblock"`. I don't think we should be removing the height, but you're of course free to ignore it.

🚀 ryanofsky merged a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356)

💬 sipa commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2236771031)
Actually, I think there is some confusion here.

@luisschwab is talking about the height/blockhash of the **UTXO** found, but the suggested diff in RPC output is about the height/blockhash of the **tip** up to where the scan occurred.

I don't think it would be hard to add a block hash to the individual UTXO records returned by `scantxoutset` (for the block they were created in), in addition to the `"height"` field returned for them.

💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236775996)
> 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 locally i
...

👍 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).