🤔 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

🤔 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