💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1789130504)
Thanks @stickies-v went with your suggestion as my previous idea was buggy again🔥

📝 hebasto opened a pull request: "build: Update `qt` package up to 5.15.11"
(https://github.com/bitcoin/bitcoin/pull/28769)
In the light of https://github.com/bitcoin/bitcoin/pull/28622, we probably have to patch Qt. It seems reasonable to update it up to the latest available version before doing that.

🤔 vasild reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1708290287)
Approach ACK c1612ea1f0

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378928478)
Is the intention here to mimic `NODE_NETWORK_LIMITED_MIN_BLOCKS` from the C++ source code, which is `288`? `MIN_BLOCKS_TO_KEEP` is a different constant defined in both the C++ and Python code which happens to be also `288`. Should `NODE_NETWORK_LIMITED_MIN_BLOCKS` be defined in the Python code (equal to 288, but not necessary equal or tied to `MIN_BLOCKS_TO_KEEP`)?

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378843710)
In the commit message of f3089e57aadac462e59499fdeb8091c42f39327b `refactor: Make FindNextBlocksToDownload friendlier`:

`s/FindNextBlocksToDownload/PeerManagerImpl::FindNextBlocks()/`

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378931951)
Here `sync_blocks()` is used and above `self.wait_until(lambda: node.getbestblockhash() == ...` is used to achieve the same purpose. For consistency maybe use only one of the methods.

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1378941093)
This sleep is non-deterministic. The good thing is that it will not cause sporadic test failures, but still, it could occasionally report success even if the full node requests an old block and gets disconnected (false positive). Also it adds unconditional 3 seconds to the tests execution time.

Is it possible to lookup something in the log or via RPC to cross check that the full node has made a decision not to request blocks from the pruned node?

💬 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