💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620134827)
Will try

📝 dewy800 opened a pull request: "Create Dewy800"
(https://github.com/bitcoin/bitcoin/pull/30196)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2138880741)
I was able to build b4535d48aed26c3b3f61c8839b041d076b02d132 on my BSD 13.2 VM.

Opening ports fails, maybe because it's a VM, but "Address family not supported by protocol family" seems an odd error for that.

```
2024-05-23T01:58:47Z [net:error] socket(AF_NETLINK): Address family not supported by protocol family (47)
2024-05-23T01:58:47Z [net] portmap: Could not determine IPv4 default gateway
2024-05-23T01:58:47Z [net:error] socket(AF_NETLINK): Address family not supported by protocol f
...

✅ fanquake closed a pull request: "Create Dewy800"
(https://github.com/bitcoin/bitcoin/pull/30196)

📝 fanquake locked a pull request: "Create Dewy800"
(https://github.com/bitcoin/bitcoin/pull/30196)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2138887357)
Thanks for testing. Sounds like AF_NETLINK is not actually supported on that kernel, even though the necessary stuff is in the headers.

💬 fanquake commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2138906583)
Guix build (aarch64):
```bash
a51639ed29704d71baa0296b6e4c834a97c256a986cb4fd761d27096280a224a guix-build-5deb0b024e14/output/aarch64-linux-gnu/SHA256SUMS.part
084f3ea9efb5d7ceea727ac3a04b10a1229afab30798360fab053e0250793217 guix-build-5deb0b024e14/output/aarch64-linux-gnu/bitcoin-5deb0b024e14-aarch64-linux-gnu-debug.tar.gz
3aa9b03d51de7f054d61a41f5ee7afd2303fff2d5f08e1afe749f4d9330fd5ad guix-build-5deb0b024e14/output/aarch64-linux-gnu/bitcoin-5deb0b024e14-aarch64-linux-gnu.tar.gz
c6f549
...

💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1620213384)
Hmm from the way I understand this, addition of 3 is helpful in the case when `(target_weight - tx.get_weight() < 4` because otherwise the floor division by 4 will cause no padding to be added and the transaction will end up being same in vbytes. Is this the right way to understand this?

Tying this to an earlier comment of mine - if `target_weight` had been called `target_weight_wu`, the code would become more self explanatory.

I feel this is worth adding as a comment for reference in the
...

💬 rkrux commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#discussion_r1620225868)
I've been trying to think why 1 is subtracted here, but could not come up with a thorough reason. Is it somehow related to excluding the already serialised length of the output script before padding?

💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2138977552)
> [1eab7f1648dd012d8efee262a9898d2a9b044fd2.bin.txt](https://github.com/bitcoin/bitcoin/files/15429349/1eab7f1648dd012d8efee262a9898d2a9b044fd2.bin.txt)

This one is just hitting the performance of our code. Some logic in the constructor of a `thresh` fragment is quadratic in the number of sub-fragments. This input is a thresh with more than 130k sub-fragments.

Technically it would be possible to not have this logic in the constructor and cache it instead. But 1) that would only punt the is
...

🚀 fanquake merged a pull request: "build, test, doc: Temporarily remove Android-related stuff"
(https://github.com/bitcoin/bitcoin/pull/30049)

💬 fanquake commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#issuecomment-2139015463)
https://cirrus-ci.com/task/5783582369644544?logs=ci#L3033
```bash
node0 2024-05-30T02:04:26.659353Z [httpworker.0] [rpc/request.cpp:222] [parse] [rpc] ThreadRPCServer method=listsinceblock user=__cookie__
test 2024-05-30T02:04:26.660000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test
...

💬 willcl-ark commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1620262537)
Is a single job chosen for a reson here? The free hosted GHA runners have 4 (v)CPUs available AFAIK...

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2139067732)
@maflcko Thank you for testing!

👍 willcl-ark approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2087672777)
tACK 8135632c33f4bad8434403b5807c48d7dba3b3d7

This works much better than the current verifier, and being able to specify an exact version to download is more convenient in general, and especially in places such as [building docker images](https://github.com/willcl-ark/bitcoin-core-docker/blob/803e20df0f2f70ca6a635f378590e6220de743b3/27/Dockerfile#L25-L27) where downloading multiple unneeded versions is purely wasteful.

Still one open comment regarding specifying bitcoincore.org, but happy
...

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2139083195)
Rebased to resolve a conflict with bitcoin/bitcoin#30049.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620295387)
SGTM.

> That address_family is the first argument to the socket(2) syscall. What's needed is to extend CreateSock*() with the other two arguments - type and protocol. I can do that.

Yes, for specifying UDP (and NETLINK, but i do not intend to make a test framework for that) it would need all three arguments.

💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2139099415)
Alright so i think we can still pinpoint this issue to avoid having to limit the size of the input. In the same way we limit the derivation paths depth, we could limit the number of sub-fragments per fragment in the input:
```cpp
bool HasLargeFrag(const FuzzBufferType& buff, const int max_subs)
{
std::stack<int> counts;
for (const auto& ch: buff) {
if (ch == '(') {
counts.push(1);
} else if (ch == ',' && !counts.empty()) {
if (++counts.top
...

💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1620315609)
If the node is connected to 8 manually defined outbound peers and 0 inbounds and 0 automatic outbounds, then it is still operational? No need to prohibit startup in this case?

💬 marcofleon commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#discussion_r1620315667)
Got it, thanks. Should be fixed now.