💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575989359)
Edited the comment to say this is a probably uncommon case

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575996002)
I definitely it's awkward and doesn't have much benefit. I'll leave the assume/handling here, and maybe in a followup we can change it to non-optional.

💬 maflcko commented on pull request "Fix typos in description.md":
(https://github.com/bitcoin/bitcoin/pull/29938#issuecomment-2071925121)
Also:

```
test/functional/test_framework/wallet_util.py:168: lenghts ==> lengths

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2071948004)
i've added some review notes for the Qt patch here: https://github.com/laanwj/qtsowrap?tab=readme-ov-file#patching-qt

🤔 hebasto reviewed a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2016884640)
Approach ACK b7a3f8e375b87e4f3ccfa7994905dfbf46a23bf2.

💬 hebasto commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576040769)
nit: For new source files we can use a copyright header with HTTPS link and future-proof dating:
```suggestion
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.
```

💬 hebasto commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576041988)
nit: It seems no symbols from that header are used in the source file.

💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2071989207)
> The rebase is wrong.

Thank you. Looks like I had overlooked the addition of `wallet_util` (`TEST_FRAMEWORK_MODULES` being moved to feature_framework_unit_tests.py made it less obvious). Will rebase/push soon.

📝 theStack opened a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939)
Different MiniWallet instances using the same mode (either ADDRESS_OP_TRUE, RAW_OP_TRUE or RAW_P2PK) currently always create and spend UTXOs with identical output scripts, which can can cause unintentional tx dependencies (see e.g. the discussion in https://github.com/bitcoin/bitcoin/pull/29827#discussion_r1565443465). In order to avoid mixing of UTXOs between instances, this PR introduces the possibility to provide a MiniWallet tag name, that is used to derive a different internal key for the t
...

👍 stickies-v approved a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904#pullrequestreview-2016873417)
ACK b7a3f8e375b87e4f3ccfa7994905dfbf46a23bf2

Good to remove this unnecessary libevent dependency, and nice work on restructuring the commits etc. Easy to review PR. Left some nits and happy to quickly review if you decide to address them.

---

Not essential for this PR, but I noticed we're doing two unnecessary string copies (through `std::string::substr()`) in code touched by this PR:

<details>
<summary>git diff on b7a3f8e375</summary>

```diff
diff --git a/src/common/url.cpp b/s
...

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576038708)
nit: `std::errc` [is part of `system_error`](https://en.cppreference.com/w/cpp/error/errc)

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576033526)
nit: this should move up a bit:

<details>
<summary>git diff on b7a3f8e375</summary>

```diff
diff --git a/src/Makefile.am b/src/Makefile.am
index 239113a056..7387eb1eaa 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -679,6 +679,7 @@ libbitcoin_common_a_SOURCES = \
common/run_command.cpp \
common/settings.cpp \
common/system.cpp \
+ common/url.cpp \
compressor.cpp \
core_read.cpp \
core_write.cpp \
@@ -711,8 +712,6 @@ libbitcoin_common_a_SOURCES = \

...

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576047415)
typo nit
```suggestion
BOOST_AUTO_TEST_CASE(decode_internal_nulls_test) {
```

💬 stickies-v commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1576053878)
nit: includes not sorted

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576145432)
Good point, I don't think this is too complex so I've added it.

While working on this, I remembered/realized that if there are any other unconfirmed parents at all, we will reject it because the package must be child-with-unconfirmed-parents, enforced here:
https://github.com/bitcoin/bitcoin/blob/256e1703197fdddd78bc6d659431cd0fc3b63cde/src/validation.cpp#L1558-L1564

IIRC this was the way to check that a package was "2 generations only". But I don't know how useful this is, and it's quite
...

👋 Eunovo's pull request is ready for review: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680)

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576164670)
FWIW: I added logging here to see what number of parents and given by different peers than the orphan. After a day of running it's 0 out of 162.

Is there a reason to think this should be a common pattern?

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576171557)
If a parent+child are relayed back to back across the network, then I would expect there to be times when we download the transactions out of order (eg because we send a request for the parent to a different peer than the request for the child).

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576185766)
I guess it'll likely be something like:

1) peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)
2) peer B has slightly out of date feefilter for your node, sends INV for parent

💬 maflcko commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2072220456)
Why is #29720 / https://github.com/bitcoin/bitcoin/issues/29328 not mentioned here?