💬 glozow commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540433286)
> This is true for 2 (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.

This particular case seems like you have configured bitcoind to do more than your cpu can handle during higher-traffic times. Consider reducing maxconnections for now. And perhaps shrink maxmempool - may I ask why you are using 2048?

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042)
Well, I mean that the `default` case can now be skipped. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures section `enum class` - `default:`

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847896)
(Otherwise, if a new chain type is added, it will silently map to `main`)

💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1540490305)
Reworked according to @MarcoFalke's [suggestion](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1538554964).

Friendly ping @martinus :)

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188851857)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188852707)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540528281)
See also https://github.com/bitcoin/bitcoin/pull/14309 , where I added the ChainType enum class.

💬 st3b1t commented on pull request "rpc: append rpcauth.py hash in config and show pass":
(https://github.com/bitcoin/bitcoin/pull/27588#issuecomment-1540536920)
> From CI:
>
> ```shell
> 102/264 -
> rpc_users.py
> failed, Duration: 1 s
> stdout:
> 2023-05-06T19:48:12.787000Z TestFramework (INFO): PRNG seed is: 4992424049312590578
> 2023-05-06T19:48:12.806000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230506_193750/rpc_users_159
> 2023-05-06T19:48:13.197000Z TestFramework (ERROR): Unexpected exception caught during testing
> Traceback (most recent call last):
> File "/
...

👍 brunoerg approved a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1419107706)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2

Strongly agree on adding @ryanofsky on it.

👋 hebasto's pull request is ready for review: "Enable HW-accelerated implementations of SHA256 for MSVC builds"
(https://github.com/bitcoin/bitcoin/pull/24773)

💬 hebasto commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1540583507)
Rebased on top of the merged https://github.com/bitcoin/bitcoin/pull/27575 and undrafted.

💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187910876)
This is a slightly different approach to `-fastprune` and others, which are set in `ApplyArgsManOptions()`.
So I wonder what is the criterion for an arg to be set in `ApplyArgsManOptions()`, as opposed to being initialized directly? And, closely connected, isn't the current approach of not calling `ApplyArgsManOptions()` in some spots (`setup_common`, some unit tests) a bit brittle? (considering that users could specify extra bitcoind args while running unit tests, which then would be ignored)

📝 sdaftuar opened a pull request: "p2p: Avoid prematurely clearing download state for other peers"
(https://github.com/bitcoin/bitcoin/pull/27608)
Avoid letting one peer send us data that clears out the download request (and related timers etc) from another peer.

The exception is if a block is definitely stored to disk, in which case we'll clear the download state (just as we do for compact blocks).

🤔 theStack reviewed a pull request: "Implement Mini version of BlockAssembler to calculate mining scores"
(https://github.com/bitcoin/bitcoin/pull/27021#pullrequestreview-1419015473)
Looks good to me overall, planning to do another review round within the next days.

For the `miniminer_overlap` test, I crafted some diagram and (ancestor) fee-rate calculation notes to get a better picture what's going on there in the mempool; will just dump it here, maybe it helps other reviewers or it even makes sense to include the diagram in a follow-up.

```
Tx graph for `miniminer_overlap` unit test:

coinbase_tx [mined] ... block-chain
---------------------------------
...

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1188846506)
nit: unused includes
```suggestion
#include <util/check.h>
```

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1188884884)
nit, follow-up material:
I think the `{tx5,tx7,tx8}_anc_feerate`s are all off here as they forget to account for their own tx's vsize/fee. Probably it would even make sense to check against the fee-rate calculated with the "real" values gathered from CTxMemPool with some checks like this:
```diff
- const auto tx4_anc_feerate = CFeeRate(low_fee + med_fee + high_fee, tx_vsizes[0] + tx_vsizes[1] + tx_vsizes[3]);
+ const auto tx4_anc_feerate = CFeeRate(low_fee + med_fee + high_fee + high_f
...

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1188935619)
For better readability and maintainability, would be nice to start naming the txs with index zero, to avoid the off-by-one relation (`txN <-> tx_vsizes[N-1]`).

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1188865405)
nit: seems odd to sometimes access `MiniMinerMempoolEntry` entrie's ancestor-set information via method (`.GetModFeesWithAncestors(...)`), and sometimes directly by member variable (`.vsize_with_ancestors`). Would suggest to be consistent here and either move the `{fee,vsize}_with_ancestors` fields to the `private:` section (i.e. the getter methods have to be always used) or remove the methods completely and hence always use field access.

💬 jamesob commented on pull request "p2p: Avoid prematurely clearing download state for other peers":
(https://github.com/bitcoin/bitcoin/pull/27608#issuecomment-1540617108)
Concept ACK. Will start running this when CI checks out.

👍 theStack approved a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1419184064)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2