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

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