💬 laanwj commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2439914238)
> I haven't thought very seriously about what high quality entropy sources are and whether or not we have enough of them on Windows without this performance data

The high quality OS entropy source on windows is `CryptGenRandom`, and we use that.

Not opposed to collecting other ancillary data to mix in, though let's try to avoid something as crude as pulling data from all drivers and processes on the system.

📝 laanwj opened a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164)
Add a method `CNetMessage::GetMemoryUsage` and use this for accounting of the size of the process receive queue instead of the raw message size.

This ensures that allocation and deserialization overhead is taken into account.

💬 laanwj commented on pull request "scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}":
(https://github.com/bitcoin/bitcoin/pull/31163#issuecomment-2439920615)
Concept ACK, always has been a pet peeve of mine.

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#issuecomment-2439923324)
i'll review this when #31130 is merged, i have some of my own ideas about portmap.cpp as well that i haven't fully worked out yet.

💬 davidgumberg commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2439923901)
> My reasoning was that there should be enough CI runs on master and all runs on PRs based on top of the merged fix.

Oh, didn't think of that, makes sense to me.

💬 laanwj commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#discussion_r1818022053)
Might want to prefix this argument with `regtest` to make it doubly clear that it doesn't do anything on other networks.

👍 rkrux approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2397501918)
tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

👍 itornaza approved a pull request: "rpc: getorphantxs follow-up"
(https://github.com/bitcoin/bitcoin/pull/31043#pullrequestreview-2397502622)
tACK 0ea84bc362f395fd247623c22942eb5ca3d1b874

Tested against all of the standard test and extended harness. Everything looks great!

👍 laanwj approved a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2397521287)
Code review ACK 40e5f26a3ff77e50df808f6f850c617aec2df203
i have also checked that:
- no mentions of miniupnp as a dependency remain
- `-natpmp` still works to forward ports on my router after this
- `-upnp`, if still present in the configuration file or command line, gives a custom message and exits

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818083019)
I think it suffices to have a `bool m_have_changeset GUARDED_BY(cs){false};`, as it's only used for tracking and enforcing the "no two changesets" rule?

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818083695)
Nit, if it's a member class of `CTxMempool` anyway, maybe a less clunky name like `ChangeSet` suffices?

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818086982)
Style: use braces/newlines/indentation when the entire `if` statement doesn't fit on a single line.

📝 andrewtoth converted_to_draft a pull request: "validation: fetch block inputs on parallel threads ~17% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/31132)
When fetching inputs in ConnectBlock, each input is fetched from the cache in series. A cache miss means a round trip to the disk db to fetch the outpoint and insert it into the cache. Since the db is locked from being written during ConnectTip, we can fetch all block inputs missing from the cache on parallel threads before entering ConnectBlock. Using this strategy resulted in a ~17% faster IBD (or master was ~21% slower).

Doing IBD with 16 vcores from a local peer with default settings, sto
...

💬 sipa commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818101205)
I do find this function a bit confusing; it seems like `m_ancestors` is effectively a cache for the result of `m_pool->CalculateMempoolAncestors()`, but then I'd expect this function to also act like a cache?

How about:

```c++
util::Result<CTxMemPool::setEntries> CalculateMemPoolAncestors(TxHandle tx, const Limits& limits)
{
LOCK(m_pool->cs);
auto it = m_ancestors.find(tx);
util::Result<CTxMemPool::setEntries> ret;
if (it == m_ancestors.end()) {
ret = m_pool-
...

📝 theStack opened a pull request: "key: clear out secret data in `DecodeExtKey`"
(https://github.com/bitcoin/bitcoin/pull/31166)
Same as in `DecodeSecret`, we should also clear out the secret data from the vector resulting from the Base58Check parsing for xprv keys. Note that the if condition is needed in order to avoid UB, see #14242 (commit d855e4cac8303ad4e34ac31cfa7634286589ce99).

👍 i-am-yuvi approved a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2397573025)
Tested ACK 40e5f26a3ff77e50df808f6f850c617aec2df203

- -upnp gives custom messages and exits
- -natpmp works

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r1818140297)
I tried this and it causes memory errors, since the remaining futures will have a dangling ref to `m_condition`.

💬 mzumsande commented on issue "Stop at header":
(https://github.com/bitcoin/bitcoin/issues/31162#issuecomment-2440142364)
For what would you need to use this kind of feature? This probably wouldn't be as simple to implement as current `-stopatheight` (one would need to keep downloading blocks after the last header was received but not accept any more headers), so I think that there should be a serious use case.

💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1818164107)
Thanks!

Updated in d7fd766feb2f579bdba0e778bacdeb13103e8282

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818172232)
Done.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818172279)
Done.