👍 alfonsoromanz approved a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953#pullrequestreview-2022398345)
ACK 9381052194a78024b3994cc6ad906858c477b88f

💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2077104799)
ACK https://github.com/bitcoin/bitcoin/pull/29708/commits/3c1ae3ee33d4d9dbea046d5ab8ee924a12982759

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

Also checked the natpmp changes since the last update.

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-2077119552)
> Are there any examples from the last couple of years where this would have helped to prevent a bug or other issue?

Here is an example where two unrelated variables were guarded by the same mutex (already fixed in the latest code):

https://github.com/bitcoin/bitcoin/blob/c42ded3d9bda8b273780a4a81490bbf1b9e9c261/src/net.cpp#L116-L118

✅ vasild closed a pull request: "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es"
(https://github.com/bitcoin/bitcoin/pull/25390)

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-2077125161)
Closing this due to no interest from reviewers for a long time plus currently my hands are full with more important things. Would be happy to revisit if there is interest.

💬 maflcko commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-2077134842)
> > Are there any examples from the last couple of years where this would have helped to prevent a bug or other issue?
>
> Here is an example where two unrelated variables were guarded by the same mutex (already fixed in the latest code):

I don't think this is an example of a bug or other issue.

This was added in commit b312cd770701aa806e9b264154646f481d212c1c to fix a tsan warning. I guess the annotation was using the wrong mutex, but I doubt this will happen in newly written code. Als
...

🚀 fanquake merged a pull request: "depends: build libnatpmp with CMake"
(https://github.com/bitcoin/bitcoin/pull/29708)

📝 m3dwards opened a pull request: "depends: pass verbose through to cmake based makefiles"
(https://github.com/bitcoin/bitcoin/pull/29960)
While testing https://github.com/bitcoin/bitcoin/pull/29708 I was not able to enable verbose output to check which flags were being given to the compiler.

With this PR, running depends with V=1 will enable verbose output from makefiles generated by cmake.

How to test:

```shell
# checkout commit where libnatpmp has been changed to being built with cmake
git checkout 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759
make -C depends libnatpmp V=1
```

👍 alfonsoromanz approved a pull request: "Fix typos in description.md and wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/29938#pullrequestreview-2022456592)
ACK 03e36b3da093e2c23cf51b46f6901cb84ddbf867

🚀 fanquake merged a pull request: "Fix typos in description.md and wallet_util.py"
(https://github.com/bitcoin/bitcoin/pull/29938)

👍 alfonsoromanz approved a pull request: "test: add missing comparison of node1's mempool in MempoolPackagesTest"
(https://github.com/bitcoin/bitcoin/pull/29948#pullrequestreview-2022468934)
Tested ACK e912717ff63f111d8f1cd7ed1fcf054e28f36409. The code looks good to me and the test execution is successful.

💬 fanquake commented on pull request "depends: pass verbose through to cmake based makefiles":
(https://github.com/bitcoin/bitcoin/pull/29960#issuecomment-2077167651)
Concept ACK

🚀 fanquake merged a pull request: "deploy: remove some tools when cross-compiling for macOS"
(https://github.com/bitcoin/bitcoin/pull/29890)

🚀 fanquake merged a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953)

💬 BrandonOdiwuor commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1579465882)
Thanks for this clarification @willcl-ark

🤔 sr-gi reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2015631578)
Third pass, reviewing the changes of the first two, plus going all the way to 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

I think the code looks good overall, but I have some questions regarding some corner cases that I'm not completely sure about (see inline).

Finally, something I've noticed: Given a `1p1c` whose parent we've seen already and we just received the child, the parent will be tried on its own twice before being considered as a package. This is because we will try it first when r
...

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576401327)
Reviewing the changes in this file I realized something seems to be odd with the way the test was initially designed:

`duplicate_input` is defined as a flag that, if set, will allow duplicate inputs to be used when building the transaction. However, the way this is approached is weird: if `duplicate_input` is not set, upon picking prevout as input, we will kick it off `outpoints` to make sure we don’t pick it up again as another input. Later on, all selected inputs are added back to outpoints
...

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

I'm not too familiar with fuzzing, but wouldn't it be better if this be set unconditionally? This will trigger on the first iteration of the loop, and then based on a coin flip AFAICT, which means that if the variable is not overwritten, we will call `orphanage.AddChildrenToWorkSet(*ptx_potential_parent);` with the same transaction multiple times. This is harmless, given the internals just add data to a set, so multiple calls won't change it, but it
...

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

Is the placeholder fee needed? `create_self_transfer_multi` already has a default fee, and the value shouldn't matter, should it?