💬 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

🤔 jamesob reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1419307024)
I've udpated this PR to remove the new `ChainstateManager::GetAllForBlockDownload()`, instead adding a parameter `assumed_first` to the existing `GetAll()`. I think this is clearer at the net_processing call site and avoids another unnecessary method.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189036653)
Fixed, thanks. Though I haven't updated the comment; having trouble figuring out what to say there.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189040133)
Fixed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189039131)
Fixed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189036219)
Hmm, would we relax this for background validation? I don't really see why. Recall that "our_tip" is referring to the tip of the chainstate we're working on (could be background) and not our network tip.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189039493)
Fixed, but using `Assume()` because that seems safer.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189039363)
Fixed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1189039290)
Fixed, and I've dropped the commit in question.

🤔 jamesob reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1419316384)
I've udpated this PR to remove the new `ChainstateManager::GetAllForBlockDownload()`, instead adding a parameter `assumed_first` to the existing `GetAll()`. I think this is clearer at the net_processing call site and avoids another unnecessary method.

👍 instagibbs approved a pull request: "p2p: Avoid prematurely clearing download state for other peers"
(https://github.com/bitcoin/bitcoin/pull/27608#pullrequestreview-1419317015)
ACK 52e52071e01f4e98d87a47e1d5f3c5c3cc6dbaf4

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1540768765)
> Also, as far as I understand it, AssumeUTXO will mess up the order of the blocks stored on disk - the first blockfiles would have the blocks between the AssumeUTXO block and the tip, and the subsequent blockfiles would have the blocks between Genesis and the AssumeUTXO block. I wonder if that could break or slow down something, have you tried a reindex for example?

It's worth noting that there's a proposed remedy for this in #27596 - see https://github.com/bitcoin/bitcoin/pull/27596/commits
...

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

done, the resultant commit, only two files:
https://github.com/bitcoin/bitcoin/pull/27588/commits/e1046eca9d637dfd29599d156a57a8a3a7440b23

💬 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#issuecomment-1540782565)
Addressed comments by @MarcoFalke and fixed a couple of logs from the tests

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1189064293)
> 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?

Initializing in the list is done for option members that don't have sane default or empty values. This also has precedent in the `ChainstateManager::Options`.

> And, closely connected, isn't the current approach of not calling ApplyArgsManOptions() in some spots
...

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