💬 achow101 commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2071037489)
ACK fa6c300a9926a1d35fdd0a80f59ea39769bd2596

🚀 achow101 merged a pull request: "test: Fix intermittent timeout in p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/29933)

💬 achow101 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575420781)
In caa5242f84a6e1ef90b8a44c1206c7b5b942a00c "cli: Sanitize ports in rpcconnect and rpcport"

This will fail if you just have a regtest node running independent of the test:

```
2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
fun(*args, **kwds)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_fram
...

👍 sdaftuar approved a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2005939457)
Code review ACK (apart from the tests, which I only skimmed). Will test...

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568781077)
This is a code style nit, but I think I agree with https://github.com/bitcoin/bitcoin/pull/21062/files#r571975710 that there's not much benefit from using a `std::optional` on `m_replaced_transactions`? It just seems to lead to extra code around understanding when the field is set or not, when I think it would be simpler to say that it's just a list which is either empty or non-empty based on whether a replacement took place.

Anyway I have no objection to the changes in this commit; just wan
...

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575419630)
Just an observation: I guess the more common case will likely be multiple children of the same parent transaction (rather than conflicting children). Still, we have no idea which child will have the best chance of successfully bumping it, so this ordering seems as good as any.

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575118464)
Let's say we've got a transaction T that is missing inputs. We loop through the missing parents and see that none are in `m_recent_rejects`, so we drop into this block of code.

If there's more than 1 missing parent in `m_recent_rejects_reconsiderable`, is there any benefit to fetching them? It seems like we could tell in advance that validation would fail in that circumstance, because we only try 1P1C packages.

I don't know if it's worth additional complexity to deal with this case, but
...

💬 achow101 commented on pull request "test: remove duplicated ban test":
(https://github.com/bitcoin/bitcoin/pull/29688#issuecomment-2071070836)
It seems like these two tests primarily revolve around the `setban` RPC and do a couple of the same things, so perhaps they can be consolidated into one test?

💬 achow101 commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2071073647)
ACK 6d91cb781c30966963f28e7577c7aa3829fa9390

🚀 achow101 merged a pull request: "test: refactor: introduce and use `calculate_input_weight` helper"
(https://github.com/bitcoin/bitcoin/pull/29777)

💬 brunoerg commented on pull request "test: remove duplicated ban test":
(https://github.com/bitcoin/bitcoin/pull/29688#issuecomment-2071086936)
> It seems like these two tests primarily revolve around the `setban` RPC and do a couple of the same things, so perhaps they can be consolidated into one test?

See #26863. Closed due to lack of interest.

💬 asctime commented on issue "Crash on fail ~CCheckQueue() destructor?":
(https://github.com/bitcoin/bitcoin/issues/29930#issuecomment-2071114099)
Thanks for your very helpful response maflcko. As you indicate, the code in question was changed anyway, just didn't see the thread. V26.1 is that latest I can compile, haven't updated to c++20. Thanks yes close.

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2071116761)
> No, it isn't. Nor is piling on the `EVENT_CFLAGS`.

Removed both, thanks @hebasto and @laanwj !

💬 jrakibi commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1575459383)
Fixed and rebased.

💬 asctime commented on issue "ReadAnchor throws exception on second run":
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071117960)
No I cannot reproduce with the binary distro, at least on Linux. Different compilers seem to manage the exception slightly differently. Regardless I'm not sure that a blind delete is the right way to do this, is all. Fixed on my side, I just wanted to point it out.

💬 fjahr commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#issuecomment-2071123104)
re-ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47

💬 tdb3 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575519821)
Thanks for taking a look and trying it out @achow101. Much appreciated.

Good point. Yes, the test would fail if an independent node is running on 18443.

Although not ideal, do you think this would be reasonable since a warning is provided when bitcoind is running independently? (i.e. `WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!`)

Alternatively, the assert statement could be removed, but that seems a bit lac
...

💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2071244090)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

Rebased to address conflict

💬 maflcko commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575636802)
I'd say it should be avoided to use test outside of the allowed range assigned to this test case. Each test is assigned 12, or so, p2p and rpc ports. I think this test can be dropped, because if the default behavior changed, pretty much every developer would notice quickly anyway?

💬 maflcko commented on issue "ReadAnchor throws exception on second run":
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071409336)
> Different compilers seem to manage the exception slightly differently?

That should not be the case. The point of standard C++ library `std::filesystem::remove` is to provide the same interface behavior, regardless of compiler or operating system.

The specification says that `false` should be returned when the file does not exist, not an exception be thrown.

However, without exact steps to reproduce, using your compiler version, there is little that can be done here.

> Regardless I'
...