💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598496074)
I would say so. We do all `ProcessOrphanTx` before processing the ping; the child will be added to workset when the parent is accepted through `ProcessOrphanTx`

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598497584)
changed to be more perscriptive

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598462421)
done

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598499228)
done

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598465418)
elaborated

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#issuecomment-2107656257)
Thanks @instagibbs @theStack @sr-gi! Addressed the comments and added a few minor log improvements.

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598532513)
this was (mostly) taken

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2107677632)
@cbergqvist I took your suggestion from https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586847044

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2107736166)
> Perhaps another approach could be a class for `RemovalReason`, which contains the Enum as a field and then uses a std::variant to hold the data needed by the wallet, if any. Would be more extensible, and then the to string method could be defined once on the class. Special logic per reason can also be handled all in the same place by switching on the Enum value and taking appropriate action per reason.

Thank you @josibake. Will this approach still require the definition of structs to hold
...

🤔 ryanofsky reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2052882564)
Updated 6ea5ee5a268cfbb9cd234e0f1381c8a4785ff04c -> 43532d9c9470ad00ecb991859f7f0c6a025fa72e ([`pr/rmutil.14`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.14) -> [`pr/rmutil.15`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.15), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.14..pr/rmutil.15)) fixing silent conflict with #29845, fixing shellcheck lint errors, and moving check-deps.sh commit first as suggested so it is easier to see effects of later commits.
...

💬 glozow commented on pull request "rpc, bugfix: Enforce maximum value for setmocktime":
(https://github.com/bitcoin/bitcoin/pull/29869#issuecomment-2107746080)
Backport for 26.x is in #29899

💬 paplorinc commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598594927)
What if we just extract it to code like it was done with the [rest of the sage files](https://github.com/search?q=repo%3Abitcoin%2Fbitcoin%20.sage&type=code), and reference the file instead of the code (I don't like dead code in comments), e.g. https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/ecdsa_impl.h#L19 ?

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2107797730)
> Will this approach still require the definition of structs to hold each data for each reason?

I don't think so: only the Conflict and Replace reasons need extra data, and from what I understand it can only be one reason at any given time. Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, `std::variant<CTransactionRef, BlockData>`, where block data holds the block hash and block number.

💬 glozow commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1598409494)
nit: just update the value beforehand?
```suggestion
expected_num_orphans -= 2;
BOOST_CHECK(orphanage.CountOrphans() == expected_num_orphans);
```

💬 glozow commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1598595375)
Is there a reason to do this manually instead of using `MakeTransactionSpending()`?

💬 glozow commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1598596266)
Maybe out of scope but I wonder if it would be best to add a `current_time` parameter to the `TxOrphanage` methods to make it easier to test and fuzz the expiration (also should have a test for the sorting for `GetChildrenFromSamePeer` results)

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598618650)
Going to leave this as is for now. The sage code in the comment is meant to provide background on where the constant comes from, so I'd prefer to leave it since it is more precise than a written out explanation. However, the code is not essential for checking the "correctness" of the value in that it would be sufficient for someone to simply check that this value matches BIP341. This value will never change, either, so I don't see any advantage to having a sage file in this repo for generating t
...

💬 maflcko commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#issuecomment-2107872102)
> Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.

IIRC this was done to start with a few outputs in the beginning, and increase the number of outputs as the size of the fuzz input increases. The hope is that the runtime is linear with the size of the fuzz input, and that minimizing a potential crash results in a transaction with less outputs. If you allow 256 in the
...

💬 sr-gi commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1595556422)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd

What is the rationale for 64? That seems like a lot of connections.

🤔 sr-gi reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2041604763)
Code review up to 28adad8eefd18a62232b5c77cbc9ca739a53308d (no tests so far).

The main thing I'm wondering currently is why are we tacking `m_private_broadcast_connections_to_open` loosely? It feels harder to reason about, but I don't see what the benefit of it is.