💬 brunoerg commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1579393843)
> I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.

Sgtm!

👍 TheCharlatan approved a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2022385975)
ACK 66022315641934bda301d61f009dae9cb3b45da0

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