💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378905232)
Should this not be `continue;`? I find `PeerManagerImpl::FindNextBlocks()` hard to follow. Are the entries in `vToFetch` sorted in ascending or descending order?

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378886266)
I agree - no need for +2, or even it could be harmful and defeat the margin on the sender side - e.g. if the sender is at height 1290, this logic would allow requesting block 1000. Then if the sender progresses to height 1291 in the meantime and sees request for 1000 they will disconnect.

Further, to make this symmetric with the sender maybe it should be -2? If we assume the sender has no margin and disconnects for anything >288 then the requester should request only up to 286 (incl)?

💬 ajtowns commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#discussion_r1378949369)
Not really a fan of adding 8 bytes to every `CTransaction`, though I guess it's not horrible?

Another approach would be to separate out the raw data and compute-logic into one class, and the optimised version with the w/txid cached into a separate class. That looks like: https://github.com/ajtowns/bitcoin/commit/89e3c808927984309a002b2cab1c0a622f5c15f8

💬 fanquake commented on pull request "build: Update `qt` package up to 5.15.11":
(https://github.com/bitcoin/bitcoin/pull/28769#issuecomment-1789184088)
Concept ACK. Lets get this merged, and then figure out what the issue in #28622. That should unblock C++20.

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#discussion_r1378994739)
Another problem is that (I presume) large single-keys-bag wallets may take hundred times longer to load. Not saying that this is a common use case, but I don't think the RPC is non-experimental for users before this is fixed?

🤔 hernanmarino reviewed a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1708559433)
Approach ACK . I'm really curious about benchmarks but I don't think they are really necessary to make a decision about this. I also believe this PR will have a higher impact in the future, should any changes to fee estimation be necessary. I 'll try to take a deeper look and code review in a couple of days, if still unmerged.

📝 maflcko opened a pull request: "refactor: Remove unused circular include dependency from validation.cpp"
(https://github.com/bitcoin/bitcoin/pull/28770)
Also, sort includes

💬 hebasto commented on pull request "build: Update `qt` package up to 5.15.11":
(https://github.com/bitcoin/bitcoin/pull/28769#issuecomment-1789305628)
Guix builds:
```
x86_64
f3d6ac10842f48884b36d704cc4303ee001197a5c39f7d65116324ab919f4497 guix-build-8047bb6feaa9/output/aarch64-linux-gnu/SHA256SUMS.part
a651622edbca1c9badf7f4018296639929f9553c257bc2cdb464c240e5ab29d2 guix-build-8047bb6feaa9/output/aarch64-linux-gnu/bitcoin-8047bb6feaa9-aarch64-linux-gnu-debug.tar.gz
6ad8004e3739c51380032a411f43054a7768188033bcd9534ebb165b1755819d guix-build-8047bb6feaa9/output/aarch64-linux-gnu/bitcoin-8047bb6feaa9-aarch64-linux-gnu.tar.gz
aaecf84b3fa
...

💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1379059681)
The lock is already being held from the top of the function.

💬 ismaelsadeeq commented on pull request "refactor: Remove unused circular include dependency from validation.cpp":
(https://github.com/bitcoin/bitcoin/pull/28770#issuecomment-1789311030)
Ack fa7d31910ab181c7e0e5f1fa1e23a49e208aec2a

👍 hebasto approved a pull request: "refactor: Remove unused circular include dependency from validation.cpp"
(https://github.com/bitcoin/bitcoin/pull/28770#pullrequestreview-1708639244)
ACK fa7d31910ab181c7e0e5f1fa1e23a49e208aec2a

💬 maflcko commented on pull request "refactor: [tidy] modernize-type-traits":
(https://github.com/bitcoin/bitcoin/pull/28664#discussion_r1379069331)
I don't think it is possible to exclude headers via bear, as they are not listed as translation unit anyway

🤔 glozow reviewed a pull request: "refactors for subpackage evaluation"
(https://github.com/bitcoin/bitcoin/pull/28758#pullrequestreview-1708690726)
I started going down the path of making Package a class and these member functions, but it touched hundreds of loc and made it really awkward to try to use it within AncestorPackage when we're checking if subsets are sorted

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1379095337)
ok

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1379095637)
fixed

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1379095561)
I made it iswellformedpackage

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1379219584)
Yes, as I understand it that is what is checked in the code block above, so this should always hold.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1379220432)
Huh, yeah. That is more straight forward.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1789568785)
Thank you for the review @ismaelsadeeq!

Updated e579bb98ba8977af284ba6914ffb2b1da7f34cdd -> 105a0f4db4ffdc25d3ad30300c949d46d5d8e647 ([simplifyMemPoolInteractions_3](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_3) -> [simplifyMemPoolInteractions_4](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_3..simplifyMemPoolInteractions_4))

* Added commit 105
...

💬 maflcko commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1789583586)
Maybe mark as draft while CI is red?