💬 MarcoFalke commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1544495872)
I presume this is still an issue? If yes, what about filing a new one, given that this issue is about `--enable-debug` (not `-debug`), and most comments here may be about a recently fixed issue #https://github.com/bitcoin/bitcoin/issues/27623 and not about `--enable-debug` performance?

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191560489)
ok, I can see that and how it reads cleaner, changed

💬 MarcoFalke commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191566131)
I think you forgot to remove the sleep?

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191569820)
done, cleaner thank you

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191573164)
My bad, thanks.

🤔 pinheadmz requested changes to a pull request: "Update best_header inside Connect/DisconnectTip"
(https://github.com/bitcoin/bitcoin/pull/26260#pullrequestreview-1423237913)
concept ACK

one question below. I haven't checked yet but I wonder if https://github.com/bitcoin/bitcoin/issues/10439 is a related issue, or how hard it would be to knock out that bug while we're working here already.

💬 pinheadmz commented on pull request "Update best_header inside Connect/DisconnectTip":
(https://github.com/bitcoin/bitcoin/pull/26260#discussion_r1191571917)
Tests still pass with this chunk removed. I wonder if checking that getblockchaininfo `headers == blocks` isn't enough? Might need to use `getchaintips` to cover.

💬 pinheadmz commented on pull request "Update best_header inside Connect/DisconnectTip":
(https://github.com/bitcoin/bitcoin/pull/26260#discussion_r1191575112)
micro-nit, this is a silly variable name. i think it's fine, but it is silly.

💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191578999)
s/FLUSH_INTERVAL/FEE_FLUSH_INTERVAL/ ?

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1191584552)
FEE_FLUSH_INTERVAL is better.
I think.

💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544546730)
> we could trim mapRelay before their scheduled expiry time has actually been reached.

Sounds like this would either degrade compact block relay when the mempool is full due to trimming `mapRelay`, or turn one DoS (OOM) into another DoS (lost fee income) due to trimming valid mempool txs? And if you use separate and dedicated size limits for `mapRelay` and the mempool, it might be easier to keep them separate data structures?

💬 glozow commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544548047)
> > > `txinfo.m_time <= mempool_req`
>
> > This condition is checking that our inv (`txinfo`) is more recent than the last `mempool_req`, meaning that once a mempool request has been processed the peer can request any transaction from our mempool.
>
> I don't get it. Is it not that the condition is checking whether `txinfo` is _older_ than the last `mempool_req`? In that case the transaction has been part of the `MEMPOOL` reply, so we have sent an `INV` about it.

Ah, I think you're righ
...

💬 glozow commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191600264)
What I mean is
Leak available without mapRelay:
1. spy sends txA. It's added to mempool.
2. spy sends txB (through the network) and continuously queries our node for txA
3. once we accept txB to mempool, we serve notfound for txA (because it's not there anymore). spy learns exactly what time we received txB.

^This is prevented with mapRelay because after txA leaves mempool, we'll still be able to serve it from mapRelay.

This is still possible with mapRelay:
1. spy sends txA. It's adde
...

💬 glozow commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544581150)
What is the benefit of `mapRelay` overlapping with mempool? If we moved it into txmempool, could we just add things to `mapRelay` from `mapTx` whenever evicting something whose timestamp is recent? If the main reason is transactions that could be in blocks, it seems reasonable to cap it at just a few thousand entries?

💬 achow101 commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1191628983)
CI Failure here:

```
C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/src/test/txpackage_tests.cpp(622): error: in "txpackage_tests/package_submission_tests": check GetVirtualTransactionSize(*tx_parent2) == 191 has failed [190 != 191]
```

I'm going to guess that this is because real signatures are being created, which can occasionally be smaller than 71 bytes, thus resulting in a tx that is one byte smaller. If the script type doesn't matter, I would suggest using tapro
...

💬 kroese commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1544593209)
I'm also seeing the same issue.. The load average suddenly increased from 0.25 to 1+. From the graph its clear it started on the 6th of May:


💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1191631520)
I'm fine if the magic number checks have a bit of wiggle room tbh, just leave a comment?

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191650786)
if the issue is during warmup, that's a pretty small window, and typically won't actually do parallel fetching, 3 seems rare even when warmed up with limit of 10. I'll gather some more stats on that front, seeing how many parallel blocks are initialized.

💬 furszy commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#issuecomment-1544648748)
Thanks for the deep review @ryanofsky!

> Reviewed [86752e0](https://github.com/bitcoin/bitcoin/commit/86752e0cc5bc48f3d4ac1cd07835c37daf078d6a) and it mostly looks good, but does have a null pointer deference currently (see comments and suggested fix). I like this PR because it will simplify #24230 and could potentially improve performance. I'm not actually how sure how much it actually would improve performance in practice though, so I'm curious if that's was the original motivation here or
...

💬 TheBlueMatt commented on issue "One core in CPU usage rate remains at 100% for a long time, causing serious delays in new blocks and forks":
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1544664280)
Are you not running any code to submit blocks across multiple bitcoin daemons quickly such as redundant nodes using the old FIBRE patchset?