💬 furszy commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1619461019)
Could set the second parameter, `change_cost` to 0 instead of providing `cost_of_change`. The parameter is only used if there is change and BnB goal is to not generate one.

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1619482803)
While you're at it, how about replacing these move assignments with `Reseed()` functions?

I find the idea of moving an rng to be unintuitive. And as far as I can tell, every case of

`RandomMixin = foo` and `FastRandomContext = foo` and `InsecureRandomContext = foo` (all in tests) really just means "reseed".

💬 ajtowns commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2138444143)
> I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4n higher than before, and stuck looking to mine a first block at full difficulty.

I don't think that's much of a concern -- all you'd need to do is invalidateblock the last block of the period, mine a new one with
...

💬 theStack commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1619684993)
Thanks, that was helpful, I could reproduce the assertion crash with this input. Ended up putting some debug outputs into the fuzz test to see what txs are created to trigger it, and it matches the scenario described in the commit message via two submitted packages [TxA <- TxB] and [TxB' <- TxC].

>> isn't possible due to V3 ancestor limits.
>
> It's only impossible due to PackageV3Checks, since TxC has no in-mempool ancestors at PreChecks/SingleV3Checks time.

Oh right, that's the part I
...

📝 kevkevinpal opened a pull request: "test: Added test coverage to listsinceblock rpc"
(https://github.com/bitcoin/bitcoin/pull/30195)
This change is meant to add test coverage to this rpc error https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/transactions.cpp#L666C53-L666C79

This is done by setting the blocks folder to have 000 permissions before `listsinceblock` is called and then reset afterwards back to 600

---

Doing a quick grep for the error code in our functional tests leads to zero results
`grep -nri "Can't read block from disk" ./test/functional/`

💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#discussion_r1620123640)
It could use the basic testing setup indeed.

💬 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!