💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188619638)
8e1d9a4bedc73b1ce2124a8583df8ed78d0c48ed: Maybe move the `CanServeBlocks(*peer)` check out of this loop (and check `< MAX_BLOCKS_IN_TRANSIT_PER_PEER` first) to make the `if` statement more readable.
You should also clarify that `|| !chainstate->IsInitialBlockDownload()` is always true for the background validation chainstate, so we never use a pruned peer for this.
I'm somewhat inclined with @ryanofsky's [observation](https://github.com/bitcoin/bitcoin/pull/24008/commits/ac00102e4712af7b04
...
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188619638)
8e1d9a4bedc73b1ce2124a8583df8ed78d0c48ed: Maybe move the `CanServeBlocks(*peer)` check out of this loop (and check `< MAX_BLOCKS_IN_TRANSIT_PER_PEER` first) to make the `if` statement more readable.
You should also clarify that `|| !chainstate->IsInitialBlockDownload()` is always true for the background validation chainstate, so we never use a pruned peer for this.
I'm somewhat inclined with @ryanofsky's [observation](https://github.com/bitcoin/bitcoin/pull/24008/commits/ac00102e4712af7b04
...
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188628966)
8e1d9a4bedc73b1ce2124a8583df8ed78d0c48ed: light preference for `--requests_available` in the for loop above.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188628966)
8e1d9a4bedc73b1ce2124a8583df8ed78d0c48ed: light preference for `--requests_available` in the for loop above.
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188647774)
Could add a TODO here to relax this for background validation. And/or explain that we expect a node with less than the minimum chainwork to still be in IBD and not willing to serve us blocks? Or that it's too much hassle to figure out which blocks they have in common with us (e.g. it's a fork-coin)?
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188647774)
Could add a TODO here to relax this for background validation. And/or explain that we expect a node with less than the minimum chainwork to still be in IBD and not willing to serve us blocks? Or that it's too much hassle to figure out which blocks they have in common with us (e.g. it's a fork-coin)?
💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188629628)
`assert(requests_available >= vToDownload.size())` ? `FindNextBlocksToDownload` itself doesn't guarantee that, because its `vBlocks` param doesn't have to be empty.
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188629628)
`assert(requests_available >= vToDownload.size())` ? `FindNextBlocksToDownload` itself doesn't guarantee that, because its `vBlocks` param doesn't have to be empty.
👍 ryanofsky approved a pull request: "refactor: Move chain constants to the util library"
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1418757860)
Code review ACK d168458d1ff987e0d741c75ac1d4b63ae0cfb7e7. Just suggested changes since last review.
If you can, it would also be helpful to mark previous review comments as resolved (github won't let me do this)
> Thank you for your patience
Likewise! Thank you for all the improvements
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1418757860)
Code review ACK d168458d1ff987e0d741c75ac1d4b63ae0cfb7e7. Just suggested changes since last review.
If you can, it would also be helpful to mark previous review comments as resolved (github won't let me do this)
> Thank you for your patience
Likewise! Thank you for all the improvements
💬 fanquake commented on pull request "refactor: Remove unused GetTimeMillis":
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1540226069)
> Yeah, I guess it can be removed as well, assuming there are no merge conflicts with other open pulls?
sgtm
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1540226069)
> Yeah, I guess it can be removed as well, assuming there are no merge conflicts with other open pulls?
sgtm
🚀 fanquake merged a pull request: "refactor: Remove unused GetTimeMillis"
(https://github.com/bitcoin/bitcoin/pull/27594)
(https://github.com/bitcoin/bitcoin/pull/27594)
👍 stickies-v approved a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1418784493)
utACK https://github.com/bitcoin/bitcoin/commit/59ebee3fb4181baf20fab263cf1b587ece1bd5e2
ryanofsky has consistently proven to be extremely skilled, thorough and helpful. Even though he's comfortable being opinionated, I always find his discourse respectful and well balanced. His PRs and reviews often serve as a benchmark for how things should be done, for me. I think he has the right attributes for being successful as a maintainer.
I cannot verify his key as I do not have it in my keyring.
...
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1418784493)
utACK https://github.com/bitcoin/bitcoin/commit/59ebee3fb4181baf20fab263cf1b587ece1bd5e2
ryanofsky has consistently proven to be extremely skilled, thorough and helpful. Even though he's comfortable being opinionated, I always find his discourse respectful and well balanced. His PRs and reviews often serve as a benchmark for how things should be done, for me. I think he has the right attributes for being successful as a maintainer.
I cannot verify his key as I do not have it in my keyring.
...
💬 fanquake commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540240043)
I'm going to merge this shortly. #27125 (which conflicts) will be rebased (some other conflicting changes may also be merged in the interim), along with its current set of review feedback addressed.
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540240043)
I'm going to merge this shortly. #27125 (which conflicts) will be rebased (some other conflicting changes may also be merged in the interim), along with its current set of review feedback addressed.
💬 fanquake commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540243436)
Is this dependant on #27573
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540243436)
Is this dependant on #27573
👍 fanquake approved a pull request: "ci: Remove CI_EXEC bloat in test/06_script_b.sh"
(https://github.com/bitcoin/bitcoin/pull/27573#pullrequestreview-1418806156)
ACK fa1dbd04cab8039440e721eddabb760a40ba8c61 - this conflicts with #27125, but that is going to be rebased soon, and this could be merged in the interim. cc @TheCharlatan
(https://github.com/bitcoin/bitcoin/pull/27573#pullrequestreview-1418806156)
ACK fa1dbd04cab8039440e721eddabb760a40ba8c61 - this conflicts with #27125, but that is going to be rebased soon, and this could be merged in the interim. cc @TheCharlatan
🚀 fanquake merged a pull request: "refactor: Move chain constants to the util library"
(https://github.com/bitcoin/bitcoin/pull/27491)
(https://github.com/bitcoin/bitcoin/pull/27491)
📝 furszy opened a pull request: "init: verify blocks data existence only once for all the indexers"
(https://github.com/bitcoin/bitcoin/pull/27607)
At present, during init, we are repeatedly traversing the chain
(once per index) to confirm that all necessary blocks to sync
each index up to the current tip are present.
To make the process more efficient, we can fetch the oldest block
from the indexers and perform the chain data existence check from
that point only once (if the oldest block passes the check, newer
blocks will also pass it).
The initial refactoring commit d7eef9f4 was stolen from #24230.
(https://github.com/bitcoin/bitcoin/pull/27607)
At present, during init, we are repeatedly traversing the chain
(once per index) to confirm that all necessary blocks to sync
each index up to the current tip are present.
To make the process more efficient, we can fetch the oldest block
from the indexers and perform the chain data existence check from
that point only once (if the oldest block passes the check, newer
blocks will also pass it).
The initial refactoring commit d7eef9f4 was stolen from #24230.
💬 MarcoFalke commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540298627)
Yes, I can't figure out how to fight `bash -c`, so I removed it there
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540298627)
Yes, I can't figure out how to fight `bash -c`, so I removed it there
💬 fanquake commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1540310529)
> done
We are likely going to defer mergeing this for the moment, as it conflicts with the next two kernel PRs, and assumeutxo. Up to you, if you'd like to keep it rebased after each merge, in the interim.
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1540310529)
> done
We are likely going to defer mergeing this for the moment, as it conflicts with the next two kernel PRs, and assumeutxo. Up to you, if you'd like to keep it rebased after each merge, in the interim.
💬 sr-gi 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_r1188745852)
Do you mean keep adding what you're interested in into the filter so it triggers an `INV` from the node and, therefore, we can request it (or, similarly, keep resetting the filter and just add the one single transaction that we want to probe)? If so, isn't that equivalent to having no filter?
I think in both of the aforementioned cases (which boil down to the same logic if I'm not mistaken) the patch prevents the attacker from requesting unannounced transactions straight-away, they will have
...
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1188745852)
Do you mean keep adding what you're interested in into the filter so it triggers an `INV` from the node and, therefore, we can request it (or, similarly, keep resetting the filter and just add the one single transaction that we want to probe)? If so, isn't that equivalent to having no filter?
I think in both of the aforementioned cases (which boil down to the same logic if I'm not mistaken) the patch prevents the attacker from requesting unannounced transactions straight-away, they will have
...
💬 gits7r commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540339751)
This is true for two full nodes that run with `pruning` off, `peerbloomfilters` off, `maxmempool` set to 2048 and `maxconnections` set to 500, where both have an average between 170 and 225 active peers.
Both use fairly reasonable CPUs and are at 100% CPU usage and a lot of RAM (like 8GB only bitcoind). For reference:
"connections": 222,
"connections_in": 212,
"connections_out": 10,
This is true for two full nodes that run with `pruning` off, `peerbloomfilters` off, `maxmempool` set to 2048 and `maxconnections` set to 500, where both have an average between 170 and 225 active peers.
Both use fairly reasonable CPUs and are at 100% CPU usage and a lot of RAM (like 8GB only bitcoind). For reference:
"connections": 222,
"connections_in": 212,
"connections_out": 10,
![bitcoind-res](https://github.com/bitcoin/bitcoin/assets/6549887/c0c43907-ca26-4af7-bedc-4058f0
...
💬 MarcoFalke 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_r1188763337)
`m_last_mempool_req` is now unused and can be removed completely? Also, it might be good to not touch the `return` here when there is no reason, to avoid code churn and review burden.
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1188763337)
`m_last_mempool_req` is now unused and can be removed completely? Also, it might be good to not touch the `return` here when there is no reason, to avoid code churn and review burden.
💬 MarcoFalke commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1188776917)
nit in d7eef9f49a827e96337febcf20a22b392dc7b51e: Can use `chaiman()`?
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1188776917)
nit in d7eef9f49a827e96337febcf20a22b392dc7b51e: Can use `chaiman()`?
💬 fanquake commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540384552)
Concept ACK on removing the continual manual updating of this list
(https://github.com/bitcoin/bitcoin/pull/27571#issuecomment-1540384552)
Concept ACK on removing the continual manual updating of this list