💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578146669)
style nit: snake_case for new code, and newline before `{`, according to clang-format

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578133898)
This must be `unsinged`, otherwise a sign (`-`) is treated as valid hex

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578145369)
style nit: The verbose member calls can be avoided by turning `i` into an iterator:

```diff
std::string urlDecode(std::string_view urlEncoded) {
std::string res;
res.reserve(urlEncoded.size());

- for (size_t i = 0; i < urlEncoded.size(); ++i) {
- char c = urlEncoded[i];
+ for (auto i = urlEncoded.begin(); i < urlEncoded.end(); ++i) {
+ char c = *i;
// Special handling for percent which should be followed by two hex digits
// represe
...

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578134591)
`%-1`

💬 maflcko commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578118369)
style nit in fcee97601f464e3330d10ff85e19e6ce17452897: Missing space, according to clang-format.

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