💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#discussion_r1578168924)
Thank you for the review!

Great question. The location chosen was within the `# vv Tests less than 30s vv` portion of `BASE_SCRIPTS`. I'm seeing run-times for `feature_framework_unit_tests.py` (on two machines I'm using) of 9s and 6s, and `TEST_FRAMEWORK_UNIT_TESTS` is around mid-way in the "30s" portion of the list, which seemed reasonable.
https://github.com/bitcoin/bitcoin/blob/f19f0a2e5af6c2a64900f1f229e21b6f1668bd3d/test/functional/test_runner.py#L392-L393

The existing comment to i
...

🤔 theStack reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2019874263)
Another round through (modulo the fuzz commit), the code looks correct to me, just left a few nits below. Planning to do another review tomorrow with focus on the main commit bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6, especially going through the `AlreadyHaveTx` call-sites and their `include_reconsiderable` values to check once more.

💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577902195)
nit: for the sake of completeness, could also check that txid and wtxid are equal for non-segwit tx 2:
```suggestion
BOOST_CHECK(txid_1.ToUint256() != wtxid_1.ToUint256());
BOOST_CHECK_EQUAL(txid_2.ToUint256(), wtxid_2.ToUint256());
BOOST_CHECK(txid_3.ToUint256() != wtxid_3.ToUint256());
```

💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578117743)
nit: as alternative, could check at the beginning of the function if the sizes of `package` and `senders` match and return early if they don't (not sure how this function is used in the future if the 1p1c limit is lifted, but i guess a size mismatch should never happen, unless there is a bug at the call-site?). that would be slightly simpler to reason imho.

💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1577900718)
Was it intended to mix both wtxids and txids for hashing in this test-case? (IIUC the meaning of "int order" here, this would be wtxid2, wtxid1, wtxid3, i.e. identical with `hash_if_by_txid`, so I guess one of the two test-cases can be removed).
```suggestion
uint256 hash_if_use_int_order = (HashWriter() << wtxid_2 << wtxid_1 << wtxid_3).GetSHA256();
```

💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578179082)
```suggestion
# All nodes receive parent_31 ahead of time.
```

💬 theStack commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578122793)
refactoring nit to avoid multiple accesses in `cpfp_candidates_different_peer` (feel free to ignore):
```suggestion
const auto [candidate_child, candidate_peer] = cpfp_candidates_different_peer.at(index);
const Package maybe_cpfp_package{ptx, candidate_child};
if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
tx_orphan = candidate_child;
orphan_sender = candidate_peer;
b
...

💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578041562)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"

Since the expected pattern is to always `GroupCoins(utxo_pool)`, it might make sense to make `utxo_pool` a `std::vector<OutputGroup>` and combine this function with `AddCoins`.

💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578039066)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"

There doesn't seem to be a purpose for having a static variable here. We basically treat it as temporary since it's cleared every time.

💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578036822)
In 3dff8f0490b4b92c4adf229a828063dfda6d3a80 "[test] Create coinselection_tests"

The elements in `coins` are `CAmount`, not `int`.

💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578043081)
In e286c6470bbaa1358fecb7c5a7e109583d158780 "[test] Recreate simple BnB success tests"

The elements of `expected_input_amounts` are `CAmount` not `int`.

💬 achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1578048284)
In e286c6470bbaa1358fecb7c5a7e109583d158780 "[test] Recreate simple BnB success tests"

The `*_MESSAGE` functions print out the message on failure, not on success. So the message should be something that better indicates what the failure was, not something that might suggest something was successful.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578189219)
It seems the addition of a new util file makes this PR conflicts with other ones. Perhaps it's better to put everything in `wallet/test/util` or leave as is? What do you think @maflcko?

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2075383041)
Rebased on top of the merged #29910.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578202506)
Yeah that's a typo :+1:

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1578202070)
I think we'd want to make sure that even if we had a bad `senders` vector we would still call `ProcessValidTx` / `ProcessInvalidTx` (seems kind of bad not to). Maybe a better approach is to use -1 when we don't know what it is - I do realize we're not handling the empty vector case.

(Yeah maybe this is moot as the 1 caller should always make `senders` properly...)

💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578211737)
Actually I don't understand why the last push compiles. CI says:

* https://cirrus-ci.com/task/6011306526900224?logs=ci#L2291 ` CXX test/fuzz/util/libtest_fuzz_a-wallet.o`
* but the wallet is disabled: ` with wallet = no` https://cirrus-ci.com/task/6011306526900224?logs=ci#L1385

Shouldn't this result in a link or even compile error?

👍 vasild 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-2020413434)
ACK 9381052194a78024b3994cc6ad906858c477b88f

💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578213646)
Perhaps this is what we want but I thought it worth mentioning that there is a difference on my Mac between this and having cmake call make with: `cmake --build . --target natpmp`.

$(MAKE) from autotools for me calls GNU MAKE 3.81 and cmake delegates to gmake being GNU MAKE 4.4.1.

Should we call cmake instead and allow it to decide the best make to delegate to?

💬 fanquake commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578220994)
Possibly, but if we are going to do that, we could do it globally/all at once, or even set something at a higher level in depends. Not sure if we have to change this during these migrations.