💬 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

🤔 instagibbs reviewed a pull request: "p2p: Avoid prematurely clearing download state for other peers"
(https://github.com/bitcoin/bitcoin/pull/27608#pullrequestreview-1419160203)
first glance, looks right to me. I was literally writing the same code as part of as learn-as-I-go resurrection of https://github.com/bitcoin/bitcoin/pull/10984

this would be required for any parallel download implementation as well