👍 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

💬 instagibbs commented on pull request "p2p: Avoid prematurely clearing download state for other peers":
(https://github.com/bitcoin/bitcoin/pull/27608#discussion_r1188940525)
should describe what the argument does and when it should be used

💬 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_r1188956722)
> m_last_mempool_req is now unused and can be removed completely?
Looks like it

> Also, it might be good to not touch the return here when there is no reason, to avoid code churn and review burden.

I thought the only reason why this was split into two lines is that it was a bit too long (?), I tried to follow the same approach as in `L2267` and, while I think it looks more compact the way it is now, I don't have a strong opinion about on styling atm, so I'm happy to amend it if there is c
...

💬 sdaftuar commented on pull request "p2p: Avoid prematurely clearing download state for other peers":
(https://github.com/bitcoin/bitcoin/pull/27608#discussion_r1188968159)
Consider it done.

💬 MarcoFalke commented on pull request "rpc: append rpcauth.py hash in config and show pass":
(https://github.com/bitcoin/bitcoin/pull/27588#issuecomment-1540662159)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 MarcoFalke commented on pull request "refactor: Replace global find_value function with UniValue::find_value method":
(https://github.com/bitcoin/bitcoin/pull/27605#issuecomment-1540674721)
> except for ...

Might be good to ask the gcc devs if they want to fix this, as it seems a different false positive for `decltype(auto)` return type lambdas?

Still, added a temporary commit here to hack around it.

👍 jamesob approved a pull request: "p2p: Avoid prematurely clearing download state for other peers"
(https://github.com/bitcoin/bitcoin/pull/27608#pullrequestreview-1419260615)
ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4 ([`jamesob/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl`](https://github.com/jamesob/bitcoin/tree/ackr/27608.1.sdaftuar.p2p_avoid_prematurely_cl))

Built, reviewed code locally. Deployed to b-02.slug on https://bmon.info; all
metrics there look normal (peer count, tip connection).


<details><summary>Show signature data</summary>
<p>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4 ([`ja
...

🤔 mzumsande reviewed a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1419313316)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2