💬 TheCharlatan commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1380467876)
In commit d525998b07be066c97a36536b58f0b98d816be39

Since you are changing the headers here, can you make them closer to what IWYU is suggesting?

💬 TheCharlatan commented on pull request "MiniMiner changes for package linearization":
(https://github.com/bitcoin/bitcoin/pull/28762#discussion_r1380802138)
Nit: The naming is confusing further down in the for loop, but I can't think of anything that's really better.

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1380681598)
Upps yeah. This was intended to be -2.. fixing it.

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1380719816)
> Is the intention here to mimic `NODE_NETWORK_LIMITED_MIN_BLOCKS` from the C++ source code, which is `288`?

Yeah, I used `MIN_BLOCKS_TO_KEEP` merely because it is already part of the test framework.

> `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`)?

If it helps readability, sur
...

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1380733259)
> 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?

Hmm, not really. We aren't logging any of the reasons behind a lack of a getdata request for a certain block from a certain peer (invalid tree, no segwit support, already in-flight). The approach would lead to excessive logging for no real gain.

What we could do, maybe in a follow-up?, to eliminate the unnecessary sleep time, is migrat
...

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

`vToFetch` is arranged in increasing order. Needs to stop once it reaches the last block the peer can provide, without continuing further.

`FindNextBlocks` begins from the last block in common with the peer. Then, during each iteration of the loop, it gets the block that is 128 blocks ahead and populates `vToFetch` by moving bac
...

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1380849706)
Thx, done as suggested.

💬 furszy commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1380849886)
Thx, done as suggested.

🤔 furszy reviewed a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120#pullrequestreview-1711359884)
Updated per feedback, thanks for the review!

Fixed the two blocks buffer extension (now is reduction). Small [diff](https://github.com/bitcoin/bitcoin/compare/c1612ea1f050b3daede6a181464bbbce6e254b2d..9eba981808a56eee61a20b8378447bd0972ad589).

💬 ariard commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1380874443)
“as unconfirmed transactions are not allowed to have no inputs at consensus validation (see CheckTransaction)"

💬 ariard commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1380876016)
I don’t know what is mean by consistency of a transaction with oneself (e.g if txouts are under 0 or over `MAX_MONEY`) or you’re only implying “does not spend same prevout multiple times”.

💬 kashifs commented on pull request "net: improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1791691848)
tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)

✅ maflcko closed an issue: "qa: Intermittent failure in `feature_fee_estimation.py`"
(https://github.com/bitcoin/bitcoin/issues/23165)

💬 vasild commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1792011287)
@maflcko, a mutex is required by readers to avoid writers modifying the blob while the read is happening, resulting in a torn read. The underlying variable is not readonly (ie declared as `const`), I explored converting the variables to `const` and only setting them at the constructor, but that didn't seem viable.

💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1792023227)
> In that case, maybe it would be easier to set something like `-DOS_ACTIVITY_OBJECT_API=1` for the Qt build, and not have to patch any source?

I think it will hit
```
#error Please change your minimum OS requirements because OS_ACTIVITY_OBJECT_API is not available
```

> Native builds (using the latest Xcode) are not broken in this way, and the minimum version handling shouldn't differ between the two?

I've verified the native build on macOS 14.1 without depends. Both macros are defi
...

💬 maflcko commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1792035522)
No write will happen to the datadir cache after the first write, and no reader will have the reference before the first write, no? I am trying to say that the current code is fine. Otherwise, it would be good to add steps to reproduce UB.

No objection to changing the code, but if this is worth it to change, it should be done for all places in the whole codebase, not just some places.

📝 hebasto converted_to_draft a pull request: "build: Patch Qt to handle minimum macOS version properly"
(https://github.com/bitcoin/bitcoin/pull/28775)
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732.

📝 BrandonOdiwuor opened a pull request: "gui: add used balance to overview page"
(https://github.com/bitcoin-core/gui/pull/775)
**Second part** of solving https://github.com/bitcoin-core/gui/issues/769

Add used balance to the overview page for wallets with the avoid_reuse flag enabled

### Prerequsite:

- **Part one**: https://github.com/bitcoin/bitcoin/pull/28776

overview page when avoid_reuse is enabled

overview page when avoid_reuse is not enabled
![Screenshot from 2023-11-03
...

💬 BrandonOdiwuor commented on issue "`used` balance should be shown on overview page":
(https://github.com/bitcoin-core/gui/issues/769#issuecomment-1792061121)
Created two PRs on this issue:

- Part one: https://github.com/bitcoin/bitcoin/pull/28776
- Part two: https://github.com/bitcoin-core/gui/pull/775

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1381323292)
I don't think it's that helpful to list more things this function *doesn't* check.

👋 hebasto's pull request is ready for review: "build: Patch Qt to handle minimum macOS version properly"
(https://github.com/bitcoin/bitcoin/pull/28775)