💬 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

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

🤔 glozow reviewed a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453#pullrequestreview-2186272759)
ACK faed5d3870f

💬 fanquake commented on pull request "depends: build FreeType with CMake":
(https://github.com/bitcoin/bitcoin/pull/29880#issuecomment-2236937905)
Guix Build (x86_64, aarch64):
```bash
bd6c916cc5fdd322bf9e7e3cecbe45a68551c0135d63b35709b32fccde289834 guix-build-ff4f3deb7b8a/output/aarch64-linux-gnu/SHA256SUMS.part
12c161c6b94ccb15d420fe27f7a812c95247a2614db5f8c6e666e04b5ef0a7d3 guix-build-ff4f3deb7b8a/output/aarch64-linux-gnu/bitcoin-ff4f3deb7b8a-aarch64-linux-gnu-debug.tar.gz
99c6d258715f38c7b56eefd0b08e80552012e8903f26d28225b690f432839545 guix-build-ff4f3deb7b8a/output/aarch64-linux-gnu/bitcoin-ff4f3deb7b8a-aarch64-linux-gnu.tar.gz
...

💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2236950771)
@sdaftuar @sipa I made a branch that combines #28676 with #29432: https://github.com/Sjors/bitcoin/tree/sv2-cluster

The main commit is https://github.com/bitcoin/bitcoin/commit/f646977ddac0622889d484683ae59c262d50d8dc. It slightly changes the `waitFeesChanged()` interface. The implementation now frequently creates a new template.

The Template Provider then uses that to see if sending a new `NewTemplate` message is justified.

It still takes 20-30ms on mainnet to create a template, withou
...

👍 ryanofsky approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2186195988)
Code review ACK c3a9c71c7077324bf0aa40f834f7537a14619340

I think the changes are all good, but the PR and the most important commit "fix: Make TxidFromString() respect string length" (c3a9c71c7077324bf0aa40f834f7537a14619340) are lacking a good description that actually says what the bug is.

I think they should say that currently `TxidFromString` takes a string_view parameter, but internally it is calling the `uint256S` function which expects a nul-terminated string. If `TxidFromString` is
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683055614)
In commit "refactor: Change base_blob::SetHex() to take std::string_view" (f77d961d926405637dfbdfb9a9baea1fab4f1b7b)

Note: This line isn't directly equivalent to previous code because it trims whitespace from the end of the string, not just the beginning, but the net effect is the same because any whitespace at the end of the string would be ignored anyway.

I'm surprised looking at the implementation of SetHex to see how unreliable it is in general. Since it can't report errors it winds up
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683086730)
In commit "refactor: Change base_blob::SetHex() to take std::string_view" (f77d961d926405637dfbdfb9a9baea1fab4f1b7b)

It seems not great, especially in the light of the unterminated string bug, that this will iterate over the entire string when only the beginning portion of the string may be needed. For example, if the string is 1MB and the blob is 32 bytes, this will iterate over 1MB. Would changing this to:
```c++
while (digits < str.size() && digits < WIDTH*2 && ::HexDigit(str[digits]) !=
...

🚀 glozow merged a pull request: "test: Non-Shy version sender"
(https://github.com/bitcoin/bitcoin/pull/30453)