💬 MarcoFalke commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553376251)
See https://github.com/bitcoin/bitcoin/issues/27492#issuecomment-1527772394

💬 MarcoFalke commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1553377467)
Needs rebase

💬 ryanofsky commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553379017)
re: https://github.com/bitcoin/bitcoin/pull/27607#issue-1702212340

> * Fixed a small race, where we set the index `m_synced` flag (which enables `BlockConnected` events) before calling to the child class init function. So, for example, the block filter index could theoretically process a block before initializing the next filter position field and end up overwriting the first stored filter.

Can you clarify this? I don't see m_synced getting set to true before the index is synced, and d
...

💬 MarcoFalke commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1553383935)
Could also make sense to double check the debug.log to ensure you restarted `bitcoind` after compiling? :sweat_smile:

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1553391517)
Updated 861582b4087f1602e7115ee4a70ef7a7263eb36b -> 5a01a63ac8267debf152c8757e24f300e18ae379 ([chainstateRmKernelUiInterface_8](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_8) -> [chainstateRmKernelUiInterface_9](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_8..chainstateRmKernelUiInterface_9)).
* Addressed @ryanofsky's [comment](https://gi
...

💬 pinheadmz commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1553393935)
> Could also make sense to double check the debug.log to ensure you restarted `bitcoind` after compiling? 😅

Phew! That would've been embarassing

```
2023-05-10T15:39:40Z Bitcoin Core version v25.99.0-5b3406094f26 (debug build)
```



> `perf top -p $(pidof bitcoind)` should do the trick.

Not familiar with this tool but it looks cool! This is at the top. Every other line in the output is < 0.10%

```
99.56% bitcoind [.] boost::multi_index::detail::safe_iterator_
...

💬 0xB10C commented on pull request "build: Detect USDT the same way how it is used in the code":
(https://github.com/bitcoin/bitcoin/pull/27458#issuecomment-1553394031)
ACK b53cab0083d99e9610d74517d1d41fc615953770

Tracepoints for FreeBSD can happen at a later point, if needed.

⚠️ dooglus opened an issue: "v25.0rc2 unusably slow at times"
(https://github.com/bitcoin/bitcoin/issues/27700)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I'm testing v25.0rc2. Occasionally it becomes unresponsive. Simple requests like 'getblockcount' time out.

I tried running in gdb to see where the code was getting hung up. It was loading the mempool.dat file, spending lots of time in `CTxMemPool::addUnchecked()`, running this line:

```vTxHashes.emplace_back(tx.GetWitnessHash(), newit);```

and also in `CTxMemPool::TrimToSize()`.

...

💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553400917)

> re: [#27607 (comment)](https://github.com/bitcoin/bitcoin/pull/27607#issue-1702212340)
>
> > * Fixed a small race, where we set the index `m_synced` flag (which enables `BlockConnected` events) before calling to the child class init function. So, for example, the block filter index could theoretically process a block before initializing the next filter position field and end up overwriting the first stored filter.
>
> Can you clarify this? I don't see m_synced getting set to true befor
...

💬 MarcoFalke commented on issue "v25.0rc2 unusably slow at times":
(https://github.com/bitcoin/bitcoin/issues/27700#issuecomment-1553401313)
See https://github.com/bitcoin/bitcoin/issues/27586

💬 MarcoFalke commented on issue "v25.0rc2 unusably slow at times":
(https://github.com/bitcoin/bitcoin/issues/27700#issuecomment-1553401790)
(I presume you run with `--enable-debug`, in which case you can work around by disabling that)

🤔 stickies-v reviewed a pull request: "httpserver, rest: improving URI validation"
(https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1430879440)
> I would have liked to remove the `replySent` argument, but that's secondary to this PR which fixes a serious bug.

Me too, especially since this is now no longer a bugfix PR. I think the workarounds that need to introduced in this PR make things unnecessarily messy, and updating the fuzzer as e.g. per your suggestion [here](https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1490015098) look like a reasonable approach for this PR to me.

💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1197670181)
The two `req->GetQueryParameter` calls in `rest_mempool` need to be updated as well.

💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1196635667)
Since docs guarantee that GetURI() returns non-nullptr, would add an Assert here

```suggestion
return Assert(m_uri);
```

💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1197694855)
What's the rationale for this change? No callsite seems to be preferring a C-style string, and returning a `std::string` would also make the non-nullptr promise explicit.

💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1197667936)
nit: clearer language
```suggestion
/** Get requested URI. Will never return nullptr.
```

💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1197688711)
Is there a reason we need to check for `m_uri` not being empty (`&& m_uri[0] != '\0'`)? Doesn't seem to be a documented requirement for `evhttp_uri_parse`.

👍 ryanofsky approved a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1433163406)
Code review ACK 5a01a63ac8267debf152c8757e24f300e18ae379

I'd also squash last commit 5a01a63ac8267debf152c8757e24f300e18ae379 into earlier commit acc42d15be26434a3d52fbbe1d35a77c72254115 so reviewers only need to look at one version of MaybeCompleteSnapshotValidation calls and the fatal error function instead of 2 versions. It would also be good to rebase the PR if it would fix the CI errors

💬 MarcoFalke commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1553406573)
>(debug build)

See https://github.com/bitcoin/bitcoin/issues/27700#issuecomment-1553401313 ?

💬 pinheadmz commented on pull request "Improve performance of p2p inv to send queues":
(https://github.com/bitcoin/bitcoin/pull/27610#issuecomment-1553409722)
ah thanks I forgot I configured that way, will follow the other threads