💬 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

💬 ryanofsky commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553414023)
Thanks! So the problem is that `m_synced` is set to true before `CustomInit` code runs, instead of after. It looks like this bug was introduced in bef4e405f3de2718dfee279a9abff4daf016da26 from #25494

💬 ryanofsky commented on pull request "indexes: Stop using node internal types":
(https://github.com/bitcoin/bitcoin/pull/25494#discussion_r1198123860)
In commit "indexes, refactor: Remove CBlockIndex* uses in index Init methods" (bef4e405f3de2718dfee279a9abff4daf016da26)

Calling the `CustomInit()` function after the `Init()` function here seems to cause a race condition, noticed by @furszy and described in https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553400917. The problem is that `Init()` can set `m_synced` to true, which enables `BlockConnected` callbacks. But if `BlockConnected` callback runs before `CustomInit` finishes t
...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1553426376)
Rebased 5a01a63ac8267debf152c8757e24f300e18ae379 -> d96c82a76775b1a41c098e6af60130fbdbba9975 ([chainstateRmKernelUiInterface_9](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_9) -> [chainstateRmKernelUiInterface_10](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_10), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_9..chainstateRmKernelUiInterface_10)).
* Squashed the last commit

💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553427942)
> Thanks! So the problem is that `m_synced` is set to true before `CustomInit` code runs, instead of after. It looks like this bug was introduced in [bef4e40](https://github.com/bitcoin/bitcoin/commit/bef4e405f3de2718dfee279a9abff4daf016da26) from #25494

Yeah!, that is more or less what I wrote in the description "Fixed a small race, where we set the index `m_synced` flag (which enables `BlockConnected` events) before calling to the child class init function".

Happy to write it differently
...

💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198122182)
could also simplify a bit:

```cpp
if (est_file.IsNull() || !Write(est_file))
LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", fs::PathToString(m_estimation_filepath));
else
LogPrintf("Flushed fee estimates to fee_estimates.dat.\n");
```

🤔 pinheadmz reviewed a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1433103480)
concept ACK

This should solve the stale fee issue, the test is clever and effective. I ensured the test fails without the patch and also checked the test failed when the mtime was set < 12 hours. Reviewed code and left some notes / questions

💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198123417)
I don't actually know for sure but couldn't this just be ...?

```cpp
static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};
```

💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198071636)
Mine as well use this constant here for the filename?

https://github.com/bitcoin/bitcoin/blob/6cc136bbd36f859a16e469bb5c016d06c19bcd50/src/policy/fees_args.cpp#L10

💬 pinheadmz commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198131760)
I think you can use `self.restart_node(0)` here

💬 ryanofsky commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1553447313)
> Happy to write it differently if it is not good enough. Maybe should change "child class init function" for "CustomInit"?. (in the commit description it's explained with more details anyway)

Yes if CustomInit was mentioned I probably would have understood better. I would say this PR fixes a race condition where an index's `CustomAppend` method might be called before its `CustomInit` method, causing the index to try to update itself before it is initialized. And that the PR fixes the proble
...