💬 hebasto commented on issue "build, android: `make apk` fails if depends source cache is in a non-default location":
(https://github.com/bitcoin/bitcoin/issues/22522#issuecomment-2143768165)
This can be closed as builds for Android are [not possible](https://github.com/bitcoin/bitcoin/issues/29360) in the master branch.

💬 theStack commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1623397692)
The main reason for using `struct.unpack` in this PR is to be consistent with the already existing code in the bdb module (i.e. avoiding to mix up different serialization methods), and I didn't want to introduce large refactoring changes in this PR. I strongly agree though that `int.{to,from}_bytes` is preferred and support #29401.

💬 theStack commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-2143824209)
Concept ACK

💬 theStack commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1623464945)
nit: not a must, but it could be interesting for users to know what exactly has improved with the new format (i.e. enhanced metadata, enabling more detailed error messages etc.)

👍 theStack approved a pull request: "doc, rpc: Release notes and follow-ups for #29612"
(https://github.com/bitcoin/bitcoin/pull/30167#pullrequestreview-2092374868)
utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2

💬 davidgumberg commented on pull request "Lint: support running individual rust linters and improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2143889113)
Rebased to address merge conflict

💬 hebasto commented on issue "test: SegFault in `ismine_tests` on SunOS / illumos":
(https://github.com/bitcoin/bitcoin/issues/29908#issuecomment-2143979930)
I've tested the recent master branch @ 457e1846d2bf6ef9d54b9ba1a330ba8bbff13091 on the updated system:
```
$ uname -a
SunOS openindiana 5.11 illumos-82079dec87 i86pc i386 i86pc
```

The issue is no longer reproducible.

Closing.

✅ hebasto closed an issue: "test: SegFault in `ismine_tests` on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/issues/29908)

✅ fanquake closed an issue: "build, android: `make apk` fails if depends source cache is in a non-default location"
(https://github.com/bitcoin/bitcoin/issues/22522)

📝 hebasto opened a pull request: "build: Fix building `fuzz` binary on on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/pull/30216)
On master branch @ 457e1846d2bf6ef9d54b9ba1a330ba8bbff13091, building the `fuzz` binary fails:
```
$ ./autogen.sh
$ ./configure
$ gmake -C src test/fuzz/fuzz
< snip >
CXX test/fuzz/fuzz-http_request.o
test/fuzz/http_request.cpp:13:10: fatal error: event2/buffer.h: No such file or directory
13 | #include <event2/buffer.h>
| ^~~~~~~~~~~~~~~~~
compilation terminated.
gmake: *** [Makefile:17138: test/fuzz/fuzz-http_request.o] Error 1
gmake: Leaving directory '/ex
...

💬 laib200 commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2144007809)
براكة الله فيك 😊

👍 tdb3 approved a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2092614645)
re ACK for 0aa7db42564408edb41b0d42103d39ba4c2787dc

Re-ran modifications of the tests to induce failure as described in https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1564345633 and https://github.com/bitcoin/bitcoin/pull/29862/commits/0aa7db42564408edb41b0d42103d39ba4c2787dc#r1578682415.

Unexpected reject reasons were caught (succesfully).

👍 tdb3 approved a pull request: "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30202#pullrequestreview-2092618199)
ACK for 77e34ded543e19ba27cab8d0cfc464af27f04bbf

Thanks. Seems like a reasonable change.

Sanity checks performed:
- Synced the last 1.5 day's worth of blocks on signet. Binding and sync seemed to work fine.
- Ran all unit and functional tests (passed).

Left some minor notes (just thinking out loud, so no need to answer/comment unless you feel it's necessary).

💬 tdb3 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1623667994)
Seems good to add this sanity check to prevent setting non-pertinent sockopts. Might be worthwhile to mention in the PR description that in addition to extending arguments, some extra safety checks were put in place.

💬 tdb3 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1623667512)
Seems ok to remove, since `protocol` is now a passthrough argument.

💬 tdb3 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1623665893)
I'm assuming changing `GetSAFamily()`'s return type was avoided to keep the PR scoped (and since the change doesn't seem necessary).

Since `GetSAFamily()` returns `sa_family_t`, looks like this is a simple promotion from an `unsigned short` (`sa_family_t`) to `int`. This seems fine (it aligns with the typical way `socket()` args are typically used).

💬 tdb3 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#discussion_r1623668596)
Since these are simple `int`s passed by value, seems ok to change from the previous const reference approach.

💬 furszy commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1623680370)
Can use `pindexNew` hash. We cache the block hash there.

💬 furszy commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1623683248)
In b67c82a6f2ab6:

This commit does not compile, shouldn't have changed `m_all_conflicts` for `m_all_conflicting`.

💬 furszy commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1623681866)
Could replace all the `std::get_if` for a more readable `IsReason<Conflict>(&reason)` if you include this function:
```c++
template <typename T>
bool IsReason(const MemPoolRemovalReason& reason)
{
return std::get_if<T>(reason) != nullptr;
}
```