💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573341141)
In ca9f03f2207854c849a14d7af2fcd91a5f675e14

Wouldn't it be easier (and simpler) to use a set here instead of a vector (as used to be the case in 9dc967195c4965973be0174ae6041be70a886c7a). It makes sense to use a `vec` in `GetChildrenFromSamePeer` given you are sorting based on a custom order, but here a set should be equivalent and less verbose (?)

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573354857)
In 42859548ab5aebf40da6089b85065f7c204b992a

To make sure the two collections are equal you also need to check the their sizes match. This applies to the four cases.

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573331603)
In 5c8aa657642ef24e711a73c28278644f14117d73

This last check is still redundant

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573353122)
In 42859548ab5aebf40da6089b85065f7c204b992a

I don't think the order of recency is being tested

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573344593)
In ca9f03f2207854c849a14d7af2fcd91a5f675e14

I think this is misleading (both the variable name and the comment).

The reason a vector is chosen is so you can sort based on `nTimeExpire`. Removing the duplicates comes later (for most of the function, the iters may not be unique).

For the sake of future readers it may be worth changing it

🤔 hebasto reviewed a pull request: "depends: build libnatpmp with CMake"
(https://github.com/bitcoin/bitcoin/pull/29708#pullrequestreview-2013138028)
Approach ACK 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759.

I've verified compiler flags for different scenarios, including cross-compiling for Windows and providing `DEBUG=1`. CMake ones lack `-Wall -Wextra` flags, but that's not a blocker.

Also this PR limits the installed headers to ones that are actually used in our code, which is fine

💬 luke-jr commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2067767242)
Many people here conflating consensus (as modified by BIPs) with spam filters/policy. The reason why it's reasonable to relax consensus rules is in a large part *because* spam is mitigated by policy. If policy is to be discarded, we will need to impose all these limits strictly in consensus - that has not been proposed, and as such, reasonable policies must be maintained until it is. You can't just throw out an important security check without replacing it by something else!

Taproot's purpose
...

💬 nflatrea commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2067767293)
> > Don't we have plethora of well documented guides and specifications on the JSON-RPC api ?
>
> No. I'd highly doubt that there is a better source of the documentation and specification than Bitcoin Core itself. (If there is, then that is a bug in Bitcoin Core, which should be fixed)
>
> > https://developer.bitcoin.org/reference/rpc/
>
> This example has many issues:
>
> * It is not machine readable
>
> * It does not indicate the version of Bitcoin Core that is documente
...

💬 hebasto commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067769916)
@furszy
> Faced this too. Updating to 14.0.3 fixes the problem.

Just to clarify, what macOS version did you update your clang on?

⚠️ Christewart opened an issue: "`keypoolrefill` doesn't fill keypool to specified parameter"
(https://github.com/bitcoin/bitcoin/issues/29924)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

According to the help manual the parameter is `newsize` and `keypoolrefill` fills the keypool to `newsize`. This was the behavior up to at least v21 of bitcoind. In v24 of bitcoind these semantics have changed. Related to https://github.com/bitcoin-s/bitcoin-s/pull/5496

```
$ bitcoin-cli -regtest getwalletinfo
{
"walletname": "",
"walletversion": 169900,
"format": "sqlite",

...

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2067781043)
Re https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2006662114 and https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868

> For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests
...
> I also really like stickies suggestion from https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable
...

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2067787575)
Updated d9bcbbf2293ef427b37eefca30f074be5eeeca26 -> 9b2c9c2fce32fe858d1361e863c72108a384a5c8 ([noGlobalReindex_0](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_0) -> [noGlobalReindex_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex_0..noGlobalReindex_1))

* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1565782441), removed double definition o
...

💬 luke-jr commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2067788551)
This seems too complicated for a testnet exception IMO. And it breaks the use case of someone testing being able to mine a block on-demand without actual mining hardware.

Shouldn't it be enough to just fix the timewarp bug?

⚠️ 1440000bytes opened an issue: "Add warnings or discontinue zip files for Windows and maOS "
(https://github.com/bitcoin/bitcoin/issues/29925)
1. One issue was shared in https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1610800583

Example URL: https://bitcoincore.org∕bin∕bitcoin-core-27.0∕@bitcoin-27.0-win64.zip this will open attackers domain which in this case is `bitcoin-27.0-win64.zip`

2. Other issue: https://www.bleepingcomputer.com/news/security/github-comments-abused-to-push-malware-via-microsoft-repo-urls/

Example URL: https://github.com/bitcoin/bitcoin/files/15049876/bitcoin-27.0-arm64-apple-darwin.zi
...

💬 pinheadmz commented on issue "Add warnings or discontinue zip files for Windows and maOS ":
(https://github.com/bitcoin/bitcoin/issues/29925#issuecomment-2067803749)
> 1. Example URL: [https://bitcoincore.org∕bin∕bitcoin-core-27.0∕@bitcoin-27.0-win64.zip](https://bitcoincore.org%E2%88%95bin%E2%88%95bitcoin-core-27.0%E2%88%95@bitcoin-27.0-win64.zip)

The @ is pretty obvious to me. But regardless what can bitcoin core do to protect users who download software from links they find anywhere outside bitcoincore.org ?

Bitcoin core could stop serving releases altogether and replace the entire website with a warning, this .zip attack would be just as effective.
...

💬 hebasto commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2067807003)
My Guix builds:
```
x86_64
a134241bc9ff9823ce078ea10dbac544ff63536ee3f66fb557414ca191b1d62d guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/SHA256SUMS.part
f5a46ab7a5faae9ce50b2d084cf65d14372b354b9f8369178f1348b70a3b675b guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu-debug.tar.gz
e2353ed4f0738026fe16cd90b503ef2f02f31e0b68ba42ea80924997225ae374 guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu.tar.gz
78060899
...

👍 hebasto approved a pull request: "depends: build libnatpmp with CMake"
(https://github.com/bitcoin/bitcoin/pull/29708#pullrequestreview-2013186915)
ACK 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759.

💬 furszy commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067809363)
Ventura 13.1.

💬 hebasto commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067810384)
> Ventura 13.1.

Well. It seems Apple does not suggest any upgrade paths for Big Sur and Monterey, which are still supported platforms for [Bitcoin Core 27.0](https://bitcoincore.org/en/2024/04/16/release-27.0/).

📝 fjahr opened a pull request: "test: Fix multiprocess CI timeout in p2p_tx_download"
(https://github.com/bitcoin/bitcoin/pull/29926)
This addresses multiprocess CI failures in `p2p_tx_download.py`, likely introduced by #29827.

I was having a hard time reproducing or rationalizing the root cause of the issue but it seemed very likely the mock time wasn't working as expected without another reset and I got a successful run with it when I temporarily introduced it to another PR I am working on: https://cirrus-ci.com/task/5109555795853312