👍 alfonsoromanz approved a pull request: "test: Assumeutxo: snapshots with less work should not be loaded"
(https://github.com/bitcoin/bitcoin/pull/29428#pullrequestreview-2020129687)
Code review and tested ACK 4922f56660ae7834598c6dc4904a6711c23edfa3

💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1578065722)
Yepp, looks good, thanks!

🚀 glozow merged a pull request: "feefrac: avoid explicitly computing diagram; compare based on chunks"
(https://github.com/bitcoin/bitcoin/pull/29757)

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2012191090)
Updated 974b16b2580f4f09c8936fbead96fdbee637cb14 -> c45cb13b9e4f6eae50ab6dc42acf471921980591 ([`pr/saferesult.1`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.1) -> [`pr/saferesult.2`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.1..pr/saferesult.2)) with suggestions

re: https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2011883406

> I'm unsure about removing the assignment ope
...

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572817468)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572639146

> nit: nice, but maybe best to clean up all 3 instances in one go?

Thanks! Applied patch

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572817364)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572648440

> `Update` is a bit of an ambiguous name imo, it doesn't clearly indicate if it means replacing or merging into the existing result. I think for this commit, where we can only overwrite the existing result, I think `Replace` would be better. For the next commits in #25665 where we actually add the merging functionality, I think `Merge` or `Combine` would be a better choice.

We could definitely have a `Replace` method
...

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572817536)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572632242

> nit: Any reason not to just wrap the args in `std::move`? Seems like the more intuitive approach, and probably slightly more performant?

Agreed that's much better. Applied suggestion.

👍 maflcko approved a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2020255232)
lgtm, but `unsigned` may be better

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