💬 jonatack commented on issue "qa: Intermittent failure in `feature_fee_estimation.py`":
(https://github.com/bitcoin/bitcoin/issues/23165#issuecomment-1791553473)
This is probably resolved by the merge of https://github.com/bitcoin/bitcoin/pull/21161 and can be closed.

📝 bufo24 opened a pull request: "log: torcontrol opt checks"
(https://github.com/bitcoin/bitcoin/pull/28780)
Aims to fix #23589.
Host and port validation on torcontrol, same logic is being used on the other flags which need this validation as well.

This is my first PR for bitcoin core and came across this good first issue, continued on already existing code for this issue.

Took some inspiration from [this comment](https://github.com/bitcoin/bitcoin/pull/25136#pullrequestreview-1152866901)

🤔 TheCharlatan reviewed a pull request: "MiniMiner changes for package linearization"
(https://github.com/bitcoin/bitcoin/pull/28762#pullrequestreview-1710802872)
Concept ACK

lgtm, but can't really comment on the breadth of test cases.

💬 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.